Skip to content
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

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

Merged
merged 14 commits into from
Feb 10, 2021

Conversation

aborrello
Copy link
Contributor

@aborrello aborrello commented Feb 14, 2020

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:

  1. Create an IAMPolicyDoc based on the provided source_json. If no source_json was defined, create a blank IAMPolicyDoc.
  2. (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.
  3. Merge the statements defined in the current Terraform block's statement attributes.
  4. (NEW) If the override_json_list attribute was specified, iterate over each policy and merge them.
  5. 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

@ghost ghost added needs-triage Waiting for first response or review from a maintainer. size/XL Managed by automation to categorize the size of a PR. service/iam Issues and PRs that pertain to the iam service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. documentation Introduces or discusses updates to documentation. labels Feb 14, 2020
@shadycuz
Copy link
Contributor

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@aborrello aborrello marked this pull request as ready for review February 16, 2020 08:27
@aborrello aborrello requested a review from a team February 16, 2020 08:27
@awoimbee
Copy link

awoimbee commented Jan 6, 2021

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 ?

Base automatically changed from master to main January 23, 2021 00:57
@breathingdust breathingdust requested a review from a team as a code owner January 23, 2021 00:57
@YakDriver YakDriver self-assigned this Feb 9, 2021
@YakDriver 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 YakDriver added this to the Roadmap milestone Feb 9, 2021
@YakDriver
Copy link
Member

@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
Copy link
Member

YakDriver commented Feb 10, 2021

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
Copy link

ghost commented Feb 12, 2021

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
Copy link

rednuht commented Feb 19, 2021

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
Copy link

ghost commented Mar 13, 2021

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!

@ghost ghost locked as resolved and limited conversation to collaborators Mar 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. partition/aws-us-gov Pertains to the aws-us-gov partition. service/iam Issues and PRs that pertain to the iam service. size/XL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants