Permit leading whitespace in IAM policies by normalizing before check… #5887

mwinters0 · 2018-09-14T18:52:35Z

Changes proposed in this pull request:

Normalize JSON before testing structure, to permit valid leading whitespace per the JSON spec

Output from acceptance testing:

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSLaunchTemplate_ -timeout 120m
=== RUN   TestAccAWSLaunchTemplate_importBasic
--- PASS: TestAccAWSLaunchTemplate_importBasic (20.12s)
=== RUN   TestAccAWSLaunchTemplate_importData
--- PASS: TestAccAWSLaunchTemplate_importData (20.22s)
=== RUN   TestAccAWSLaunchTemplate_basic
--- PASS: TestAccAWSLaunchTemplate_basic (18.14s)
=== RUN   TestAccAWSLaunchTemplate_BlockDeviceMappings_EBS
--- PASS: TestAccAWSLaunchTemplate_BlockDeviceMappings_EBS (49.48s)
=== RUN   TestAccAWSLaunchTemplate_BlockDeviceMappings_EBS_DeleteOnTermination
--- PASS: TestAccAWSLaunchTemplate_BlockDeviceMappings_EBS_DeleteOnTermination (86.00s)
=== RUN   TestAccAWSLaunchTemplate_data
--- PASS: TestAccAWSLaunchTemplate_data (17.86s)
=== RUN   TestAccAWSLaunchTemplate_update
--- PASS: TestAccAWSLaunchTemplate_update (83.03s)
=== RUN   TestAccAWSLaunchTemplate_tags
--- PASS: TestAccAWSLaunchTemplate_tags (32.64s)
=== RUN   TestAccAWSLaunchTemplate_nonBurstable
--- PASS: TestAccAWSLaunchTemplate_nonBurstable (16.86s)
=== RUN   TestAccAWSLaunchTemplate_networkInterface
--- PASS: TestAccAWSLaunchTemplate_networkInterface (47.23s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	391.625s```

bflad

This is passing all related acceptance testing except for one:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSIAM\(Role\|User\)?Policy_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSIAM\(Role\|User\)?Policy_ -timeout 120m
=== RUN   TestAccAWSIAMRolePolicy_importBasic
--- PASS: TestAccAWSIAMRolePolicy_importBasic (10.30s)
=== RUN   TestAccAWSIAMUserPolicy_importBasic
--- PASS: TestAccAWSIAMUserPolicy_importBasic (8.33s)
=== RUN   TestAccAWSIAMPolicy_basic
--- PASS: TestAccAWSIAMPolicy_basic (8.74s)
=== RUN   TestAccAWSIAMPolicy_description
--- PASS: TestAccAWSIAMPolicy_description (8.91s)
=== RUN   TestAccAWSIAMPolicy_namePrefix
--- PASS: TestAccAWSIAMPolicy_namePrefix (8.16s)
=== RUN   TestAccAWSIAMPolicy_path
--- PASS: TestAccAWSIAMPolicy_path (9.15s)
=== RUN   TestAccAWSIAMPolicy_policy
--- PASS: TestAccAWSIAMPolicy_policy (13.76s)
=== RUN   TestAccAWSIAMRolePolicy_basic
--- PASS: TestAccAWSIAMRolePolicy_basic (15.34s)
=== RUN   TestAccAWSIAMRolePolicy_namePrefix
--- PASS: TestAccAWSIAMRolePolicy_namePrefix (13.46s)
=== RUN   TestAccAWSIAMRolePolicy_generatedName
--- PASS: TestAccAWSIAMRolePolicy_generatedName (14.88s)
=== RUN   TestAccAWSIAMRolePolicy_invalidJSON
--- FAIL: TestAccAWSIAMRolePolicy_invalidJSON (4.51s)
    testing.go:520: Step 0, expected error:

        Error applying: 1 error occurred:
        	* aws_iam_role_policy.foo: 1 error occurred:
        	* aws_iam_role_policy.foo: Error putting IAM role policy tf_test_policy_6ujbe3zmes: MalformedPolicyDocument: The policy failed legacy parsing
        	status code: 400, request id: a6aba47e-baba-11e8-a156-0f77be764b45





        To match:

        invalid JSON


=== RUN   TestAccAWSIAMUserPolicy_basic
--- PASS: TestAccAWSIAMUserPolicy_basic (13.47s)
=== RUN   TestAccAWSIAMUserPolicy_namePrefix
--- PASS: TestAccAWSIAMUserPolicy_namePrefix (13.49s)
=== RUN   TestAccAWSIAMUserPolicy_generatedName
--- PASS: TestAccAWSIAMUserPolicy_generatedName (14.34s)
=== RUN   TestAccAWSIAMUserPolicy_multiplePolicies
--- PASS: TestAccAWSIAMUserPolicy_multiplePolicies (25.58s)
FAIL
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	183.132s

Presumably this breaks a certain scenario where the validation error used to be caught during plan time and now returning an error during apply. Do you mind taking a look at that error?

mwinters0 · 2018-09-17T23:03:49Z

Sorry @bflad , this is my first PR here. I didn't know those tests existed.

So, the old validation code would reject the test policy (https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_iam_role_policy_test.go#L434-L443) as invalid JSON because that policy starts with whitespace. We no longer care about whitespace, so validation passes because it is in fact valid JSON, but the test policy contains a second issue which is that the Statement is not an array, and that doesn't match the IAM policy spec.

I've modified the test policy to consist of an otherwise-valid schema but with with an invalid character so that it's no longer valid JSON. This now passes:

% make testacc TEST=./aws TESTARGS='-run=TestAccAWSIAMRolePolicy_invalidJSON'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSIAMRolePolicy_invalidJSON -timeout 120m
=== RUN   TestAccAWSIAMRolePolicy_invalidJSON
--- PASS: TestAccAWSIAMRolePolicy_invalidJSON (1.77s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1.814s

Let me know if you'd like me to dig deeper in this PR and validate the schema.

However, TestAccAWSIAMPolicy_importBasic is now failing, presumably because the Go-normalized JSON doesn't match what AWS is returning. I'm digging into that. Sorry for the noisy PR.

…ing structure

mwinters0 · 2018-09-29T10:52:54Z

@bflad Rebased on master, acceptance tests are now passing.

% make testacc TEST=./aws TESTARGS='-run=TestAccAWSIAM\(Role\|User\)?Policy_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSIAM\(Role\|User\)?Policy_ -timeout 120m
=== RUN   TestAccAWSIAMPolicy_basic
--- PASS: TestAccAWSIAMPolicy_basic (13.92s)
=== RUN   TestAccAWSIAMPolicy_description
--- PASS: TestAccAWSIAMPolicy_description (13.52s)
=== RUN   TestAccAWSIAMPolicy_namePrefix
--- PASS: TestAccAWSIAMPolicy_namePrefix (12.29s)
=== RUN   TestAccAWSIAMPolicy_path
--- PASS: TestAccAWSIAMPolicy_path (13.72s)
=== RUN   TestAccAWSIAMPolicy_policy
--- PASS: TestAccAWSIAMPolicy_policy (21.51s)
=== RUN   TestAccAWSIAMRolePolicy_importBasic
--- PASS: TestAccAWSIAMRolePolicy_importBasic (12.84s)
=== RUN   TestAccAWSIAMRolePolicy_basic
--- PASS: TestAccAWSIAMRolePolicy_basic (20.62s)
=== RUN   TestAccAWSIAMRolePolicy_namePrefix
--- PASS: TestAccAWSIAMRolePolicy_namePrefix (19.95s)
=== RUN   TestAccAWSIAMRolePolicy_generatedName
--- PASS: TestAccAWSIAMRolePolicy_generatedName (19.06s)
=== RUN   TestAccAWSIAMRolePolicy_invalidJSON
--- PASS: TestAccAWSIAMRolePolicy_invalidJSON (1.40s)
=== RUN   TestAccAWSIAMUserPolicy_importBasic
--- PASS: TestAccAWSIAMUserPolicy_importBasic (17.48s)
=== RUN   TestAccAWSIAMUserPolicy_basic
--- PASS: TestAccAWSIAMUserPolicy_basic (20.81s)
=== RUN   TestAccAWSIAMUserPolicy_namePrefix
--- PASS: TestAccAWSIAMUserPolicy_namePrefix (21.27s)
=== RUN   TestAccAWSIAMUserPolicy_generatedName
--- PASS: TestAccAWSIAMUserPolicy_generatedName (20.01s)
=== RUN   TestAccAWSIAMUserPolicy_multiplePolicies
--- PASS: TestAccAWSIAMUserPolicy_multiplePolicies (36.20s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	264.673s

mwinters0 · 2018-10-25T15:07:06Z

@bflad Sorry to ping you - is there anything else I need to do to get this merged? I don't know the workflow on this repo.

copumpkin · 2019-03-03T23:50:05Z

@bflad can you merge this? It's a pretty awkward UX right now

copumpkin · 2019-03-03T23:50:40Z

aws/resource_aws_iam_role_policy_test.go

@@ -479,13 +479,15 @@ resource "aws_iam_role_policy" "foo" {
 	name = "tf_test_policy_%s"
 	role = "${aws_iam_role.role.name}"
 	policy = <<EOF
-  {
+  x{


Is this deliberate?

Yes. That resource is intended to trigger the "invalid JSON" path. Before this patch, it was triggered by the indentation, which is now considered valid. In retrospect, instead of "x" I should have put something ilke "this_json_is_intentionally_invalid_for_testing".

I haven't touched this code in ~6 months but I will try to find time this week to rebase, verify tests still pass, and make this intent clearer.

BenCoughlan15 · 2019-07-02T11:56:44Z

@MikeSchuette any updates?

vdhpieter · 2020-11-13T15:02:47Z

Could this get merged?

zhelding · 2021-10-19T16:49:52Z

Pull request #21306 has significantly refactored the AWS Provider codebase. As a result, most PRs opened prior to the refactor now have merge conflicts that must be resolved before proceeding.

Specifically, PR #21306 relocated the code for all AWS resources and data sources from a single aws directory to a large number of separate directories in internal/service, each corresponding to a particular AWS service. This separation of code has also allowed for us to simplify the names of underlying functions -- while still avoiding namespace collisions.

We recognize that many pull requests have been open for some time without yet being addressed by our maintainers. Therefore, we want to make it clear that resolving these conflicts in no way affects the prioritization of a particular pull request. Once a pull request has been prioritized for review, the necessary changes will be made by a maintainer -- either directly or in collaboration with the pull request author.

For a more complete description of this refactor, including examples of how old filepaths and function names correspond to their new counterparts: please refer to issue #20000.

For a quick guide on how to amend your pull request to resolve the merge conflicts resulting from this refactor and bring it in line with our new code patterns: please refer to our Service Package Refactor Pull Request Guide.

ewbankkit · 2022-01-02T12:49:31Z

Superseded by #22067.

github-actions · 2022-05-31T02:35:39Z

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.

ghost added the size/S Managed by automation to categorize the size of a PR. label Sep 14, 2018

mwinters0 mentioned this pull request Sep 14, 2018

Bug: leading whitespace causes aws_iam_policy to incorrectly report valid JSON policies as invalid #1873

Closed

bflad added enhancement Requests to existing resources that expand the functionality or scope. service/iam Issues and PRs that pertain to the iam service. labels Sep 17, 2018

bflad reviewed Sep 17, 2018

View reviewed changes

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

Mike Schuette added 3 commits September 29, 2018 05:40

Permit leading whitespace in IAM policies by normalizing before check…

ecc9b2e

…ing structure

Minor cleanup

0bc3b85

Modify the IAM Role test policy to be invalid JSON

b766c61

ghost added size/S Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Sep 29, 2018

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

goetzc mentioned this pull request Dec 18, 2018

Invalid JSON policy Error #3970

Closed

copumpkin reviewed Mar 3, 2019

View reviewed changes

aeschright requested a review from a team June 25, 2019 21:28

Base automatically changed from master to main January 23, 2021 00:55

breathingdust requested a review from a team as a code owner January 23, 2021 00:55

ewbankkit closed this Jan 2, 2022

github-actions bot locked as resolved and limited conversation to collaborators May 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Permit leading whitespace in IAM policies by normalizing before check… #5887

Permit leading whitespace in IAM policies by normalizing before check… #5887

mwinters0 commented Sep 14, 2018 •

edited

Loading

bflad left a comment

mwinters0 commented Sep 17, 2018

mwinters0 commented Sep 29, 2018

mwinters0 commented Oct 25, 2018

copumpkin commented Mar 3, 2019

copumpkin Mar 3, 2019

mwinters0 Mar 4, 2019

BenCoughlan15 commented Jul 2, 2019

vdhpieter commented Nov 13, 2020

zhelding commented Oct 19, 2021

ewbankkit commented Jan 2, 2022

github-actions bot commented May 31, 2022

Permit leading whitespace in IAM policies by normalizing before check… #5887

Permit leading whitespace in IAM policies by normalizing before check… #5887

Conversation

mwinters0 commented Sep 14, 2018 • edited Loading

bflad left a comment

Choose a reason for hiding this comment

mwinters0 commented Sep 17, 2018

mwinters0 commented Sep 29, 2018

mwinters0 commented Oct 25, 2018

copumpkin commented Mar 3, 2019

copumpkin Mar 3, 2019

Choose a reason for hiding this comment

mwinters0 Mar 4, 2019

Choose a reason for hiding this comment

BenCoughlan15 commented Jul 2, 2019

vdhpieter commented Nov 13, 2020

zhelding commented Oct 19, 2021

ewbankkit commented Jan 2, 2022

github-actions bot commented May 31, 2022

mwinters0 commented Sep 14, 2018 •

edited

Loading