WIP: support for opsworks for chef automate #7817
Conversation
It's missing a bunch of stuff, for sure, like parameters, tests, docs, etc, and I haven't actually run it against aws, but I have what I think is all of the boilerplate required, so it's a start
I grabbed all of these from the API docs: https://docs.aws.amazon.com/sdk-for-go/api/service/opsworkscm/#CreateServerInput It's still not entirely there, and I'll need to document the instance/service profile ARN stuff, and the security groups since I think the security groups need to be made outside and not automanaged by the server resource on AWS's side
everything is there but the EngineAttributes bit and validation. also debating still on whether some of those required params should be optional. I'd like them to be required, but I'll have to ask upstream their thoughts
ghost
added
size/XL
They already are required because of the way I made the Create function, so while it's still an open question whether or not they should actually be required required, I at least want to make it consistent at the moment
There was a problem hiding this comment.
Obviously I also need to write tests and docs, but I wanted to get the code up first for comments on the questions I have.
Any advice would be greatly appreciated.
Also, the underlying API here is the opsworkscm, which actually manages both opsworks for chef and opsworks for puppet. I'm not sure style-wise whether this resource should be made generic for both or if opsworks for puppet would have its own resource that uses the same underlying api. I'm a fan of having them be separate, for a number of reasons, but I defer to y'all's judgement.
Thanks!
|
|
||
| Schema: map[string]*schema.Schema{ | ||
| // TODO: do we want this? I think this is another "magic" thing that happens | ||
| // I think we can still associate a public IP later if we want? :shrug: |
There was a problem hiding this comment.
I'm curious to know what y'all think about this. I'm on the side of not having magic happen that the user might not be expecting, meaning this attribute would go away and we'd just hardcode it to no behind the scenes.
|
|
||
| // TODO: | ||
| // I kinda want to make this required because how else are we going to get at that info? | ||
| // Also, since these are write-only and not returned by the API, we'll probably need a custom diff function that ignores them? |
There was a problem hiding this comment.
Also interested in y'all's thoughts on this:
What's happening here is these are optional things to the API, and amazon will generate values for you and return them in the CreateServerResponse, but you'll never see it again.
I don't know if there's a way then to take the output of that (data provider-like?) and shove it into another resource like a vault password or something. And I'm fairly certain the whole thing would be pretty useless if you threw away these values. So I'm leaning on the side of making these required up front.
"these" being the chef_pivotal_key and chef_delivery_admin_password attributes, both of which behave this way.
There was a problem hiding this comment.
Note, this is just the pubkey component. Due to sensitive information leak in state the privkey should ideally be kept out of terraform - making this argument required.
Also needs to be base64 encoded, which differs from what is provided in your testcase (which would include -----BEGIN...)
aws/resource_aws_opsworks_chef.go
Outdated
|
|
||
| // TODO: | ||
| // default and only valid. I'll leave it configurable for future proofing but | ||
| // validation failure, maybe? |
There was a problem hiding this comment.
These are required parameters to the API call, but they only have one valid value at the moment.
Should I remove these from the resource and hardcode the values, leave them on the resource but enforce them in validation, or leave them on the resource, make them optional with these values as the default, and let the API handle validation? Or something else entirely?
| // TODO: | ||
| // if you don't specify this, it'll create one. I'm of the opinion that users should | ||
| // create it themselves. Or maybe some sort of wrapper module? I dunno. | ||
| // if it's optional, will they be computed? if so, is that something we can express here? |
There was a problem hiding this comment.
Here's another "magic happens" thing. If you don't specify security groups, it'll make one for you. Is this something we want to allow or should we require folks to specify the security groups explicitly? I'm leaning on the side of explicitly.
| // your EC2 instances are created in a default subnet that is selected by Amazon | ||
| // EC2. If you specify subnet IDs, the VPC must have "Auto Assign Public IP" | ||
| // enabled. | ||
| // if it's optional, will they be computed? if so, is that something we can express here? |
There was a problem hiding this comment.
All sorts of :wat: here.
I'm thinking we should force people to specify subnet IDs. But that's just me.
There was a problem hiding this comment.
Make subnets required argument.
The subnet only needs "Auto Assign Public IP" if associate_public_ip_address == true, otherwise it won't be internet accessible. AWS docs may have drifted, you can now make chef servers with private ips.
This, together with EC2-Classic requirements should just be covered in documentation.
| } | ||
|
|
||
| func resourceAwsOpsworksChefValidate(d *schema.ResourceData) error { | ||
| // TODO: validation function |
There was a problem hiding this comment.
For validation, how much should I replicate the validation that amazon is going to do on the backend here?
The maintenance and backup windows have a simple format that could be validated, and then there's the chef admin password. Oh, and the chef_pivotal_key. How much should I validate before just leaving it to the API to explode when the input is wrong?
There was a problem hiding this comment.
ok so window format is already a thing that is validated elsewhere so I'll validate it too. I wanted to wrap the existing validators but wasn't really sure how to do it so I made another that did the needful. Definitely open to any suggestions for improvement :)
There was a problem hiding this comment.
and nevermind, I found https://github.com/hashicorp/terraform/blob/a95d97f0665f905411d4e6ab400136c50539d6c6/helper/validation/validation.go#L33-L47 and am incorporating :)
There was a problem hiding this comment.
annnnd nevermind, that's in a newer version of terraform than this package is pulling in (I think, I'm not super familiar with golang development tbh). I'll use the one in this provider: https://github.com/terraform-providers/terraform-provider-aws/blob/85e58f8cf3c79621d32db94f657e8a258ee44fd3/aws/validators.go#L27-L42
| resp, cerr = client.CreateServer(req) | ||
| if cerr != nil { | ||
| if opserr, ok := cerr.(awserr.Error); ok { | ||
| // TODO: does this also happen with this resource? |
There was a problem hiding this comment.
Should I err on the side of "we can add this back later if it turns out it's a problem" or should I try to recreate the circumstances described to see if it needs to stay?
| } | ||
|
|
||
| // TODO: these are the only values that are allowed to be changed | ||
| // according to the API, is there a way to express this in the provider somehow? |
There was a problem hiding this comment.
question here: only some of the values are able to be updated. The others are write-once. How to handle?
There are actually 2 more that can be updated: the chef_pivotal_key and chef_delivery_admin_password, but those are actually set using a different API call (easy enough to handle, though) but the question there is how do we know if those values need to change. Is there a way we can store the output of the createserver in a hashed form somehow so we can compare the inputs to what we last knew the values were on the server and update them? Or something else?
There was a problem hiding this comment.
Seems like ForceNew on the attribute is the Schema is what I'm after for the first part of that question.
There was a problem hiding this comment.
followup question on the ForceNew: will that propagate to any node associations that exist? Also, I should probably create a resource for those.
https://docs.aws.amazon.com/sdk-for-go/api/service/opsworkscm/#OpsWorksCM.AssociateNode
There was a problem hiding this comment.
hmm. Now that I think of it, the AssociateNode is probably going to be done outside of terraform scope, which means terraform wouldn't have the info needed to be able to recreate the associations. So a ForceNew would be a pretty destructive thing.
I'm curious how this is even handled on AWS side. If instance type is one of the forcenew params, then I'm assuming they're not able to roll the backend chef machine(s) to new sizes, which makes me wonder if they even have multiples or if it's HA or whatever, what happens when one dies? I'm probably reading too much into that part of things, but I might contact amazon support and ask for more details.
There was a problem hiding this comment.
Ok, I filed a support request with amazon to see what they have to say about this. For now, I'll leave ForceNew on, but I actually think forcing new here would be a really bad idea, so I'm hoping there are better ways to handle that situation :) I'm open to any suggestions.
I think this is the baseline requirements for setting up the opsworks_chef instance. It's ... verbose. But this is also a fairly complex thing to be creating, so that's kinda to be expected, methinks.
per discussion in the PR, I think ForceNew here is probably a bad idea because it will almost certainly destroy any node associations, but at least it won't silently do nothing :)
ghost
added
the
tests
since this doesn't seem to be running yet (I think this only runs in acceptance tests?) it doesn't explode, but I'm testing something like it elsewhere
| Optional: true, | ||
| Default: 1, | ||
| // https://docs.aws.amazon.com/opsworks/latest/userguide/opscm-chef-backup.html | ||
| // I could swear I found this 1-30 limitation elsewhere, but I can't seem to find it now other than there |
There was a problem hiding this comment.
Found docs here
A maximum of 30 backups are kept; when the maximum is reached, AWS OpsWorks for Chef Automate deletes the oldest backups to make room for new ones.
It's also on the console wizard
|
Hello! Just curious if this PR is still being worked on; it would be a great benefit to me. 🎉 Is there anything I can do to assist -- or just shut up and wait 🤐? Thanks! |
|
It is not currently being worked on, by me anyways, sorry :(
I don’t remember quite where I left off with it but I imagine it’s missing docs, some fixes on the comments I had and had gotten replies to, and maybe a few other things. I will be out of town for 2 weeks starting in a few days so I won’t have much of an opportunity to work in it for a while.
… On Sep 3, 2019, at 9:46 AM, Tyler Mauthe ***@***.***> wrote:
Hello! Just curious if this PR is still being worked on; it would be a great benefit to me. 🎉
Is there anything I can do to assist -- or just shut up and wait 🤐?
Thanks!
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
You can now add tags to an AWS OpsWorks for Chef Automate server or an AWS OpsWorks for Puppet Enterprise master, or to server backups. Requires AWS SDK v1.26.5: |
Notification of Recent and Upcoming Changes to ContributionsThank you for this contribution! There have been a few recent development changes that affect this pull request. We apologize for the inconvenience, especially if there have been long review delays up until now. Please note that this is automated message from an unmonitored account. See the FAQ for additional information on the maintainer team and review prioritization. If you are unable to complete these updates, please leave a comment for the community and maintainers so someone can potentially continue the work. The maintainers will encourage other contributors to use the existing contribution as the base for additional changes as appropriate. Otherwise, contributions that do not receive updated code or comments from the original contributor may be closed in the future so the maintainers can focus on active items. For the most up to date information about Terraform AWS Provider development, see the Contributing Guide. Additional technical debt changes can be tracked with the As part of updating a pull request with these changes, the most current unit testing and linting will run. These may report issues that were not previously reported. Action Required: Terraform 0.12 SyntaxReference: #8950 Version 3 and later of the Terraform AWS Provider, which all existing contributions would potentially be added, only supports Terraform 0.12 and later. Certain syntax elements of Terraform 0.11 and earlier show deprecation warnings during runs with Terraform 0.12. Documentation and test configurations, such as those including deprecated string interpolations ( Action Required: Terraform Plugin SDK Version 2Reference: #14551 The Terraform AWS Provider has been upgraded to the latest version of the Terraform Plugin SDK. Generally, most changes to contributions should only involve updating Go import paths in source code files. Please see the referenced issue for additional information. Removal of website/aws.erb FileReference: #14712 Any changes to the Upcoming Change of Git Branch NamingReference: #14292 Development environments will need their upstream Git branch updated from Upcoming Change of GitHub OrganizationReference: #14715 This repository will be migrating from https://github.com/terraform-providers/terraform-provider-aws to https://github.com/hashicorp/terraform-provider-aws. No practitioner or developer action is anticipated and most GitHub functionality will automatically redirect to the new location. Go import paths including |
|
Closing as stale due to inactivity. |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
Fixes #403
Changes proposed in this pull request:
Output from acceptance testing: