data-source/aws_iam_policy_document: Support layering via source_json and override_json attributes

[devonbleak]

Adds a source_json property to aws_iam_policy_document to allow for merging of IAM policy statements. Useful for abstracting resource policies in modules where it's only possible to attach one and reducing repetition when policies are nearly identical.

[bflad]

Thank you so much for your contribution here! Honestly, I am surprised that more folks have not upvoted this PR, this is a huge enhancement if you need to provide a standard policy but also add customizations. 😅

One thing I do want to double check before I approve and merge this is what happens when there are conflicting source and "added" Sids.

[bflad]

Agreed! I think this type of refactoring is not necessary for this PR, so not marking that as a requested change 👍

[devonbleak]

That was boilerplate copied from the existing test 😄

[bflad]

Hi @devonbleak! Overall this is awesome, but I did find one issue that will definitely need to be resolved before this can be pulled in. Please let us know if you are able to fix this!

I created an additional acceptance test that tries to use overwrite the same Sid, which unfortunately it winds up creating duplicate Sid statements (incorrect 😢 ). We can choose to either error out in this scenario (not preferred, I think) or just overwrite that statement by deleting it from the source JSON (preferred, I think). Can you add the following test and ensure it passes please? 😄 Thanks!

Check this out:

func TestAccAWSDataSourceIAMPolicyDocument_sourceConflicting(t *testing.T) {
	resource.Test(t, resource.TestCase{
		PreCheck:  func() { testAccPreCheck(t) },
		Providers: testAccProviders,
		Steps: []resource.TestStep{
			{
				Config: testAccAWSIAMPolicyDocumentSourceConflictingConfig,
				Check: resource.ComposeTestCheckFunc(
					testAccCheckStateValue("data.aws_iam_policy_document.test_source_conflicting", "json",
						testAccAWSIAMPolicyDocumentSourceConflictingExpectedJSON,
					),
				),
			},
		},
	})
}

var testAccAWSIAMPolicyDocumentSourceConflictingConfig = `
data "aws_iam_policy_document" "test_source" {
    statement {
        sid       = "SourceJSONTestConflicting"
        actions   = ["iam:*"]
        resources = ["*"]
    }
}

data "aws_iam_policy_document" "test_source_conflicting" {
    source_json = "${data.aws_iam_policy_document.test_source.json}"

    statement {
        sid       = "SourceJSONTestConflicting"
        actions   = ["*"]
        resources = ["*"]
    }
}
`

var testAccAWSIAMPolicyDocumentSourceConflictingExpectedJSON = `{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Sid": "SourceJSONTestConflicting",
      "Effect": "Allow",
      "Action": "*",
      "Resource": "*"
    }
  ]
}`

make testacc TEST=./aws TESTARGS='-run=TestAccAWSDataSourceIAMPolicyDocument_sourceConflicting'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSDataSourceIAMPolicyDocument_sourceConflicting -timeout 120m
=== RUN   TestAccAWSDataSourceIAMPolicyDocument_sourceConflicting
--- FAIL: TestAccAWSDataSourceIAMPolicyDocument_sourceConflicting (7.64s)
	testing.go:503: Step 0 error: Check failed: Check 1/1 error: Value for json is {
		  "Version": "2012-10-17",
		  "Statement": [
		    {
		      "Sid": "SourceJSONTestConflicting",
		      "Effect": "Allow",
		      "Action": "iam:*",
		      "Resource": "*"
		    },
		    {
		      "Sid": "SourceJSONTestConflicting",
		      "Effect": "Allow",
		      "Action": "*",
		      "Resource": "*"
		    }
		  ]
		}, not {
		  "Version": "2012-10-17",
		  "Statement": [
		    {
		      "Sid": "SourceJSONTestConflicting",
		      "Effect": "Allow",
		      "Action": "*",
		      "Resource": "*"
		    }
		  ]
		}
FAIL
exit status 1
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	7.682s
make: *** [testacc] Error 1

[devonbleak]

@bflad Added your acceptance test and created functionality for merging duplicate SIDs. Also created an override_json property to allow for bringing in json that overrides the current document while source_json continues to be the base that the current document can override.

Also added some examples to the website docs.

[bflad]

LGTM. One nitpick, but I think its fine the way it is. Thanks!

make testacc TEST=./aws TESTARGS='-run=TestAccAWSDataSourceIAMPolicyDocument'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSDataSourceIAMPolicyDocument -timeout 120m
=== RUN   TestAccAWSDataSourceIAMPolicyDocument_basic
--- PASS: TestAccAWSDataSourceIAMPolicyDocument_basic (9.69s)
=== RUN   TestAccAWSDataSourceIAMPolicyDocument_source
--- PASS: TestAccAWSDataSourceIAMPolicyDocument_source (11.79s)
=== RUN   TestAccAWSDataSourceIAMPolicyDocument_sourceConflicting
--- PASS: TestAccAWSDataSourceIAMPolicyDocument_sourceConflicting (9.04s)
=== RUN   TestAccAWSDataSourceIAMPolicyDocument_override
--- PASS: TestAccAWSDataSourceIAMPolicyDocument_override (7.80s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	38.365s

[bflad]

Nitpick: I think this function would be a little easier to grok if it was something like Merge(newDoc *IAMPolicyDoc), then we can just replace a *Statement if the Sid is non-empty and matches an existing one otherwise append() the new statement. Albeit the below is less optimized since its doing multiple traversals. e.g.

var seen bool
for _, newStatement := range newDoc.Statements {
  if len(newStatement.Sid) == 0 {
    self.Statements = append(self.Statements, newStatement)
    continue
  }
  seen = false
  for i, existingStatement := range self.Statements {
    if existingStatement.Sid == newStatement.Sid {
      self.Statements[i] = newStatement
      seen = true
      break
    }
  }
  if !seen {
    self.Statements = append(self.Statements, newStatement)
  }
}

[devonbleak]

I agree, I wasn't super happy with that implementation but changing to a more generic Merge function ended up requiring a heavy refactoring of dataSourceAwsIamPolicyDocumentRead without a lot of additional benefit as the model doesn't seem to be used beyond there.

Let me know if you want an additional PR to do that refactoring at some point.

[bflad]

This has been released in terraform-provider-aws version 1.9.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

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

[YakDriver]

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 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! 🎉