-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
provider/aws: validation: Add validation function for IAM Policies #14669
Conversation
The previous JSON validator that we were using for IAM policy documents wouldn't catch AWS IAM Policy errors. The supplied policy document would pass our validator, then fail with the following API error: ``` * aws_iam_role_policy.foo: Error putting IAM role policy tf_test_policy_ymw7hbil9w: MalformedPolicyDocument: The policy failed legacy parsing status code: 400, request id: e7615d90-3c99-11e7-babc-c14e741605bf ``` This happens if the Policy Document doesn't start with the opening JSON bracket, and often happens in the following case: ``` policy = <<EOF { "Version": "2012-10-17", "Statement": [ { ... } ] } EOF ``` Where, when using a HEREDOC, the policy document is indented incorrectly. The new validation function for the IAM policies verifies that the first character of the supplied policy document is the leading JSON bracket, prior to validating the JSON string. Test Output: ``` $ make test TEST=./builtin/providers/aws/ TESTARGS="-v -run=TestValidateIAMPolicyJsonString" ==> Checking that code complies with gofmt requirements... ==> Checking AWS provider for unchecked errors... ==> NOTE: at this time we only look for uncheck errors in the AWS package go generate $(go list ./... | grep -v /terraform/vendor/) 2017/05/19 10:56:32 Generated command/internal_plugin_list.go go test -i ./builtin/providers/aws/ || exit 1 echo ./builtin/providers/aws/ | \ xargs -t -n4 go test -v -run=TestValidateIAMPolicyJsonString -timeout=60s -parallel=4 go test -v -run=TestValidateIAMPolicyJsonString -timeout=60s -parallel=4 ./builtin/providers/aws/ === RUN TestValidateIAMPolicyJsonString --- PASS: TestValidateIAMPolicyJsonString (0.00s) PASS ok github.com/hashicorp/terraform/builtin/providers/aws 0.009s ``` ``` $ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAWSPolicy_' ==> Checking that code complies with gofmt requirements... go generate $(go list ./... | grep -v /terraform/vendor/) 2017/05/19 10:38:43 Generated command/internal_plugin_list.go TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAWSPolicy_ -timeout 120m === RUN TestAWSPolicy_namePrefix --- PASS: TestAWSPolicy_namePrefix (20.01s) === RUN TestAWSPolicy_invalidJson --- PASS: TestAWSPolicy_invalidJson (0.00s) PASS ok github.com/hashicorp/terraform/builtin/providers/aws 20.027s ``` ``` $ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSIAMRolePolicy_' ==> Checking that code complies with gofmt requirements... go generate $(go list ./... | grep -v /terraform/vendor/) 2017/05/19 11:02:56 Generated command/internal_plugin_list.go TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSIAMRolePolicy_ -timeout 120m === RUN TestAccAWSIAMRolePolicy_importBasic --- PASS: TestAccAWSIAMRolePolicy_importBasic (18.45s) === RUN TestAccAWSIAMRolePolicy_basic --- PASS: TestAccAWSIAMRolePolicy_basic (35.92s) === RUN TestAccAWSIAMRolePolicy_namePrefix --- PASS: TestAccAWSIAMRolePolicy_namePrefix (14.78s) === RUN TestAccAWSIAMRolePolicy_generatedName --- PASS: TestAccAWSIAMRolePolicy_generatedName (20.20s) === RUN TestAccAWSIAMRolePolicy_invalidJSON --- PASS: TestAccAWSIAMRolePolicy_invalidJSON (0.00s) PASS ok github.com/hashicorp/terraform/builtin/providers/aws 89.363s ```
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.
This looks overall good, just one question.
Steps: []resource.TestStep{ | ||
{ | ||
Config: testAccAWSPolicyInvalidJsonConfig, | ||
ExpectError: regexp.MustCompile("invalid JSON"), |
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.
A bit overkill to test validation function on a real resource, but 🤷♂️ I guess it does the job 😄
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.
Mostly wanted to test the HEREDOC evaluation would fail prior to the apply
phase.
builtin/providers/aws/structure.go
Outdated
@@ -1886,7 +1886,11 @@ func normalizeJsonString(jsonString interface{}) (string, error) { | |||
return s, err | |||
} | |||
|
|||
bytes, _ := json.Marshal(j) | |||
bytes, err := json.Marshal(j) |
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.
🤔 wasn't this here to allow empty policies to passthrough? e.g. when it's interpolated from a variable and the only way to make it empty is to pass an empty string (not to omit the argument).
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.
Ah, possibly. Noticed we didn't catch the error returned there, and thought we should. I'll add a comment that it's on purpose.
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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
The previous JSON validator that we were using for IAM policy documents wouldn't catch AWS IAM Policy errors.
The supplied policy document would pass our validator, then fail with the following API error:
This happens if the Policy Document doesn't start with the opening JSON bracket, and often happens in the following case:
Where, when using a HEREDOC, the policy document is indented incorrectly.
The new validation function for the IAM policies verifies that the first character of the supplied policy document is the leading JSON bracket, prior to validating the JSON string.
Test Output:
Fixes: #14652