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

[StructuredAuthorizationConfig] - CEL integration #121223

Merged
merged 1 commit into from Oct 31, 2023
Merged

[StructuredAuthorizationConfig] - CEL integration #121223

merged 1 commit into from Oct 31, 2023

Conversation

ritazh
Copy link
Member

@ritazh ritazh commented Oct 13, 2023 •
edited

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add functions to authorization for validating / compiling / evaluating expressions with subjectAccessReview context

Which issue(s) this PR fixes:

Fixes #118873

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Adds support for CEL expressions to v1alpha1 AuthorizationConfiguration webhook matchConditions.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://kep.k8s.io/3221

/sig auth
/triage accepted
/milestone v1.29
/priority important-soon

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Oct 13, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.29 milestone Oct 13, 2023
@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. sig/auth Categorizes an issue or PR as relevant to SIG Auth. triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 13, 2023
@k8s-ci-robot k8s-ci-robot added area/apiserver kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Oct 13, 2023
@ritazh ritazh force-pushed the authz-cel branch 4 times, most recently from 0fb33d4 to e3eb5ab Compare October 16, 2023 03:04
@ritazh
Copy link
Member Author

ritazh commented Oct 16, 2023

/retest

enj
enj suggested changes Oct 16, 2023
View reviewed changes
Copy link
Member

@enj enj left a comment

Choose a reason for hiding this comment

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

Initial feedback, still looking through the PR.

staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go Outdated Show resolved Hide resolved
@MaryamTavakkoli
Copy link

Hello!
Bug triage for the 1.29 release cycle is here! I want to check the status.
The code freeze is starting 01:00 UTC Wednesday 1st November 2023 / 18:00 PDT Tuesday 31st October 2023 (about two weeks from now), and while there is still plenty of time, we want to ensure that each PR has a chance to be merged and issue’s are addressed on time.
As the PR is targeting 1.29, is it still planned for this release?

@ritazh
Copy link
Member Author

ritazh commented Oct 18, 2023

@MaryamTavakkoli Yes it is.

@ritazh ritazh changed the title [WIP] [StructuredAuthorizationConfig] - CEL integration [StructuredAuthorizationConfig] - CEL integration Oct 18, 2023
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 18, 2023
aramase
aramase suggested changes Oct 19, 2023
View reviewed changes
staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/authorization/cel/compile.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 20, 2023
@ritazh ritazh mentioned this pull request Oct 27, 2023
Open
aramase
aramase reviewed Oct 28, 2023
View reviewed changes
staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation_test.go Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation_test.go Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/authorization/cel/compile.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/authorization/cel/compile_test.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/authorization/cel/mapper.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation_test.go Show resolved Hide resolved
staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook_v1_test.go Show resolved Hide resolved
staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook_v1_test.go Show resolved Hide resolved
@palnabarun
Copy link
Member

/test pull-kubernetes-e2e-kind-ipv6

@ritazh ritazh requested a review from enj October 30, 2023 13:02
liggitt
liggitt reviewed Oct 30, 2023
View reviewed changes
staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/authorization/cel/mapper.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/authorization/cel/mapper.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go Outdated Show resolved Hide resolved
@liggitt
Copy link
Member

liggitt commented Oct 30, 2023 •
edited

Comments in WebhookAuthorizer#match are the only ones where the current behavior doesn't look correct and needs to be fixed up and tested.

Other comments are tweaks / suggestions about naming, feature gate checking, micro-optimizations, etc. I'm open to discussion on those or if you want to follow-up.

In a follow-up (ideally before test freeze), it would also be good to add a benchmark around compilation and evaluation in a follow-up so we can quantify the cost of putting match expressions of a few different levels of complexity in a webhook and make sure our evaluation is as cheap / fast as we think it is.

liggitt
liggitt reviewed Oct 31, 2023
View reviewed changes
staging/src/k8s.io/apiserver/pkg/authorization/cel/mapper.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/authorization/cel/mapper.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/authorization/cel/mapper.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/authorization/cel/mapper.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/authorization/cel/mapper.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/authorization/cel/mapper.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/authorization/cel/mapper.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/authorization/cel/mapper.go Outdated Show resolved Hide resolved
@ritazh
Copy link
Member Author

ritazh commented Oct 31, 2023

/retest

@liggitt
Copy link
Member

liggitt commented Oct 31, 2023

can go ahead and squash down when you address the last couple comments, and we'll be all set 🎉

@ritazh
authz: add cel expression to webhook matchconditions
31c76e9 
Signed-off-by: Rita Zhang <rita.z.zhang@gmail.com>
@liggitt
Copy link
Member

liggitt commented Oct 31, 2023

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 31, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 482814d076dfb1332a0d1b48686acb0fca044648

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, ritazh

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 31, 2023
@k8s-ci-robot k8s-ci-robot merged commit 064e86b into kubernetes:master Oct 31, 2023
13 of 15 checks passed
@ritazh ritazh mentioned this pull request Oct 31, 2023
Closed
@palnabarun palnabarun mentioned this pull request Nov 17, 2023
Open
25 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@cici37 cici37 cici37 left review comments

@dims dims Awaiting requested review from dims

@enj enj Awaiting requested review from enj

@aramase aramase Awaiting requested review from aramase

@palnabarun palnabarun Awaiting requested review from palnabarun

Assignees

@liggitt liggitt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
API Reviews
Status: API review completed, 1.29
SIG Auth
Archived in project
Structured Authz Enhancement
Status: ✅ Done
[sig-release] Bug Triage
Archived in project
Milestone
v1.29
Development

Successfully merging this pull request may close these issues.

[StructuredAuthorizationConfig] - CEL integration
8 participants
@ritazh @MaryamTavakkoli @palnabarun @liggitt @k8s-ci-robot @enj @cici37 @aramase