Update CRD annotations for 1.x parity #1757
Conversation
|
Draft pending:
|
Add shortNames to CRDs matching the shortNames available in 1.x.
Add validation to CRD schemas, mostly aligned with the 1.x CRDs. Omits type validations where 1.x specified the type explicitly even though it did not differ from the underlying Go type. These validations are implicit. Omits KongIngress validations. KongIngress does not specify fields of its own and inherits everything from go-kong.
Copy the 1.x KongIngress openAPIV3Schema verbatim into the 2.x CRD via Kustomize patch. This works around generation pulling in undesired fields from the underlying go-kong structures we include inside KongIngress, as well as adding validation to fields even though the go-kong types have no Kubebuilder tags.
Add additional printer columns included in the 1.x CRDs to the 2.x CRDs.
rainest
force-pushed
the
feat/crd-improvements
branch
from
02becc2
to
12b2dde
Compare
|
I'm on board for moving forward with generators for our CRDs, particularly given some updates we've had today on our 2.0 timeline and given that there's already a lot of work done here. Is #1763 useless in the face of this and should we close it? Or do you think there's something of value still in there we should keep? |
|
Per https://kubernetes.slack.com/archives/C0EG7JC6T/p1630010315014600 the new test failures like are because non-pointer struct fields don't truly omit when empty, but rather create a phantom empty struct of that type. I'm not sure why this wasn't an issue before, since |
Convert KongPlugin and KongClusterPlugin ConfigFrom fields from structs to pointers to structs. Currently, a struct field within another struct with the "omitempty" JSON serialization tag does not truly omit that field when empty. Instead, it creates an empty struct of that type. Ref: golang/go#11939 This is at odds with required fields on optional object fields in a CRD schema. When present, KongPlugin.ConfigFrom is set, KongPlugin.ConfigFrom.secretKeyRef.name and KongPlugin.ConfigFrom.secretKeyRef.key must be set. An omitempty struct creates a KongPlugin.ConfigFrom == SecretValueFromSource{}, failing validation even though the user intent is to set Config instead. Using pointers to these structs avoids these behaviors; the fields are truly omitted.
|
@shaneutt what did you do locally for the controller-gen version update in #1754? For some reason my trying to So I'd expect that should put 0.6.2 in the annotations, but it isn't. Fake edit: the make target uses the local |
Yeah I ran |
|
Validations like this pose a problem: It results in a schema entry like: but we want i.e. that validation needs to apply to the items in the array, and the generator is making a rule that instead tries to validate the array (which won't work). This is due to kubernetes-sigs/controller-tools#342, which doesn't appear like it will change any time soon, so we need dedicated types for the array items. |
Here is the openAPI standard (the first one) |
What this PR does / why we need it:
Adds additional kubebuilder annotations to CRD types to (mostly) align them with 1.x. Type validations are mostly omitted because most did not set a validation different from the underlying Go type, and such explicit validations are unnecessary.
Copies the 1.x KongIngress schema into the 2.x CRD verbatim using a Kustomize patch. Generating this properly is difficult.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged):fixes #1745
fixes #1748
fixes #1752
fixes #1749 (but needs future follow-up)
Special notes for your reviewer:
This is somewhat at odds with #1762. If we do proceed with that, I move that we merge this as-is and then make a separate change to pull in old CRDs after. Old CRDs only need to go into the final deploy manifests, and all the annotation changes here are something we'd want for an eventual transition to Kubebuilder CRDs, even if we're not doing that now.
IMO we should proceed with generated CRDs. fdf8552 does represent a review of the schema validation differences and adds back those that looked relevant. It includes validations that did not simply assert the underlying Go type, which are unnecessary. Per
validation:Typein https://book.kubebuilder.io/reference/markers/crd-validation.html that happens automatically, and type validations should only be used when the desired type differs from the underlying type (e.g. forKongPlugin.Config, we want anobject, whereasapiextesions.JSONwill allow any valid JSON, so also standalone ints or strings).My own review of differences between crd.yaml.txt and oldcrd.yaml.txt for #1761 using https://github.com/homeport/dyff is that the remaining differences are because of the inherent v1/v1beta1 differences (e.g. the addition of
versions,validationmoving into theschemasection underversions, etc.). It's easy to get lost though, because those differences are significant. I don't think piecemeal verification of every field is a great idea because it's prone to error, so if we intend to proceed with #1762 I think we'd want to instead compare against a v1 version of the old CRDs, which we'd need for #1762 anyway.PR Readiness Checklist:
Complete these before marking the PR as
ready to review:CHANGELOG.mdrelease notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR