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

Role resolvers #122

Merged
merged 3 commits into from
Sep 28, 2022
Merged

Conversation

KevinJoiner
Copy link
Contributor

This PR adds the ability to get a user's rules in a downstream cluster/project via CRTBs/PRTBs. This removes the need for the escalate verb check.

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.

Good job finding a solution that adds to the resolver.

Some thoughts:

Are we comfortable using xrtb's to determine someone's access? Afaik this should be fine in this context since it is for validating the creation of xrtb's however it is worth noting that this will totally ignore any bindings that are added via kubectl or one that an app adds. cc: @cbron @MbolotSuse

@cbron can we have scaling QA see if there are any spikes when create a new prtb/crtb. If there are so many projects (from all clusters), will there be a spike when creating these slices of rules? I have witnessed 15k+ prtb's in a setup. There is probably no reason to optimize it at this time but I feel like there will be so many redundant rules returned by the resolvers that I want to make sure it isn't necessary to refine the rules we are passing around. I do worry about this less if we switch to using pointers for the PolicyRules.

pkg/resolvers/resolvers.go Outdated Show resolved Hide resolved
pkg/resolvers/resolvers.go Outdated Show resolved Hide resolved
pkg/resolvers/resolvers.go Show resolved Hide resolved
@KevinJoiner
Copy link
Contributor Author

@rmweir I think this is a good question, and I will just add a little more context.

Are we comfortable using xrtb's to determine someone's access?

The previous behavior was to only check a users permissions in the local cluster to determine access for both local and downstream clusters. This new approach keeps that behavior, but also checks the users permissions in local and downstream clusters via xrtbs as well.

however it is worth noting that this will totally ignore any bindings that are added via kubectl or one that an app adds.

I don't believe this is true since all kube-api xrtb operation will pass through the webhook regardless of who originated them. Furthermore, the xrtbs that are used to determine permissions are taken from kube-api via a wrangler cache.

Create test for resolvers and updates validator test.
Copy link
Contributor

@MbolotSuse MbolotSuse left a comment

Choose a reason for hiding this comment

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

minor test comments but overall LGTM.

I would like us to get this in ASAP. This could have widespread effects in rancher, and I think a longer-than-usual testing period would be advisable for something like this.

pkg/resolvers/aggregateResolver_test.go Show resolved Hide resolved
pkg/resolvers/aggregateResolver_test.go Show resolved Hide resolved
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.

My question was actually around role bindings that were created by something other then the xrtb controllers. @KevinJoiner pointed out to me that the default resolver addresses this. LGTM.

@KevinJoiner KevinJoiner merged commit d41563d into rancher:release/v0.3 Sep 28, 2022
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