[WIP]: CNTRLPLANE-312: Add generation logic for new API fields in the Authentication CR #810

ShazaAldawamneh · 2025-11-17T11:06:30Z

No description provided.

Signed-off-by: Shaza Aldawamneh <shaza.aldawamneh@hotmail.com>

openshift-ci-robot · 2025-11-17T11:06:35Z

@ShazaAldawamneh: This pull request references CNTRLPLANE-312 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.21.0" version, but no target version was set.

Details

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.

coderabbitai · 2025-11-17T11:06:35Z

Important

Review skipped

Auto reviews are limited based on label configuration.

🚫 Excluded labels (none allowed) (1)

do-not-merge/work-in-progress

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

openshift-ci · 2025-11-17T11:09:27Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ShazaAldawamneh
Once this PR has been reviewed and has the lgtm label, please assign liouk for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

everettraven

Overall, the structure looks pretty good.

I have a handful of comments.

pkg/controllers/externaloidc/externaloidc_controller.go

everettraven · 2025-11-18T13:21:51Z

pkg/controllers/externaloidc/externaloidc_controller.go

+		// validate CEL expression
+		if err := validateCELExpression(&authenticationcel.ClaimMappingExpression{
+			Expression: claimValidationRule.Expression.Expression,
+		}); err != nil {
+			return apiserverv1beta1.ClaimValidationRule{}, fmt.Errorf("invalid CEL expression: %v", err)
+		}


As a note, we should update https://github.com/openshift/kubernetes/blob/891f5bb0306166d5625b89fc8dc86bbc8c85f549/openshift-kube-apiserver/admission/customresourcevalidation/authentication/validate_authentication.go#L240 to perform admission time validation of these expressions.

It doesn't hurt to also include the runtime validations here, but admission validations should be sufficient here because we are embedding that in the kube-apiserver

I'm not entirely sure what you mean here, can you explain more in details on what needs to be done ?

I mean that we will also need to take an action item to update our admission plugin that we inject into the Kubernetes API server to validate that the CEL expressions here compile successfully.

There is nothing that needs to be done in this PR to address this comment.

I see, make sense, will make a note on that!

pkg/controllers/externaloidc/externaloidc_controller.go

everettraven · 2025-11-18T13:49:26Z

pkg/controllers/externaloidc/externaloidc_controller_test.go

+					DiscoveryURL:         "https://example.com/.well-known/openid-configuration",
 				},
 				OIDCClients: []configv1.OIDCClientConfig{
 					{
 						ComponentName:      "console",
 						ComponentNamespace: "openshift-console",
 						ClientID:           "console-oidc-client",
 					},
 					{
 						ComponentName:      "kube-apiserver",
 						ComponentNamespace: "openshift-kube-apiserver",
 						ClientID:           "test-oidc-client",
 					},
 				},
 				ClaimMappings: configv1.TokenClaimMappings{
 					Username: configv1.UsernameClaimMapping{
 						Claim:        "username",
 						PrefixPolicy: configv1.Prefix,
 						Prefix: &configv1.UsernamePrefix{
 							PrefixString: "oidc-user:",
 						},
 					},
 					Groups: configv1.PrefixedClaimMapping{
 						TokenClaimMapping: configv1.TokenClaimMapping{
 							Claim: "groups",
 						},
 						Prefix: "oidc-group:",
 					},
 				},
 				ClaimValidationRules: []configv1.TokenClaimValidationRule{
 					{
-						Type: configv1.TokenValidationRuleTypeRequiredClaim,
+						Type: configv1.TokenValidationRuleRequiredClaim,
 						RequiredClaim: &configv1.TokenRequiredClaim{
 							Claim:         "username",
 							RequiredValue: "test-username",
 						},
 					},
 					{
-						Type: configv1.TokenValidationRuleTypeRequiredClaim,
-						RequiredClaim: &configv1.TokenRequiredClaim{
-							Claim:         "email",
-							RequiredValue: "test-email",
+						Type: configv1.TokenValidationRuleExpression,
+						Expression: configv1.TokenExpressionRule{
+							Expression: "claims.email.endsWith('@example.com')",
+							Message:    "email domain must be example.com",
 						},
 					},
 				},
+				UserValidationRules: []configv1.TokenUserValidationRule{
+					{
+						Expression: "user.groups.exists(g, g == 'system:authenticated')",
+						Message:    "user must be authenticated",
+					},
+				},


I don't think we should be making these changes to the base configuration because these new fields are only present on a cluster where the associated feature gate is enabled.

These tests will run against clusters where the feature gate is and is not enabled.

Do you mean that the existing tests should remain unchanged, and that I should avoid modifying the base expected configuration?

Yes, you should not modify existing tests to fit the requirements of a tech-preview feature. Instead, add new tests that are gated on that feature-gate being present and make modifications accordingly.

As an example:

cluster-authentication-operator/test/e2e-oidc/external_oidc_test.go

Lines 158 to 166 in e6c52f8

{

name: "uncompilable CEL expression for uid claim mapping",

specUpdate: func(s *configv1.AuthenticationSpec) {

s.OIDCProviders[0].ClaimMappings.UID = &configv1.TokenClaimOrExpressionMapping{

Expression: "^&*!@#^*(",

}

},

requireFeatureGates: []configv1.FeatureGateName{features.FeatureGateExternalOIDCWithAdditionalClaimMappings},

},

pkg/controllers/externaloidc/externaloidc_controller_test.go

pkg/controllers/externaloidc/externaloidc_controller.go

Signed-off-by: Shaza Aldawamneh <shaza.aldawamneh@hotmail.com>

openshift-ci · 2025-12-08T18:22:03Z

@ShazaAldawamneh: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/unit	`07257c7`	link	true	`/test unit`
ci/prow/e2e-oidc-techpreview	`07257c7`	link	true	`/test e2e-oidc-techpreview`
ci/prow/e2e-oidc	`07257c7`	link	true	`/test e2e-oidc`

Full PR test history. Your PR dashboard.

Details

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.

adding the API new fields to the cluster-auth-operator

810a537

Signed-off-by: Shaza Aldawamneh <shaza.aldawamneh@hotmail.com>

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 17, 2025

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 17, 2025

openshift-ci bot requested review from ibihim and liouk November 17, 2025 11:09

everettraven reviewed Nov 18, 2025

View reviewed changes

ShazaAldawamneh added 2 commits December 2, 2025 12:16

feature gating all new fields

0a90751

Signed-off-by: Shaza Aldawamneh <shaza.aldawamneh@hotmail.com>

featuregating tests

07257c7

Signed-off-by: Shaza Aldawamneh <shaza.aldawamneh@hotmail.com>

	{
	name: "uncompilable CEL expression for uid claim mapping",
	specUpdate: func(s *configv1.AuthenticationSpec) {
	s.OIDCProviders[0].ClaimMappings.UID = &configv1.TokenClaimOrExpressionMapping{
	Expression: "^&!@#^(",
	}
	},
	requireFeatureGates: []configv1.FeatureGateName{features.FeatureGateExternalOIDCWithAdditionalClaimMappings},
	},

[WIP]: CNTRLPLANE-312: Add generation logic for new API fields in the Authentication CR #810

Are you sure you want to change the base?

[WIP]: CNTRLPLANE-312: Add generation logic for new API fields in the Authentication CR #810

Conversation

ShazaAldawamneh commented Nov 17, 2025

Uh oh!

openshift-ci-robot commented Nov 17, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

openshift-ci bot commented Nov 17, 2025

Uh oh!

everettraven left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

everettraven Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

ShazaAldawamneh Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

everettraven Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

ShazaAldawamneh Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

everettraven Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

ShazaAldawamneh Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

everettraven Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

openshift-ci bot commented Dec 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

openshift-ci-robot commented Nov 17, 2025 •

edited by openshift-ci bot

Loading

coderabbitai bot commented Nov 17, 2025 •

edited

Loading