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

Allow Specification of the CRD API Version a Controller Watches & Reconciles #400

Merged
merged 6 commits into from
May 14, 2024

Conversation

ulucinar
Copy link
Collaborator

@ulucinar ulucinar commented May 10, 2024

Description of your changes

This PR adds config.Resource.ControllerReconcileVersion through which the CRD API version which the associated controller will watch for & reconcile can be specified. This will allow us to set the controller's watch API version to the storage version of the CRD to prevent webhook conversions for multi-versioned CRDs.

In order to support this, we also add config.Resource.TerraformConversions which accepts a list of config.TerraformConversion implementations. These TerraformConversions are responsible for converting data before it's passed down to the Terraform layer from the Crossplane layer and also for converting it back when it's carried up to the Crossplane layer from the Terraform layer. Currently, the only implementation is the config.singletonListConversion, which is responsible for converting between the singleton lists and embedded objects for converted APIs. Previously, this bidirectional conversion logic was hard-coded into the external client. This PR allows us to abstract this conversion logic behind the config.TerraformConversion interface.

We also address a bug in this PR discovered by @sergenyalcin in the context of crossplane-contrib/provider-upjet-gcp#508. The singleton list CRD API converter (invoked by the conversion webhook) only handled API list API conversions in spec.forProvider, ignoring spec.initProvider and status.atProvider, where such conversions are still relevant.

This PR also deprecates config.Reference.Type in favor of config.Reference.TerraformName. We had introduced config.Reference.TerraformName in #12 and it's a more stable and less error prone API compared to config.Reference.Type because it automatically accounts for the configuration changes affecting the resource kind name, group or version. We've already been discouraging the config.Reference.Type in favor of config.Reference.TerraformName since it's been introduced.

I have:

  • Read and followed Upjet'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

Manually tested via provider-upjet-{azuread,gcp} resources. Will also be tested in the uptest runs in crossplane-contrib/provider-upjet-gcp#508.

…the specific CR API version

the associated controller will watch & reconcile.

- For backwards-compatibility, ControllerReconcileVersion defaults to Version if unspecified.

Signed-off-by: Alper Rifat Ulucinar <[email protected]>
…ersions between

the Crossplane & Terraform layers.

Signed-off-by: Alper Rifat Ulucinar <[email protected]>
… path prefixes

- Previously, it only converted parameters under spec.forProvider
- This missed CRD API conversions (of singleton lists) under
  spec.initProvider & status.atProvider

Signed-off-by: Alper Rifat Ulucinar <[email protected]>
(cherry picked from commit 08b3f3260ce86457271dda7401dfd9a69a10f656)
…rsion to convert between

singleton lists and embedded objects when parameters pass Crossplane and Terraform boundaries.

Signed-off-by: Alper Rifat Ulucinar <[email protected]>
…mName

- TerraformName will automatically handle name & version configurations that will affect
  the generated cross-resource reference. This is crucial especially if the
  provider generates multiple versions for its MR APIs.

Signed-off-by: Alper Rifat Ulucinar <[email protected]>
Copy link
Member

@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!

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.

2 participants