CNTRLPLANE-333: Add generation logic for new `uid` and `extra` fields in the Authentication CR #763

everettraven · 2025-03-17T19:56:39Z

No description provided.

openshift-ci-robot · 2025-03-18T13:47:20Z

@everettraven: This pull request references CNTRLPLANE-333 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.19.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.

liouk · 2025-06-05T14:03:51Z

pkg/controllers/externaloidc/externaloidc_controller.go

@@ -111,7 +118,7 @@ func (c *externalOIDCController) sync(ctx context.Context, syncCtx factory.SyncC
 		return nil
 	}

-	if err := validateAuthConfig(*authConfig); err != nil {
+	if err := validateAuthConfig(*authConfig, []string{auth.Spec.ServiceAccountIssuer}); err != nil {


Maybe add a comment explaining why we're adding the ServiceAccountIssuer as a disallowed issuer for the OIDC config.

This is actually something I missed in cleaning up the vendoring of the KAS validation logic.

We should however still check this when generating the issuer URL so I'll move this check to the right place.

liouk · 2025-06-05T14:07:18Z

pkg/controllers/externaloidc/externaloidc_controller.go

+	if extraMapping.Key == "" {
+		return out, fmt.Errorf("extra mapping must set a key, but none was provided: %v", extraMapping)
+	}
+
+	if extraMapping.ValueExpression == "" {


Same as above, use string length comparisons.

liouk · 2025-06-05T14:08:09Z

pkg/controllers/externaloidc/externaloidc_controller.go

+	// Currently, the authentications.config.openshift.io CRD only allows setting a claim for the mapping
+	// and does not allow setting a CEL expression like the upstream. This is likely to change in the future,
+	// but for now just set the claim.
+	if usernameClaimMapping.Claim == "" {


AFAIK it's an openshift convention to use len(str) ==/> 0 comparisons instead of str ==/!= "".

Okay. I'll update this to follow conventions.

liouk · 2025-06-05T14:08:47Z

pkg/controllers/externaloidc/externaloidc_controller.go

+	case uid.Claim != "" && uid.Expression == "":
+		out.Claim = uid.Claim
+	case uid.Expression != "" && uid.Claim == "":
+		out.Expression = uid.Expression
+	case uid.Claim != "" && uid.Expression != "":


Same as above, use string length comparisons.

liouk · 2025-06-05T14:14:00Z

pkg/controllers/externaloidc/externaloidc_controller_test.go

 	}{
 		{
 			name:        "auth resource not found",
 			expectError: true,
+			featureGates: featuregates.NewFeatureGate(


I would only create featureGates in the tests that they're needed to make it easier to follow each test case. For example this makes me think, why do we need FeatureGateExternalOIDCWithAdditionalClaimMappings as disabled for the test? What about ExternalOIDC?

I would only create featureGates in the tests that they're needed to make it easier to follow each test case.

I originally tried that, but without explicitly setting it for each test we break the expectations that we in our actual implementation that we will always have access to feature gates and that the features.FeatureGateExternalOIDCWithAdditionalClaimMappings feature has been registered in that set of feature gates.

Instead of changing the expectations of the implementation in the controller, which I felt were correct, I opted for changing the structure of the tests to be more in line with what the actual state of the world would look like.

For example this makes me think, why do we need FeatureGateExternalOIDCWithAdditionalClaimMappings as disabled for the test?

Anywhere it is disabled is not reliant on it being enabled to function.

What about ExternalOIDC?

There is no logic within the controller itself that is gated on the ExternalOIDC feature gate being enabled.

liouk · 2025-06-05T14:17:23Z

pkg/operator/starter.go

@@ -685,7 +685,7 @@ func prepareExternalOIDC(
 		return nil, nil, err
 	}

-	if !featureGates.Enabled(features.FeatureGateExternalOIDC) {
+	if !(featureGates.Enabled(features.FeatureGateExternalOIDC) || featureGates.Enabled(features.FeatureGateExternalOIDCWithAdditionalClaimMappings)) {


nit, personal preference, feel free to ignore:

Suggested change

if !(featureGates.Enabled(features.FeatureGateExternalOIDC) || featureGates.Enabled(features.FeatureGateExternalOIDCWithAdditionalClaimMappings)) {

if !featureGates.Enabled(features.FeatureGateExternalOIDC) && !featureGates.Enabled(features.FeatureGateExternalOIDCWithAdditionalClaimMappings) {

liouk · 2025-06-05T14:20:16Z

pkg/controllers/externaloidc/externaloidc_controller.go

+	return &authConfig, nil
+}
+
+func generateJWTForProvider(provider configv1.OIDCProvider, configMapLister corev1listers.ConfigMapLister, featureGates featuregates.FeatureGate) (apiserverv1beta1.JWTAuthenticator, error) {


I feel it's a more common pattern to return (*obj, error) so that you can return nil, err where no valid object is returned. Sometimes this can mess up non-simple error checking with multiple conditions and cause confusion, as it's much easier to distinguish the nil pointer from a zero value for complex types.

It generally is, but I decided to break from this pattern to avoid having to litter nil checks everywhere to prevent panics.

The type this bubbles up to and populates is not a pointer so doing the (*obj, error) pattern means that every call to these methods would need to essentially do:

obj, err := generateThing(...) if err != nil { handleErr() } if obj == nil { handleNilObj() } doStuffWithNonPointerObj(*obj)

While I understand the point about distinguishing between nil and zero value, I don't think we actually care about the difference between nil and zero value in this case. We really only care if we got an error and if we did we assume that the generation for the provider was unsuccessful.

test/e2e-oidc/external_oidc_test.go

everettraven · 2025-06-09T12:18:09Z

/retest-required

everettraven · 2025-06-09T19:19:08Z

Because openshift/api#2352 has merged, we will no longer be able to create an Authentication resource without a username claim. I will need to remove the e2e for it, but I will keep the validation logic in place.

Holding until I get that taken care of.
/hold

everettraven · 2025-06-11T16:20:54Z

/hold cancel

liouk · 2025-06-17T11:01:54Z

pkg/controllers/externaloidc/externaloidc_controller.go

+	out.URL = issuer.URL
+	out.AudienceMatchPolicy = apiserverv1beta1.AudienceMatchPolicyMatchAny

-			caData, ok := caConfigMap.Data["ca-bundle.crt"]
-			if !ok || len(caData) == 0 {
-				return nil, fmt.Errorf("configmap %s/%s key \"ca-bundle.crt\" missing or empty", configNamespace, provider.Issuer.CertificateAuthority.Name)
-			}
+	for _, audience := range issuer.Audiences {
+		out.Audiences = append(out.Audiences, string(audience))
+	}

-			jwt.Issuer.CertificateAuthority = caData
+	if len(issuer.CertificateAuthority.Name) > 0 {
+		ca, err := getCertificateAuthorityFromConfigMap(issuer.CertificateAuthority.Name, configMapLister)
+		if err != nil {
+			return out, fmt.Errorf("getting CertificateAuthority for issuer: %v", err)


Since we're sticking to returning objects rather than pointers, let's at least make sure we return empty objects when we encounter an error. We can only set the values to out when we're ready to return the valid result. I'm worried that this might cause confusion if there's an error but some valid values in the result.

This pattern occurs in other places in this file as well.

I think that is reasonable.

My thinking was along the lines of "if an error is returned, ignore any returned values", but I can understand the potential for confusion there.

Signed-off-by: Bryce Palmer <[email protected]>

everettraven · 2025-06-17T12:29:30Z

Should be ready to go, removing hold.

/hold cancel

liouk · 2025-06-17T13:30:42Z

/lgtm

openshift-ci · 2025-06-17T13:31:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: everettraven, liouk

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:

~~OWNERS~~ [liouk]

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

openshift-ci-robot · 2025-06-17T14:05:57Z

/retest-required

Remaining retests: 0 against base HEAD ad0f9dd and 2 for PR HEAD 6bbed73 in total

openshift-ci-robot · 2025-06-17T16:53:31Z

/retest-required

Remaining retests: 0 against base HEAD ad0f9dd and 2 for PR HEAD 6bbed73 in total

everettraven · 2025-06-17T19:24:21Z

/retest-required

everettraven · 2025-06-18T11:55:44Z

/retest-required

Signed-off-by: Bryce Palmer <[email protected]>

openshift-ci · 2025-06-18T15:24:20Z

New changes are detected. LGTM label has been removed.

everettraven · 2025-06-24T17:27:00Z

/retest

Signed-off-by: Bryce Palmer <[email protected]>

openshift-ci · 2025-06-25T20:20:03Z

@everettraven: 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/okd-scos-e2e-aws-ovn	`5869544`	link	false	`/test okd-scos-e2e-aws-ovn`
ci/prow/test-operator-integration	`5869544`	link	false	`/test test-operator-integration`

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.

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 Mar 17, 2025

openshift-ci bot requested review from ibihim and liouk March 17, 2025 19:57

everettraven changed the title ~~WIP: Add generation logic for new uid and extra fields in the Authentication CR~~ WIP: CNTRLPLANE-333: Add generation logic for new uid and extra fields in the Authentication CR Mar 18, 2025

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

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2025

everettraven force-pushed the feature/oidc-uid-extra branch from 1cedc4b to 042cceb Compare June 2, 2025 19:28

openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 2, 2025

everettraven force-pushed the feature/oidc-uid-extra branch from 40c4b3c to 2d8d6a2 Compare June 4, 2025 19:21

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 4, 2025

xingxingxia mentioned this pull request Jun 5, 2025

OSDOCS-13556 4.19 update to TechPreviewNoUpgrade openshift/openshift-docs#94206

Merged

1 task

everettraven changed the title ~~WIP: CNTRLPLANE-333: Add generation logic for new uid and extra fields in the Authentication CR~~ CNTRLPLANE-333: Add generation logic for new uid and extra fields in the Authentication CR Jun 5, 2025

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

liouk suggested changes Jun 5, 2025

View reviewed changes

everettraven commented Jun 5, 2025

View reviewed changes

test/e2e-oidc/external_oidc_test.go Outdated Show resolved Hide resolved

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 9, 2025

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 11, 2025

liouk suggested changes Jun 17, 2025

View reviewed changes

everettraven added 2 commits June 17, 2025 08:28

externaloidc: add generation logic for uid and extra claim mappings

c6c2aba

Signed-off-by: Bryce Palmer <[email protected]>

e2e-oidc: update tests and take into account the new feature gate

6bbed73

Signed-off-by: Bryce Palmer <[email protected]>

everettraven force-pushed the feature/oidc-uid-extra branch from a0beebc to 6bbed73 Compare June 17, 2025 12:28

openshift-ci bot assigned liouk Jun 17, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2025

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 17, 2025

wip: bump precondition timeout

47f657a

Signed-off-by: Bryce Palmer <[email protected]>

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 18, 2025

everettraven added 2 commits June 25, 2025 08:10

wip: even longer timeout

b722c4a

Signed-off-by: Bryce Palmer <[email protected]>

wip: 20 minute timeout

5869544

Signed-off-by: Bryce Palmer <[email protected]>

	if !(featureGates.Enabled(features.FeatureGateExternalOIDC) \|\| featureGates.Enabled(features.FeatureGateExternalOIDCWithAdditionalClaimMappings)) {
	if !featureGates.Enabled(features.FeatureGateExternalOIDC) && !featureGates.Enabled(features.FeatureGateExternalOIDCWithAdditionalClaimMappings) {

CNTRLPLANE-333: Add generation logic for new uid and extra fields in the Authentication CR #763

Are you sure you want to change the base?

CNTRLPLANE-333: Add generation logic for new uid and extra fields in the Authentication CR #763

Conversation

everettraven commented Mar 17, 2025

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

everettraven Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

everettraven commented Jun 9, 2025

Uh oh!

everettraven commented Jun 9, 2025

Uh oh!

everettraven commented Jun 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

everettraven commented Jun 17, 2025

Uh oh!

liouk commented Jun 17, 2025

Uh oh!

openshift-ci bot commented Jun 17, 2025

Uh oh!

openshift-ci-robot commented Jun 17, 2025

Uh oh!

openshift-ci-robot commented Jun 17, 2025

Uh oh!

everettraven commented Jun 17, 2025

Uh oh!

everettraven commented Jun 18, 2025

Uh oh!

openshift-ci bot commented Jun 18, 2025

Uh oh!

everettraven commented Jun 24, 2025

Uh oh!

openshift-ci bot commented Jun 25, 2025

Uh oh!

Uh oh!

CNTRLPLANE-333: Add generation logic for new `uid` and `extra` fields in the Authentication CR #763

CNTRLPLANE-333: Add generation logic for new `uid` and `extra` fields in the Authentication CR #763

openshift-ci-robot commented Mar 18, 2025 •

edited by openshift-ci bot

Loading

everettraven Jun 5, 2025 •

edited

Loading