Skip to content

Add CRDCompatibilityRequirement #2479

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.

Sign up for GitHub

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add CRDCompatibilityRequirement #2479

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6d46566
Add CRDCompatibilityRequirement
mdbooth Jun 19, 2025
7a761c4
crdchecker: update api according to EP
chrischdi Oct 8, 2025
699de0b
add k8s.io/api/admissionregistration/v1 to openapi
chrischdi Oct 9, 2025
51dcb42
crdchecker: fix godoc due to review
chrischdi Oct 9, 2025
74c12d8
ai review fixes
chrischdi Oct 9, 2025
133e7a3
make generate
chrischdi Oct 9, 2025
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
22 changes: 19 additions & 3 deletions operator/v1alpha1/types_crdchecker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ type CRDCompatibilityRequirementSpec struct {
// CompatibilitySchema defines the schema used by crdSchemaValidation and objectSchemaValidation.
type CompatibilitySchema struct {
// crdYAML contains the complete YAML document of the CRD for schema and object validation purposes.
// This field is required.
// This field is required and must be between 1 and 1,572,864 characters in length.
Copy link
Contributor

Choose a reason for hiding this comment

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

1 rune (the characters are runes), could be 4 bytes in the worst case, which means that actually, with the current limitation we are in the worst case limiting this field to 6 mibibytes of data, which one, will be rejected as the maximum request size is much smaller than this, and two, is also 4 times larger than what would be an acceptable CRD yaml.

Have we checked the approximate character count of some of the CRDs we care about to see what length they are? I suspect we want to divide this figure by 4

// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=1572864
// +required
Expand All @@ -75,6 +75,9 @@ type CompatibilitySchema struct {

// additionalVersions is a set of versions to require in addition to those discovered by requireVersions.
// Overlap with requireVersions is explicitly permitted.
// When present, each version string must be between 1 and 255 characters in length.
// The list may contain at most 255 items.
// When not specified, no additional versions are required.
// +kubebuilder:validation:items:MinLength=1
// +kubebuilder:validation:items:MaxLength=255
// +kubebuilder:validation:MaxItems=255
Expand All @@ -84,7 +87,10 @@ type CompatibilitySchema struct {

// excludeFields is a set of fields in the yaml which will not be validated by either
// crdSchemaValidation or objectSchemaValidation.
// FIXME(chrischdi): explain the format which is
// Each field path string must be between 1 and 8,192 characters in length.
// The list may contain at most 1,024 field paths.
// When not specified, all fields in the YAML will be validated.
// FIXME(chrischdi): explain the format for it. // FIXME(chrischdi): explain the format for it.
Copy link
Contributor

Choose a reason for hiding this comment

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

(TODO) resolve

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a regex we can apply to give some early feedback on the format of the strings here?

// +kubebuilder:validation:items:MinLength=1
// +kubebuilder:validation:items:MaxLength=8192
// +kubebuilder:validation:MaxItems=1024
Expand Down Expand Up @@ -115,14 +121,19 @@ type ObjectSchemaValidation struct {
Action CRDAdmitAction `json:"action,omitempty"`

// namespaceSelector defines the namespaceSelector field of the resulting ValidatingWebhookConfiguration.
// When not specified, objects from all namespaces matching the objectSelector will be subject to validation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Somewhere we should explain what happens when neither namespace selector or object selector are specified

// +optional
NamespaceSelector *metav1.LabelSelector `json:"namespaceSelector,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC, making this a pointer means that {} is a valid choice, which means match everything. Is that any different from the behaviour when this is omitted completely? I don't think it is right?


// objectSelector defines the objectSelector field of the resulting ValidatingWebhookConfiguration.
// When not specified, all objects matching the namespaceSelector will be subject to validation.
// +optional
ObjectSelector *metav1.LabelSelector `json:"objectSelector,omitempty"`

// matchConditions defines the matchConditions field of the resulting ValidatingWebhookConfiguration.
// When present, must contain between 1 and 64 match conditions.
// When not specified, the webhook will match all requests according to its other selectors.
// FIXME(chrischdi): should we embed this type? Or maintain our own copy of MatchCondition?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless there's any OpenShift-specific prohibition on this, my strong recommendation is to embed. We're explicitly and deliberately proxying the behaviour of a kube api here. By embedding it we automatically inherit changes to it and we don't have to convert types internally. This requires us to trust that upstream will be a responsible steward of this API. I suspect that, in practise, they will be a more reliable steward.

A potential issue I see with this is pulling a newer API into an older release. However, this PR doesn't change go.mod so it seems it doesn't impose any version restrictions which don't already exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

The upstream type is small, so copying it should not be too onerous. The upstream type also has no validation markers at present (though DV will fix that), which means that if we borrow the upstream type now, the schema generated for the type will change over time underneath us.

I would prefer we implement a copy type for now, at least until the upstream has sufficient validation markers that controller-tools will understand to implement the correct validation.

// +listType=map
// +listMapKey=name
// +kubebuilder:validation:MinItems=1
Expand Down Expand Up @@ -226,8 +237,10 @@ type CRDCompatibilityRequirementStatus struct {
// exist, as we may legitimately place requirements on it before it is
// created. The observed CRD is given in status.observedCRD, which will be
// empty if no CRD is observed.
// When present, must be between 1 and 253 characters and conform to RFC 1123 subdomain format:
// lowercase alphanumeric characters, '-' or '.', starting and ending with alphanumeric characters.
// When not specified, the requirement applies to any CRD name discovered from the compatibility schema.
// This field is optional.
Copy link
Contributor

Choose a reason for hiding this comment

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

If I wanted to work my way around the system, I could remove this field currently, how disastrous would that be?

We may want to add CEL to make this immutable and un-unsettable 🤔

// crdRef must be at most 253 characters in length and must consist only of lower-case alphanumeric characters, periods (.) and hyphens (-). Each period separated label must start and end with an alphanumeric character and be at most 63 characters in length.
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=253
// +kubebuilder:validation:XValidation:rule="!format.dns1123Subdomain().validate(self).hasValue()",message="a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character."
Expand All @@ -239,6 +252,8 @@ type CRDCompatibilityRequirementStatus struct {
// +kubebuilder:validation:MinProperties=1
type ObservedCRD struct {
// uid is the uid of the observed CRD.
// Must be a valid UUID consisting of lowercase hexadecimal digits in 5 hyphenated blocks (8-4-4-4-12 format).
// Length must be between 1 and 36 characters.
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=36
// +kubebuilder:validation:Format=uuid
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, the API server won't actually validate this at admission time, so we should validate this ourselves with a CEL rule (please test)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm convinced I've written an API validation for this before, but I can't find it now 🤔

Is there a canned one you're aware of that's already battle hardened? It's not actually important, so if this requires anything more than a trivial amount of work I'll just remove the validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking about it further: this is status. A validation failure here is only ever likely to result in a bug. I'm just going to remove it because it's unnecessary tight coupling.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even though this is status, it should be validated correctly

// +kubebuilder:validation:Format=uuid
// +kubebuilder:validation:XValidation:rule="!format.uuid().validate(self).hasValue()",message="uid must be a valid UUID. It must consist only of lower-case hexadecimal digits, in 5 hyphenated blocks, where the blocks are of length 8-4-4-4-12 respectively."

Expand All @@ -247,6 +262,7 @@ type ObservedCRD struct {
UID string `json:"uid,omitempty"`

// generation is the observed generation of the CRD.
// Must be a positive integer (minimum value of 1).
// +kubebuilder:validation:Minimum=1
// +required
Generation int64 `json:"generation,omitempty"`
Expand Down