🐛 Fix namespaced GVK check to use version #2875
Conversation
A particular Kind may only be present in a specific version of a group. When querying the RESTMapper we should include the version to ensure the cached group is updated to pick up new versions as needed. Signed-off-by: Griffin Davis <[email protected]>
|
k8s-ci-robot
added
the
cncf-cla: no
|
Welcome @griffindvs! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @griffindvs. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
k8s-ci-robot
added
size/XS
| @@ -72,7 +72,7 @@ func IsObjectNamespaced(obj runtime.Object, scheme *runtime.Scheme, restmapper m | |||
| // IsGVKNamespaced returns true if the object having the provided | |||
| // GVK is namespace scoped. | |||
| func IsGVKNamespaced(gvk schema.GroupVersionKind, restmapper meta.RESTMapper) (bool, error) { | |||
| restmapping, err := restmapper.RESTMapping(schema.GroupKind{Group: gvk.Group, Kind: gvk.Kind}) | |||
| restmapping, err := restmapper.RESTMapping(schema.GroupKind{Group: gvk.Group, Kind: gvk.Kind}, gvk.Version) | |||
There was a problem hiding this comment.
Could we add a test case and the PR description reasoning as a comment above here?
There was a problem hiding this comment.
I added a comment and four unit tests in eb03471. If we compare the results of these tests with and without the change:
Without the change (no Version included):
--- FAIL: TestApiMachinery (3.70s)
--- PASS: TestApiMachinery/IsGVKNamespaced_should_report_scope_for_GVK_registered_at_initialization (0.01s)
--- FAIL: TestApiMachinery/IsGVKNamespaced_should_report_scope_for_new_Kind_and_Version_added_to_existing_Group (0.03s)
--- PASS: TestApiMachinery/IsGVKNamespaced_should_report_scope_for_new_Kind_added_to_existing_Group_and_Version (0.03s)
--- PASS: TestApiMachinery/IsGVKNamespaced_should_report_scope_for_new_GVK (0.04s)
With the change (version included):
--- PASS: TestApiMachinery (3.63s)
--- PASS: TestApiMachinery/IsGVKNamespaced_should_report_scope_for_GVK_registered_at_initialization (0.00s)
--- PASS: TestApiMachinery/IsGVKNamespaced_should_report_scope_for_new_Kind_and_Version_added_to_existing_Group (0.02s)
--- PASS: TestApiMachinery/IsGVKNamespaced_should_report_scope_for_new_Kind_added_to_existing_Group_and_Version (0.02s)
--- PASS: TestApiMachinery/IsGVKNamespaced_should_report_scope_for_new_GVK (0.02s)
The test case that fails without the change mimics the linked issue where a new Kind and Version are added to a pre-existing Group.
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
Include unit tests for varying combinations of new GVKs introduced at runtime to validate cache updates. Signed-off-by: Griffin Davis <[email protected]>
k8s-ci-robot
added
size/L
alvaroaleman
added
the
tide/merge-method-squash
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, griffindvs 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 |
|
LGTM label has been added. Git tree hash: 618350361e7743d495a523a09a0ecf6d38625dfc
|
k8s-ci-robot
added
the
approved
Fixes #2869
A particular Kind may only be present in a specific version of a Group. When querying the RESTMapper we should include the Version to ensure the cached Group is updated to pick up new Versions as needed.
Without this fix, the failure noted in the linked issue can occur. A new Version can be introduced to a pre-existing Group with new Kinds. Because the cached Group does not include this Version, the namespaced GVK check can fail to find the Kind and will not update the cache. The issue will continue to occur until the controller pod is restarted to re-initialize the cache with new CRDs found in the cluster.