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

Enable Conversion Webhooks #676

Merged
merged 1 commit into from
Mar 21, 2024

Conversation

ulucinar
Copy link
Collaborator

@ulucinar ulucinar commented Mar 19, 2024

Description of your changes

This PR enables the conversion webhooks in preparation for the v1.0.0 release, which will potentially be introducing the v1beta2 versions of some API groups together with their associated conversion functions. This PR also adds the --certs-dir command-line options to the family's resource providers, with the following protocol for configuring the conversion Webhook TLS certificate & key:

  1. If the --certs-dir command-line option is supplied, it's used.
  2. If the --certs-dir command-line option is not supplied, the following environment variables are used in the given order: CERTS_DIR (for consistency with the upbound/provider-aws), TLS_SERVER_CERTS_DIR (the new environment variable, which has replaced the WEBHOOK_TLS_CERT_DIR env. variable in Crossplane), and WEBHOOK_TLS_CERT_DIR (for backwards-compatibility).
  3. The default path for the webhook TLS certificate and key is /tls/server.

It also incorporates the resolver transformer from crossplane/upjet#331 to prevent the import cycles that frequently occur when a managed resource's API version is bumped leaving some other managed resources in its group at their previous versions. The transformer is run as part of the code generation pipeline defined in apis/generate.go as follows:

go run github.com/crossplane/upjet/cmd/resolver -g azure.upbound.io -a github.com/upbound/provider-azure/internal/apis -s

The dynamic reference source initialization is implemented using a type registry in internal/apis/scheme.go

When --certs-dir is set to the empty string (--certs-dir=""), the conversion webhooks are disabled to facilitate the local development of resources. --certs-dir="" is also used for the run make target so that developers running the provider locally will not need to bother with configuring the webhook TLS certificates. When running the webhooks are needed for local development, --certs-dir can be set to the folder containing the tls.crt and the tls.key files.

Apart from the discussion in crossplane/upjet#321, this PR takes care of generating CRDs with the conversion strategy set to Webhook. However, the generated CRDs containing the conversion strategy are not stored under package/crds under the repo root. That path still contains the generated CRDs but without the conversion stanzas and they can still be used during development of the managed resources, and for running the provider process locally without the need to configure any TLS certificates. While developing the implementation of a managed resource, there's generally no need to involve the conversion webhooks and the conversion function chains they invoke, unless you're specifically interested in the conversion functions (or the upjet generated spoke implementations). Although the generated CRDs at package/crds do not contain the conversion stanzas, if there's at least one resource in the provider with multiple API versions, the provider, by default, attempts to start the conversion Webhook server, which will fail if the TLS certificates are not properly configured. In such cases, if you'd like to run the provider with the Webhook server disabled, then you can just set the --certs-dir command line argument to an empty string to disable the conversion webhook server, i.e., by specifying --certs-dir="".

The PR uses kustomize together with the yq to generate the CRDs with the proper conversion stanzas specifying the conversion strategy of Webhook for converting between the hub & spoke API versions. As discussed above, the CRDs specifying the conversion strategies are not stored at the path package/crds and instead, they can be found under the path _output/package/crds in the repo root. yq is used to split the multi-doc YAML output from kustomize as the up xpkg batch command relies on the generated CRD filenames while preparing the provider family (resource) packages. Various alternatives have been considered instead of incorporating yq in the code generation pipeline as detailed in the description of upbound/build#249. Notably a shell script implementation of the split function was considerably slower than the yq --split-exp command and we've chosen to incorporate yq into our code generation pipeline.

yq has been incorporated as a kustomize plugin with the name SplitTransformer. Another alternative was to just pipe the output from kustomize (via a shell pipe) to the yq --split-exp command. However, the plugin approach is better for bundling the kustomization transformations as a reusable module. We will consider moving the kustomization configuration for generating CRDs with the proper conversion stanzas as a reusable component to uptest (where there are already some reusable components related with the official provider repos' build pipelines) or to upjet.

This PR also:

  • Pins the UXP version to v1.14.6-up.1
  • Bumps uptest to v0.11.1
  • Bump the build submodule to commit 75a9fe3a

I have:

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

How has this code been tested

  • make run succeeds to provision a ResourceGroup.azure resource after the CRDs without the conversion stanzas are applied with the command kubectl apply -f package/crds in the repo root.

@ulucinar
Copy link
Collaborator Author

/test-examples="examples/azure/resourcegroup.yaml"

@ulucinar ulucinar force-pushed the multiversion-crds branch from 79d6dd1 to afc1947 Compare March 19, 2024 18:46
- Run the resolver transformer to prevent import cycles in cross-resource reference resolvers
- Pin the UXP version to v1.14.6-up.1
- Bump uptest to v0.11.1
- Bump the build submodule to commit 75a9fe3a

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
@ulucinar ulucinar force-pushed the multiversion-crds branch from afc1947 to 41eff46 Compare March 19, 2024 18:48
@ulucinar
Copy link
Collaborator Author

/test-examples="examples/azure/resourcegroup.yaml"

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
Copy link
Collaborator Author

We need to decrease the memory footprint of the linter in this repo also. A local run of make lint succeeded.

@ulucinar ulucinar merged commit bd59c29 into crossplane-contrib:main Mar 21, 2024
9 of 10 checks passed
@ulucinar ulucinar deleted the multiversion-crds branch March 21, 2024 09:39
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