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.

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

Updating ByNames to not return nil, nil #132

Merged
merged 1 commit into from
Oct 16, 2023

Conversation

MbolotSuse
Copy link
Contributor

@MbolotSuse MbolotSuse commented Oct 13, 2023

ByNames could previously return a nil value and a nil error. This caused issues when other parts of the application
(pkg/stores/partition/parallel.go) tried to use the result. Now this will return an empty list on the error condition, instead of nil.

Related to rancher/rancher#43030.

Overview

Steve's partion lister makes in internal call to a Lister, which is (in turn) a proxy/rbac store. After making this call, it attempts to set several values from the result of that call, including the ResourceVersion of the list (which is useful for future calls to these resources, potentially including paginated use cases). In the case where the user has a permission granting permissions on specific resourceNames in the entire cluster (see below for a production use-case) this will result in a panic - since the proxy store returns nil for both the err and the list in this case.

This results in a panic in steve and the binary using steve (in this case, the cluster agent), which causes the linked issue above.

This case is triggered in (at least) the monitoring v2 use case - the monitoring-ui-view cluster role grants permissions on specific resource names, and can be assigned to users at the cluster level using a cluster role binding (see rancher/dashboard#9792 for an example).

Solution

Change the proxy store to return a valid value in the identified use case (user has permissions on all resources of a specific name at the cluster scope). Long term, we may want this to return valid values, but given the explicit warnings in the code I think this needs more investigation before we go that route.

pkg/stores/proxy/proxy_store.go Outdated Show resolved Hide resolved
pkg/stores/proxy/proxy_store.go Outdated Show resolved Hide resolved
ByNames could previously return a nil value and a nil error. This caused
issues when other parts of the application
(pkg/stores/partition/parallel.go) tried to use the result. Now this
will return an empty list on the error condition, instead of nil
Copy link
Contributor

@rmweir rmweir left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants