Fix CRD storage strategy validator to accept all allowed versions #68038
Conversation
|
@mbohlool: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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/test-infra repository. |
k8s-ci-robot
added
size/M
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mbohlool If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: yes
|
To my understanding we have a storage codec which decodes to the handler version. I.e. every CR coming from storage and going into validation has the handler version already. Why do we have to weaken validation? Do we have tests that
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
| if typeAccessor.GetKind() != a.kind.Kind { | ||
| allErrs = append(allErrs, field.Invalid(field.NewPath("kind"), typeAccessor.GetKind(), fmt.Sprintf("must be %v", a.kind.Kind))) | ||
| if typeAccessor.GetKind() != a.groupKind.Kind { | ||
| allErrs = append(allErrs, field.Invalid(field.NewPath("kind"), typeAccessor.GetKind(), fmt.Sprintf("must be %v", a.groupKind.Kind))) |
| @@ -426,6 +426,10 @@ func (r *crdHandler) getOrCreateServingInfoFor(crd *apiextensions.CustomResource | |||
| statusScopes := map[string]handlers.RequestScope{} | |||
| scaleScopes := map[string]handlers.RequestScope{} | |||
|
|
|||
| var allowedAPIVersions []string | |||
There was a problem hiding this comment.
how about leveraging sets.string?
| @@ -52,6 +52,18 @@ func NewNoxuSubresourcesCRD(scope apiextensionsv1beta1.ResourceScope) *apiextens | |||
| ListKind: "NoxuItemList", | |||
| }, | |||
| Scope: scope, | |||
| Versions: []apiextensionsv1beta1.CustomResourceDefinitionVersion{ | |||
There was a problem hiding this comment.
the test passes even without all the other changes in that PR.
There was a problem hiding this comment.
correction: TestStatusSubresource passes, TestScaleSubresource does not.
There was a problem hiding this comment.
The root cause of the problem is here:
That is what different between scale and status. Scale is reading object again from the storage which reads it as storage version not request version, but then will try to pass it to a validator that expect request version. Looking into a proper solution and suggestions are welcome.
There was a problem hiding this comment.
Working on a fix in the versioning decoder.
If I cannot work it out today, I can do a quickfix for Unstructured. It's not pretty, but will unblock versioning.
|
close in favor of #68452 |
The customResource validator was expecting an specific version of CRD (the request one) while the storage may have any version. This change will allow any apiVersion that is listed in CR definition.
Fixes #68035
@sttts @roycaihw