provider/aws: Validate effect in aws_iam_policy_document data source

[dougneal]

AWS allows only the case-sensitive strings Allow and Deny to appear in the Effect fields of IAM policy documents. Catch deviations from this, including mis-casing, before hitting the API and generating an error (the error is a generic 400 and doesn't indicate what part of the policy doc is invalid).

[apparentlymart]

Thanks for this, @dougneal. Seems like a very sensible addition!

Do you think it would be clearer to write this as a switch statement instead of a set of if branches?

switch v.(string) {
case "Allow", "Deny":
    // ...
default:
    // ...
}

My eye likes the compactness of this, but that's pretty subjective. What do you think?

[dougneal]

Thanks @apparentlymart, I agree, that's neater. Will change.

[apparentlymart]

Thanks for the update, @dougneal. I was planning to merge this but I'm on the road right now and I just found that my laptop has a stale version of Go that can't build Terraform to run the tests, so I'll need to leave this for someone else to look at or until I have a chance to build a new Go version. Sorry for the delay.

[stack72]

Hi @dougneal (and @apparentlymart)

I just looked at this and tested it - LGTM! Thanks for the work here :)

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSIAMPolicyDocument'                    ✭
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/12/08 12:05:50 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSIAMPolicyDocument -timeout 120m
=== RUN   TestAccAWSIAMPolicyDocument
--- PASS: TestAccAWSIAMPolicyDocument (13.87s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	13.894s

Paul

[dougneal]

Thanks @stack72! In terms of forward-porting this to 0.8, would another PR be helpful?

[stack72]

It will be in 0.8 by default :) 0.8 is being built from master

[dougneal]

The PR was targeted at maint-0.7 tho ;)

[stack72]

ok, I am going to revert that - can you then retarget it to master?

[dougneal]

@stack72: np, done: #10608

[ghost]

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.