New provisioner for Chef-Client #1868

svanharmelen · 2015-05-08T16:24:40Z

The PR is pretty complete and has a tested provisioner that works for both SSH and WinRM. There is a pretty decent test coverage and docs are included in this PR as well.

Closes #1158

The commit is pretty complete and has a tested/working provisioner for both SSH and WinRM. There are a few tests, but we maybe need another few to have better coverage. Docs are also included…

phinze · 2015-05-08T16:40:29Z

🙊 ❗

At first glance this looks delightfully complete. We'll follow up with a thorough review.

svanharmelen · 2015-05-08T16:46:48Z

Yes, that would be great 😃 Just let me know any issues and/or comments...

sethvargo · 2015-05-08T17:06:37Z

builtin/provisioners/chef-client/resource_provisioner.go

+
+const (
+	firstBoot      = "first-boot.json"
+	logfileDir     = "logfiles"


Should this be /var/chef/log or am I thinking of the wrong config option?

Yes, I think you are missing the actual point/use case for this one. All comments you made to code related to logging, seems to suggest that I'm trying to configure the logging of the Chef Client. But in fact I'm not... If you could have a quick look at the docs in this PR describing the log_to_file parameter, that will probably give you a push in the right direction 😃

In short the main purpose of the logging related code in the PR is to enable you to not have TF spit out all the Chef Client logging from all nodes on your console at once. If your spinning up 10+ machines which will possibly be build simultaneously in parallel, your console will be on fire with log lines from all nodes running their initial Chef run at the same time. So trying to follow a single host or to scroll back to find out why node x failed, will be undoable.

So if you set the option log_to_file to true it will redirect those logs to separate files (one per node) and put them in a subdirectory of your current location on your local machine. That way you can review any log of any hosts initial run afterwards if you need to checkup on anything.

So... Long story short, this const is pointing to the location on your local machine where it will create the logs if you enabled it to log_to_file. And with that additional info, I would say this is OK as is...

sethvargo · 2015-05-08T17:45:59Z

Hey @svanharmelen

Thanks for the PR! I did a Chef review, and I think @phinze is going to do a TF review shortly. One overarching question I have - this is specific to chef-client, but it seems like we could easily make this a generic "Chef" provisioner that supports chef-client, chef-zero, and chef-solo. What do you think?

svanharmelen · 2015-05-08T18:14:06Z

@sethvargo I wouldn't expect anyone else to review the Chef bits and pieces 😉 So thanks for all the great feedback! I'll be going over them in just a few...

As for your comment to make this a generic Chef provisioner, I think that's a great idea and I did think about it myself also. Just not sure how clean or messy the code paths would become when having so much different flavours, but there's only one way to find out of course 😃

But before diving into that discussion I would suggest to first work our way through the current PR (enough to talk about already 😉) and when all is said and done (could be before actually merging of course) we can cycle back to discussing what it would take to make this more generic.

So let me dive into your comments now first...

sethvargo · 2015-05-08T18:17:26Z

@svanharmelen that sounds good. I wouldn't be opposed to merging this to master and then issuing smaller, follow-up PRs to make it more generic. I think that might be easier to review.

svanharmelen · 2015-05-08T18:19:36Z

Yes, that sounds logical to me too...

Also renamed the provisioner to just `chef` as it’s out intention to end up with one provisioner for all types of `chef` clients.

svanharmelen · 2015-05-08T21:31:33Z

@sethvargo please have a look at https://github.com/svanharmelen/terraform/commit/c19d92fb6790ac8626afcfe8c93abdbad928b3fd I think everything we talked about is covered in this commit, except for the retryFunc. I'll do that one coming Monday (it's getting late here).

Let me know if I missed anything and/or if there are still other things you would like to discuss before giving your 'OK' 😉 Thanks...

sethvargo · 2015-05-08T21:48:26Z

Sounds good to me!

jeroendekorte · 2015-05-09T10:08:18Z

Just did some testing on Windows and Linux with the Chef provisioner and everything works great!

I added a debug log line in the last commit, only to find out it’s now logging the same info twice. So removed the double entry and tweaked the existing once.

svanharmelen · 2015-05-11T13:47:00Z

@sethvargo I've gave it another look, but I don't think I would like to change the current retryFunc right now. The current func is a direct copy of the one used in the existing provisioners. So I would think it's best to keep them in sync (through not required of course) and maybe update them all at once if needed.

More over because when also adding a close channel (which sounds like a good idea), the terraform.ResourceProvisionerinterface would need to change as currently there is nog way to pass a close channel from the core to the provisioners. So IMO this is a separate improvement which should be outside this PR.

So with that all comments should now be addressed. Still a few things that you commented about that are not addressed, but those where things we agreed on could be added later on in smaller PR's as the need for them arises.

Since it were quite a few comments it would be great if you could go over them one more time to confirm that the stuff that would need to be updated in this PR is now OK so we can move on to the detailed code review...

Thx!

sethvargo · 2015-05-11T14:08:21Z

No problem! Paging @phinze

phinze · 2015-05-12T01:57:35Z

builtin/provisioners/chef/resource_provisioner.go

+	default:
+		return raw, nil
+	}
+}


I'm confused about this function. How does it output JSON?

Maybe a look at the tests will help giving some insights. Take for example this test. If you look at what is going into the test you'll notice it's a bunch of []map[string]interface{}'es which are generated by TF reading in your config file which started out like something like this:

provisioner "chef" { attributes { "key1" { "subkey1" { "subkey2a" = ["val1", "val2", "val3"], "subkey2b" { "subkey3" = "value3" } } } } node_name = "webserver1" server_url = "https://chef.company.com/organizations/org1" validation_client_name = "chef-validator" validation_key_path = "../chef-validator.pem" }

And then ended up in TF like something like this:

"attributes": []map[string]interface{}{ map[string]interface{}{ "key1": []map[string]interface{}{ map[string]interface{}{ "subkey1": []map[string]interface{}{ map[string]interface{}{ "subkey2a": []interface{}{ "val1", "val2", "val3", }, "subkey2b": []map[string]interface{}{ map[string]interface{}{ "subkey3": "value3", }, }, }, }, }, }, "key2": "value2", }, }

So as this part should be used as plain JSON in the first-boot.json file needed for the initial Chef run, this function converts the format above to the output seen here:

{ "key1":{ "subkey1":{ "subkey2a":["val1", "val2", "val3"], "subkey2b":{ "subkey3":"value3" } } }, "key2":"value2" }

So that's about the what and why. As for the how... You can see the input is very consistent (everything is a []map[string]interface{}) so given how Go encodes/decodes arbitrary data you can see we only have to convert the slices of maps to just maps.

So I check for the length to confirm there is exactly 1 item in the slice and then return that one item. And by also doing this recursively for any values inside that map we iterate through the whole map ending up with a value map[string]interface{} which can be properly mashalled into JSON.

Thanks for the incredibly clear explanation! Very helpful. 😀

phinze · 2015-05-12T02:18:19Z

Noted a couple of very minor things, but overall this is looking great! Excited to land it. 😀

svanharmelen · 2015-05-12T08:40:59Z

@phinze just updated the PR with some minor tweaks following up on your comments... Please also see the inline reactions to your comments for some more elaborate answers.

So with that I think were good to go with this one, but I'll leave it up to you to walk through the PR one more time 😃

phinze · 2015-05-12T20:38:06Z

Looks great @svanharmelen - I'll let you do the honors. Merge at will! 🎊

New provisioner for Chef-Client

svanharmelen · 2015-05-12T21:06:59Z

Again thanks for the review @phinze! On to the next one 😉

phinze · 2015-05-12T21:08:16Z

Hooray @svanharmelen! 🎉

Can you update the CHANGELOG on master? You can copy the formatting from the "Features" block of 0.5.0.

squaresurf · 2015-05-12T21:11:50Z

Thank you @svanharmelen for doing this! 👏

Is there a way to define roles or tags via this provisioner?

svanharmelen · 2015-05-12T21:25:24Z

@phinze yes, will do...

@squaresurf what do you mean with "define roles or tags"? Defining a role sounds like creating a role? That will (of course) not be possible with this provisioner. But it does allow you to set the run-list of your new instance. So you can assign roles (and recipes) to the new instances. Is that what your mean or am I misunderstanding your question?

squaresurf · 2015-05-12T21:41:25Z

Yeah, I meant assign, not define. Sorry about that. So within the run list I can do something like this?
role[ROLE_NAME]

By tags I'm wondering if there is a way to assign a tag to the resource I'm controlling with terraform.

Thanks again for all your hard work.

svanharmelen · 2015-05-12T21:50:59Z

Yes, you can... See the docs for some examples. The run_list argument expects a list of strings, so you can do something like this:

    provisioner "chef"  {
        attributes {
          "key1" {
            "subkey1" {
              "subkey2a" = ["val1", "val2", "val3"],
                "subkey2b" {
                  "subkey3" = "value3"
                }
              }
           }
        }
        node_name = "webserver1"
        run_list = ["recipe[COOKBOOK::RECIPE]", "COOKBOOK::RECIPE", "role[NAME]"]
        server_url = "https://chef.company.com/organizations/org1"
        validation_client_name = "chef-validator"
        validation_key_path = "../chef-validator.pem"
    }

You can also set attributes as the example above, but tags are currently not supported.

sandstrom · 2015-05-12T23:08:05Z

Looks awesome! ⛵ We're currently using chef-solo plus stemcell by airbnb. With the forthcoming support for chef-solo, this provisioner will make a switch to terraform a lot easier.

squaresurf · 2015-05-13T01:27:46Z

Awesome! Thanks @svanharmelen.

vmorris · 2016-05-03T14:50:23Z

builtin/provisioners/chef/ssh_provisioner.go

+)
+
+const (
+	installURL = "https://www.chef.io/chef/install.sh"


Can we get this changed to be a default parameter that can be overridden? Not everyone uses chef in a public internet configuration and HTTP proxies are not always available.

Hey @vmorris

Thank you for your comment. This Pull Request has been merged for over a year, and it's difficult for the Terraform team to triage closed and merged issues. Could you please open a new issue for them to track. Thanks! 😄

ghost · 2020-04-26T02:01:34Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

Sander van Harmelen added 3 commits May 8, 2015 14:54

This commit adds a Chef Client provisioner

60984b2

The commit is pretty complete and has a tested/working provisioner for both SSH and WinRM. There are a few tests, but we maybe need another few to have better coverage. Docs are also included…

Small update/fix to properly clean ANSI going to the logfile...

4a99cf9

Adding the tests...

d4150d5

phinze added the enhancement label May 8, 2015

sethvargo reviewed May 8, 2015
View reviewed changes

svanharmelen mentioned this pull request May 8, 2015

Chef provisioner? #1158

Closed

Sander van Harmelen added 2 commits May 8, 2015 23:25

Refactored quite a few things after review...

c19d92f

Also renamed the provisioner to just `chef` as it’s out intention to end up with one provisioner for all types of `chef` clients.

Small textual update...

2a5fffc

Improved some logging...

a8daced

I added a debug log line in the last commit, only to find out it’s now logging the same info twice. So removed the double entry and tweaked the existing once.

phinze reviewed May 12, 2015
View reviewed changes

Tweaking a few minor things according to the feedback on GH

11314a3

svanharmelen pushed a commit that referenced this pull request May 12, 2015

Merge pull request #1868 from svanharmelen/f-chef-client-provisioner

4e0aa33

New provisioner for Chef-Client

svanharmelen merged commit 4e0aa33 into hashicorp:master May 12, 2015

svanharmelen deleted the f-chef-client-provisioner branch May 12, 2015 21:28

vmorris reviewed May 3, 2016
View reviewed changes

ghost locked and limited conversation to collaborators Apr 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New provisioner for Chef-Client #1868

New provisioner for Chef-Client #1868

svanharmelen commented May 8, 2015

phinze commented May 8, 2015

svanharmelen commented May 8, 2015

sethvargo May 8, 2015

svanharmelen May 8, 2015

sethvargo May 8, 2015

sethvargo commented May 8, 2015

svanharmelen commented May 8, 2015

sethvargo commented May 8, 2015

svanharmelen commented May 8, 2015

svanharmelen commented May 8, 2015

sethvargo commented May 8, 2015

jeroendekorte commented May 9, 2015

svanharmelen commented May 11, 2015

sethvargo commented May 11, 2015

phinze May 12, 2015

svanharmelen May 12, 2015

phinze May 12, 2015

phinze commented May 12, 2015

svanharmelen commented May 12, 2015

phinze commented May 12, 2015

svanharmelen commented May 12, 2015

phinze commented May 12, 2015

squaresurf commented May 12, 2015

svanharmelen commented May 12, 2015

squaresurf commented May 12, 2015

svanharmelen commented May 12, 2015

sandstrom commented May 12, 2015

squaresurf commented May 13, 2015

vmorris May 3, 2016

sethvargo May 3, 2016

ghost commented Apr 26, 2020

New provisioner for Chef-Client #1868

New provisioner for Chef-Client #1868

Conversation

svanharmelen commented May 8, 2015

phinze commented May 8, 2015

svanharmelen commented May 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sethvargo commented May 8, 2015

svanharmelen commented May 8, 2015

sethvargo commented May 8, 2015

svanharmelen commented May 8, 2015

svanharmelen commented May 8, 2015

sethvargo commented May 8, 2015

jeroendekorte commented May 9, 2015

svanharmelen commented May 11, 2015

sethvargo commented May 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phinze commented May 12, 2015

svanharmelen commented May 12, 2015

phinze commented May 12, 2015

svanharmelen commented May 12, 2015

phinze commented May 12, 2015

squaresurf commented May 12, 2015

svanharmelen commented May 12, 2015

squaresurf commented May 12, 2015

svanharmelen commented May 12, 2015

sandstrom commented May 12, 2015

squaresurf commented May 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 26, 2020