Readme should be updated to not imply that data bags are the only way to provide credentials

[et304383]

Not every project uses data bags, and while the recipe does support passing in credentials through

default[:sumologic][:accessID]  = "TODO"
default[:sumologic][:accessKey] = "TODO"

This doesn't work out of the box without forcing:

default[:sumologic][:credentials] = nil

None of this is documented and the readme implies that data bags are the only way to provide credentials.

Please provide instructions on both ways to provide credentials. Also, you should flip the order of the conditional in the recipe to use credentials specified in attributes FIRST before checking the data bag. Having to go through the hoops we had to in order to NOT use data bags was a pain.

[et304383]

I'll also throw in there that the conditional logic in recipe file that checks if credentials is defined and takes other actions based on that conditional is a poor Chef patterns as it becomes difficult to override that pattern. One must match the attribute precedence exactly in [:sumologic][:credentials] or the logic in the recipe gets executed during the Chef compile phase.

Chef's recommendations for most reusable cookbooks these days is to write light weight resource providers to alleviate attribute configuration nightmare when trying to control the behaviour of community cookbooks.

If you want an example of how NOT to do things (that you're moving dangerously close towards) please look at the Tomcat cookbook prior to their 2.0 rewrite to use LWRPs. It was an absolute nightmare.

[KierranM]

There is the sumologic_collector LWRP, that seems like it would fit your purpose.

[et304383]

Edit: I did not see the sumo logic source LWRP. Thanks.

[majormoses]

if you ask me we should remove setting sensitive information from attributes and we should remove this functionality in the next major release. but for now we should document and discourage this behavior.

[dee-kryvenko]

Not only people may or may not be using data bags, but they also may have standards as to the data bags formats. In my case, we have a little helper function that is a mandatory to use everywhere we want to read encrypted data bags, and it implies to the data bag format to have somewhat a naming convention to override secrets for different environments. As to example, I need a data bag secrets/sumologic.json to have the following content:

{
  "id": "sumologic",
  "description": "Sumo keys",
  "development": {
    ...
  },
...
  "production": {
    ...
  }

In the current implementation it doesn't sounds possible. It is true that attributes should never be used for storing sensitive data as this is persistent data goes to chef server node object in a plain text, but there is node.run_state that is designed specifically for non-persistent data.

[majormoses]

In addition to the LWRP, you can always specify the credentials on your own like I did on this old slide deck you can skip to page 32

[dee-kryvenko]

Yeah, I can create my own cookbook, true...

[majormoses]

From my perspective the documentation does not tell you how to set the attributes directly because it is unsafe. There are several safer methods such as using the LWRP and using encrypted data_bags to get the secrets safely to it without exposing it to everyone who has read access to chef and they are documented. If someone wants to document setting attributes directly they can submit a PR with very strong verbiage on why you should not do it and I am willing to accept that.

Yeah, I can create my own cookbook, true...

Wrapper cookbooks are a (good) common pattern and at my last job (I am no longer working at a place that uses sumo) we had a sumo wrapper cookbook. Default recipes are just that default, if you want something else you can remove the default recipe and control everything you want. Chef is a framework, every company has different needs and different ways of solving the same base problem sets.

[dee-kryvenko]

There is two ways of using a dependency cookbook in the wrapper and both should be generally supported and using the same actual code to do stuff. (LWRPs and including recipes with configuring attributes). From what I can see this cookbook does two different implementations as per LWRP and include recipe approach, which violates DRY.

Wrapper cookbook should not re-implement but wrap. What you shared in the slides sounds like re-implementing, I am not sure what I need this cookbook for if I go down this road.

And again, what I said is, it should not be an attributes used for sensitive stuff but node.run_state. My comment was partly irrelevant in this ticket, probably I should submit a new one, since what I propose is not to update readme but change the cookbook code to start using node.run_state and by that enable wrapper cookbooks to supply credentials the way they want.

[dee-kryvenko]

Just to demonstrate what I mean: https://github.com/SumoLogic/sumologic-collector-chef-cookbook/pull/152 (not tested)

[majormoses]

Sorry I did not respond I thought I did I have been traveling.

From what I can see this cookbook does two different implementations as per LWRP and include recipe approach, which violates DRY.

I agree, this cookbook has a lot of old cruft and I am all for cleaning this up.

Wrapper cookbook should not re-implement but wrap.

In a general sense I agree however it is common to need to disagree with the upstream cookbook and do things how you see fit. That was a long time ago and I can't recall if the lwrp even existed at that point I was just demonstrating that if you violently disagree with the cookbooks implementation or have very basic understanding of chef you can do what you want as chef is a framework.

[dee-kryvenko]

In a general sense I agree however it is common to need to disagree with the upstream cookbook and do things how you see fit. That was a long time ago and I can't recall if the lwrp even existed at that point I was just demonstrating that if you violently disagree with the cookbooks implementation or have very basic understanding of chef you can

I think we are talking about the same thing here. I just meant that this cookbook should provide reusable pieces wrapper cookbook can wrap around to change behaviour, whether that given example was suggesting more like a lot of custom logic with almost no benefits of using this one.

[majormoses]

closed via #152