internal/verify: `ValidIAMPolicyJSON` helpful error messages #27502

apparentlymart · 2022-10-27T00:55:14Z

Description

In community forums I have seen various different people unable to determine what they've done wrong when making the following three apparently-common mistakes:

    # jsonencode(...) instead of file(...)
    policy = jsonencode(var.policy_filename)

    # jsonencode(...) on a string that already contains JSON,
    # rather than just the string content directly.
    policy = jsonencode(var.policy_json)

    # jsonencode(...) on the content of a file containing JSON,
    # rather than just the file content directly.
    policy = jsonencode(file(var.policy_filename))

My main motivation is to offer some more actionable feedback hints for those specific situations, but while here I also made some other additions to the different error messages to try to give some more detail about what's wrong. Being more specific about what has gone wrong (or what seems to have gone wrong) will hopefully allow more authors to self-diagnose their own mistake, rather than having to ask for help in a community forum.

The test for this function now tests the exact error message text rather than just whether there is an error at all, because returning helpful error messages is an important part of a validation function's behavior.

Output from Acceptance Testing

This test does not change anything covered by acceptance tests, but the following is the output from running the unit tests for this function:

$ go test -v ./internal/verify -run '^TestValidIAMPolicyJSONString'
=== RUN   TestValidIAMPolicyJSONString
=== RUN   TestValidIAMPolicyJSONString/{}
=== RUN   TestValidIAMPolicyJSONString/{"abc":["1","2"]}
=== RUN   TestValidIAMPolicyJSONString/{0:"1"}
=== RUN   TestValidIAMPolicyJSONString/{'abc':1}
=== RUN   TestValidIAMPolicyJSONString/{"def":}
=== RUN   TestValidIAMPolicyJSONString/{"xyz":[}}
=== RUN   TestValidIAMPolicyJSONString/#00
=== RUN   TestValidIAMPolicyJSONString/____{"xyz":_"foo"}
=== RUN   TestValidIAMPolicyJSONString/"blub"
=== RUN   TestValidIAMPolicyJSONString/"../some-filename.json"
=== RUN   TestValidIAMPolicyJSONString/"{\"Version\":\"...\"}"
=== RUN   TestValidIAMPolicyJSONString/[{}]
--- PASS: TestValidIAMPolicyJSONString (0.00s)
    --- PASS: TestValidIAMPolicyJSONString/{} (0.00s)
    --- PASS: TestValidIAMPolicyJSONString/{"abc":["1","2"]} (0.00s)
    --- PASS: TestValidIAMPolicyJSONString/{0:"1"} (0.00s)
    --- PASS: TestValidIAMPolicyJSONString/{'abc':1} (0.00s)
    --- PASS: TestValidIAMPolicyJSONString/{"def":} (0.00s)
    --- PASS: TestValidIAMPolicyJSONString/{"xyz":[}} (0.00s)
    --- PASS: TestValidIAMPolicyJSONString/#00 (0.00s)
    --- PASS: TestValidIAMPolicyJSONString/____{"xyz":_"foo"} (0.00s)
    --- PASS: TestValidIAMPolicyJSONString/"blub" (0.00s)
    --- PASS: TestValidIAMPolicyJSONString/"../some-filename.json" (0.00s)
    --- PASS: TestValidIAMPolicyJSONString/"{\"Version\":\"...\"}" (0.00s)
    --- PASS: TestValidIAMPolicyJSONString/[{}] (0.00s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/verify

github-actions · 2022-10-27T00:55:27Z

Community Note

Voting for Prioritization

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

For Submitters

Review the contribution guide relating to the type of change you are making to ensure all of the necessary steps have been taken.
For new resources and data sources, use skaff to generate scaffolding with comments detailing common expectations.
Whether or not the branch has been rebased will not impact prioritization, but doing so is always a welcome surprise.

In community forums I have seen many different people unable to determine what they've done wrong when making the following two apparently-common mistakes: # jsonencode(...) instead of file(...) policy = jsonencode(var.policy_filename) # jsonencode(...) on a string that already contains JSON, # rather than just the string content directly. policy = jsonencode(var.policy_json) # jsonencode(...) on the content of a file containing JSON, # rather than just the file content directly. policy = jsonencode(file(var.policy_filename)) My main motivation is to offer some more actionable feedback hints for those specific situations, but while here I also made some other additions to the different error messages to try to give some more detail about what's wrong. Being more specific about what has gone wrong (or what _seems_ to have gone wrong) will hopefully allow more authors to self-diagnose their own mistake, rather than having to ask for help in a community forum. The test for this function now tests the exact error message text rather than just whether there is an error at all, because returning helpful error messages is an important part of a validation function's behavior.

ewbankkit

LGTM 🚀.

% go test -v ./internal/verify 
=== RUN   TestBase64Encode
=== PAUSE TestBase64Encode
=== RUN   TestCIDRBlocksEqual
=== PAUSE TestCIDRBlocksEqual
=== RUN   TestCanonicalCIDRBlock
=== PAUSE TestCanonicalCIDRBlock
=== RUN   TestSuppressEquivalentRoundedTime
=== PAUSE TestSuppressEquivalentRoundedTime
=== RUN   TestDiffStringMaps
=== PAUSE TestDiffStringMaps
=== RUN   TestLooksLikeJSONString
=== PAUSE TestLooksLikeJSONString
=== RUN   TestJSONBytesEqualQuotedAndUnquoted
=== PAUSE TestJSONBytesEqualQuotedAndUnquoted
=== RUN   TestJSONBytesEqualWhitespaceAndNoWhitespace
=== PAUSE TestJSONBytesEqualWhitespaceAndNoWhitespace
=== RUN   TestSecondJSONUnlessEquivalent
=== PAUSE TestSecondJSONUnlessEquivalent
=== RUN   TestNormalizeJSONOrYAMLString
=== PAUSE TestNormalizeJSONOrYAMLString
=== RUN   TestSuppressEquivalentJSONDiffsWhitespaceAndNoWhitespace
=== PAUSE TestSuppressEquivalentJSONDiffsWhitespaceAndNoWhitespace
=== RUN   TestSuppressEquivalentJSONOrYAMLDiffs
=== PAUSE TestSuppressEquivalentJSONOrYAMLDiffs
=== RUN   TestLegacyPolicyNormalize
=== PAUSE TestLegacyPolicyNormalize
=== RUN   TestSemVerLessThan
=== PAUSE TestSemVerLessThan
=== RUN   TestSemVerGreaterThanOrEqual
=== PAUSE TestSemVerGreaterThanOrEqual
=== RUN   TestValidAmazonSideASN
=== PAUSE TestValidAmazonSideASN
=== RUN   TestValid4ByteASNString
=== PAUSE TestValid4ByteASNString
=== RUN   TestValidTypeStringNullableFloat
=== PAUSE TestValidTypeStringNullableFloat
=== RUN   TestValidAccountID
=== PAUSE TestValidAccountID
=== RUN   TestValidARN
=== PAUSE TestValidARN
=== RUN   TestValidateCIDRBlock
=== PAUSE TestValidateCIDRBlock
=== RUN   TestValidCIDRNetworkAddress
=== PAUSE TestValidCIDRNetworkAddress
=== RUN   TestValidIPv4CIDRBlock
=== PAUSE TestValidIPv4CIDRBlock
=== RUN   TestValidIPv6CIDRBlock
=== PAUSE TestValidIPv6CIDRBlock
=== RUN   TestIsIPv4CIDRBlockOrIPv6CIDRBlock
=== PAUSE TestIsIPv4CIDRBlockOrIPv6CIDRBlock
=== RUN   TestValidIAMPolicyJSONString
=== PAUSE TestValidIAMPolicyJSONString
=== RUN   TestValidStringIsJSONOrYAML
=== PAUSE TestValidStringIsJSONOrYAML
=== RUN   TestValidOnceAWeekWindowFormat
=== PAUSE TestValidOnceAWeekWindowFormat
=== RUN   TestValidOnceADayWindowFormat
=== PAUSE TestValidOnceADayWindowFormat
=== RUN   TestValidLaunchTemplateName
=== PAUSE TestValidLaunchTemplateName
=== RUN   TestValidLaunchTemplateID
=== PAUSE TestValidLaunchTemplateID
=== RUN   TestValidUTCTimestamp
=== PAUSE TestValidUTCTimestamp
=== RUN   TestValidateTypeStringIsDateOrInt
=== PAUSE TestValidateTypeStringIsDateOrInt
=== RUN   TestFloatGreaterThan
=== PAUSE TestFloatGreaterThan
=== RUN   TestCheckYAMLString
=== PAUSE TestCheckYAMLString
=== CONT  TestBase64Encode
=== CONT  TestValidAccountID
--- PASS: TestBase64Encode (0.00s)
=== CONT  TestValidTypeStringNullableFloat
=== CONT  TestValid4ByteASNString
--- PASS: TestValid4ByteASNString (0.00s)
=== CONT  TestValidAmazonSideASN
=== CONT  TestSuppressEquivalentJSONDiffsWhitespaceAndNoWhitespace
--- PASS: TestValidTypeStringNullableFloat (0.00s)
=== CONT  TestNormalizeJSONOrYAMLString
--- PASS: TestValidAccountID (0.00s)
=== CONT  TestJSONBytesEqualWhitespaceAndNoWhitespace
--- PASS: TestValidAmazonSideASN (0.00s)
=== CONT  TestSecondJSONUnlessEquivalent
--- PASS: TestJSONBytesEqualWhitespaceAndNoWhitespace (0.00s)
=== CONT  TestJSONBytesEqualQuotedAndUnquoted
=== CONT  TestSemVerGreaterThanOrEqual
=== CONT  TestLooksLikeJSONString
=== CONT  TestDiffStringMaps
=== CONT  TestSemVerLessThan
=== CONT  TestSuppressEquivalentRoundedTime
=== CONT  TestLegacyPolicyNormalize
=== CONT  TestValidIPv6CIDRBlock
=== CONT  TestSuppressEquivalentJSONOrYAMLDiffs
=== RUN   TestLegacyPolicyNormalize/basic
=== PAUSE TestLegacyPolicyNormalize/basic
--- PASS: TestSuppressEquivalentJSONDiffsWhitespaceAndNoWhitespace (0.00s)
=== RUN   TestLegacyPolicyNormalize/normalWhitespace
=== PAUSE TestLegacyPolicyNormalize/normalWhitespace
--- PASS: TestJSONBytesEqualQuotedAndUnquoted (0.00s)
=== RUN   TestLegacyPolicyNormalize/badJSON
--- PASS: TestLooksLikeJSONString (0.00s)
--- PASS: TestSuppressEquivalentRoundedTime (0.00s)
--- PASS: TestDiffStringMaps (0.00s)
=== CONT  TestCanonicalCIDRBlock
=== PAUSE TestLegacyPolicyNormalize/badJSON
=== RUN   TestLegacyPolicyNormalize/principal
=== CONT  TestValidCIDRNetworkAddress
=== CONT  TestValidStringIsJSONOrYAML
=== PAUSE TestLegacyPolicyNormalize/principal
=== RUN   TestLegacyPolicyNormalize/id
=== CONT  TestIsIPv4CIDRBlockOrIPv6CIDRBlock
=== PAUSE TestLegacyPolicyNormalize/id
=== CONT  TestValidIAMPolicyJSONString
=== RUN   TestLegacyPolicyNormalize/newOrder
=== PAUSE TestLegacyPolicyNormalize/newOrder
=== RUN   TestLegacyPolicyNormalize/complex1
=== PAUSE TestLegacyPolicyNormalize/complex1
=== RUN   TestLegacyPolicyNormalize/complex2
=== RUN   TestValidIAMPolicyJSONString/{}
=== PAUSE TestLegacyPolicyNormalize/complex2
=== CONT  TestValidIPv4CIDRBlock
=== CONT  TestValidUTCTimestamp
=== CONT  TestFloatGreaterThan
=== CONT  TestCheckYAMLString
=== CONT  TestValidateTypeStringIsDateOrInt
=== CONT  TestValidateCIDRBlock
=== CONT  TestValidARN
=== CONT  TestValidOnceAWeekWindowFormat
=== CONT  TestValidOnceADayWindowFormat
=== CONT  TestCIDRBlocksEqual
=== CONT  TestValidLaunchTemplateID
=== CONT  TestValidLaunchTemplateName
=== CONT  TestLegacyPolicyNormalize/basic
--- PASS: TestNormalizeJSONOrYAMLString (0.00s)
--- PASS: TestSemVerGreaterThanOrEqual (0.00s)
--- PASS: TestSuppressEquivalentJSONOrYAMLDiffs (0.00s)
=== CONT  TestLegacyPolicyNormalize/newOrder
--- PASS: TestValidIPv6CIDRBlock (0.00s)
--- PASS: TestCanonicalCIDRBlock (0.00s)
--- PASS: TestValidCIDRNetworkAddress (0.00s)
--- PASS: TestValidStringIsJSONOrYAML (0.00s)
=== RUN   TestValidIAMPolicyJSONString/{"abc":["1","2"]}
--- PASS: TestValidOnceADayWindowFormat (0.00s)
=== CONT  TestLegacyPolicyNormalize/id
=== CONT  TestLegacyPolicyNormalize/badJSON
=== CONT  TestLegacyPolicyNormalize/principal
=== CONT  TestLegacyPolicyNormalize/normalWhitespace
--- PASS: TestIsIPv4CIDRBlockOrIPv6CIDRBlock (0.00s)
--- PASS: TestFloatGreaterThan (0.00s)
--- PASS: TestValidUTCTimestamp (0.00s)
--- PASS: TestValidateCIDRBlock (0.00s)
=== CONT  TestLegacyPolicyNormalize/complex2
--- PASS: TestCheckYAMLString (0.00s)
--- PASS: TestCIDRBlocksEqual (0.00s)
=== CONT  TestLegacyPolicyNormalize/complex1
--- PASS: TestValidARN (0.00s)
--- PASS: TestValidLaunchTemplateID (0.00s)
--- PASS: TestSemVerLessThan (0.00s)
--- PASS: TestValidLaunchTemplateName (0.00s)
--- PASS: TestValidIPv4CIDRBlock (0.00s)
--- PASS: TestValidateTypeStringIsDateOrInt (0.00s)
=== RUN   TestValidIAMPolicyJSONString/{0:"1"}
=== RUN   TestValidIAMPolicyJSONString/{'abc':1}
--- PASS: TestSecondJSONUnlessEquivalent (0.00s)
=== RUN   TestValidIAMPolicyJSONString/{"def":}
=== RUN   TestValidIAMPolicyJSONString/{"xyz":[}}
=== RUN   TestValidIAMPolicyJSONString/#00
--- PASS: TestLegacyPolicyNormalize (0.00s)
    --- PASS: TestLegacyPolicyNormalize/basic (0.00s)
    --- PASS: TestLegacyPolicyNormalize/badJSON (0.00s)
    --- PASS: TestLegacyPolicyNormalize/id (0.00s)
    --- PASS: TestLegacyPolicyNormalize/newOrder (0.00s)
    --- PASS: TestLegacyPolicyNormalize/principal (0.00s)
    --- PASS: TestLegacyPolicyNormalize/normalWhitespace (0.00s)
    --- PASS: TestLegacyPolicyNormalize/complex2 (0.00s)
    --- PASS: TestLegacyPolicyNormalize/complex1 (0.00s)
=== RUN   TestValidIAMPolicyJSONString/____{"xyz":_"foo"}
=== RUN   TestValidIAMPolicyJSONString/"blub"
=== RUN   TestValidIAMPolicyJSONString/"../some-filename.json"
=== RUN   TestValidIAMPolicyJSONString/"{\"Version\":\"...\"}"
--- PASS: TestValidOnceAWeekWindowFormat (0.00s)
=== RUN   TestValidIAMPolicyJSONString/[{}]
--- PASS: TestValidIAMPolicyJSONString (0.00s)
    --- PASS: TestValidIAMPolicyJSONString/{} (0.00s)
    --- PASS: TestValidIAMPolicyJSONString/{"abc":["1","2"]} (0.00s)
    --- PASS: TestValidIAMPolicyJSONString/{0:"1"} (0.00s)
    --- PASS: TestValidIAMPolicyJSONString/{'abc':1} (0.00s)
    --- PASS: TestValidIAMPolicyJSONString/{"def":} (0.00s)
    --- PASS: TestValidIAMPolicyJSONString/{"xyz":[}} (0.00s)
    --- PASS: TestValidIAMPolicyJSONString/#00 (0.00s)
    --- PASS: TestValidIAMPolicyJSONString/____{"xyz":_"foo"} (0.00s)
    --- PASS: TestValidIAMPolicyJSONString/"blub" (0.00s)
    --- PASS: TestValidIAMPolicyJSONString/"../some-filename.json" (0.00s)
    --- PASS: TestValidIAMPolicyJSONString/"{\"Version\":\"...\"}" (0.00s)
    --- PASS: TestValidIAMPolicyJSONString/[{}] (0.00s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/verify	0.741s

% make testacc TESTARGS='-run=TestAccIAMPolicy_' PKG=iam ACCTEST_PARALLELISM=2
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/iam/... -v -count 1 -parallel 2  -run=TestAccIAMPolicy_ -timeout 180m
=== RUN   TestAccIAMPolicy_basic
=== PAUSE TestAccIAMPolicy_basic
=== RUN   TestAccIAMPolicy_description
=== PAUSE TestAccIAMPolicy_description
=== RUN   TestAccIAMPolicy_tags
=== PAUSE TestAccIAMPolicy_tags
=== 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
=== CONT  TestAccIAMPolicy_basic
=== CONT  TestAccIAMPolicy_diffs
--- PASS: TestAccIAMPolicy_basic (17.38s)
=== CONT  TestAccIAMPolicy_policy
--- PASS: TestAccIAMPolicy_policy (26.65s)
=== CONT  TestAccIAMPolicy_path
--- PASS: TestAccIAMPolicy_path (14.43s)
=== CONT  TestAccIAMPolicy_namePrefix
--- PASS: TestAccIAMPolicy_namePrefix (14.62s)
=== CONT  TestAccIAMPolicy_disappears
--- PASS: TestAccIAMPolicy_disappears (11.15s)
=== CONT  TestAccIAMPolicy_tags
--- PASS: TestAccIAMPolicy_diffs (105.65s)
=== CONT  TestAccIAMPolicy_description
--- PASS: TestAccIAMPolicy_tags (35.59s)
--- PASS: TestAccIAMPolicy_description (15.10s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/iam	126.995s

ewbankkit · 2023-04-25T20:29:14Z

@apparentlymart Thanks for the contribution 🎉 👏.

github-actions · 2023-04-28T00:34:13Z

This functionality has been released in v4.65.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!

github-actions · 2023-05-28T02:17:06Z

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

apparentlymart force-pushed the f-iam-policy-json-validate-messages branch from c51b693 to 19d18cf Compare October 27, 2022 00:56

ewbankkit removed the needs-triage Waiting for first response or review from a maintainer. label Oct 27, 2022

ewbankkit added 2 commits April 25, 2023 16:07

Merge branch 'main' into HEAD

25c1709

Fix golangci-lint 'nakedret'.

46c3091

ewbankkit approved these changes Apr 25, 2023

View reviewed changes

ewbankkit added 3 commits April 25, 2023 17:07

Fix golangci-lint 'paralleltest' and 'tparallel'.

71af2b8

Fix golangci-lint 'errorlint'.

6631fc6

Fix golangci-lint 'nakedret'.

1e27de9

ewbankkit merged commit 40eddaf into hashicorp:main Apr 25, 2023

github-actions bot added this to the v4.65.0 milestone Apr 25, 2023

github-actions bot locked as resolved and limited conversation to collaborators May 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/verify: `ValidIAMPolicyJSON` helpful error messages #27502

internal/verify: `ValidIAMPolicyJSON` helpful error messages #27502

apparentlymart commented Oct 27, 2022 •

edited

Loading

github-actions bot commented Oct 27, 2022

ewbankkit left a comment

ewbankkit commented Apr 25, 2023

github-actions bot commented Apr 28, 2023

github-actions bot commented May 28, 2023

internal/verify: ValidIAMPolicyJSON helpful error messages #27502

internal/verify: ValidIAMPolicyJSON helpful error messages #27502

Conversation

apparentlymart commented Oct 27, 2022 • edited Loading

Description

Output from Acceptance Testing

github-actions bot commented Oct 27, 2022

Community Note

ewbankkit left a comment

Choose a reason for hiding this comment

ewbankkit commented Apr 25, 2023

github-actions bot commented Apr 28, 2023

github-actions bot commented May 28, 2023

internal/verify: `ValidIAMPolicyJSON` helpful error messages #27502

internal/verify: `ValidIAMPolicyJSON` helpful error messages #27502

apparentlymart commented Oct 27, 2022 •

edited

Loading