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

Fix Topic.sns update loops #1347

Merged
merged 2 commits into from
Jun 7, 2024

Conversation

ulucinar
Copy link
Collaborator

@ulucinar ulucinar commented Jun 6, 2024

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:

  • Missing Version node. An example is as follows:
      {
        "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"
        }
        ]
      }

What's observed constains a Version node:

      {
        "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 the Version node in the observed policy document.

  • Topology differences:
    What's declared in spec could be:
      {
        "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"
        }
        ]
      }

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:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
@ulucinar
Copy link
Collaborator Author

ulucinar commented Jun 6, 2024

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

@ulucinar ulucinar marked this pull request as draft June 6, 2024 19:03
@ulucinar
Copy link
Collaborator Author

ulucinar commented Jun 6, 2024

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

…fs for Topic.sns

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
@ulucinar ulucinar force-pushed the fix-topic-updates branch from 7439bd3 to 747f4cb Compare June 6, 2024 19:42
@ulucinar
Copy link
Collaborator Author

ulucinar commented Jun 6, 2024

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

1 similar comment
@ulucinar
Copy link
Collaborator Author

ulucinar commented Jun 6, 2024

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

@ulucinar ulucinar force-pushed the fix-topic-updates branch from 4e5071f to 5ad677f Compare June 6, 2024 21:52
@ulucinar
Copy link
Collaborator Author

ulucinar commented Jun 6, 2024

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

@ulucinar ulucinar force-pushed the fix-topic-updates branch from 5ad677f to 747f4cb Compare June 7, 2024 08:16
@ulucinar
Copy link
Collaborator Author

ulucinar commented Jun 7, 2024

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

@ulucinar ulucinar marked this pull request as ready for review June 7, 2024 08:37
Copy link
Collaborator

@turkenf turkenf left a comment

Choose a reason for hiding this comment

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

Thank you @ulucinar, LGTM.

Copy link
Collaborator

@sergenyalcin sergenyalcin left a comment

Choose a reason for hiding this comment

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

Thanks @ulucinar LGTM!

@ulucinar ulucinar merged commit 0e54c26 into crossplane-contrib:main Jun 7, 2024
22 checks passed
@ulucinar ulucinar deleted the fix-topic-updates branch June 7, 2024 09:19
@mbbush
Copy link
Collaborator

mbbush commented Jun 7, 2024

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?

@haarchri haarchri mentioned this pull request Jun 12, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants