provider/aws: Normalize IAM policy documents #7785

dtolnay · 2016-07-23T21:47:21Z

Fixes #3634 and fixes #7495.

Two normalizations are applied:

Arrays of length 1 become single strings because this is how they are returned from AWS APIs.
Arrays of length more than 1 are sorted because AWS stores them as sets and returns them in random order.

Two normalizations are applied: - Arrays of length 1 become single strings because this is how they are returned from AWS APIs. - Arrays of length more than 1 are sorted.

apparentlymart · 2016-07-25T14:27:36Z

builtin/providers/aws/json.go

+	var policy IAMPolicyDoc
+	err := json.Unmarshal([]byte(policyString.(string)), &policy)
+	if err != nil {
+		return fmt.Sprintf("Error parsing IAM policy document: %s", err)


This doesn't feel right... seems like if an error occurs here we'd end up storing this error message in the state, which I don't think is what you intended here.

Probably the best we could do here is to make this function silently change nothing in the event of an error (just return the provided string verbatim) and trust the backend to catch and return any syntax errors. I suppose we could couple this with a policy validation function that we could use as ValidateFunc on these attributes and thus catch certain errors locally during Terraform validation, and thus assume that the policy will always be valid by the time it gets in here.

Also wondering about the rationale for the policyString argument being interface{} rather than string here... it seems like this code would panic at runtime if the policy were anything other than a string, so could we just make the argument be of type string in the first place?

Re: returning an error. This matches the behavior of the existing normalizeJson function. I can file a separate ticket to fix both.

Re: interface{}. This allows normalizePolicyDocument to be used as a StateFunc.

Ahh got it... I didn't make the connection that you were using this directly as a StateFunc. Sorry... should've woken up a bit more before asking that question. 😀

apparentlymart · 2016-07-25T14:42:57Z

Thanks for this @dtolnay! Apart from a couple minor comments this looks great to me.

dtolnay · 2016-07-25T14:44:42Z

Thanks for the review @apparentlymart. Can you take a look at my response to the comments? I don't think any of them should be addressed in this PR.

ronaldtse · 2016-08-08T06:14:29Z

This is an awesome fix that makes the iam_policy_document data source actually useable. Right now it's pretty much breaking all AWS Elasticsearch and S3 bucket policies.

As an avid terraform user, may I ask that this gets merged soon? 👍

aerickson · 2016-08-08T23:15:36Z

@apparentlymart, can you please respond to @dtolnay's comments?

Sorry for the ping... this is a pretty nasty 'bug' that's affecting me (every apply now takes 15 minutes in my environment).

sstarcher · 2016-08-08T23:19:28Z

@aerickson agreed it's very painful with AWS ES as it takes a very long time to apply

apparentlymart · 2016-08-20T17:06:00Z

@dtolnay sorry for sitting on this one for so long. It looks like in the mean time a comparable change was submitted and merged in #6956.

Apparently @vancluever took the opposite approach there: rather than normalizing what we send to AWS, instead we "de-normalize" what we retrieve from AWS. This addresses the part of the problem where we get spurious diffs, but only addressed aws_iam_policy_document itself and didn't deal with all the various resource arguments where IAM policies can be specified.

I think perhaps the path of least resistance to get from where master is to what you wanted to achieve here would be for us to to split out the "de-normalization" part into a separate function that can be used as a StateFunc, and then we can use it in all the places you identified in your patch here... so it would have a similar result to your change but with Terraform's "normalized form" being arrays-always, rather than having the special case for single items as your patch would've done. I don't think it will matter which we we go with the normalization as long as we're always consistent.

I'm going to work on that now, using your changeset here as a prompt for which attributes need to be normalized.

dtolnay · 2016-08-20T17:12:09Z

Another option is to revert #6956 and merge this. As far as I can tell, this is strictly better. Is there any advantage to the fix in #6956?

I am on vacation this week and won't be able to make changes until 8/27 so I will leave it to you.

…source" This reverts commit 9872026.

dtolnay · 2016-08-20T17:25:13Z

@apparentlymart I pushed a revert + merge.

git checkout -b revert origin/master
git revert 9872026235b094ada8ef017cae9db2a1c0cac399
git checkout dtolnay/normalize
git merge revert

Some earlier work in #6956 implemented IAM policy normalization within the aws_iam_policy_document data source. Some other (unmerged) work in #7785 implemented normalization across many different IAM policy attributes in the provider. The two are in conflict due to a difference in approach. This is an attempt to reconcile the two by generalizing the normalization already implemented in #6956 and then applying it to the various places that were addressed by #7785.

Earlier work in #6956 caused the IAM policy documents generated by the aws_iam_policy_document data source to follow the normalization conventions used by most AWS services. However, use of this data source is optional and so hand-authored IAM policy documents used with other resources can still suffer from normalization issues. By reorganizing the code a little we can make re-usable normalization and validation functions, which we will be able to use across many different resource implementations, pending changes in subsequent commits. This is a continuation of initial work done by David Tolnay in issue #7785. This will cause some minor changes to the result of the aws_iam_policy_document data source: string sets are now sorted in forward lexographic order rather than reverse, and "Statements" and "Sid" will now be omitted when empty, for consistency with all of the other attributes.

vancluever · 2016-08-23T00:39:21Z

@dtolnay it's disappointing to me to see you say that that work in #6956 should just be "reverted" - judging from other feedback that I've read in referenced issues, there does not appear to be any regressions - just that the only thing that I fixed was the data source. Which seemed fair enough in my opinion - I didn't really think that one could expect Terraform to be able to properly normalize all policy documents that were sent thru it and if a user wanted a guarantee, they could use the data source, now that it's available.

Regardless on whether or not yours (or #8350 for that matter) is a better approach, I think that we should not simply dismiss other people's hard work on an open source project (especially considering that I've been trying to get this fixed for a very long time - see #4278). #8350 does a much better job in explaining the outstanding issues with my approach without making it sound like my work should just be thrown out.

dtolnay · 2016-08-23T01:13:14Z

Thanks for the feedback @vancluever. I agree with everything you wrote. Now that #8350 is available I am closing this PR in favor of that one. It is nice to see that after many months (counting from your first PR) we are finally making progress on resolving this.

ghost · 2020-04-23T01:56:59Z

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.

provider/aws: Normalize IAM policy documents

72b3f5f

Two normalizations are applied: - Arrays of length 1 become single strings because this is how they are returned from AWS APIs. - Arrays of length more than 1 are sorted.

radeksimko added provider/aws enhancement labels Jul 25, 2016

apparentlymart reviewed Jul 25, 2016
View reviewed changes

apparentlymart self-assigned this Aug 9, 2016

dtolnay mentioned this pull request Aug 20, 2016

provider/aws: Fixing IAM data source policy generation to prevent spurious diffs #6956

Merged

dtolnay added 2 commits August 20, 2016 13:21

Revert "provider/aws: Various IAM policy normalizations for IAM data …

e7f825d

…source" This reverts commit 9872026.

Merge branch hashicorp/master into dtolnay/normalize

ff07f4a

apparentlymart mentioned this pull request Aug 21, 2016

provider/aws: IAM Policy Normalization Helpers #8350

Closed

dtolnay closed this Aug 23, 2016

dtolnay deleted the normalize branch August 23, 2016 01:13

ghost locked and limited conversation to collaborators Apr 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/aws: Normalize IAM policy documents #7785

provider/aws: Normalize IAM policy documents #7785

dtolnay commented Jul 23, 2016 •

edited

Loading

apparentlymart Jul 25, 2016

dtolnay Jul 25, 2016

apparentlymart Jul 25, 2016

apparentlymart commented Jul 25, 2016

dtolnay commented Jul 25, 2016

ronaldtse commented Aug 8, 2016

aerickson commented Aug 8, 2016

sstarcher commented Aug 8, 2016

apparentlymart commented Aug 20, 2016

dtolnay commented Aug 20, 2016

dtolnay commented Aug 20, 2016

vancluever commented Aug 23, 2016

dtolnay commented Aug 23, 2016

ghost commented Apr 23, 2020

provider/aws: Normalize IAM policy documents #7785

provider/aws: Normalize IAM policy documents #7785

Conversation

dtolnay commented Jul 23, 2016 • edited Loading

apparentlymart Jul 25, 2016

Choose a reason for hiding this comment

dtolnay Jul 25, 2016

Choose a reason for hiding this comment

apparentlymart Jul 25, 2016

Choose a reason for hiding this comment

apparentlymart commented Jul 25, 2016

dtolnay commented Jul 25, 2016

ronaldtse commented Aug 8, 2016

aerickson commented Aug 8, 2016

sstarcher commented Aug 8, 2016

apparentlymart commented Aug 20, 2016

dtolnay commented Aug 20, 2016

dtolnay commented Aug 20, 2016

vancluever commented Aug 23, 2016

dtolnay commented Aug 23, 2016

ghost commented Apr 23, 2020

dtolnay commented Jul 23, 2016 •

edited

Loading