Add CRDCompatibilityRequirement #2479

mdbooth · 2025-09-16T15:51:48Z

No description provided.

openshift-ci · 2025-09-16T15:52:05Z

Hello @mdbooth! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

openshift-ci · 2025-09-16T15:55:15Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign everettraven for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

JoelSpeed · 2025-09-16T16:04:54Z

operator/v1alpha1/types_crdchecker.go

+	// ensure that only a target CRD compatible with compatibilityCRD may be admitted.
+	// This field is required.
+	// +kubebuilder:validation:MinLength=1
+	// +kubebuilder:validation:MaxLength=1000000


The maximum request size supported is 1.5MiB, in the worst case, each rune will be exactly 1 byte, so the maximum length here should be

Suggested change

// +kubebuilder:validation:MaxLength=1000000

// +kubebuilder:validation:MaxLength=1572864

JoelSpeed · 2025-09-16T16:35:02Z

operator/v1alpha1/types_crdchecker.go

+	// metadata is the standard object's metadata.
+	// More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata
+	// +optional
+	metav1.ObjectMeta `json:"metadata"`


Suggested change

metav1.ObjectMeta `json:"metadata"`

metav1.ObjectMeta `json:"metadata,omitzero"`

The linter didn't warn me about that one!

I am not sure why, a bug to look into I guess 🤔

JoelSpeed · 2025-09-16T16:36:32Z

operator/v1alpha1/types_crdchecker.go

+	// +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."
+	// +required
+	CRDRef string `json:"crdRef,omitempty"`


We don't use Ref in openshift APIs. I would generally expect this to be more like a local object reference, so to be consistent with other APIs, WDYT of

crd: name:

Potentially, instead of crd, what about target?

Certainly ok on dropping ref if that's the convention.

Are you channeling https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#object-references by any chance here?

It is okay to have the "{field}" component indicate the resource type. For example, "secretRef" when referencing a secret. However, this comes with the risk of the field being a misnomer in the case that the field is expanded to reference more than one type.

Could this ever reference a different type? It feels unlikely. That would likely require a different tool. With that in mind, I think crd is more explicit and therefore more obvious. This object is about adding requirements to a CRD, and the CRD is specified in the crd field.

Is there any circumstance when we might add additional information to a CRD reference? The only things I can think of are:

UID - this precise CRD and not its successors

APIVersion - CRD version v2beta1

So that's realistic, at least.

I'll change it to

crd: name:

I had leant on target for a few reasons, yes, it's more generic, but also I wondered if it might have been more intuitive and idiomatic, I have seen many targetRef before across K8s APIs. We do talk about the field being a target in the godoc, so it felt like it made sense.

JoelSpeed · 2025-09-16T16:43:31Z

operator/v1alpha1/types_crdchecker.go

+	// +kubebuilder:validation:MinLength=1
+	// +kubebuilder:validation:MaxLength:=253


Inconsistent use of := vs = at the end here?

Is there a difference between the 2?

I can't find := in the docs at all now I look for it. I'm assuming I've just cargo-culted this from somewhere and should stop.

Kubebuilder supports both := and = on all of its markers. Upstream markers (e.g. +default) do not support :=, so we would prefer all markers use = and we shouldn't use :=.

Has lead to issues in the past where upstream markers have been ignored because of the := typo

JoelSpeed · 2025-09-16T16:46:04Z

operator/v1alpha1/types_crdchecker.go

+	// creatorDescription is a string describing the owner of this CRDCompatibilityRequirement. It will be printed in any error or
+	// warning emitted by any of the CRDCompatibilityRequirement's webhooks. It should indicate to the recipient who they need to coordinate
+	// with in order to safely update the target CRD. The message emitted will be: "This requirement was added by <creatorDescription>".
+	// This field is required.
+	// +kubebuilder:validation:MinLength=1
+	// +kubebuilder:validation:MaxLength=253
+	// +required
+	CreatorDescription string `json:"creatorDescription,omitempty"`


Have you considered what a valid set of characters might look like for this field? Since this is being used as input to a logging output, I assume it would be bad if I tried to inject some json into this?

creatorDescription: "\" nastySomething:\"doBadThingToLogs\""

I'm assuming that any JSON logging library we're using escapes its output. If it doesn't we already have severe problems! We certainly don't want to ask the user to do their own escaping here.

It's a while since I wrote this, and I just re-read it with fresh eyes. I think it's usefully descriptive? I can't think how I would improve it.

I was thinking along the lines of restricting the valid character set to [a-zA-Z0-9\ ] to prevent folks from putting any data into this string that we need to be worried about or even consider the safety element of

That just feels like a footgun. For example, it would prevent us from using non-US ASCII characters in a string which is intended to describe people. I don't think we can usefully validate this string, and I think we can trust logging software to escape any possible byte sequence correctly.

Kubernetes is built on US ASCII already, all of our APIs, code and even user facing logging is also US ASCII. If we implement this validation, we can always relax it later if we were to get complaints. I'm open to a less restrictive set if you have a suggestion, but I don't think we should leave it unbounded

JoelSpeed · 2025-09-16T16:49:02Z

operator/v1alpha1/types_crdchecker.go

+	// +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."


All validations like this must be explained in prose inside the godoc

Suggested change

// +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."

// 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."

Please apply similar feedback to other fields

Presumably we do this for UX, and in general I get it. However, in this case (and presumably many similar cases), I believe the additional text actually makes the UX worse because it's complex, distracting, and irrelevant. The 'correct' validation here is 'the name of a CRD'. This CRD may or may not exist yet, so we can't even validate it with a VAP. All we're trying to do here with this somewhat complex validation (which I presumably just copied from somewhere) is exclude a certain class of thing which is obviously wrong because it would fail k8s naming validation. If it excludes the valid name of a CRD, that would be a bug. If this ever did happen, the user would open a bug against the product because the field did not accept the name of their CRD, not because they mentally executed the pseudocode in the documentation¹. Even if they did read the pseudocode, it might lead them to the incorrect belief that we only intend to support CRDs whose names conform to some arbitrary additional restrictions.

I can see that this kind of validation is useful when we're trying to guide the user to form correct input for something we're going to create, but here we're just referencing something which was validated elsewhere with rules we don't control. Our own validation is nothing more than a loose proxy to exclude a small class of obvious error.

Footnotes

Which in any case hasn't corresponded to the actual validation for the last 18 months since we tweaked it but forgot to make the corresponding change to the pseudocode in the docs, but nobody noticed because we don't run tests against the docs². ↩

A linter which uses AI to attempt to validate that godoc remains consistent with the code it describes 🤔 ↩

Validation markers are not documented in any user readable documentation by default. We document all validations in the text to explain what the valid values are, so that folks using oc explain or reading our generated API documentation in the product docs have a sense of what value input is.

While I get your point about us documenting a validation that isn't actually ours, we aim to be consistent in validating the field validations across all fields in our APIs, independent of whether its our own restriction, or something imposed upon us

Ack. I'll also make it clear that it's a proxy validation, just in case.

The validation of a CRD name is never going to change, I don't think we need to document that this is proxy validation

JoelSpeed · 2025-09-16T16:49:45Z

operator/v1alpha1/types_crdchecker.go

+// CRDCompatibilityRequirementStatus defines the observed status of the CRD Compatibility Requirement.
+// +kubebuilder:validation:MinProperties=1
+type CRDCompatibilityRequirementStatus struct {
+	// conditions is a list of conditions and their status.


Document valid condition types?

JoelSpeed · 2025-09-16T16:49:56Z

operator/v1alpha1/types_crdchecker.go

+	// +optional
+	// +listType=map
+	// +listMapKey=type
+	// +kubebuilder:validation:MaxItems=16


Please add a MinItems 1 as well

JoelSpeed · 2025-09-16T16:50:53Z

operator/v1alpha1/types_crdchecker.go

+	// uid is the uid of the observed CRD.
+	// +kubebuilder:validation:MaxLength=36
+	// +kubebuilder:validation:MinLength=1
+	// +kubebuilder:validation:Format=uuid


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

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.

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.

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."

JoelSpeed · 2025-09-16T16:51:41Z

operator/v1alpha1/types_crdchecker.go

+	// metadata is the standard list's metadata.
+	// More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata
+	// +optional
+	metav1.ListMeta `json:"metadata"`


Suggested change

metav1.ListMeta `json:"metadata"`

metav1.ListMeta `json:"metadata,omitzero"`

damdo · 2025-09-16T19:14:00Z

/assign @damdo

mdbooth · 2025-10-09T11:19:09Z

operator/v1alpha1/types_crdchecker.go

+
+// CompatibilitySchema defines the schema used by crdSchemaValidation and objectSchemaValidation.
+type CompatibilitySchema struct {
+	// crdYAML contains the complete YAML document of the CRD from transport config map.


I wouldn't mention transport config maps here: they're a detail of where CCAPIO specifically gets its data from. Other actors won't be using transport config maps.

100% agree :-)

mdbooth · 2025-10-09T11:29:57Z

operator/v1alpha1/types_crdchecker.go

+	ObjectSelector *metav1.LabelSelector `json:"objectSelector,omitempty"`
+
+	// matchConditions defines the matchConditions field of the resulting ValidatingWebhookConfiguration.
+	// FIXME(chrischdi): should we embed this type? Or maintain our own copy of MatchCondition?


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.

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.

mdbooth · 2025-10-09T11:30:37Z

operator/v1alpha1/types_crdchecker.go

+}
+
+// CRDAdmitAction determines the action taken when a CRD is not compatible.
+// +kubebuilder:validation:Enum=Enforce;Warn


bikeshed: I was looking at VAPs where the verb is Deny. I wonder if we should switch Enforce to Deny.

+1 for Deny from my side.

chrischdi · 2025-10-09T17:42:28Z

operator/v1alpha1/types_crdchecker.go

+	// 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.


(TODO) resolve

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

openshift-ci · 2025-10-09T18:22:47Z

@mdbooth: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/verify	`133e7a3`	link	true	`/test verify`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

operator/v1alpha1/types_crdchecker.go

JoelSpeed · 2025-10-10T10:32:46Z

operator/v1alpha1/types_crdchecker.go

+	// 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.


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

JoelSpeed · 2025-10-10T10:34:09Z

operator/v1alpha1/types_crdchecker.go

 	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.


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

JoelSpeed · 2025-10-10T10:38:01Z

operator/v1alpha1/types_crdchecker.go

+	ObjectSelector *metav1.LabelSelector `json:"objectSelector,omitempty"`
+
+	// matchConditions defines the matchConditions field of the resulting ValidatingWebhookConfiguration.
+	// FIXME(chrischdi): should we embed this type? Or maintain our own copy of MatchCondition?


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.

JoelSpeed · 2025-10-10T12:02:00Z

operator/v1alpha1/types_crdchecker.go

+}
+
+// CRDAdmitAction determines the action taken when a CRD is not compatible.
+// +kubebuilder:validation:Enum=Enforce;Warn


JoelSpeed · 2025-10-10T12:02:43Z

operator/v1alpha1/types_crdchecker.go

+	Conditions []metav1.Condition `json:"conditions,omitempty"`
+
+	// observedCRD documents the uid and generation of the CRD object when the current status was written.
+	// This field will not be emitted if the target CRD does not exist or could not be retrieved.


Consistent language with other APIs

Suggested change

// This field will not be emitted if the target CRD does not exist or could not be retrieved.

// This field will be omitted if the target CRD does not exist or could not be retrieved.

JoelSpeed · 2025-10-10T12:03:15Z

operator/v1alpha1/types_crdchecker.go

+	// +optional
+	ObservedCRD ObservedCRD `json:"observedCRD,omitzero"`
+
+	// crdName is the name of the target CRD. The target CRD is not required to


Why is this not part of the observedCRD?

Why not have

observedCRD: name: ...

And then populate the UID and generation when it is actually observed?

JoelSpeed · 2025-10-10T12:04:17Z

operator/v1alpha1/types_crdchecker.go

+)
+
+// CRDCompatibilityRequirementStatus defines the observed status of the CRD Compatibility Requirement.
+// +kubebuilder:validation:MinProperties=1


Conditions should be required? Or maybe the CRD name? Possibly both tbh

JoelSpeed · 2025-10-10T12:05:39Z

operator/v1alpha1/types_crdchecker.go

+	// 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.


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 🤔

openshift-ci bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 16, 2025

openshift-ci bot requested review from JoelSpeed and everettraven September 16, 2025 15:54

JoelSpeed reviewed Sep 16, 2025

View reviewed changes

openshift-ci bot assigned damdo Sep 16, 2025

openshift-ci bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 9, 2025

chrischdi force-pushed the crd-compatibility-checker branch from 6f108bd to a8e1698 Compare October 9, 2025 07:48

mdbooth commented Oct 9, 2025

View reviewed changes

chrischdi force-pushed the crd-compatibility-checker branch from 74f30c5 to 91af025 Compare October 9, 2025 15:02

mdbooth and others added 6 commits October 9, 2025 19:33

Add CRDCompatibilityRequirement

6d46566

crdchecker: update api according to EP

7a761c4

add k8s.io/api/admissionregistration/v1 to openapi

699de0b

crdchecker: fix godoc due to review

51dcb42

ai review fixes

74c12d8

make generate

133e7a3

chrischdi force-pushed the crd-compatibility-checker branch from 91af025 to 133e7a3 Compare October 9, 2025 17:41

chrischdi reviewed Oct 9, 2025

View reviewed changes

JoelSpeed reviewed Oct 10, 2025

View reviewed changes

	// +kubebuilder:validation:MaxLength=1000000
	// +kubebuilder:validation:MaxLength=1572864

	metav1.ObjectMeta `json:"metadata"`
	metav1.ObjectMeta `json:"metadata,omitzero"`

		// +kubebuilder:validation:MinLength=1
		// +kubebuilder:validation:MaxLength:=253

	metav1.ListMeta `json:"metadata"`
	metav1.ListMeta `json:"metadata,omitzero"`

	// This field will not be emitted if the target CRD does not exist or could not be retrieved.
	// This field will be omitted if the target CRD does not exist or could not be retrieved.

Add CRDCompatibilityRequirement #2479

Are you sure you want to change the base?

Add CRDCompatibilityRequirement #2479

Uh oh!

Conversation

mdbooth commented Sep 16, 2025

Uh oh!

openshift-ci bot commented Sep 16, 2025

Uh oh!

openshift-ci bot commented Sep 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdbooth Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damdo commented Sep 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdbooth Sep 17, 2025 •

edited

Loading