fix: versioned validation test avoid incorrect conversion #132465
Conversation
|
/sig api-machinery |
|
Thanks for fixing. I agree that this fix is safe and a good filter to have in this code flow. I'm a bit confused about how the invalid conversion is requested. Shouldn't all calls to |
| // skip if the version is not the same as the unversioned object. | ||
| // We should not convert between different versions of the same object. | ||
| if gvk.Version != unversionedGVK.Version { | ||
| continue | ||
| } |
There was a problem hiding this comment.
This comment might be confusing to someone without a lot of context. Should we instead has a if unversionedGVK.Version != runtime.APIVersionInternal { continue } precondition just inside the outer for loop? That way all the unversionGVK entries are guaranteed to be internal types and then the inner loop can continue to skip internal?
We might also be able to get rid of the gvk := gv.WithKind(unversionedGVK.Kind) mutation.
There was a problem hiding this comment.
The unversioned runtime.Object is actually versioned, passed from
It seems that this test is always trying to convert versioned to versioned, should we consider refactor this test instead?
There was a problem hiding this comment.
I think the fix is ok as we want to reuse the same VerifyVersionedValidationEquivalence, though we always pass in a versioned obj and convert from a versioned to versioned.
There was a problem hiding this comment.
Updated the test to always convert the versioned to unversioned(internal) first before passing it to RunValidationForEachVersion
0beea4a to
2f3ca54
Compare
|
LGTM label has been added. Git tree hash: ab6ea969e25e26756032c13a9001079ce3abb228
|
|
/retest |
|
/cc @thockin for api-approver |
|
@yongruilin: GitHub didn't allow me to request PR reviews from the following users: for. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
pkg/api/testing/validation.go
Outdated
| } | ||
| if internalObj == nil { | ||
| return | ||
| } | ||
| if old == nil { | ||
| runtimetest.RunValidationForEachVersion(t, legacyscheme.Scheme, []string{}, obj, accumulate, subresources...) |
There was a problem hiding this comment.
It's not clear to me why this uses obj but update now uses internalObj -- Can you add comments here explaining what's going on a bit more?
There was a problem hiding this comment.
Oh.. it should be internalObj. Updated and added some comments.
| @@ -52,6 +52,10 @@ func runValidation(t *testing.T, scheme *runtime.Scheme, options []string, unver | |||
| t.Fatal(err) | |||
| } | |||
| for _, unversionedGVK := range unversionedGVKs { | |||
| // skip if passed in unversioned object is not internal. | |||
| if unversionedGVK.Version != runtime.APIVersionInternal { | |||
There was a problem hiding this comment.
Given this started with unversioned runtime.Object and scheme.ObjectKinds(unversioned), how would we end up with a non-internal version?
There was a problem hiding this comment.
It's because the previous implementation passed here
is an versioned obj.
2f3ca54 to
a55318f
Compare
|
LGTM label has been added. Git tree hash: 64914f1069e48891a454e0fe7af0de7cf3b5d029
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jpbetz, thockin, yongruilin 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 |
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PRs fix test failure by avoid conversion between versioned types.
e.g.
Which issue(s) this PR is related to:
Ref:
kubernetes/enhancements#5073
Special notes for your reviewer:
This is the prerequisite to enable the declarative validation for APIs with multiple versions. (e.g. certificates/(v1,v1alpha1,v1beta1).
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: