crossplane-contrib / provider-upjet-aws

Official AWS Provider for Crossplane by Upbound.
https://marketplace.upbound.io/providers/upbound/provider-aws
Apache License 2.0
137 stars 112 forks source link

Fix Topic.sns update loops #1347

Closed ulucinar closed 3 weeks ago

ulucinar commented 3 weeks ago

Description of your changes

We've observed update loops with the Topic.sns resources when inline policies are given. The desired policy document in the spec can differ from the actual (observed) document in the following two ways:

And what's observed could then be:

      {
        "Version": "2008-10-17",
        "Statement": [
        {
          "Sid": "publish",
          "Effect": "Allow",
          "Principal": {
            "AWS": "arn:aws:iam::123456789110:role/sample-role"
          },
          "Action": "sns:Publish",
          "Resource": "arn:aws:sns:us-west-1:123456789110:resource1"
        },
        {
          "Sid": "get-attributes",
          "Effect": "Allow",
          "Principal": {
            "AWS": "arn:aws:iam::123456789110:role/sample-role"
          },
          "Action": "sns:GetTopicAttributes",
          "Resource": "arn:aws:sns:us-west-1:123456789110:resource1"
        },
        {
          "Sid": "list-subscriptions",
          "Effect": "Allow",
          "Principal": {
            "AWS": "arn:aws:iam::123456789110:role/sample-role"
          },
          "Action": "sns:ListSubscriptionsByTopic",
          "Resource": "arn:aws:sns:us-west-1:123456789110:resource1"
        }
        ]
      }

Please note that the declared AWS IAM principals are JSON arrays whereas the observed ones are strings.

This PR proposes to introduce a custom Terraform diff to filter out such differences that result in an update loop. This is already implemented as a diff suppress function in the underlying Terraform provider. We should consider making sure that these suppress functions are properly invoked in a future iteration but it will result in a larger change that will require more rigorous testing.

This PR also adds a Topic.sns example manifest with an inline policy document to test the fix.

I have:

How has this code been tested

ulucinar commented 3 weeks ago

/test-examples="examples/sns/v1beta1/topic-with-policy.yaml"

ulucinar commented 3 weeks ago

/test-examples="examples/sns/v1beta1/topic-with-policy.yaml"

ulucinar commented 3 weeks ago

/test-examples="examples/sns/v1beta1/topic-with-policy.yaml"

ulucinar commented 3 weeks ago

/test-examples="examples/sns/v1beta1/topic-with-policy.yaml"

ulucinar commented 3 weeks ago

/test-examples="examples/sns/v1beta1/topic-with-policy.yaml"

ulucinar commented 3 weeks ago

/test-examples="examples/sns/v1beta1/topic-with-policy.yaml"

mbbush commented 3 weeks ago

I've definitely observed this same update loop in other resources that contain an IAM policy, in addition to one more: converting an AWS account id to the ARN of that account's root in a Principal element.

Do we have an issue to track these improvements?