CNTRLPLANE-2553: Update the KAS admission plugin for fields to be compilable at admission time#2627
CNTRLPLANE-2553: Update the KAS admission plugin for fields to be compilable at admission time#2627ShazaAldawamneh wants to merge 3 commits intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
backports/unvalidated-commits
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@ShazaAldawamneh: This pull request references CNTRLPLANE-2553 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@ShazaAldawamneh: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughUpdates dependency pins and refactors OIDC CEL validation to compile and propagate per-expression Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@ShazaAldawamneh: This pull request references CNTRLPLANE-2553 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@openshift-kube-apiserver/admission/customresourcevalidation/authentication/validate_authentication.go`:
- Around line 518-576: validateEmailVerifiedUsage currently re-runs
validateCELExpression (and records cost) for expressions already validated in
validateClaimMappings and validateClaimValidationRules; change it to reuse
earlier compilation results or perform AST-only checks without touching
costRecorder: modify validateClaimMappings and validateClaimValidationRules to
return the validated AST/results (e.g., a map or struct of expressions ->
AST/validationResult from validateCELExpression) and update
validateEmailVerifiedUsage to accept those precompiled results (or call a new
helper like inspectASTUsesEmail* that only inspects an AST without invoking
validateCELExpression or costRecorder). Ensure references to
validateCELExpression, costRecorder, cel (celStore), validateClaimMappings,
validateClaimValidationRules, and validateEmailVerifiedUsage are updated so no
duplicate costRecorder recordings occur.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5ad82031-bef3-4c8b-a0f7-bd7700cce715
⛔ Files ignored due to path filters (38)
go.sumis excluded by!**/*.sumstaging/src/k8s.io/apiextensions-apiserver/go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/apps/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/v1/zz_prerelease_lifecycle_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/authorization/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/authorization/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/build/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/build/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_insights.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/image/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/image/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/network/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/network/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/oauth/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/oauth/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/project/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/project/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/security/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/security/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/template/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/template/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/user/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/user/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (4)
go.modopenshift-kube-apiserver/admission/customresourcevalidation/authentication/validate_authentication.goopenshift-kube-apiserver/admission/customresourcevalidation/authentication/validate_authentication_test.gostaging/src/k8s.io/apiextensions-apiserver/go.mod
|
@ShazaAldawamneh you may want to change this line to _, results <- validateCELExpression(context.TODO(), cel, &costRecorder{}, fieldPath, expression)=== Errors
openshift-kube-apiserver/admission/customresourcevalidation/authentication/validate_authentication_test.go:701:15: multiple-value validateCELExpression(context.TODO(), cel, &costRecorder{}, fieldPath, expression) (value of type ("k8s.io/apiserver/pkg/authentication/cel".CompilationResult, "k8s.io/apimachinery/pkg/util/validation/field".ErrorList)) in single-value context |
|
Also, you'll need to run The above comment will address the other errors there too. :) |
|
For |
|
You may also need to re-run the tests. I believe some of them were infrastructure related failures. |
ShazaAldawamneh
force-pushed
the
CNTRLPLANE-2553-2
branch
from
d813c4e to
20f395b
Compare
|
@ShazaAldawamneh: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
@ShazaAldawamneh: This pull request references CNTRLPLANE-2553 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openshift-kube-apiserver/admission/customresourcevalidation/authentication/validate_authentication.go (1)
449-452:⚠️ Potential issue | 🔴 CriticalMake CEL compilation and cache keys accessor-aware.
Line 474 always calls
CompileClaimsExpression, which provides theclaimsvariable in the CEL environment. However, whenUserValidationConditionis passed (line 401), it should instead callCompileUserExpressionto provide theuservariable. Additionally, cache/singleflight keys (lines 449, 451, 506) use only raw expression text, causing different accessor types with identical expressions to incorrectly reuse compilation results compiled for the wrong CEL environment.🛠️ Proposed fix
+func compileByAccessor(cel *celStore, accessor authenticationcel.ExpressionAccessor) (authenticationcel.CompilationResult, error) { + switch accessor.(type) { + case *authenticationcel.UserValidationCondition: + return cel.compiler.CompileUserExpression(accessor) + default: + return cel.compiler.CompileClaimsExpression(accessor) + } +} + func validateCELExpression(ctx context.Context, cel *celStore, costRecorder *costRecorder, path *field.Path, accessor authenticationcel.ExpressionAccessor) (authenticationcel.CompilationResult, field.ErrorList) { // if context has been canceled, don't try to compile any expressions @@ - result, err, _ := cel.compilingGroup.Do(accessor.GetExpression(), func() (interface{}, error) { + cacheKey := fmt.Sprintf("%T:%s", accessor, accessor.GetExpression()) + result, err, _ := cel.compilingGroup.Do(cacheKey, func() (interface{}, error) { @@ - if val, ok := cel.compiledStore.Get(accessor.GetExpression()); ok { + if val, ok := cel.compiledStore.Get(cacheKey); ok { @@ - compRes, compErr := cel.compiler.CompileClaimsExpression(accessor) + compRes, compErr := compileByAccessor(cel, accessor) @@ - cel.compiledStore.Add(accessor.GetExpression(), res) + cel.compiledStore.Add(cacheKey, res)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift-kube-apiserver/admission/customresourcevalidation/authentication/validate_authentication.go` around lines 449 - 452, The compilation code currently always calls CompileClaimsExpression and uses the raw expression text as the singleflight/cache key; update the logic around cel.compilingGroup.Do(accessor.GetExpression(), ...), the compiledStore lookup (cel.compiledStore.Get(accessor.GetExpression())), and any further cache keys so they incorporate the accessor type (e.g., accessor.Type() or reflect.TypeOf(accessor)) to make keys accessor-aware, and branch on the accessor kind so that when accessor is a UserValidationCondition you call CompileUserExpression(accessor.GetExpression()) instead of CompileClaimsExpression, otherwise call CompileClaimsExpression; ensure both the singleflight key and compiledStore key use the combined identifier (accessor-type + expression) and return the corresponding celCompileResult.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@openshift-kube-apiserver/admission/customresourcevalidation/authentication/validate_authentication.go`:
- Around line 558-559: Update the error message string in
validate_authentication.go that currently ends with
"claimValidationRules[*].expression" to instead reference the accurate validated
field path "claimValidationRules[*].cel.expression"; locate the message
construction around the claims/email_verified validation (the string passed into
the field error creation where claims.email is mentioned) and replace the
incorrect substring so users see "claimValidationRules[*].cel.expression".
---
Outside diff comments:
In
`@openshift-kube-apiserver/admission/customresourcevalidation/authentication/validate_authentication.go`:
- Around line 449-452: The compilation code currently always calls
CompileClaimsExpression and uses the raw expression text as the
singleflight/cache key; update the logic around
cel.compilingGroup.Do(accessor.GetExpression(), ...), the compiledStore lookup
(cel.compiledStore.Get(accessor.GetExpression())), and any further cache keys so
they incorporate the accessor type (e.g., accessor.Type() or
reflect.TypeOf(accessor)) to make keys accessor-aware, and branch on the
accessor kind so that when accessor is a UserValidationCondition you call
CompileUserExpression(accessor.GetExpression()) instead of
CompileClaimsExpression, otherwise call CompileClaimsExpression; ensure both the
singleflight key and compiledStore key use the combined identifier
(accessor-type + expression) and return the corresponding celCompileResult.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c5022cb4-c143-4f70-8b4c-bd930a62a359
⛔ Files ignored due to path filters (38)
go.sumis excluded by!**/*.sumstaging/src/k8s.io/apiextensions-apiserver/go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/apps/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/v1/zz_prerelease_lifecycle_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/authorization/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/authorization/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/build/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/build/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_insights.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/image/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/image/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/network/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/network/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/oauth/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/oauth/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/project/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/project/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/security/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/security/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/template/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/template/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/user/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/user/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (4)
go.modopenshift-kube-apiserver/admission/customresourcevalidation/authentication/validate_authentication.goopenshift-kube-apiserver/admission/customresourcevalidation/authentication/validate_authentication_test.gostaging/src/k8s.io/apiextensions-apiserver/go.mod
|
@ShazaAldawamneh: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
@ShazaAldawamneh: This pull request references CNTRLPLANE-2553 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
openshift-kube-apiserver/admission/customresourcevalidation/authentication/validate_authentication.go (1)
528-596: Consider extracting shared logic to reduce duplication.
validateUserCELExpressionduplicates ~70 lines fromvalidateCELExpression, differing only in the compiler method call (CompileUserExpressionvsCompileClaimsExpression). Consider extracting the common logic into a shared helper that accepts a compile function.♻️ Suggested refactor approach
type compileFunc func(authenticationcel.ExpressionAccessor) (authenticationcel.CompilationResult, error) func validateCELExpressionWithCompiler( ctx context.Context, cel *celStore, costRecorder *costRecorder, path *field.Path, accessor authenticationcel.ExpressionAccessor, cachePrefix string, compile compileFunc, ) (*authenticationcel.CompilationResult, field.ErrorList) { // shared implementation with compile function parameter cacheKey := cachePrefix + accessor.GetExpression() // ... rest of shared logic using compile(accessor) instead of direct calls } func validateCELExpression(...) (*authenticationcel.CompilationResult, field.ErrorList) { return validateCELExpressionWithCompiler(ctx, cel, costRecorder, path, accessor, "claims:", cel.compiler.CompileClaimsExpression) } func validateUserCELExpression(...) (*authenticationcel.CompilationResult, field.ErrorList) { return validateCELExpressionWithCompiler(ctx, cel, costRecorder, path, accessor, "user:", cel.compiler.CompileUserExpression) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift-kube-apiserver/admission/customresourcevalidation/authentication/validate_authentication.go` around lines 528 - 596, The two functions validateUserCELExpression and validateCELExpression duplicate compilation/caching logic; extract the shared behavior into a helper (e.g., validateCELExpressionWithCompiler) that accepts the celStore, costRecorder, path, accessor, a cache prefix string, and a compile function (type: func(authenticationcel.ExpressionAccessor) (authenticationcel.CompilationResult, error)); move the shared steps (ctx.Err check, compilingGroup.Do wrapper, cache lookup via compiledStore, warning timer, calling checker.Cost, storing to compiledStore, handling compileErr and costRecorder.AddRecording) into that helper and replace the direct calls to cel.compiler.CompileUserExpression / CompileClaimsExpression with the injected compile function in the helper; then implement validateUserCELExpression and validateCELExpression as thin wrappers that pass "user:" or "claims:" cache prefixes and cel.compiler.CompileUserExpression or cel.compiler.CompileClaimsExpression respectively.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@openshift-kube-apiserver/admission/customresourcevalidation/authentication/validate_authentication.go`:
- Around line 614-622: The code calls usesEmailClaim(usernameResult.AST) and
usesEmailVerifiedClaim(usernameResult.AST) without ensuring usernameResult.AST
is non-nil; add a defensive nil check for usernameResult.AST in
validateEmailVerifiedUsage so that you only call usesEmailClaim and
usesEmailVerifiedClaim when usernameResult.AST != nil (otherwise treat those
booleans as false), while keeping the existing
anyUsesEmailVerifiedClaim(extraResults) /
anyUsesEmailVerifiedClaim(claimValidationResults) logic unchanged; reference:
validateEmailVerifiedUsage, usernameResult.AST, usesEmailClaim,
usesEmailVerifiedClaim, anyUsesEmailVerifiedClaim.
- Around line 535-542: The cache key currently uses accessor.GetExpression() for
both claims and user compilation paths which causes collisions; update
validateCELExpression and validateUserCELExpression so the keys passed into
cel.compilingGroup.Do and cel.compiledStore.Get/Add are namespaced by expression
type (e.g. prefix with "claims:" for CompileClaimsExpression and "user:" for
CompileUserExpression) instead of the raw accessor.GetExpression(), and ensure
the same prefixed key is used consistently wherever cel.compilingGroup.Do,
cel.compiledStore.Get, or cel.compiledStore.Add are called.
---
Nitpick comments:
In
`@openshift-kube-apiserver/admission/customresourcevalidation/authentication/validate_authentication.go`:
- Around line 528-596: The two functions validateUserCELExpression and
validateCELExpression duplicate compilation/caching logic; extract the shared
behavior into a helper (e.g., validateCELExpressionWithCompiler) that accepts
the celStore, costRecorder, path, accessor, a cache prefix string, and a compile
function (type: func(authenticationcel.ExpressionAccessor)
(authenticationcel.CompilationResult, error)); move the shared steps (ctx.Err
check, compilingGroup.Do wrapper, cache lookup via compiledStore, warning timer,
calling checker.Cost, storing to compiledStore, handling compileErr and
costRecorder.AddRecording) into that helper and replace the direct calls to
cel.compiler.CompileUserExpression / CompileClaimsExpression with the injected
compile function in the helper; then implement validateUserCELExpression and
validateCELExpression as thin wrappers that pass "user:" or "claims:" cache
prefixes and cel.compiler.CompileUserExpression or
cel.compiler.CompileClaimsExpression respectively.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 203ba4e9-5c1b-47d4-a036-5c1215161fd8
📒 Files selected for processing (1)
openshift-kube-apiserver/admission/customresourcevalidation/authentication/validate_authentication.go
ShazaAldawamneh
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
ShazaAldawamneh
force-pushed
the
CNTRLPLANE-2553-2
branch
from
abc1f3f to
1d616d3
Compare
|
New changes are detected. LGTM label has been removed. |
|
@ShazaAldawamneh: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
ShazaAldawamneh
force-pushed
the
CNTRLPLANE-2553-2
branch
from
6876095 to
4205b96
Compare
|
@ShazaAldawamneh: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
/retest-required |
|
@ShazaAldawamneh: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
ShazaAldawamneh
force-pushed
the
CNTRLPLANE-2553-2
branch
from
eccecd2 to
a7349c7
Compare
|
@ShazaAldawamneh: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
A few comments that I think need to be addressed.
Aside from this, I'm double checking whether or not there are additional things we need to be aware of from a commit ordering / rollup standpoint for the kube rebase process.
Closing the loop - doesn't look like we need to do anything special here. |
ShazaAldawamneh
force-pushed
the
CNTRLPLANE-2553-2
branch
from
a7349c7 to
9e24058
Compare
|
@ShazaAldawamneh: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Aside from needing to remove a now unnecessary test this LGTM
ShazaAldawamneh
force-pushed
the
CNTRLPLANE-2553-2
branch
from
9e24058 to
a8fefbc
Compare
|
@ShazaAldawamneh: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
ShazaAldawamneh
force-pushed
the
CNTRLPLANE-2553-2
branch
from
a8fefbc to
9d57875
Compare
|
@ShazaAldawamneh: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
ShazaAldawamneh
force-pushed
the
CNTRLPLANE-2553-2
branch
from
9d57875 to
87f253d
Compare
|
@ShazaAldawamneh: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
…e when claims.email is used in username expression Signed-off-by: Shaza Aldawamneh <shaza.aldawamneh@hotmail.com>
Signed-off-by: Shaza Aldawamneh <shaza.aldawamneh@hotmail.com>
Signed-off-by: Shaza Aldawamneh <shaza.aldawamneh@hotmail.com>
ShazaAldawamneh
force-pushed
the
CNTRLPLANE-2553-2
branch
from
87f253d to
0cdaa75
Compare
|
@ShazaAldawamneh: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ehearne-redhat, ShazaAldawamneh The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/retest |
|
@ShazaAldawamneh: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
7 participants
What type of PR is this?
kind api-change
What this PR does / why we need it:
Extends the existing CEL validation framework in the authentication admission plugin to compile and cost-estimate the new CEL expression fields introduced in openshift/api#2487 and openshift/api#2719:
claimMappings.username.expression,claimMappings.groups.expression,claimValidationRules[].cel.expression, anduserValidationRules[].expression. This gives users an early feedback loop at admission time rather than discovering invalid expressions at runtime.Which issue(s) this PR is related to:
CNTRLPLANE-2553
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: