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.

Sign up for GitHub

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

Jump to bottom

Allow parameters to follow standard lifecycle rules #1556

Closed
wants to merge 1 commit into from
Closed

Allow parameters to follow standard lifecycle rules #1556

wants to merge 1 commit into from

Conversation

pmorton
Copy link
Contributor

@pmorton pmorton commented Aug 31, 2017

This pull request refines the commits that were merged in response to #1004 and #1006. When 1006 was merged it introduced a new flag which allows the user to overwrite an existing parameter. I understand that this can be helpful if the parameter already exists in your system, however the introduction of this flags has broken the update lifecycle. Specifically the way that updates interacts with overwrite is non-sensical. Under the introduced in 1006 you must have overwrite set to true if the value of that parameter needs to be updated. This creates an unsafe scenario in that a new resource will overwrite an existing value that it did not create and this should throw an error unless the user decides that it should always be overwritten.

I am proposing the following the following rules to govern update.

  1. If the user has specified the overwrite flag, overwrite the parameters value.
  2. If the resource is new, do not overwrite the parameters value. This will result in an error if the parameter already exists and is not managed by terraform. By default we do not want to overwrite parameters which we don't not own.
  3. All other cases, allow the parameter to be overwritten.

@pmorton pmorton mentioned this pull request Aug 31, 2017
Merged
@smphhh
Copy link

smphhh commented Sep 1, 2017

I created an issue (#1481) earlier with the same proposed behavior so 👍.

@pmorton
Copy link
Contributor Author

pmorton commented Sep 5, 2017

@Ninir I was wondering if this PR would be within your domain to review? The PR is pretty straight forward and passes unit and acceptance tests.

@Ninir
Copy link
Contributor

Ninir commented Sep 6, 2017

Hi @pmorton

I have to admit I love the idea and your implementation :) 👍

Here is what I did:

  1. Create a parameter in the store with name test_parameter and value foo in the console
  2. Create the aws_ssm_parameter resource with the same name but value bar, without the overwrite argument
  3. Ran the apply: resource is created BUT the initial parameter is overridden, even though it was new on Terraform.
  4. Expected and had the ParameterAlreadyExists exception
  5. Updated the overwrite configuration with overwrite = false, and still expected and had the ParameterAlreadyExists exception
  6. Updated the overwrite configuration with overwrite = true, and still expected not to have the ParameterAlreadyExists exception, and didn't have it

This seems like the proper way to do it, just want to clarify everything :)

Thanks!

Ninir
Ninir suggested changes Sep 6, 2017
View reviewed changes
Copy link
Contributor

@Ninir Ninir left a comment

Choose a reason for hiding this comment

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

Hey @pmorton

Did a review of your work. Just left cosmetic changes to address :)

Thanks!

aws/resource_aws_ssm_parameter.go

func shouldUpdateSsmParameter(d *schema.ResourceData) bool {
// If the user has specifed a preference, return their preference
if value, ok := d.GetOkExists("overwrite"); ok == true {
Copy link
Contributor

Choose a reason for hiding this comment

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

the ok == true isn't needed here

website/docs/r/ssm_parameter.html.markdown
@@ -56,7 +56,7 @@ The following arguments are supported:
* `type` - (Required) The type of the parameter. Valid types are `String`, `StringList` and `SecureString`.
* `value` - (Required) The value of the parameter.
* `key_id` - (Optional) The KMS key id or arn for encrypting a SecureString.
* `overwrite` - (Optional) Overwrite an existing parameter. If not specified, will default to `false`.
* `overwrite` - (Optional) Overwrite an existing parameter even if it was created outside of terraform.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should really detail this argument a bit more here, to provide better inputs about what is going on.
Just for the case "if specified and set to true,...". Thoughts? :)

@Ninir Ninir added enhancement Requests to existing resources that expand the functionality or scope. waiting-response Maintainers are waiting on response from community or contributor. labels Sep 6, 2017
jocgir added a commit to coveooss/terraform-provider-aws that referenced this pull request Sep 25, 2017
@jocgir
Fix many problem with the SSM parameter
62254c3 
- The type and the key_id should not force new resource (that may result in data lost especially if we add a lifecycle rule to protect the value)
- Implement the exist function to check if the resource already exist
- Handle a bug in AWS that avoid removing a description after it has been set
- Handle the key_id properly (if it is not specified by the user, should default to alias/aws/ssm)
- Implemented the logic developed by @pmorton in PR hashicorp#1556 (to better handle the overwrite parameter)
@radeksimko radeksimko added the size/M Managed by automation to categorize the size of a PR. label Nov 15, 2017
@schmod
Copy link

schmod commented Nov 16, 2017

any chance of this getting merged soon? the current behavior is moderately unusable (and, frankly, having overwrite as a special-case with SSM parameters is kind of odd, given that Terraform generally doesn't understand "upsert" operations on other kinds of resources)

@bflad bflad added the service/ssm Issues and PRs that pertain to the ssm service. label Jan 28, 2018
@kid kid mentioned this pull request Jan 29, 2018
Closed
@bflad
Copy link
Contributor

bflad commented Apr 26, 2018

It looks like this functionality made it in via #1520, which was previously released in version 1.11.0 of the AWS provider and has been available in all releases since. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@bflad bflad closed this Apr 26, 2018
@bflad bflad added this to the v1.11.0 milestone Apr 26, 2018
@ghost
Copy link

ghost commented Apr 6, 2020

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 6, 2020
@breathingdust breathingdust removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Ninir Ninir Ninir requested changes

Assignees
No one assigned
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/ssm Issues and PRs that pertain to the ssm service. size/M Managed by automation to categorize the size of a PR.
Projects
None yet
Milestone
v1.11.0
Development

Successfully merging this pull request may close these issues.
7 participants
@pmorton @smphhh @Ninir @schmod @bflad @breathingdust @radeksimko