Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Basic Puppet provisioner #18851

Merged
merged 13 commits into from Jun 10, 2019
Merged

Basic Puppet provisioner #18851

merged 13 commits into from Jun 10, 2019

Conversation

rodjek
Copy link
Contributor

@rodjek rodjek commented Sep 13, 2018

Opening this up for general review of the Go code & style (I'm sure there's some code there that isn't idiomatic Go or not in the preferred style used at Hashicorp).

This PR adds a provisioner to provision instances with Puppet. It supports both open source Puppet and Puppet Enterprise. Docs will be added when the code is closer to being finalised.

This closes #21335.

Copy link
Contributor

@mildwonkey mildwonkey left a comment

Hi @rodjek! Thanks for submitting this.

I've left a few comments and questions inline, but overall this is great to see.
I don't have experience with provisioners in terraform prior to this PR, so I'm going to get a second pair of eyes as well - don't take it personally :)

Thanks!

builtin/provisioners/puppet/bolt/bolt.go Outdated Show resolved Hide resolved
builtin/provisioners/puppet/resource_provisioner.go Outdated Show resolved Hide resolved
builtin/provisioners/puppet/resource_provisioner.go Outdated Show resolved Hide resolved
builtin/provisioners/puppet/resource_provisioner.go Outdated Show resolved Hide resolved
builtin/provisioners/puppet/resource_provisioner.go Outdated Show resolved Hide resolved
@rodjek rodjek force-pushed the puppet-provisioner branch 2 times, most recently from dc141cf to c09ecad Compare Dec 12, 2018
Copy link
Contributor

@svanharmelen svanharmelen left a comment

Thank you very much for this PR @rodjek! Overall this looks great but I left a few comments that could maybe improve a few bits. Please have a look and let me know what you think about the suggestions.

Thanks!

}
cmdargs = append(cmdargs, command)

ctx, cancel := context.WithTimeout(context.Background(), 300*time.Second)
Copy link
Contributor

@svanharmelen svanharmelen Dec 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you share the reasoning for adding this particular (hardcoded) timeout?

Copy link
Contributor Author

@rodjek rodjek Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a safety precaution in case bolt hangs for some reason. I've changed the timeout to be configurable with a default value of 5 minutes.

builtin/provisioners/puppet/nix_provisioner.go Outdated Show resolved Hide resolved
builtin/provisioners/puppet/bolt/bolt.go Show resolved Hide resolved
builtin/provisioners/puppet/nix_provisioner.go Outdated Show resolved Hide resolved
builtin/provisioners/puppet/nix_provisioner.go Outdated Show resolved Hide resolved
builtin/provisioners/puppet/resource_provisioner.go Outdated Show resolved Hide resolved
builtin/provisioners/puppet/windows_provisioner.go Outdated Show resolved Hide resolved
builtin/provisioners/puppet/windows_provisioner.go Outdated Show resolved Hide resolved
builtin/provisioners/puppet/windows_provisioner.go Outdated Show resolved Hide resolved
builtin/provisioners/puppet/windows_provisioner.go Outdated Show resolved Hide resolved
@hashicorp-cla
Copy link

@hashicorp-cla hashicorp-cla commented Jan 29, 2019

CLA assistant check
All committers have signed the CLA.

@raphink
Copy link
Contributor

@raphink raphink commented Mar 4, 2019

@svanharmelen what is the status of this PR? I'd be really great to have this feature in terraform!

Copy link
Contributor

@svanharmelen svanharmelen left a comment

@rodjek I just got around to doing another review after you added the updates/fixes (sorry for taking so long!). I think it looks really good now, so thank you very much for all your work and the updates/fixes you added 👍

I do have a couple of small followup comments, but they should be very straightforward to address. After that I'm good to merge this, but I need to check in with the team to see if we want to add this to the current v0.12 release cycle, or wait until that is released and add this for v0.12.1.

@mildwonkey can you comment on when you think we can/should merge this (once the remaining comments are addressed)?

Thanks!

builtin/provisioners/puppet/resource_provisioner.go Outdated Show resolved Hide resolved
builtin/provisioners/puppet/resource_provisioner.go Outdated Show resolved Hide resolved
builtin/provisioners/puppet/resource_provisioner.go Outdated Show resolved Hide resolved
builtin/provisioners/puppet/resource_provisioner.go Outdated Show resolved Hide resolved
builtin/provisioners/puppet/resource_provisioner.go Outdated Show resolved Hide resolved
builtin/provisioners/puppet/bolt/bolt.go Show resolved Hide resolved
builtin/provisioners/puppet/bolt/bolt.go Outdated Show resolved Hide resolved
@rodjek
Copy link
Contributor Author

@rodjek rodjek commented Mar 12, 2019

Thanks @svanharmelen, I think I've addressed all the outstanding items!

Copy link
Contributor

@svanharmelen svanharmelen left a comment

One minor suggestion left, but the PR looks really nice now! Thanks for bearing with me and for updating the PR!

I'll leave merging the PR to @mildwonkey so she can coordinate it with the current v0.12 releases 👍

builtin/provisioners/puppet/resource_provisioner.go Outdated Show resolved Hide resolved
Co-Authored-By: rodjek <tim@sharpe.id.au>
@rodjek rodjek changed the title [WIP] Basic Puppet provisioner Basic Puppet provisioner Mar 12, 2019
@randomcamel randomcamel added this to the v0.12.1 milestone Mar 12, 2019
@turbodog
Copy link
Contributor

@turbodog turbodog commented Mar 14, 2019

@mildwonkey is there an ETA for the 0.12.1 release?

@mildwonkey mildwonkey self-assigned this Mar 14, 2019
@mildwonkey
Copy link
Contributor

@mildwonkey mildwonkey commented Mar 14, 2019

No estimate yet @turbodog; we're still working on the 0.12.0 general release! I will update this as soon as I can.

@randomcamel
Copy link
Contributor

@randomcamel randomcamel commented Mar 14, 2019

@rodjek @turbodog We don't have a specific timeline on 0.12.1, but I'd like us to get it out ASAP after 0.12.0. In practice, this means letting 0.12.0 bake a little bit until we're confident any major issues have been dealt with. HCL2 alone means 0.12.0 has a fundamentally (very) large surface area, and we want a feature like the Puppet provisioner to ship on a version of 0.12.x that provides a stable platform on top of the 0.12.0 refactor.

@turbodog
Copy link
Contributor

@turbodog turbodog commented Mar 14, 2019

Thanks. Any ETA on 0.12.0?

Also, is there a possibility of backporting this into 0.11.x? As you said 0.12 has a large surface area and would be great to get this into people's hands in their current release line.

@rodjek
Copy link
Contributor Author

@rodjek rodjek commented Apr 9, 2019

@mildwonkey @svanharmelen Just pushed a minor change to the configuration schema.

@mildwonkey
Copy link
Contributor

@mildwonkey mildwonkey commented May 23, 2019

Hi @rodjek !

TERRAFORM 0.12 HAS RELEASED 🎉 🎉 🎉
We can finally move on with our lives 🤣

I'm going to dive back into this PR. You've already addressed tons of our feedback, so hopefully all that's left to do is add documentation. It would be nice to see more code comments, for the benefit of folks not as familiar with puppet (or terraform provisioners, for that matter), but that's not a blocker.

Out of sheer curiosity, do you have some sort of local acceptance testing set up? I'd love to get some hands-on experience with this provisioner, or even just watch a demo of it in action. Again, this is just me being nosy, no pressure 😄

return err
}

if err = p.runPuppetAgent(); err != nil {
Copy link
Contributor

@mildwonkey mildwonkey May 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine documentation will cover this, but what's the expected workflow if autosign is false? Would a user just expect the puppet run fail until the cert is signed?

Copy link
Contributor Author

@rodjek rodjek Jun 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If autosign is false, the Puppet run will probably fail. If they've configured their master to use naive certname matching autosigning (i.e. just trusting and signing any request for *.mycompany.com) then that will succeed. For other cases, I'm assuming they'll use something like a custom remote-exec provisioner before the puppet provisioner to take whatever action their setup requires (adding an entry to their CMBD perhaps).

@pselle pselle removed this from the v0.12.1 milestone Jun 4, 2019
@pselle pselle added this to the TBD milestone Jun 4, 2019
@mildwonkey
Copy link
Contributor

@mildwonkey mildwonkey commented Jun 10, 2019

🎉This will be included in the next release - thanks @rodjek!!

@mildwonkey mildwonkey merged commit 615110e into hashicorp:master Jun 10, 2019
2 checks passed
@llomgui
Copy link

@llomgui llomgui commented Jun 13, 2019

Do we have any documentation about this new provisioner?

@lfarnell
Copy link

@lfarnell lfarnell commented Jun 13, 2019

@llomgui No. This PR didn't have any documentation. @rodjek Is this something you can do? If not I can submit a PR.

cc @mildwonkey

@turbodog
Copy link
Contributor

@turbodog turbodog commented Jun 16, 2019

@lfarnell could you point us to an example or similar set of docs?

@rkst
Copy link
Collaborator

@rkst rkst commented Jun 17, 2019

@turbodog The Chef provisioner is probably a good similar base for docs.

@rodjek
Copy link
Contributor Author

@rodjek rodjek commented Jun 19, 2019

PR opened to add some docs at #21792

Out of sheer curiosity, do you have some sort of local acceptance testing set up? I'd love to get some hands-on experience with this provisioner, or even just watch a demo of it in action.

@mildwonkey I've pushed up a repo with a basic setup that you can use on AWS at https://github.com/rodjek/terraform-puppet-example/ (just don't judge me based on the quick and dirty execs provisioning the puppet master instance! 😀)

@ghost
Copy link

@ghost ghost commented Jul 25, 2019

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.

@hashicorp hashicorp locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.