provider/aws: normalize json policy for sns topic policy attribute

[keymon]

Summary

For the policy attribute of the resource aws_sns_topic, AWS returns the policy
in JSON format with the fields in a different order.
If we store and compare the values without normalizing, terraform
will unnecesary trigger and update of the resource.

To avoid that, we must add a normalization function in the StateFunc of
the policy attribute and also when we read the attribute from AWS.

Details

Given a manifest like this:

resource "aws_sns_topic" "CloudTrailWatch" {
  name = "CloudTrailWatch"
  policy = "${template_file.cloudtrailwatch_sns_permissions.rendered}"
}

resource "template_file" "cloudtrailwatch_sns_permissions" {
  template = <<EOF
{
   "Statement" : [
      {
         "Effect" : "Allow",
         "Resource" : "arn:aws:sns:${region}:${account_id}:CloudTrailWatch",
         "Condition" : {
            "StringEquals" : {
               "AWS:SourceOwner" : "${account_id}"
            }
         },
         "Action" : [
            "SNS:Subscribe",
            "SNS:ListSubscriptionsByTopic",
            "SNS:DeleteTopic",
            "SNS:GetTopicAttributes",
            "SNS:Publish",
            "SNS:RemovePermission",
            "SNS:AddPermission",
            "SNS:Receive",
            "SNS:SetTopicAttributes"
         ],
         "Sid" : "__default_statement_ID",
         "Principal" : {
            "AWS" : "*"
         }
      },
      {
         "Action" : "sns:Publish",
         "Principal" : {
            "Service" : "events.amazonaws.com"
         },
         "Sid" : "AWSEvents_cloudtrail-manipulation_SendToSNS",
         "Effect" : "Allow",
         "Resource" : "arn:aws:sns:${region}:${account_id}:CloudTrailWatch"
      }
   ],
   "Version" : "2012-10-17",
   "Id" : "__default_policy_ID"
}
EOF
  vars {
    account_id = "1234567890"
    region = "us-east-1"
  }
}

It will trigger update resource every time you run it:

$ terraform apply -target aws_sns_topic.CloudTrailWatch
provider.aws.region
  The region where AWS operations will take place. Examples
  are us-east-1, us-west-2, etc.

  Default: us-east-1
  Enter a value:

template_file.cloudtrailwatch_sns_permissions: Refreshing state... (ID: 88f56cab9e1a2b98a6751da9e316615e9e9dbf9e23b339063563e2e864570bf2)
aws_sns_topic.CloudTrailWatch: Refreshing state... (ID: arn:aws:sns:us-east-1:736306583401:CloudTrailWatch)
aws_sns_topic.CloudTrailWatch: Modifying...
  policy: "{\"Version\":\"2012-10-17\",\"Id\":\"__default_policy_ID\",\"Statement\":[{\"Sid\":\"__default_statement_ID\",\"Effect\":\"Allow\",\"Principal\":{\"AWS\":\"*\"},\"Action\":[\"SNS:Subscribe\",\"SNS:ListSubscriptionsByTopic\",\"SNS:DeleteTopic\",\"SNS:GetTopicAttributes\",\"SNS:Publish\",\"SNS:RemovePermission\",\"SNS:AddPermission\",\"SNS:Receive\",\"SNS:SetTopicAttributes\"],\"Resource\":\"arn:aws:sns:eu-west-1:736306583401:CloudTrailWatch\",\"Condition\":{\"StringEquals\":{\"AWS:SourceOwner\":\"736306583401\"}}},{\"Sid\":\"AWSEvents_cloudtrail-manipulation_SendToSNS\",\"Effect\":\"Allow\",\"Principal\":{\"Service\":\"events.amazonaws.com\"},\"Action\":\"sns:Publish\",\"Resource\":\"arn:aws:sns:eu-west-1:736306583401:CloudTrailWatch\"}]}" => "{\"Statement\":[{\"Effect\":\"Allow\",\"Resource\":\"arn:aws:sns:eu-west-1:736306583401:CloudTrailWatch\",\"Condition\":{\"StringEquals\":{\"AWS:SourceOwner\":\"736306583401\"}},\"Action\":[\"SNS:Subscribe\",\"SNS:ListSubscriptionsByTopic\",\"SNS:DeleteTopic\",\"SNS:GetTopicAttributes\",\"SNS:Publish\",\"SNS:RemovePermission\",\"SNS:AddPermission\",\"SNS:Receive\",\"SNS:SetTopicAttributes\"],\"Sid\":\"__default_statement_ID\",\"Principal\":{\"AWS\":\"*\"}},{\"Action\":\"sns:Publish\",\"Principal\":{\"Service\":\"events.amazonaws.com\"},\"Sid\":\"AWSEvents_cloudtrail-manipulation_SendToSNS\",\"Effect\":\"Allow\",\"Resource\":\"arn:aws:sns:eu-west-1:736306583401:CloudTrailWatch\"}],\"Version\":\"2012-10-17\",\"Id\":\"__default_policy_ID\"}"
aws_sns_topic.CloudTrailWatch: Modifications complete

Apply complete! Resources: 0 added, 1 changed, 0 destroyed.

The state of your infrastructure has been saved to the path
below. This state is required to modify and destroy your
infrastructure, so keep it safe. To inspect the complete state
use the `terraform show` command.

State path: terraform.tfstate

Outputs:

  CloudTrailWatch_arn = arn:aws:sns:us-east-1:1234567890:CloudTrailWatch

Expected result

Only applies changes if required.

[stack72]

Hi @keymon

This if statement feels redundant - we are doing the same thing in the if & else - is this actually the case?

[keymon]

Ops, sorry. It should not be the same fixed. I don't want to change the behaviour of the other attributes, that is why I got the if.

[keymon]

If you consider necessary I could write a test for this case.

But I wonder if you could point me to an example that checks that the plan is empty.

Or maybe we can change https://github.com/hashicorp/terraform/blob/master/helper/resource/testing.go#L307 to add the option: ExpectEmptyPlan

[keymon]

@dcarley pointed me that the main terraform tests do that already here so I only need to change the order of the policy.

Doing so...

[keymon]

I added a test by modifying the order of the attributes in the policy:

$ git checkout HEAD~
....
$ TF_LOG=1 make testacc TEST=./builtin/providers/aws TESTARGS='-run=.*AWSSNSTopic.*' 2>~/tf.log
==> Checking that code complies with gofmt requirements...
/home/keymon/.gvm/pkgsets/go1.6/global/bin/stringer
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=.*AWSSNSTopic.* -timeout 120m
=== RUN   TestAccAWSSNSTopicSubscription_basic
--- PASS: TestAccAWSSNSTopicSubscription_basic (16.11s)
=== RUN   TestAccAWSSNSTopic_basic
--- PASS: TestAccAWSSNSTopic_basic (9.95s)
=== RUN   TestAccAWSSNSTopic_withIAMRole
--- FAIL: TestAccAWSSNSTopic_withIAMRole (13.63s)
    testing.go:154: Step 0 error: After applying this step, the plan was not empty:

        DIFF:

        UPDATE: aws_sns_topic.test_topic
          policy: "{\"Version\":\"2012-10-17\",\"Id\":\"Policy1445931846145\",\"Statement\":[{\"Sid\":\"Stmt1445931846145\",\"Effect\":\"Allow\",\"Principal\":{\"AWS\":\"arn:aws:iam::123456789:role/test/terraform_bug\"},\"Action\":\"sns:Publish\",\"Resource\":\"arn:aws:sns:us-west-2::example\"}]}" => "{\"Statement\":[{\"Sid\":\"Stmt1445931846145\",\"Effect\":\"Allow\",\"Principal\":{\"AWS\":\"arn:aws:iam::123456789:role/test/terraform_bug\"},\"Action\":\"sns:Publish\",\"Resource\":\"arn:aws:sns:us-west-2::example\"}],\"Version\":\"2012-10-17\",\"Id\":\"Policy1445931846145\"}"

        STATE:

        aws_iam_role.example:
          ID = terraform_bug
          arn = arn:aws:iam::736306583401:role/test/terraform_bug
          assume_role_policy = {
          "Version": "2012-10-17",
          "Statement": [
            {
              "Action": "sts:AssumeRole",
              "Principal": {
                "Service": "ec2.amazonaws.com"
              },
              "Effect": "Allow",
              "Sid": ""
            }
          ]
        }

          name = terraform_bug
          path = /test/
          unique_id = AROAIAPQB3HKCRKZQ2NJC
        aws_sns_topic.test_topic:
          ID = arn:aws:sns:us-west-2:123456789:example
          arn = arn:aws:sns:us-west-2:123456789:example
          display_name = 
          name = example
          policy = {"Version":"2012-10-17","Id":"Policy1445931846145","Statement":[{"Sid":"Stmt1445931846145","Effect":"Allow","Principal":{"AWS":"arn:aws:iam::123456789:role/test/terraform_bug"},"Action":"sns:Publish","Resource":"arn:aws:sns:us-west-2::example"}]}

          Dependencies:
            aws_iam_role.example
FAIL
exit status 1
FAIL    github.com/hashicorp/terraform/builtin/providers/aws    39.701s

And with the new code:

$ git checkout bugfix/format_sns_topic_policy
....
$ TF_LOG=1 make testacc TEST=./builtin/providers/aws TESTARGS='-run=.*AWSSNSTopic.*' 2>~/tf.log
==> Checking that code complies with gofmt requirements...
/home/keymon/.gvm/pkgsets/go1.6/global/bin/stringer
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=.*AWSSNSTopic.* -timeout 120m
=== RUN   TestAccAWSSNSTopicSubscription_basic
--- PASS: TestAccAWSSNSTopicSubscription_basic (16.81s)
=== RUN   TestAccAWSSNSTopic_basic
--- PASS: TestAccAWSSNSTopic_basic (9.85s)
=== RUN   TestAccAWSSNSTopic_withIAMRole
--- PASS: TestAccAWSSNSTopic_withIAMRole (19.69s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    46.362s

[stack72]

Thanks for the explanations @keymon

Will test this now

P

[stack72]

Hi @keymon

this looks good - the tests pass as expected - thanks so much for the PR!

Paul

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