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

Skip late initialization for several duplicate resource policy fields #1213

Merged
merged 10 commits into from
Mar 28, 2024

Conversation

mbbush
Copy link
Collaborator

@mbbush mbbush commented Mar 13, 2024

Description of your changes

In several places in the terraform aws provider, there are two ways to specify a resource policy: a field named "policy" on the "foo" resource, and a separate resource named "foo_policy".

If we'd realized this before we'd committed to a stable API, perhaps the best option would have been to only provide one of those options, but it's too late for that now.

This PR removes conflicts between the resource policy managed on the parent resource, and the resource policy managed on the Policy resource, by skipping Late Initialization of the policy field on the parent resource, for the following resources:

  • Queue.sqs
  • Topic.sns
  • Role.iam (inlinePolicies)
  • Key.kms
  • VpcEndpoint.ec2

I made a semi-manual pass through the terraform aws provider docs, and these were the only examples I found, although I was not systematic and may have missed something. It seems like the resources with this behavior are the older resources, added to the terraform aws provider before they adopted more uniform best practices.

I initially included the VPC endpoint and KMS key, which both have a spec.forProvider.policy field, and a separate resource in terraform provider aws that sets that policy. However, neither the aws_kms_key_policy nor aws_vpc_endpoint_policy resources exist in this provider yet. I'm undecided about whether it's better to skip late init for these parameters because we may someday implement the corresponding policy resource, whether we should avoid implementing those resources because the field already exists on the parent resource, or whether we should leave them be and defer the decision to the future. For now I've backed those commits out, but left them in the git history so they can be easily restored if desireable.

While I was going through I updated the config files to the latest standards as well.

Fixes #1207

I have:

  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Running uptest on this PR.

I verified by looking at the uptest logs that the policy parameter was set in the status.atProvider and unset in the spec.forProvider, when that was expected, for:

  • Topic.sns (default policy)
  • Topic.sns (custom policy from a TopicPolicy resource)
  • Queue.sqs (custom policy from a QueuePolicy resource)
  • Role.iam (custom policy from a RolePolicy resource)
  • Key.kms (on the commit that included it)
  • Key.kms (now that it's excluded, I verified that the policy is being late initialized.)

SQS queues have a default policy of "", and IAM roles have a default inlinePolicies that is null/empty array, so there's nothing to verify in the default case for those resources.

@mbbush mbbush force-pushed the matt/policy-late-init branch 4 times, most recently from 91042c7 to 68c8964 Compare March 16, 2024 07:18
@mbbush
Copy link
Collaborator Author

mbbush commented Mar 24, 2024

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

mbbush added 7 commits March 23, 2024 21:50
Signed-off-by: Matt Bush <mbbush@gmail.com>
Signed-off-by: Matt Bush <mbbush@gmail.com>
Signed-off-by: Matt Bush <mbbush@gmail.com>
Signed-off-by: Matt Bush <mbbush@gmail.com>
Signed-off-by: Matt Bush <mbbush@gmail.com>
Signed-off-by: Matt Bush <mbbush@gmail.com>
Signed-off-by: Matt Bush <mbbush@gmail.com>
@mbbush mbbush force-pushed the matt/policy-late-init branch from 2b9a651 to 7d447bf Compare March 24, 2024 04:51
@mbbush mbbush marked this pull request as draft March 24, 2024 04:51
@mbbush
Copy link
Collaborator Author

mbbush commented Mar 24, 2024

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

@mbbush
Copy link
Collaborator Author

mbbush commented Mar 24, 2024

/test-examples="examples/iam/v1beta1/role.yaml,examples/iam/v1beta1/role-with-inline-policy.yaml,examples/iam/v1beta1/rolepolicy.yaml"

@mbbush
Copy link
Collaborator Author

mbbush commented Mar 24, 2024

/test-examples="examples/sqs/v1beta1/queue.yaml,examples/sqs/v1beta1/queuepolicy.yaml"

@mbbush
Copy link
Collaborator Author

mbbush commented Mar 24, 2024

/test-examples="examples/kms/v1beta1/key.yaml"

mbbush added 2 commits March 23, 2024 22:22
It turns out we don't implement the kms_key_policy terraform resource
yet.

This reverts commit b164b1b.

Signed-off-by: Matt Bush <mbbush@gmail.com>
We don't implement the VpcEndpointPolicy resource yet.

This reverts commit b336310.

Signed-off-by: Matt Bush <mbbush@gmail.com>
@mbbush
Copy link
Collaborator Author

mbbush commented Mar 24, 2024

/test-examples="examples/kms/v1beta1/key.yaml"

@mbbush mbbush marked this pull request as ready for review March 24, 2024 05:39
@turkenf
Copy link
Collaborator

turkenf commented Mar 27, 2024

/test-examples="examples/iam/v1beta1/user.yaml"

Uptest run: https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/8445029595

@turkenf
Copy link
Collaborator

turkenf commented Mar 27, 2024

/test-examples="examples/iam/v1beta1/role.yaml"

Uptest run: https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/8445085712

@turkenf
Copy link
Collaborator

turkenf commented Mar 27, 2024

/test-examples="examples/iam/v1beta1/usergroupmembership.yaml"

Uptest run: https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/8445097629

@turkenf
Copy link
Collaborator

turkenf commented Mar 27, 2024

/test-examples="examples/sns/v1beta1/topicsubscription.yaml"

Uptest run: https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/8445107700

@turkenf
Copy link
Collaborator

turkenf commented Mar 27, 2024

/test-examples="examples/sqs/v1beta1/queue.yaml"

Uptest run: https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/8445120881

@turkenf
Copy link
Collaborator

turkenf commented Mar 27, 2024

Not blocker: If possible and if not take much time, could you please make uptesable the InstanceProfile.iam.aws.upbound.io/v1beta1 resource with this PR?

Signed-off-by: Matt Bush <mbbush@gmail.com>
@turkenf
Copy link
Collaborator

turkenf commented Mar 27, 2024

/test-examples="examples/iam/v1beta1/instanceprofile.yaml"

Uptest run: https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/8457149351

@turkenf
Copy link
Collaborator

turkenf commented Mar 27, 2024

/test-examples="examples/s3/v1beta1/bucketnotification.yaml"

Uptest run: https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/8458801047

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 for your effort in this PR @mbbush 🙌

@turkenf turkenf merged commit f87412a into crossplane-contrib:main Mar 28, 2024
13 checks passed
@mbbush mbbush deleted the matt/policy-late-init branch April 20, 2024 00:30
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.

[Bug]: IAM Role inlinePolicy conflicting with IAM RolePolicy
2 participants