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

Migrate AWS to Simple Resource Class Selection #44

Merged
merged 15 commits into from
Oct 24, 2019

Conversation

negz
Copy link
Member

@negz negz commented Oct 23, 2019

Description of your changes

Fixes #41

This PR:

  • Adds comment markers to make all providers, resources, and classes cluster scoped.
  • Regenerates all generated code and manifests (using Support simple resource class selection crossplane/crossplane-tools#7) to support cluster scoped managed resources.
  • Updates all controller logic to reflect that providers and managed resources are no longer namespaced, and thus they must specify which namespace their secrets live in.

It's a pretty huge diff but I've tried to keep everything broken down into logical commits to make the reviewing a little easier. Any changes to managed resource reconcilers that aren't mechanically removing namespaces and updating types are to ensure managed resource connection secrets are persisted to their desired namespace, and handle the case in which the (optional) writeConnectionSecretToRef field is nil.

I'm pretty confident in the claim defaulting and scheduling logic after testing it in per crossplane/crossplane-runtime#48 and crossplane-contrib/provider-azure#35. For this stack specifically so far I have tested:

  • Creating a ReplicationGroup via a RedisCluster claim.
  • Creating an RDSInstance via a PostgreSQLInstance claim.
  • Creating an RDSInstance via a MySQLInstance claim.
  • Creating an EKSCluster via a KubernetesCluster claim.
  • Creating an S3Bucket via a Bucket claim.

At this stage I'm leaning toward not explicitly testing all the connectivity primitives. The changes to their types were very mechanical - they have no claims or connection secrets so it was just updating the logic that gets the provider secret. We should still test them holistically once all our in flight changes land - I imagine any bugs I might have introduced here would be quickly fixable.

Checklist

I have:

  • Run make reviewable to ensure this PR is ready for review.
  • Ensured this PR contains a neat, self documenting set of commits.
  • Updated any relevant documentation, examples, or release notes.
  • Updated the dependencies in app.yaml to include any new role permissions.

@negz negz requested a review from soorena776 October 23, 2019 05:00
@negz negz force-pushed the notsoclassy branch 3 times, most recently from bae7c13 to fe6605e Compare October 23, 2019 05:04
@negz
Copy link
Member Author

negz commented Oct 23, 2019

@soorena776 fe6605e may be of interest. It seems the resource.CanReference argument to Build and GetStatus will be superfluous once all managed resources are cluster scoped.

@negz
Copy link
Member Author

negz commented Oct 23, 2019

Uh, note to morning self: you forgot to add the new resource claim scheduling and defaulting controllers in this PR.

@upbound-bot
Copy link
Collaborator

63% (-2.88%) vs master 66%

@upbound-bot
Copy link
Collaborator

63% (-2.88%) vs master 66%

Copy link
Contributor

@soorena776 soorena776 left a comment

Choose a reason for hiding this comment

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

@negz This looks great, thanks for putting all these together so quickly!
As for the passed CanReference object, they don't seem necessary here, but I think the api verbosity is still worth to support potential local referencers that we might come up later.
/LGTM

@upbound-bot
Copy link
Collaborator

63% (-2.89%) vs master 66%

Also updates the (now cluster scoped) provider to require the namespace of its
secret be specified.

Signed-off-by: Nic Cope <[email protected]>
It seems like resource.CanReference probably doesn't need metav1.Object embedded
anymore. We might consider just using runtime.Object rather than
resource.CanReference.

Signed-off-by: Nic Cope <[email protected]>
@upbound-bot
Copy link
Collaborator

63% (-2.89%) vs master 66%

@negz
Copy link
Member Author

negz commented Oct 24, 2019

Noticed #46 while testing this out.

@negz
Copy link
Member Author

negz commented Oct 24, 2019

At this stage I'm leaning toward not explicitly testing all the connectivity primitives.

I actually ended up testing this using @soorena776's resource referencing work per soorena776/crossplane#1 and it has all worked.

@negz
Copy link
Member Author

negz commented Oct 24, 2019

Looks like everything is working as expected. I'm going to hit merge.

@negz negz merged commit 60869ac into crossplane-contrib:master Oct 24, 2019
@negz negz deleted the notsoclassy branch October 24, 2019 03:43
wolffbe pushed a commit to wolffbe/provider-aws that referenced this pull request Feb 12, 2021
Migrate AWS to Simple Resource Class Selection
namku pushed a commit to namku/provider-aws that referenced this pull request Mar 9, 2021
Migrate AWS to Simple Resource Class Selection
hanlins pushed a commit to hanlins/provider-aws that referenced this pull request Jun 7, 2022
teeverr pushed a commit to swisscom/provider-aws that referenced this pull request Jun 7, 2024
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.

Implement Simple Resource Class Selection for AWS
3 participants