Don't send empty kms_arn in glue_security_configuration if mode is DISABLED #13618

AndresCidoncha · 2020-06-04T20:22:34Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Release note for CHANGELOG:

NONE

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSGlueSecurityConfiguration_Basic'

...

* If logs and/or bookmark encryption are DISABLED, don't send these fields empty to the API call

DrFaust92 · 2020-09-23T21:02:31Z

Hey @AndresCidoncha, can you also add an acceptance test to verify the use case?

AndresCidoncha · 2020-09-24T17:56:27Z

Hey @AndresCidoncha, can you also add an acceptance test to verify the use case?

Hey @DrFaust92 , I wanted to do a new test but I found a problem: The empty configuration is already tested in TestAccAWSGlueSecurityConfiguration_Basic but the TestCheckResourceAttr method only accepts a string for the comparation value and it's currently using an empty string, so I can't check if the assigned value is nil instead of a empty string.

There's a way to validate the fields sent in the request to AWS?

DrFaust92

Minor comment, lets just run the basic test and ill verify all other. Looks good to me.

DrFaust92 · 2020-10-16T18:07:05Z

aws/resource_aws_glue_security_configuration.go

@@ -214,7 +214,9 @@ func expandGlueCloudWatchEncryption(l []interface{}) *glue.CloudWatchEncryption
 	}

 	if v, ok := m["kms_key_arn"]; ok {


lets remove the extra if by doing this:

if v, ok := m["kms_key_arn"]; ok && v.(string) != "" {

DrFaust92 · 2020-10-16T18:07:15Z

aws/resource_aws_glue_security_configuration.go

@@ -248,7 +250,9 @@ func expandGlueJobBookmarksEncryption(l []interface{}) *glue.JobBookmarksEncrypt
 	}

 	if v, ok := m["kms_key_arn"]; ok {


same as above

DrFaust92

LGTM

--- PASS: TestAccAWSGlueSecurityConfiguration_S3Encryption_S3EncryptionMode_SSES3 (46.12s)
--- PASS: TestAccAWSGlueSecurityConfiguration_Basic (46.38s)
--- PASS: TestAccAWSGlueSecurityConfiguration_CloudWatchEncryption_CloudWatchEncryptionMode_SSEKMS (61.96s)
--- PASS: TestAccAWSGlueSecurityConfiguration_JobBookmarksEncryption_JobBookmarksEncryptionMode_CSEKMS (61.98s)
--- PASS: TestAccAWSGlueSecurityConfiguration_S3Encryption_S3EncryptionMode_SSEKMS (63.91s)

breathingdust · 2020-10-21T16:20:42Z

LGTM 🚀 Thanks @AndresCidoncha!

Verified Acceptance Tests in Commercial (us-west-2)

make testacc TEST=./aws TESTARGS='-run=TestAccAWSGlueSecurityConfiguration_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSGlueSecurityConfiguration_ -timeout 120m
=== RUN   TestAccAWSGlueSecurityConfiguration_Basic
=== PAUSE TestAccAWSGlueSecurityConfiguration_Basic
=== RUN   TestAccAWSGlueSecurityConfiguration_CloudWatchEncryption_CloudWatchEncryptionMode_SSEKMS
=== PAUSE TestAccAWSGlueSecurityConfiguration_CloudWatchEncryption_CloudWatchEncryptionMode_SSEKMS
=== RUN   TestAccAWSGlueSecurityConfiguration_JobBookmarksEncryption_JobBookmarksEncryptionMode_CSEKMS
=== PAUSE TestAccAWSGlueSecurityConfiguration_JobBookmarksEncryption_JobBookmarksEncryptionMode_CSEKMS
=== RUN   TestAccAWSGlueSecurityConfiguration_S3Encryption_S3EncryptionMode_SSEKMS
=== PAUSE TestAccAWSGlueSecurityConfiguration_S3Encryption_S3EncryptionMode_SSEKMS
=== RUN   TestAccAWSGlueSecurityConfiguration_S3Encryption_S3EncryptionMode_SSES3
=== PAUSE TestAccAWSGlueSecurityConfiguration_S3Encryption_S3EncryptionMode_SSES3
=== CONT  TestAccAWSGlueSecurityConfiguration_Basic
=== CONT  TestAccAWSGlueSecurityConfiguration_S3Encryption_S3EncryptionMode_SSEKMS
=== CONT  TestAccAWSGlueSecurityConfiguration_S3Encryption_S3EncryptionMode_SSES3
=== CONT  TestAccAWSGlueSecurityConfiguration_JobBookmarksEncryption_JobBookmarksEncryptionMode_CSEKMS
=== CONT  TestAccAWSGlueSecurityConfiguration_CloudWatchEncryption_CloudWatchEncryptionMode_SSEKMS
--- PASS: TestAccAWSGlueSecurityConfiguration_S3Encryption_S3EncryptionMode_SSES3 (18.15s)
--- PASS: TestAccAWSGlueSecurityConfiguration_Basic (18.20s)
--- PASS: TestAccAWSGlueSecurityConfiguration_S3Encryption_S3EncryptionMode_SSEKMS (19.85s)
--- PASS: TestAccAWSGlueSecurityConfiguration_CloudWatchEncryption_CloudWatchEncryptionMode_SSEKMS (20.19s)
--- PASS: TestAccAWSGlueSecurityConfiguration_JobBookmarksEncryption_JobBookmarksEncryptionMode_CSEKMS (20.21s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	21.582s

Verified Acceptance Tests in GovCloud (us-gov-west-1)

make testacc TEST=./aws TESTARGS='-run=TestAccAWSGlueSecurityConfiguration_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSGlueSecurityConfiguration_ -timeout 120m
=== RUN   TestAccAWSGlueSecurityConfiguration_Basic
=== PAUSE TestAccAWSGlueSecurityConfiguration_Basic
=== RUN   TestAccAWSGlueSecurityConfiguration_CloudWatchEncryption_CloudWatchEncryptionMode_SSEKMS
=== PAUSE TestAccAWSGlueSecurityConfiguration_CloudWatchEncryption_CloudWatchEncryptionMode_SSEKMS
=== RUN   TestAccAWSGlueSecurityConfiguration_JobBookmarksEncryption_JobBookmarksEncryptionMode_CSEKMS
=== PAUSE TestAccAWSGlueSecurityConfiguration_JobBookmarksEncryption_JobBookmarksEncryptionMode_CSEKMS
=== RUN   TestAccAWSGlueSecurityConfiguration_S3Encryption_S3EncryptionMode_SSEKMS
=== PAUSE TestAccAWSGlueSecurityConfiguration_S3Encryption_S3EncryptionMode_SSEKMS
=== RUN   TestAccAWSGlueSecurityConfiguration_S3Encryption_S3EncryptionMode_SSES3
=== PAUSE TestAccAWSGlueSecurityConfiguration_S3Encryption_S3EncryptionMode_SSES3
=== CONT  TestAccAWSGlueSecurityConfiguration_Basic
=== CONT  TestAccAWSGlueSecurityConfiguration_S3Encryption_S3EncryptionMode_SSEKMS
=== CONT  TestAccAWSGlueSecurityConfiguration_S3Encryption_S3EncryptionMode_SSES3
=== CONT  TestAccAWSGlueSecurityConfiguration_CloudWatchEncryption_CloudWatchEncryptionMode_SSEKMS
=== CONT  TestAccAWSGlueSecurityConfiguration_JobBookmarksEncryption_JobBookmarksEncryptionMode_CSEKMS
--- PASS: TestAccAWSGlueSecurityConfiguration_Basic (13.44s)
--- PASS: TestAccAWSGlueSecurityConfiguration_S3Encryption_S3EncryptionMode_SSES3 (13.49s)
--- PASS: TestAccAWSGlueSecurityConfiguration_JobBookmarksEncryption_JobBookmarksEncryptionMode_CSEKMS (15.47s)
--- PASS: TestAccAWSGlueSecurityConfiguration_S3Encryption_S3EncryptionMode_SSEKMS (15.68s)
--- PASS: TestAccAWSGlueSecurityConfiguration_CloudWatchEncryption_CloudWatchEncryptionMode_SSEKMS (15.80s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	17.126s

ghost · 2020-10-22T21:37:40Z

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

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

ghost · 2020-11-20T17:10:56Z

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!

Avoid empty kms_arn

b0d6dba

* If logs and/or bookmark encryption are DISABLED, don't send these fields empty to the API call

AndresCidoncha requested a review from a team June 4, 2020 20:22

ghost added size/XS Managed by automation to categorize the size of a PR. service/glue Issues and PRs that pertain to the glue service. needs-triage Waiting for first response or review from a maintainer. labels Jun 4, 2020

AndresCidoncha changed the title ~~Don't send empty kms_arn in glue_securityconfiguration if mode isDISABLED~~ Don't send empty kms_arn in glue_securityconfiguration if mode is DISABLED Jun 4, 2020

AndresCidoncha changed the title ~~Don't send empty kms_arn in glue_securityconfiguration if mode is DISABLED~~ Don't send empty kms_arn in glue_security_configuration if mode is DISABLED Jun 4, 2020

AndresCidoncha mentioned this pull request Jun 4, 2020

aws_glue_security_configuration send empty kms_arn in DISABLED configs #13620

Closed

anGie44 force-pushed the master branch from 54db70d to 48ba2f7 Compare July 21, 2020 18:26

DrFaust92 added bug Addresses a defect in current functionality. and removed needs-triage Waiting for first response or review from a maintainer. labels Aug 29, 2020

DrFaust92 suggested changes Oct 16, 2020

View reviewed changes

Remove extra if

7a21594

DrFaust92 self-assigned this Oct 20, 2020

DrFaust92 approved these changes Oct 20, 2020

View reviewed changes

breathingdust added this to the v3.12.0 milestone Oct 21, 2020

breathingdust merged commit dbb0115 into hashicorp:master Oct 21, 2020

breathingdust added a commit that referenced this pull request Oct 21, 2020

Update CHANGELOG.md for #13618

1b570e6

hashicorp locked as resolved and limited conversation to collaborators Nov 20, 2020

DrFaust92 removed their assignment Jun 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't send empty kms_arn in glue_security_configuration if mode is DISABLED #13618

Don't send empty kms_arn in glue_security_configuration if mode is DISABLED #13618

AndresCidoncha commented Jun 4, 2020 •

edited

DrFaust92 commented Sep 23, 2020

AndresCidoncha commented Sep 24, 2020

DrFaust92 left a comment

DrFaust92 Oct 16, 2020

AndresCidoncha Oct 19, 2020

DrFaust92 Oct 16, 2020

AndresCidoncha Oct 19, 2020

DrFaust92 left a comment

breathingdust commented Oct 21, 2020

ghost commented Oct 22, 2020

ghost commented Nov 20, 2020

		@@ -214,7 +214,9 @@ func expandGlueCloudWatchEncryption(l []interface{}) *glue.CloudWatchEncryption
		}

		if v, ok := m["kms_key_arn"]; ok {

		@@ -248,7 +250,9 @@ func expandGlueJobBookmarksEncryption(l []interface{}) *glue.JobBookmarksEncrypt
		}

		if v, ok := m["kms_key_arn"]; ok {

Don't send empty kms_arn in glue_security_configuration if mode is DISABLED #13618

Don't send empty kms_arn in glue_security_configuration if mode is DISABLED #13618

Conversation

AndresCidoncha commented Jun 4, 2020 • edited

Community Note

DrFaust92 commented Sep 23, 2020

AndresCidoncha commented Sep 24, 2020

DrFaust92 left a comment

Choose a reason for hiding this comment

DrFaust92 Oct 16, 2020

Choose a reason for hiding this comment

AndresCidoncha Oct 19, 2020

Choose a reason for hiding this comment

DrFaust92 Oct 16, 2020

Choose a reason for hiding this comment

AndresCidoncha Oct 19, 2020

Choose a reason for hiding this comment

DrFaust92 left a comment

Choose a reason for hiding this comment

breathingdust commented Oct 21, 2020

ghost commented Oct 22, 2020

ghost commented Nov 20, 2020

AndresCidoncha commented Jun 4, 2020 •

edited