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 #468

Merged
merged 3 commits into from Mar 5, 2024

Conversation

ulucinar
Copy link
Collaborator

@ulucinar ulucinar commented Mar 4, 2024

Description of your changes

This PR enables the conversion webhooks in preparation for the v1.0.0 release, which will 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 gcp.upbound.io -a github.com/upbound/provider-gcp/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.

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 ServiceAccount.cloudplatform resource after the CRDs without the conversion stanzas are applied with the command kubectl apply -f package/crds in the repo root.

@sergenyalcin
Copy link
Collaborator

sergenyalcin commented Mar 4, 2024

@ulucinar thanks for this PR. We need to remove the following two statements from the apis/generate.go file

  • //go:generate bash -c "find . -iname 'zz_*' ! -iname 'zz_generated.managed*.go' -delete"
  • //go:generate rm -rf ../examples-generated

We removed these statements during enabling the multiversion crd generation. If we do not delete these statements, we remove old crd versions and generated example manifests for old versions.

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
…ce reference resolvers

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
@ulucinar
Copy link
Collaborator Author

ulucinar commented Mar 4, 2024

/test-examples="examples/cloudplatform/serviceaccount.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 ulucinar merged commit 3767a46 into crossplane-contrib:main Mar 5, 2024
10 checks passed
@ulucinar ulucinar deleted the enable-webhooks branch March 5, 2024 13:24
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.

None yet

2 participants