CNTRLPLANE-940: Add CEL expression compile validation for Authentication admission #2353
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
|
Skipping CI for Draft Pull Request. |
|
@everettraven: This pull request references CNTRLPLANE-940 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.20.0" version, but no target version was set. In 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. |
|
@everettraven: 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 |
|
/test all |
1 similar comment
|
/test all |
|
/retest-required |
everettraven
force-pushed
the
feature/oidc-cel-admission-plugin
branch
from
435949d to
ec4738b
Compare
|
@everettraven: 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
force-pushed
the
feature/oidc-cel-admission-plugin
branch
from
ec4738b to
9b472e3
Compare
|
@everettraven: 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: This pull request references CNTRLPLANE-940 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.20.0" version, but no target version was set. In 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. |
|
/retest-required |
| } | ||
| } | ||
|
|
||
| if spec.Type == configv1.AuthenticationTypeOIDC { |
There was a problem hiding this comment.
It might make sense to split the validations of this func into two sub-funcs, one for OAuth and one for OIDC, as it's currently mixed.
There was a problem hiding this comment.
Are you thinking along the lines of having something like:
switch spec.Type {
case configv1.AuthenticationTypeOIDC:
validateOIDC(...)
case ...:
...
}?
I considered doing something like that, but wasn't sold on it being worth the potential scope creep. If you think it would be useful to do that though I can make some changes.
There was a problem hiding this comment.
I think I would prefer it, but no strong opinion. You could maybe at least separate the respective validations (i.e. put the oidc validation after the oauth metadata) and add some comments to section them out; that'd be fine as well.
There was a problem hiding this comment.
When I went to change towards a switch-based approach I noticed that there are not any other admission validations that ensure that the oauthMetadata field is only set when a particular type is set. Nor do we have a guarantee for the oidcProviders field on a type requirement.
For now, I'll section with comments, but I'd like to avoid any changes in this PR that might change how this validation has worked historically
...-kube-apiserver/admission/customresourcevalidation/authentication/validate_authentication.go
Outdated
Show resolved
Hide resolved
|
@everettraven: 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
force-pushed
the
feature/oidc-cel-admission-plugin
branch
from
f0026aa to
f92c8e7
Compare
|
@everettraven: 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
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
Should not merge until after kube 1.33 rebase is complete. /hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
/lgtm |
openshift-ci
bot
added
backports/validated-commits
1 similar comment
|
/retest-required |
|
/hold Revision 2c5cdef was retested 3 times: holding |
openshift-ci
bot
added
the
do-not-merge/hold
|
/retest-required |
|
Required tests look to be passing. Non-required tests seem to be unrelated failures. Removing hold. /hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
1 similar comment
1 similar comment
1 similar comment
|
Looks like the The regression appears to be because the test itself is extremely flaky. From what I can tell from the PR test history, these tests have passed on the latest changes but are flaking while re-running jobs while this PR is in the merge pool. |
|
@p0lyn0mial @benluddy Are both of you comfortable with an override on the failures hung up on the known regression? |
|
/test e2e-aws-ovn-fips |
|
/retest |
openshift-merge-bot
bot
merged commit 9faed01
into
openshift:master
|
@everettraven: The following tests failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
6 participants
What this PR does / why we need it:
Updates the OpenShift-specific admission plugin for admission time validation of the
authentications.config.openshift.ioresource to add validation for CEL expressions that can be specified in claim mappings as of openshift/api#2234 in TPNU clusters.This changes makes it so that we can reject, at admission time, CEL expressions that will not successfully compile.