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

✨ Webhook URL based CRD conversions #3090

Merged
merged 5 commits into from
Apr 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 77 additions & 0 deletions config/crds/apis.kcp.io_apiresourceschemas.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,80 @@ spec:
spec:
description: Spec holds the desired state.
properties:
conversion:
description: conversion defines conversion settings for the defined
custom resource.
properties:
strategy:
description: 'strategy specifies how custom resources are converted
between versions. Allowed values are: - `"None"`: The converter
only change the apiVersion and would not touch any other field
in the custom resource. - `"Webhook"`: API Server will call
to an external webhook to do the conversion. Additional information
is needed for this option. This requires spec.preserveUnknownFields
to be false, and spec.conversion.webhook to be set.'
enum:
- None
- Webhook
type: string
webhook:
description: webhook describes how to call the conversion webhook.
Required when `strategy` is set to `"Webhook"`.
properties:
clientConfig:
description: clientConfig is the instructions for how to call
the webhook if strategy is `Webhook`.
properties:
caBundle:
description: caBundle is a PEM encoded CA bundle which
will be used to validate the webhook's server certificate.
If unspecified, system trust roots on the apiserver
are used.
format: byte
type: string
url:
description: "url gives the location of the webhook, in
standard URL form (`scheme://host:port/path`). \n Please
note that using `localhost` or `127.0.0.1` as a `host`
is risky unless you take great care to run this webhook
on all hosts which run an apiserver which might need
to make calls to this webhook. Such installs are likely
to be non-portable, i.e., not easy to turn up in a new
cluster. \n The scheme must be \"https\"; the URL must
begin with \"https://\". \n A path is optional, and
if present may be any string permissible in a URL. You
may use the path to pass an arbitrary string to the
webhook, for example, a cluster identifier. \n Attempting
to use a user or basic auth e.g. \"user:password@\"
is not allowed. Fragments (\"#...\") and query parameters
(\"?...\") are not allowed, either. \n Note: kcp does
not support provided service names like Kubernetes does."
format: uri
type: string
type: object
conversionReviewVersions:
description: conversionReviewVersions is an ordered list of
preferred `ConversionReview` versions the Webhook expects.
The API server will use the first version in the list which
it supports. If none of the versions specified in this list
are supported by API server, conversion will fail for the
custom resource. If a persisted Webhook configuration specifies
allowed versions and does not include any versions known
to the API Server, calls to the webhook will fail.
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- conversionReviewVersions
type: object
required:
- strategy
type: object
x-kubernetes-validations:
- message: Webhook must be specified if strategy=Webhook
rule: (self.strategy == 'None' && !has(self.webhook)) || (self.strategy
== 'Webhook' && has(self.webhook))
group:
description: "group is the API group of the defined custom resource.
Empty string means the core API group. \tThe resources are served
Expand Down Expand Up @@ -277,6 +351,9 @@ spec:
- scope
- versions
type: object
x-kubernetes-validations:
- message: Conversion must be specified when multiple versions exist
rule: size(self.versions) == 1 || (size(self.versions) > 1 && has(self.conversion))
type: object
served: true
storage: true
Expand Down
110 changes: 109 additions & 1 deletion pkg/openapi/zz_generated.openapi.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

59 changes: 39 additions & 20 deletions pkg/reconciler/apis/apibinding/apibinding_reconcile.go
Original file line number Diff line number Diff line change
Expand Up @@ -271,27 +271,24 @@ func (r *bindingReconciler) reconcile(ctx context.Context, apiBinding *apisv1alp
return reconcileStatusStopAndRequeue, err
}

// If there are multiple versions, there must be an APIConversion
if len(schema.Spec.Versions) > 1 {
if _, err := r.getAPIConversion(logicalcluster.From(schema), schema.Name); err != nil {
// Need to wait until the APIConversion is present before we can proceed to create the bound CRD
conditions.MarkFalse(
apiBinding,
apisv1alpha1.APIExportValid,
apisv1alpha1.InternalErrorReason,
conditionsv1alpha1.ConditionSeverityError,
"Invalid APIExport. Please contact the APIExport owner to resolve",
)
// If there are multiple versions, the conversion strategy must be defined in the APIResourceSchema
if len(schema.Spec.Versions) > 1 && schema.Spec.Conversion == nil {
conditions.MarkFalse(
apiBinding,
apisv1alpha1.APIExportValid,
apisv1alpha1.InternalErrorReason,
conditionsv1alpha1.ConditionSeverityError,
"Invalid APIExport. Please contact the APIExport owner to resolve",
)

return reconcileStatusContinue, fmt.Errorf(
"error getting APIConversion %s|%s for APIBinding %s|%s, APIExport %s|%s, APIResourceSchema %s|%s: %w",
apiExportPath, schemaName,
bindingClusterName, apiBinding.Name,
apiExportPath, apiExport.Name,
apiExportPath, schemaName,
err,
)
}
return reconcileStatusContinue, fmt.Errorf(
"conversion strategy not specified %s|%s for APIBinding %s|%s, APIExport %s|%s, APIResourceSchema %s|%s: %w",
apiExportPath, schemaName,
bindingClusterName, apiBinding.Name,
apiExportPath, apiExport.Name,
apiExportPath, schemaName,
err,
)
}

// Try to get the bound CRD
Expand Down Expand Up @@ -541,5 +538,27 @@ func generateCRD(schema *apisv1alpha1.APIResourceSchema) (*apiextensionsv1.Custo
crd.Spec.Versions = append(crd.Spec.Versions, crdVersion)
}

if len(schema.Spec.Versions) > 1 && schema.Spec.Conversion == nil {
Copy link
Member

Choose a reason for hiding this comment

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

do we have validation in place that check this? Here we are pretty late in the process.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't at the moment. This should either be in admission or checked through a CEL expression in the APIResourceSchema. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

If CEL, this might be an option: size(self.versions) == 1 || (size(self.versions) > 1 && has(self.conversion))

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if one can put a Kubebuilder validation on the spec field? Never tried.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added this in the latest commit and verified by trying to create a CRD.

return nil, fmt.Errorf("multiple versions specified but no conversion strategy")
}

if len(schema.Spec.Versions) > 1 {
conversion := &apiextensionsv1.CustomResourceConversion{
Strategy: apiextensionsv1.ConversionStrategyType(schema.Spec.Conversion.Strategy),
}

if schema.Spec.Conversion.Strategy == "Webhook" {
conversion.Webhook = &apiextensionsv1.WebhookConversion{
ConversionReviewVersions: schema.Spec.Conversion.Webhook.ConversionReviewVersions,
ClientConfig: &apiextensionsv1.WebhookClientConfig{
URL: &(schema.Spec.Conversion.Webhook.ClientConfig.URL),
CABundle: schema.Spec.Conversion.Webhook.ClientConfig.CABundle,
},
}
}

crd.Spec.Conversion = conversion
}

return crd, nil
}
Loading
Loading