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

Add Support for Observe Only Resources #672

Merged
merged 5 commits into from
Apr 18, 2023

Conversation

turkenh
Copy link
Contributor

@turkenh turkenh commented Apr 13, 2023

Description of your changes

This PR adds support for Observe Only resources.

I have:

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

How has this code been tested

Basic Observe Scenario

I have created an Observe Only RDS instance as follows:

apiVersion: rds.aws.upbound.io/v1beta1
kind: Instance
metadata:
  annotations:
    crossplane.io/external-name: example-dbinstance-hasan
  name: rds-to-observe
spec:
  managementPolicy: ObserveOnly
  forProvider:
    region: us-west-1
  writeConnectionSecretToRef:
    name: observed-rds-out
    namespace: default

And verified that the resource was ready&synced and all the fields at status.atProvider were appropriately filled.

Upgrade Path

  1. Installed upbound/provider-aws:v0.32.1 from the Marketplace.
  2. Created an RDS instance and KMS Key using the example file.
  3. Wait until both are Ready & Synced.
  4. Upgraded the provider to my local built from this PR.
  5. Verified that the provider upgrade was successful and reported as Installed & Healthy.
  6. Previous resources are still Ready & Synced.
  7. Created a controller config and referred to it from provider to enable management policies.
  8. Previous resources are still Ready & Synced and provider runs properly with the alpha feature enabled.
  9. Created an Observe Only RDS instance using the yaml above.
  10. Verified that it becomes Ready & Synced with all observed fields filled appropriately.

@ulucinar
Copy link
Collaborator

Locally rebased this PR so that up dependency is v0.16.1, built ulucinar/provider-aws:v0.34.0-rc.0.10.gad098f2c from this PR, and successfully validated it. We can merge this after @sergenyalcin's performance regression tests.

// region as a parameter for all resources to be consistent with
// the native aws provider, and now, we need to add manually it to
// the identifier fields for all resources.
r.ExternalName.IdentifierFields = append(r.ExternalName.IdentifierFields, "region")
Copy link
Collaborator

Choose a reason for hiding this comment

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

My understanding is that we do not need to treat the iam group specially because although spec.forProvider.region is declared as an identifier field because that field is missing for the managed resources in that group, it becomes ineffective.

@sergenyalcin
Copy link
Collaborator

We can merge this after @sergenyalcin's performance regression tests.

I did some performance tests by building an image from this branch. Any performance regression was not observed in these tests.

image

Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Thanks @turkenh, lgtm.

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.

3 participants