New ssm parameters features #1520

jocgir · 2017-08-28T19:18:43Z

Resubmit of the PR #1295.

I made some changes on the aws_ssm_parameter resource.

I added support for the new features (description, tags and allowed pattern).
I also fix some problems with the current implementation. If a parameter was manually deleted from the parameter store, that causes problems with terraform plan, apply and destroy. A resource that no longer exists must not be considered as an error.

I also changed the default value for overwrite. Forcing the value to be false by default is not very useful in a terraform project. If we wish to preserve the value, we should use the default lifecycle rules to protect manually changed value. Setting this new parameter to false by default causes breaking change in the already written code since this parameter did not exists before and the values were overwritten by default.

Allow detection of key_id change Fixing problems with ssm-parameter. The resource should not return an error if the parameter no longer exist. If a user manually delete a parameter, Terraform is no longer able to recreate it or destroy it. The defautt value for overwrite should be true. Otherwise, this is causing a breaking change with previously written code where overwrite is not set and the behaviour was to overwrite the parameter by default. Moreover, terraform already provides a mechanism to handle the lifecycle of the values.

pmorton · 2017-08-29T23:27:29Z

@jocgir +1 on reverting the overwrite behavior of overwrite. I was perplexed to find the original implementation reverted. You reasoning is how it was originally implanted. The lifecycle rules you introduced make sense to me.

Out of curiosity, on the allowed pattern, what is the use case for this? Pattern validation seems to be a concern that superceeds the responsibility of the aws provider. My spidy sense is that this feature may hold up many of the other great improvements in his PR.

IHMO if you need to validate variable input, we should have a generic provider that takes a pattern and an input and emits the validated value. That way various strategies can be implemented in a provider agnostic way.

jocgir · 2017-08-30T22:50:02Z

@pmorton, The allowed pattern has been added by aws to enable custom validation when the parameter is edited manually through the console. I just added support to be able to define that parameter when defining parameter store resource.

jocgir · 2017-08-30T22:53:37Z

I noticed a problem when using lifecycle rule to ignore change on the value. The resource is destroyed and replaced. I must investigate this behaviour to check what is the issue.

Please, do not merge until confirmation.

pmorton · 2017-08-31T20:39:33Z

@jocgir BTW I created a pull request that makes the update/overwrite lifecycle more robust. #1556

@pmorton

- 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)

jocgir · 2017-09-26T11:02:26Z

OK, I made several changes to ensure proper lifecycle rules management.

I integrated the idea from @pmorton on #1556 to set the overwrite parameter to false by default on new resource, but I changed the behaviour a bit to let terraform handle the overwrite on already existing resource through it's normal lifecycle rules.

I did not see any reason why changing the encryption key or the type should replace the resource. Parameters values are importants and I think we must try to protect them as much as possible.

jocgir · 2017-11-11T00:54:03Z

Hi @Ninir,

Is it possible to check if that PR could be integrated it the the next release. It has been submitted several week ago. We use it for a while and we did not face any issue.

I just merged in some changes that have been made by my colleague @julienduchesne to avoid conflit since its PR has been merged very quickly by @radeksimko.

We also have some other PR also waiting in the pipeline for a while #1945, #1745.

Regards,

Jo
Coveo Team

…parameters-features

pccowboy · 2018-01-23T21:33:01Z

@Ninir @radeksimko FWIW, we have had this PR integrated into our custom AWS provider for a little over a month now, and it is operating as expected. Any idea when it might make it to a review?

bflad · 2018-02-06T03:48:29Z

Hi @jocgir it looks like aws/resource_aws_ssm_parameter_test.go is conflicting at the moment, can you please resolve that so we can start reviewing this? Thanks and sorry for the delay in getting you a review. 😅

…parameters-features * commit '5293a0e3b1366ee16d8742b9b2354781a79bfbd9': (752 commits) v1.9.0 Update CHANGELOG for hashicorp#1101 and hashicorp#3283 docs/resource/aws_sns_platform_application: Add note about platform_credential and platform_principal hashing resource/aws_sns_platform_application: Refactor ID parsing to its own function, use testing.T in testAccAwsSnsPlatformApplicationPlatformFromEnv Add lambda example (hashicorp#3168) Update CHANGELOG for hashicorp#3157 docs/data-source/aws_region: Remove now deprecated current argument data-source/aws_region: Refactor logic into findRegionByEc2Endpoint and findRegionByName functions Update CHANGELOG for hashicorp#3301 Update CHANGELOG for hashicorp#2559 and hashicorp#3240 Update CHANGELOG.md resource/aws_kinesis_stream: Retry deletion on LimitExceededException (hashicorp#3108) Update CHANGELOG.md resource/aws_dynamodb_table_item: Cleanup + add missing bits Added dynamodb_table_item resource hashicorp#517 Update CHANGELOG.md New Resource: aws_cloud9_environment_ec2 Update CHANGELOG.md Fixed markdown typo in docs resource/aws_kinesis_firehose_delivery_stream: Prevent crashes on empty CloudWatchLoggingOptions and fix extended_s3_configuration kms_key_arn ... # Conflicts: # aws/resource_aws_ssm_parameter_test.go

jocgir · 2018-02-12T22:53:08Z

I fixed the conflict. Thank you for the review.

bashims · 2018-03-02T17:51:27Z

Hi,

Any idea when this PR will be merged? We would love to take advantage of the new features, especially tags!

Thanks,

-ben

bflad

Thanks for the rebase @jocgir! This is looking pretty good we just need to fix up a few things, add some additional error handling, and not mentioned in any comment below add acceptance tests for handling of tags and allowed_pattern

Please let us know if you have any questions or don't have time to cover this feedback. 👍

bflad · 2018-03-02T20:05:08Z

aws/resource_aws_ssm_parameter.go

 		WithDecryption: aws.Bool(true),
+	})
+	if err != nil {
+		return errwrap.Wrapf("[ERROR] Error getting SSM parameter: {{err}}", err)


Minor nitpick: We don't need to use errwrap.Wrapf normally if we are just returning an error message, can simply be return fmt.Errorf("error getting SSM parameter: %s", err)

bflad · 2018-03-02T20:06:11Z

aws/resource_aws_ssm_parameter.go

 	if err != nil {
 		return errwrap.Wrapf("[ERROR] Error describing SSM parameter: {{err}}", err)
 	}

-	if len(resp.Parameters) == 0 {


Is there a reason why this logic was removed? We should force recreation of the parameter if its missing or at least prevent a Terraform crash by ensuring we won't get an index out of bounds error if resp.Parameters has no elements.

bflad · 2018-03-02T20:08:39Z

aws/resource_aws_ssm_parameter.go

+		// Trailing spaces are not considered as a difference
+		*detail.Description = strings.TrimSpace(*detail.Description)
+	}
+	if _, ok := d.GetOk("key_id"); !ok && detail.KeyId != nil && *detail.KeyId == "alias/aws/ssm" {


We should be able to avoid this logic by adding Computed: true to the attribute.

bflad · 2018-03-02T20:14:40Z

aws/resource_aws_ssm_parameter.go

-		log.Printf("[WARN] SSM Param %q not found, removing from state", d.Id())
-		d.SetId("")
-		return nil
+	detail := respDetailed.Parameters[0]


We should ensure there is a check for len(respDetailed.Parameters) == 0 that at least returns an error otherwise it can cause a Terraform crash.

bflad · 2018-03-02T20:18:47Z

aws/resource_aws_ssm_parameter.go

+			"allowed_pattern": {
+				Type:     schema.TypeString,
+				Optional: true,
+				Default:  "",


Minor nitpick: you can remove this, "" is the default for strings.

bflad · 2018-03-02T20:32:58Z

aws/resource_aws_ssm_parameter.go

+	if description, ok := d.GetOk("description"); ok {
+		paramInput.SetDescription(description.(string))
+	} else if d.HasChange("description") {
+		// There is a "bug" in the AWS API and is it not possible to unset a description once


Is this still the case? It sounds like it will cause a perpetual diff if this logic is left as-is (e.g. always showing description "" => " " when its removed until a configuration is re-updated to description = " " instead). What happens if you set description = "" in your configuration?

In either case if the AWS API bug still exists, we should instead force a new resource and document the behavior in the resource documentation.

bflad · 2018-03-02T21:00:18Z

aws/resource_aws_ssm_parameter_test.go

-  type  = "%s"
-  value = "%s"
+  name  = "test_parameter-%[1]s"
+  description  = "description for parameter %[1]s"


Since description is an optional field, we should probably give it its own acceptance test for adding it, updating it, and removing it in configuration. Especially given the API bug mentioned in the code.

julienduchesne · 2018-03-05T16:09:57Z

Updated the PR @bflad

bflad · 2018-03-06T11:08:09Z

aws/resource_aws_ssm_parameter.go

@@ -47,35 +51,47 @@ func resourceAwsSsmParameter() *schema.Resource {
 			"key_id": {
 				Type:     schema.TypeString,
 				Optional: true,
-				ForceNew: true,


Is key_id modifiable now? Can you please update TestAccAWSSSMParameter_secure_with_key to test this?

Yes it works well, I tested it manually but I'll be adding tests of course.

bflad

This is getting really close! A few minor comments then we should be good to go. Thank you! 😄

bflad · 2018-03-06T11:10:45Z

aws/resource_aws_ssm_parameter.go

 	if err != nil {
-		return errwrap.Wrapf("[ERROR] Error describing SSM parameter: {{err}}", err)
+		return errwrap.Wrapf("[ERROR] Error getting SSM parameter: {{err}}", err)


Minor nitpick: It's currently in a lot of the codebase 😢 , but we do not need errwrap for returning simple error strings. fmt.Errorf("error getting SSM parameter: %s", err) is fine here

bflad · 2018-03-06T11:11:18Z

aws/resource_aws_ssm_parameter.go

+			return !lastPage
+		})
+	if err != nil {
+		return errwrap.Wrapf("[ERROR] Error describing SSM parameter: {{err}}", err)


Minor nitpick: same here with fmt.Errorf()

bflad · 2018-03-06T11:15:08Z

aws/resource_aws_ssm_parameter.go

+
+	detail := detailedParameters[0]
+	if detail.Description != nil {
+		// Trailing spaces are not considered as a difference


Does this still apply? If so, looks like we should move this to a DiffSuppressFunc on the attribute to make the behavior more clear

This actually was a hack due to the API bug which is apparently resolved. Removing.

bflad · 2018-03-06T11:23:56Z

aws/resource_aws_ssm_parameter.go

+	}
+
+	if description, ok := d.GetOk("description"); ok {
+		paramInput.SetDescription(description.(string))


We should probably avoid using the Set...() methods of the AWS SDK as they are likely going away in v2 of the SDK: paramInput.Description = description.(string)

That said, this is always writing a value to the description parameter and should be able to match similar code of other resources:

if d.HasChange("description") { _, n := d.GetChange("description") paramInput.Description = aws.String(n.(string)) }

In the case where its removed from the configuration, it should still write "". Just something to consider.

bflad · 2018-03-06T11:25:29Z

aws/resource_aws_ssm_parameter.go

 		return errwrap.Wrapf("[ERROR] Error creating SSM parameter: {{err}}", err)
 	}

+	if err := setTagsSSM(ssmconn, d, d.Get("name").(string), "Parameter"); err != nil {
+		return errwrap.Wrapf("[ERROR] Error creating SSM parameter tags: {{err}}", err)


Minor nitpick: same here with fmt.Errorf()

julienduchesne · 2018-03-06T12:47:23Z

Here you go @bflad. Thanks for the review. Let me know if there's anything else that I should change!

bflad

Thanks for all your work here - let's ship it!

 10 tests passed (all tests)
=== RUN   TestAccAWSSSMParameter_disappears
--- PASS: TestAccAWSSSMParameter_disappears (5.37s)
=== RUN   TestAccAWSSSMParameter_basic
--- PASS: TestAccAWSSSMParameter_basic (7.61s)
=== RUN   TestAccAWSSSMParameter_fullPath
--- PASS: TestAccAWSSSMParameter_fullPath (12.48s)
=== RUN   TestAccAWSSSMParameter_changeNameForcesNew
--- PASS: TestAccAWSSSMParameter_changeNameForcesNew (14.01s)
=== RUN   TestAccAWSSSMParameter_update
--- PASS: TestAccAWSSSMParameter_update (17.56s)
=== RUN   TestAccAWSSSMParameter_secure
--- PASS: TestAccAWSSSMParameter_secure (18.02s)
=== RUN   TestAccAWSSSMParameter_updateDescription
--- PASS: TestAccAWSSSMParameter_updateDescription (20.23s)
=== RUN   TestAccAWSSSMParameter_importBasic
--- PASS: TestAccAWSSSMParameter_importBasic (20.46s)
=== RUN   TestAccAWSSSMParameter_secure_keyUpdate
--- PASS: TestAccAWSSSMParameter_secure_keyUpdate (39.64s)
=== RUN   TestAccAWSSSMParameter_secure_with_key
--- PASS: TestAccAWSSSMParameter_secure_with_key (42.49s)

bflad · 2018-03-06T14:09:08Z

aws/resource_aws_ssm_parameter_test.go

+  description  = "description for parameter %[1]s"
+  type  = "SecureString"
+  value = "%[2]s"
+  key_id = "alias/%[3]s"


Minor nitpick: ${aws_kms_alias.test_alias.name} is a little easier to understand here instead of depends_on 😉

bflad · 2018-03-09T19:56:21Z

This has been released in version 1.11.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2020-04-07T17:39:54Z

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!

Ninir added the enhancement Requests to existing resources that expand the functionality or scope. label Aug 28, 2017

Merge branch 'master' into new-ssm-parameters-features

eba16bc

matt40k mentioned this pull request Oct 17, 2017

Feature Request - description and tags fields in aws_ssm_parameter #1584

Closed

julienduchesne force-pushed the new-ssm-parameters-features branch 2 times, most recently from c1b2a77 to a80788f Compare November 10, 2017 13:50

jocgir force-pushed the new-ssm-parameters-features branch from 76b16fa to eba16bc Compare November 13, 2017 19:07

Include the import for ssm_parameter as suggested by jduchesne

6da6a49

jocgir force-pushed the new-ssm-parameters-features branch from 8ac7cb9 to 6da6a49 Compare November 13, 2017 19:46

Merge branch 'upstream' into new-ssm-parameters-features

ca0b6dd

jocgir force-pushed the new-ssm-parameters-features branch from 7aa47ee to ca0b6dd Compare November 13, 2017 19:53

radeksimko added the size/L Managed by automation to categorize the size of a PR. label Nov 15, 2017

Merge commit '009addb94e8c2829b71d4848d60db7898c5af8ef' into new-ssm-…

36d0346

…parameters-features

bflad added the service/ssm Issues and PRs that pertain to the ssm service. label Jan 24, 2018

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Feb 6, 2018

pccowboy mentioned this pull request Feb 9, 2018

Integration/1.8.0 ValiMail/terraform-provider-aws#1

Closed

ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 12, 2018

bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Feb 13, 2018

julienduchesne deleted the new-ssm-parameters-features branch February 26, 2018 11:53

julienduchesne restored the new-ssm-parameters-features branch February 26, 2018 12:00

bflad reviewed Mar 2, 2018

View reviewed changes

Corrected issues with SSM features PR

85289a4

ghost added the size/L Managed by automation to categorize the size of a PR. label Mar 5, 2018

bflad reviewed Mar 6, 2018

View reviewed changes

bflad added this to the v1.11.0 milestone Mar 6, 2018

Corrected further issues with SSM features PR + added kms test

bd9088e

ghost added size/XL Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. labels Mar 6, 2018

bflad approved these changes Mar 6, 2018

View reviewed changes

bflad merged commit 774cb1b into hashicorp:master Mar 6, 2018

bflad added a commit that referenced this pull request Mar 6, 2018

Update CHANGELOG for #1520

55c2ba4

julienduchesne deleted the new-ssm-parameters-features branch March 6, 2018 14:24

bflad mentioned this pull request Mar 15, 2018

aws_ssm_parameter: add "description" #3120

Closed

smphhh mentioned this pull request Mar 23, 2018

Not possible to allow SSM Parameter overwrites only for updates (and not creates) #1481

Closed

This was referenced Apr 26, 2018

Allow parameters to follow standard lifecycle rules #1556

Closed

Changing aws_ssm_parameter.overwrite trigger a resource update #3181

Closed

ghost locked and limited conversation to collaborators Apr 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New ssm parameters features #1520

New ssm parameters features #1520

jocgir commented Aug 28, 2017

pmorton commented Aug 29, 2017

jocgir commented Aug 30, 2017

jocgir commented Aug 30, 2017

pmorton commented Aug 31, 2017

jocgir commented Sep 26, 2017 •

edited

Loading

jocgir commented Nov 11, 2017

pccowboy commented Jan 23, 2018

bflad commented Feb 6, 2018

jocgir commented Feb 12, 2018

bashims commented Mar 2, 2018

bflad left a comment

bflad Mar 2, 2018

bflad Mar 2, 2018

bflad Mar 2, 2018

bflad Mar 2, 2018

bflad Mar 2, 2018

bflad Mar 2, 2018

bflad Mar 2, 2018

julienduchesne commented Mar 5, 2018

bflad Mar 6, 2018

julienduchesne Mar 6, 2018

bflad left a comment

bflad Mar 6, 2018

bflad Mar 6, 2018

bflad Mar 6, 2018

julienduchesne Mar 6, 2018

bflad Mar 6, 2018

bflad Mar 6, 2018

julienduchesne commented Mar 6, 2018

bflad left a comment

bflad Mar 6, 2018

bflad commented Mar 9, 2018

ghost commented Apr 7, 2020

New ssm parameters features #1520

New ssm parameters features #1520

Conversation

jocgir commented Aug 28, 2017

pmorton commented Aug 29, 2017

jocgir commented Aug 30, 2017

jocgir commented Aug 30, 2017

pmorton commented Aug 31, 2017

jocgir commented Sep 26, 2017 • edited Loading

jocgir commented Nov 11, 2017

pccowboy commented Jan 23, 2018

bflad commented Feb 6, 2018

jocgir commented Feb 12, 2018

bashims commented Mar 2, 2018

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienduchesne commented Mar 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienduchesne commented Mar 6, 2018

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad commented Mar 9, 2018

ghost commented Apr 7, 2020

jocgir commented Sep 26, 2017 •

edited

Loading