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

Rewrite rewrite authorizer impl #246

Merged
merged 1 commit into from
Jul 10, 2023
Merged

Rewrite rewrite authorizer impl #246

merged 1 commit into from
Jul 10, 2023

Conversation

ibihim
Copy link
Collaborator

@ibihim ibihim commented Jun 22, 2023

What

  • Re-order authz-validation-logic to match previous (correct) order.
  • Split attributes rewrite from authorizer.Authorizer logic.
  • Don't load all variations, even though config picks one.
  • Minor improvements from @enj's review.

Why

  • The authorization logic is messy.
  • Templates are parse per request.
  • All authorizers are loaded, even though only one is picked.

@ibihim ibihim force-pushed the rewrite-rewrite-authorizer-impl branch from d46f214 to 6637774 Compare June 22, 2023 21:06
stlaz
stlaz reviewed Jun 23, 2023
View reviewed changes
Copy link
Collaborator

@stlaz stlaz left a comment

Choose a reason for hiding this comment

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

please squash the cosmetic commits where you're moving unchanged code around within a single module

cmd/kube-rbac-proxy/app/kube-rbac-proxy.go Outdated Show resolved Hide resolved
cmd/kube-rbac-proxy/app/kube-rbac-proxy.go Show resolved Hide resolved
cmd/kube-rbac-proxy/app/kube-rbac-proxy.go Outdated Show resolved Hide resolved
pkg/authorization/rewrite/attributes.go Show resolved Hide resolved
pkg/authorization/rewrite/rewrite.go Outdated Show resolved Hide resolved
pkg/authorization/rewrite/attributes.go Show resolved Hide resolved
pkg/authorization/rewrite/attributes.go Show resolved Hide resolved
pkg/authorization/rewrite/rewrite.go Outdated Show resolved Hide resolved
pkg/authorization/rewrite/middleware_test.go Show resolved Hide resolved
pkg/authorization/rewrite/rewrite.go Outdated Show resolved Hide resolved
@ibihim ibihim force-pushed the rewrite-rewrite-authorizer-impl branch from 6637774 to 239be71 Compare June 23, 2023 14:10
stlaz
stlaz requested changes Jul 4, 2023
View reviewed changes
pkg/authorization/rewrite/attributes.go Outdated Show resolved Hide resolved
@ibihim ibihim force-pushed the rewrite-rewrite-authorizer-impl branch from 30c4607 to c554701 Compare July 4, 2023 09:56
@ibihim ibihim force-pushed the rewrite-rewrite-authorizer-impl branch 4 times, most recently from 0caf062 to 8bd8c70 Compare July 5, 2023 17:08
@ibihim ibihim force-pushed the rewrite-rewrite-authorizer-impl branch from a539b29 to 66ca64c Compare July 7, 2023 17:44
@s-urbaniak
Copy link
Collaborator

looks great! Thanks for the 1:1 OOB presentation @ibihim 🎉

@ibihim ibihim force-pushed the rewrite-rewrite-authorizer-impl branch from 7df10fa to 788b693 Compare July 10, 2023 14:40
@ibihim
p/a/rewrite: rework authorization, fix regression
788b693 
1. There was a regression in the authorization logic that got fixed.
2. Splitting up the authorization that is based on the attributes
   rewrite. There are now 3 distinct authorizers that are set up based
   on the config.
3. Fix re-templating every request.
@ibihim ibihim merged commit 1f9a8d3 into brancz:sig-auth-acceptance Jul 10, 2023
7 checks passed
@ibihim ibihim deleted the rewrite-rewrite-authorizer-impl branch July 10, 2023 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s-urbaniak s-urbaniak s-urbaniak left review comments

@stlaz stlaz stlaz requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ibihim @s-urbaniak @stlaz