data-source/aws_iam_policy_document: Add functionality to merge multiple policy documents #12055

aborrello · 2020-02-14T11:15:16Z

Community Note

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

TL;DR

This PR introduces the two new attributes source_json_list and override_json_list into the aws_iam_policy_document data source. This enhancement allows for the layering and combining of IAM policy documents which is incredibly useful generating permission boundaries and reducing duplicated statements across Terraform definitions.

Design

This PR models the behaviour described by @bflad in this comment on issue #5047.

source_json_list provides a method to generate a base policy from multiple policy documents. Each non-empty SID across all statements (including statements defined in the source_json) must be unique, or a "Found duplicate sid" will be thrown.

override_json_list provides a method iteratively combine policy documents, performing an overwrite / merge on any duplicated SIDs.

This behaviour was achieved by utilising the IAMPolicyDoc.Merge method. The proposed conceptual flow for generating policy documents is as follows:

Create an IAMPolicyDoc based on the provided source_json. If no source_json was defined, create a blank IAMPolicyDoc.
(NEW) If the source_json_list attribute was specified, iterate over each policy and merge them. Before each individual policy is merged in, check that none of the non-blank SIDs have already been defined.
Merge the statements defined in the current Terraform block's statement attributes.
(NEW) If the override_json_list attribute was specified, iterate over each policy and merge them.
Merge the override_json document if the attribute was specified.

Tests

The following tests were added to verify this new functionality:

TestAccAWSDataSourceIAMPolicyDocument_sourceList
TestAccAWSDataSourceIAMPolicyDocument_sourceListConflicting
TestAccAWSDataSourceIAMPolicyDocument_overrideList

The outputs from these tests (and all other TestAccAWSDataSourceIAMPolicyDocument) can be found in the "Output from acceptance testing" section of this PR.

Closes the following issues:

Release note for CHANGELOG:

data-source/aws_iam_policy_document: Add `source_json_list` attribute
data-source/aws_iam_policy_document: Add `override_json_list` attribute

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSDataSourceIAMPolicyDocument''

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSDataSourceIAMPolicyDocument -timeout 120m
=== RUN   TestAccAWSDataSourceIAMPolicyDocument_basic
=== PAUSE TestAccAWSDataSourceIAMPolicyDocument_basic
=== RUN   TestAccAWSDataSourceIAMPolicyDocument_source
=== PAUSE TestAccAWSDataSourceIAMPolicyDocument_source
=== RUN   TestAccAWSDataSourceIAMPolicyDocument_sourceList
=== PAUSE TestAccAWSDataSourceIAMPolicyDocument_sourceList
=== RUN   TestAccAWSDataSourceIAMPolicyDocument_sourceConflicting
=== PAUSE TestAccAWSDataSourceIAMPolicyDocument_sourceConflicting
=== RUN   TestAccAWSDataSourceIAMPolicyDocument_sourceListConflicting
=== PAUSE TestAccAWSDataSourceIAMPolicyDocument_sourceListConflicting
=== RUN   TestAccAWSDataSourceIAMPolicyDocument_override
=== PAUSE TestAccAWSDataSourceIAMPolicyDocument_override
=== RUN   TestAccAWSDataSourceIAMPolicyDocument_overrideList
=== PAUSE TestAccAWSDataSourceIAMPolicyDocument_overrideList
=== RUN   TestAccAWSDataSourceIAMPolicyDocument_noStatementMerge
=== PAUSE TestAccAWSDataSourceIAMPolicyDocument_noStatementMerge
=== RUN   TestAccAWSDataSourceIAMPolicyDocument_noStatementOverride
=== PAUSE TestAccAWSDataSourceIAMPolicyDocument_noStatementOverride
=== RUN   TestAccAWSDataSourceIAMPolicyDocument_duplicateSid
=== PAUSE TestAccAWSDataSourceIAMPolicyDocument_duplicateSid
=== RUN   TestAccAWSDataSourceIAMPolicyDocument_Statement_Principal_Identifiers_StringAndSlice
=== PAUSE TestAccAWSDataSourceIAMPolicyDocument_Statement_Principal_Identifiers_StringAndSlice
=== RUN   TestAccAWSDataSourceIAMPolicyDocument_Statement_Principal_Identifiers_MultiplePrincipals
=== PAUSE TestAccAWSDataSourceIAMPolicyDocument_Statement_Principal_Identifiers_MultiplePrincipals
=== RUN   TestAccAWSDataSourceIAMPolicyDocument_Version_20081017
=== PAUSE TestAccAWSDataSourceIAMPolicyDocument_Version_20081017
=== CONT  TestAccAWSDataSourceIAMPolicyDocument_basic
=== CONT  TestAccAWSDataSourceIAMPolicyDocument_noStatementMerge
=== CONT  TestAccAWSDataSourceIAMPolicyDocument_sourceListConflicting
=== CONT  TestAccAWSDataSourceIAMPolicyDocument_overrideList
=== CONT  TestAccAWSDataSourceIAMPolicyDocument_override
=== CONT  TestAccAWSDataSourceIAMPolicyDocument_Statement_Principal_Identifiers_MultiplePrincipals
=== CONT  TestAccAWSDataSourceIAMPolicyDocument_duplicateSid
=== CONT  TestAccAWSDataSourceIAMPolicyDocument_Version_20081017
=== CONT  TestAccAWSDataSourceIAMPolicyDocument_Statement_Principal_Identifiers_StringAndSlice
=== CONT  TestAccAWSDataSourceIAMPolicyDocument_noStatementOverride
=== CONT  TestAccAWSDataSourceIAMPolicyDocument_sourceList
=== CONT  TestAccAWSDataSourceIAMPolicyDocument_source
=== CONT  TestAccAWSDataSourceIAMPolicyDocument_sourceConflicting
--- PASS: TestAccAWSDataSourceIAMPolicyDocument_sourceListConflicting (11.10s)
--- PASS: TestAccAWSDataSourceIAMPolicyDocument_override (46.15s)
--- PASS: TestAccAWSDataSourceIAMPolicyDocument_Statement_Principal_Identifiers_MultiplePrincipals (46.34s)
--- PASS: TestAccAWSDataSourceIAMPolicyDocument_basic (46.37s)
--- PASS: TestAccAWSDataSourceIAMPolicyDocument_overrideList (46.48s)
--- PASS: TestAccAWSDataSourceIAMPolicyDocument_noStatementOverride (46.50s)
--- PASS: TestAccAWSDataSourceIAMPolicyDocument_sourceList (46.71s)
--- PASS: TestAccAWSDataSourceIAMPolicyDocument_sourceConflicting (46.71s)
--- PASS: TestAccAWSDataSourceIAMPolicyDocument_Statement_Principal_Identifiers_StringAndSlice (46.80s)
--- PASS: TestAccAWSDataSourceIAMPolicyDocument_noStatementMerge (46.80s)
--- PASS: TestAccAWSDataSourceIAMPolicyDocument_duplicateSid (50.75s)
--- PASS: TestAccAWSDataSourceIAMPolicyDocument_Version_20081017 (68.06s)
--- PASS: TestAccAWSDataSourceIAMPolicyDocument_source (72.62s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	72.661s

website/docs/d/iam_policy_document.html.markdown

shadycuz · 2020-02-14T13:50:46Z

@aborrello Thanks for implementing these new features. It's going to make the aws_iam_policy_document so much more useful 🎉

aborrello · 2020-02-16T08:22:44Z

website/docs/d/iam_policy_document.html.markdown

  Statements without an `sid` cannot be overwritten.
+* `override_json_list` (Optional) - A list of IAM policy documents to import and


Open to suggestions around the documentation for this attribute. I found it quite hard to find a simple way to describe the iterative merge based on the ordering of the array...

aborrello · 2020-02-16T08:24:03Z

aws/data_source_aws_iam_policy_document.go

@@ -30,6 +30,11 @@ func dataSourceAwsIamPolicyDocument() *schema.Resource {
 				Type:     schema.TypeString,
 				Optional: true,
 			},
+			"override_json_list": {


There was some discussion around override_json_list or override_jsons. I think the _list is cleaner, but completely open for discussion.

aborrello · 2020-02-16T08:26:07Z

aws/data_source_aws_iam_policy_document.go

+			for stmtIndex, stmt := range sourceDoc.Statements {
+				if stmt.Sid != "" {
+					if _, sidExists := sidMap[stmt.Sid]; sidExists {
+						return fmt.Errorf("Found duplicate sid (%s) in source_json_list (item %d; statement %d). Either remove the sid or ensure the sid is unique across all statements.", stmt.Sid, sourceJSONIndex, stmtIndex)


This is slightly different to the normal error messages. I wanted to include the json index and statement index for developer ease, but am open to discuss different ways to convey this information.

awoimbee · 2021-01-06T17:43:49Z

This feature would be really helpful in a lot of scenarios and seem requested by a lot of people. (I'm really desperate for this feature 😅)
Any reason why this PR went stale ?

YakDriver · 2021-02-09T16:01:35Z

@aborrello Thank you for your work on this PR! I will be working on this before long. In order to expedite the process, I will likely make some changes. Make sure that you have checked the box "Allow edits from maintainers." (Don't worry though, you will receive all credit for your contribution and code!) Also, please coordinate with us before making any commits to this branch. Again, thank you for your help and we look forward to this popular addition to the AWS provider!

YakDriver · 2021-02-10T21:26:42Z

We have merged #12055 in to the Terraform AWS Provider. With this, aws_iam_policy_document provides the ability to merge multiple source and override policy documents. This is available now on the main branch and generally when version 3.28.0 is released (likely Feb. 11, 2021). If you have problems with the functionality or need further enhancements, please open a new issue. Thanks for your interest in the AWS Provider! 🎉

ghost · 2021-02-12T17:36:04Z

This has been released in version 3.28.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 for triage. Thanks!

rednuht · 2021-02-19T10:38:25Z

This change causes this to happen:

    statement {
        effect    = "Allow"
        actions   = [
            "s3:ListBucket"
        ]
        resources = [
           ....
        ]
        condition {
            test     = "ForAnyValue:StringLike"
            variable = "s3:prefix"
            values   = flatten([
                for path in [... some paths]:
                    ["${path}/*", path, ""]
            ])
        }
    }

To drop the "" path. We use this to allow listing bucket objects on the "root" level and the paths specified in the condition.

ghost · 2021-03-13T17:10:01Z

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

shadycuz reviewed Feb 14, 2020

View reviewed changes

website/docs/d/iam_policy_document.html.markdown Outdated Show resolved Hide resolved

aborrello commented Feb 16, 2020

View reviewed changes

aborrello marked this pull request as ready for review February 16, 2020 08:27

aborrello requested a review from a team February 16, 2020 08:27

anGie44 force-pushed the master branch from 54db70d to 48ba2f7 Compare July 21, 2020 18:27

scalen mentioned this pull request Jan 7, 2021

better AWS policy layering #5047

Closed

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

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

YakDriver self-assigned this Feb 9, 2021

YakDriver added partition/aws-us-gov Pertains to the aws-us-gov partition. and removed needs-triage Waiting for first response or review from a maintainer. labels Feb 9, 2021

YakDriver added this to the Roadmap milestone Feb 9, 2021

aborrello added 8 commits February 10, 2021 10:44

Add source_json_list attribute to aws_iam_policy_document datasource

c3c9e88

Add override_json_list attribute to aws_iam_policy_document datasource

0cf0ce7

Add tests for enhancements

512ca94

Fix sdk schema errors

e19c661

Correct mis-spelling of override

8f1fdbd

Remove newlines after expected test policy outputs

c3c6b94

Add documentation for new attributes

5309682

Fix spelling mistake in docs

37c0e36

YakDriver added 6 commits February 10, 2021 11:33

data-source/iam_policy_document: Fix lint issues

6249e5c

data-source/iam_policy_document: Make provider consistent

e426a9b

data-source/iam_policy_document: Clean up arguments

7275b3e

docs/iam_policy_document: Clean up documentation

373e889

tests/iam_policy_document: Clean up test names

bf950e9

data-source/iam_policy_document: Add changelog entry

0935bb5

YakDriver merged commit a8825cf into hashicorp:main Feb 10, 2021

github-actions bot modified the milestones: Roadmap, v3.28.0 Feb 10, 2021

This was referenced Feb 10, 2021

Clarify that overriding policy document statements can be added to current document if sid does not match #17468

Merged

data-source/iam_policy_document: Deprecate source_json and override_json #17555

Closed

duganth-va mentioned this pull request Feb 15, 2021

IAM Policy Document Always Changes With Single Conditions v3.28.0 #17623

Closed

ghost locked as resolved and limited conversation to collaborators Mar 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

data-source/aws_iam_policy_document: Add functionality to merge multiple policy documents #12055

data-source/aws_iam_policy_document: Add functionality to merge multiple policy documents #12055

aborrello commented Feb 14, 2020 •

edited

Loading

shadycuz commented Feb 14, 2020

aborrello Feb 16, 2020

aborrello Feb 16, 2020

aborrello Feb 16, 2020

awoimbee commented Jan 6, 2021

YakDriver commented Feb 9, 2021

YakDriver commented Feb 10, 2021 •

edited

Loading

ghost commented Feb 12, 2021

rednuht commented Feb 19, 2021 •

edited

Loading

ghost commented Mar 13, 2021

		Statements without an `sid` cannot be overwritten.
		* `override_json_list` (Optional) - A list of IAM policy documents to import and

data-source/aws_iam_policy_document: Add functionality to merge multiple policy documents #12055

data-source/aws_iam_policy_document: Add functionality to merge multiple policy documents #12055

Conversation

aborrello commented Feb 14, 2020 • edited Loading

Community Note

TL;DR

Design

Tests

shadycuz commented Feb 14, 2020

aborrello Feb 16, 2020

Choose a reason for hiding this comment

aborrello Feb 16, 2020

Choose a reason for hiding this comment

aborrello Feb 16, 2020

Choose a reason for hiding this comment

awoimbee commented Jan 6, 2021

YakDriver commented Feb 9, 2021

YakDriver commented Feb 10, 2021 • edited Loading

ghost commented Feb 12, 2021

rednuht commented Feb 19, 2021 • edited Loading

ghost commented Mar 13, 2021

aborrello commented Feb 14, 2020 •

edited

Loading

YakDriver commented Feb 10, 2021 •

edited

Loading

rednuht commented Feb 19, 2021 •

edited

Loading