Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

🐛 RESTMapper: don't treat non-existing GroupVersions as errors #2425

Closed
wants to merge 2 commits into from
Closed

🐛 RESTMapper: don't treat non-existing GroupVersions as errors #2425

wants to merge 2 commits into from

Conversation

timebertt
Copy link
Contributor

/kind bug

This PR fixes the new RESTMapper implementation to properly handle cases where an API Group / GroupVersion is not present in the system. Clients expect No{Kind,Resource}MatchErrors in this case.

The first commit increases the accuracy of the RESTMapper tests to verify the described expectations.
The second commit implements the actual fix.
/label tide/merge-method-squash

Fixes #2424

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. labels Jul 26, 2023
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 26, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: timebertt
Once this PR has been reviewed and has the lgtm label, please assign fillzpp for approval. For more information see the Kubernetes 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:

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

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 26, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @timebertt. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 26, 2023
@vincepri
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 27, 2023
vincepri
vincepri reviewed Jul 27, 2023
View reviewed changes
pkg/client/apiutil/restmapper.go
@@ -166,8 +167,10 @@ func (m *mapper) addKnownGroupAndReload(groupName string, versions ...string) er
if err != nil {
return err
}
for _, version := range apiGroup.Versions {
versions = append(versions, version.Version)
if apiGroup != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this check here? If apiGroup is nil, the range would throw a panic

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, if the queried group name cannot be found, apiGroup is nil and the next line will cause a nil pointer dereference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, we could make mapper.apiGroups a map[string]metav1.APIGroup so that we don't have to check for nil pointers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we have findAPIGroupByName return a well-known not found error, and catch it here instead of returning nil, nil?

pkg/client/apiutil/restmapper.go
Comment on lines +263 to +265
// Don't return an error here if the API group is not present.
// The reloaded RESTMapper will take care of returning a NoMatchError.
return m.apiGroups[groupName], nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @alvaroaleman

@vincepri
Copy link
Member

We might need to categorize this as a breaking change, rather than a bug?

@timebertt
Copy link
Contributor Author

We might need to categorize this as a breaking change, rather than a bug?

Hmm, it's hard for me to draw the line here. As such, you are right, this is a breaking change. However, this is only because a previous breaking change incorrectly altered the behavior.
These are just my two cents, I'm fine to change this to ⚠️ if you prefer.

@timebertt
Copy link
Contributor Author

/retest

ary1992 added a commit to ary1992/gardener that referenced this pull request Aug 7, 2023
@ary1992
Use old restmapper
43a9d7b 
- Latest restmapper implementation doesnot handle cases where an API Group / GroupVersion is not present in the system. In such cases Clients expect No{Kind,Resource}MatchErrors.
- Upstream fix is already open kubernetes-sigs/controller-runtime#2425.
- Revert this commit once the fix is available.
@tenzen-y tenzen-y mentioned this pull request Aug 8, 2023
Merged
ary1992 added a commit to ary1992/gardener that referenced this pull request Aug 10, 2023
@ary1992
Use old restmapper
1bd269f 
- Latest restmapper implementation doesnot handle cases where an API Group / GroupVersion is not present in the system. In such cases Clients expect No{Kind,Resource}MatchErrors.
- Upstream fix is already open kubernetes-sigs/controller-runtime#2425.
- Revert this commit once the fix is available.
ary1992 added a commit to ary1992/gardener that referenced this pull request Aug 10, 2023
@ary1992
Use old restmapper
5432f15 
- Latest restmapper implementation doesnot handle cases where an API Group / GroupVersion is not present in the system. In such cases Clients expect No{Kind,Resource}MatchErrors.
- Upstream fix is already open kubernetes-sigs/controller-runtime#2425.
- Revert this commit once the fix is available.
@tenzen-y tenzen-y mentioned this pull request Aug 11, 2023
Closed
ary1992 added a commit to ary1992/gardener that referenced this pull request Aug 21, 2023
@ary1992
Use old restmapper
b190b7d 
- Latest restmapper implementation doesnot handle cases where an API Group / GroupVersion is not present in the system. In such cases Clients expect No{Kind,Resource}MatchErrors.
- Upstream fix is already open kubernetes-sigs/controller-runtime#2425.
- Revert this commit once the fix is available.
ary1992 added a commit to ary1992/gardener that referenced this pull request Aug 22, 2023
@ary1992
Use old restmapper
99fd90f 
- Latest restmapper implementation doesnot handle cases where an API Group / GroupVersion is not present in the system. In such cases Clients expect No{Kind,Resource}MatchErrors.
- Upstream fix is already open kubernetes-sigs/controller-runtime#2425.
- Revert this commit once the fix is available.
ary1992 added a commit to ary1992/gardener that referenced this pull request Aug 27, 2023
@ary1992
Use old restmapper
27d3bc0 
- Latest restmapper implementation doesnot handle cases where an API Group / GroupVersion is not present in the system. In such cases Clients expect No{Kind,Resource}MatchErrors.
- Upstream fix is already open kubernetes-sigs/controller-runtime#2425.
- Revert this commit once the fix is available.
shafeeqes pushed a commit to ary1992/gardener that referenced this pull request Aug 28, 2023
@ary1992 @shafeeqes
Use old restmapper
233a90c 
- Latest restmapper implementation doesnot handle cases where an API Group / GroupVersion is not present in the system. In such cases Clients expect No{Kind,Resource}MatchErrors.
- Upstream fix is already open kubernetes-sigs/controller-runtime#2425.
- Revert this commit once the fix is available.
ary1992 added a commit to ary1992/gardener that referenced this pull request Aug 28, 2023
@ary1992
Use old restmapper
e91af52 
- Latest restmapper implementation doesnot handle cases where an API Group / GroupVersion is not present in the system. In such cases Clients expect No{Kind,Resource}MatchErrors.
- Upstream fix is already open kubernetes-sigs/controller-runtime#2425.
- Revert this commit once the fix is available.
@ary1992 ary1992 mentioned this pull request Aug 29, 2023
Merged
@awels awels mentioned this pull request Sep 1, 2023
Merged
@mimowo
Copy link

mimowo commented Sep 7, 2023

@timebertt any progress on this?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 12, 2023
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

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.

@akalenyu
Copy link
Contributor

I may be off, but wasn't this fixed with #2472?

@alvaroaleman
Copy link
Member

Yes

@shafeeqes shafeeqes mentioned this pull request Sep 15, 2023
Merged
@akalenyu
Copy link
Contributor

Yes

I was wrong, this is still needed, as with non existing CRDs you would get this err
instead of IsNoMatch:

controller-runtime/pkg/client/apiutil/restmapper.go

Line 267 in 26a23a9

return nil, fmt.Errorf("failed to find API group %q", groupName)

@ary1992 ary1992 mentioned this pull request Nov 2, 2023
Merged
@akalenyu akalenyu mentioned this pull request Nov 13, 2023
Merged
@timebertt timebertt deleted the restmapper-no-match-error branch January 2, 2024 16:28
@troy0820 troy0820 mentioned this pull request Jan 4, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vincepri vincepri vincepri left review comments

@FillZpp FillZpp Awaiting requested review from FillZpp

@varshaprasad96 varshaprasad96 Awaiting requested review from varshaprasad96

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DynamicRESTMapper does not return No{Kind,Resource}MatchError for non-present API groups
6 participants
@timebertt @k8s-ci-robot @vincepri @mimowo @akalenyu @alvaroaleman