AUTH-541: OIDC structured auth config #1760

liouk · 2024-10-28T12:46:31Z

This PR does the following:

adds unit tests for the AuthMetadata config observer
adds a new config observer for direct OIDC; the config observer:
- detects auth type OIDC and the structured auth config CM in openshift-config-managed
- copies it to openshift-kube-apiserver where it will be used as a revisioned configmap and synced to a static file on all KAS nodes
- enables OIDC config via the --authentication-config CLI flag of the KAS pods
modifies the AuthMetadata and WebhookTokenAuthenticator config observers to delete their respective resources when auth type OIDC is detected

Enhancement: openshift/enhancements#1632

openshift-ci-robot · 2024-10-28T12:46:35Z

@liouk: This pull request references AUTH-541 which is a valid jira issue.

In response to this:

This PR does the following:

adds unit tests for the AuthMetadata config observer

adds a new config observer for direct OIDC; the config observer:

detects auth type OIDC and the structured auth config CM in openshift-config-managed

copies it to openshift-kube-apiserver where it will be used as a revisioned configmap and synced to a static file on all KAS nodes

enables OIDC config via the --authentication-config CLI flag of the KAS pods

modifies the AuthMetadata and WebhookTokenAuthenticator config observers to delete their respective resources when auth type OIDC is detected

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.

openshift-ci · 2024-10-28T12:49:20Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

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

openshift-ci-robot · 2024-10-30T15:48:16Z

@liouk: This pull request references AUTH-541 which is a valid jira issue.

In response to this:

This PR does the following:

adds unit tests for the AuthMetadata config observer

adds a new config observer for direct OIDC; the config observer:

detects auth type OIDC and the structured auth config CM in openshift-config-managed

copies it to openshift-kube-apiserver where it will be used as a revisioned configmap and synced to a static file on all KAS nodes

enables OIDC config via the --authentication-config CLI flag of the KAS pods

modifies the AuthMetadata and WebhookTokenAuthenticator config observers to delete their respective resources when auth type OIDC is detected

Enhancement: openshift/enhancements#1632

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 · 2024-10-31T08:50:10Z

/retest

liouk · 2024-10-31T13:07:24Z

/retest

liouk · 2024-11-01T10:01:18Z

/retest

liouk · 2024-11-05T14:50:17Z

/retest

liouk · 2024-11-15T09:49:12Z

Investigating issue found in https://issues.redhat.com/browse/OCPBUGS-44592, will hold.

/hold

liouk · 2024-11-15T14:01:24Z

/retest

liouk · 2024-11-18T09:38:52Z

/retest

liouk · 2024-11-19T09:46:27Z

/retest

liouk · 2024-11-20T08:37:38Z

Investigating issue found in https://issues.redhat.com/browse/OCPBUGS-44592, will hold.

Issue fixed, cancelling hold.

/hold cancel

benluddy · 2024-11-20T14:10:01Z

pkg/operator/configobservation/auth/external_oidc.go

+		return fmt.Errorf("value of key 'auth-config.json' is empty")
+	}
+
+	var authConfig apiserver.AuthenticationConfiguration


The serialized object should be from one of the external versions of apiserver.config.k8s.io, not the internal version. You'll want to use one of the functions in https://pkg.go.dev/k8s.io/apiserver/pkg/apis/apiserver/load which handle decoding from all external versions and conversion to the internal version for you.

https://pkg.go.dev/k8s.io/apiserver/pkg/apis/apiserver/load only contains Load*() funcs for the AuthorizationConfiguration configuration type, instead of AuthenticationConfiguration which we need here.

Oh, sorry, missed that. Then you'll need to set up the codec yourself here.

benluddy · 2024-11-20T14:15:18Z

pkg/operator/configobservation/auth/external_oidc.go

+	}
+
+	observedConfig := make(map[string]interface{})
+	if err := unstructured.SetNestedField(observedConfig, []interface{}{staticAuthConfigPath}, apiServerArgumentsPath, argAuthConfig); err != nil {


Nit: It's equivalent, but you might consider using SetNestedStringSlice here.

benluddy · 2024-11-20T14:16:59Z

pkg/operator/configobservation/configobservercontroller/observe_config_controller.go

@@ -38,7 +39,7 @@ type ConfigObserver struct {
 	factory.Controller
 }

-func NewConfigObserver(operatorClient v1helpers.StaticPodOperatorClient, kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces, configInformer configinformers.SharedInformerFactory, operatorInformer operatorv1informers.SharedInformerFactory, resourceSyncer resourcesynccontroller.ResourceSyncer, featureGateAccessor featuregates.FeatureGateAccess, eventRecorder events.Recorder, groupVersionsByFeatureGate map[configv1.FeatureGateName][]schema.GroupVersion) *ConfigObserver {


Why is this taking a clientset now? I don't see it being referenced anywhere.

Good catch, leftover from previous changes I'd presume.

benluddy · 2024-11-20T14:38:42Z

pkg/operator/configobservation/auth/webhook_authenticator.go

@@ -62,7 +63,7 @@ func ObserveWebhookTokenAuthenticator(genericListers configobserver.Listers, rec
 	}

 	observedWebhookConfigured := len(webhookSecretName) > 0
-	if observedWebhookConfigured {
+	if observedWebhookConfigured && auth.Spec.Type != configv1.AuthenticationTypeOIDC {


The doc for spec.webhookTokenAuthenticator says:

Can only be set if "Type" is set to "None".

But this seems to consume it when type is either "IntegratedOAuth" or "None". Is this relying on API validation or something else?

That comment is misleading indeed -- this relies on a validation admission plugin: https://github.com/openshift/kubernetes/blob/8540bd493e8a42ffb61a3f470f71290488332b1c/openshift-kube-apiserver/admission/customresourcevalidation/authentication/validate_authentication.go#L103-L130

benluddy · 2024-11-20T14:50:48Z

pkg/operator/configobservation/auth/external_oidc.go

+	if len(sourceName) == 0 {
+		recorder.Eventf("ObserveExternalOIDC", "deleted configmap %s/%s", operatorclient.TargetNamespace, AuthConfigCMName)
+	} else {
+		recorder.Eventf("ObserveExternalOIDC", "synced configmap %s/%s to %s/%s", sourceNamespace, sourceName, operatorclient.TargetNamespace, AuthConfigCMName)
+	}


SyncConfigMap appears to do the actual writes asynchronously, so a nil return value doesn't mean that the writes are done (or will ever succeed). Any errors will show up later in the operator status. If these events are important then I would make the message clear that the write has been requested.

benluddy · 2024-11-20T14:58:09Z

pkg/operator/configobservation/auth/external_oidc.go

+	listers := genericListers.(configobservation.Listers)
+	auth, err := listers.AuthConfigLister.Get("cluster")
+	if errors.IsNotFound(err) {
+		klog.Warningf("authentications.config.openshift.io/cluster: not found")


Other config observers seem to be recording events for similar conditions, is that reasonable to do here?

I followed the example of auth_metadata and webhook_authenticator config observers that don't record events for that case; I'll add events to all three of them, since that's what we usually do.

benluddy · 2024-11-20T15:01:30Z

pkg/operator/configobservation/auth/external_oidc.go

+		if _, err := listers.ConfigMapLister().ConfigMaps(operatorclient.TargetNamespace).Get(AuthConfigCMName); errors.IsNotFound(err) {
+			return nil, nil
+
+		} else if err != nil {
+			return existingConfig, []error{fmt.Errorf("failed to get configmap %s/%s: %v", operatorclient.TargetNamespace, AuthConfigCMName, err)}
+		}


Will the following call to syncConfigMap do effectively the same thing? What's the advantage of short-circuiting here?

I wanted to avoid another sync when we know we don't have that CM anymore, mainly because the syncer seems to be doing a Get API call.

benluddy · 2024-11-20T15:08:46Z

pkg/operator/configobservation/auth/external_oidc.go

+
+	sourceAuthConfig, err := listers.ConfigMapLister().ConfigMaps(SourceAuthConfigCMNamespace).Get(AuthConfigCMName)
+	if errors.IsNotFound(err) {
+		klog.Warningf("configmap %s/%s not found; skipping configuration of OIDC", SourceAuthConfigCMNamespace, AuthConfigCMName)


For my understanding, if the target CM exists and the source CM is missing, the resource syncer will eventually delete the target CM asynchronously, right?

Yes, that's correct, This check exists here to guard from the case where type has been set to OIDC but CAO isn't ready yet and the source CM hasn't appeared yet -- no point in trying to configure OIDC until it does.

benluddy · 2024-11-20T15:11:29Z

pkg/operator/configobservation/auth/external_oidc.go

+		return existingConfig, []error{err}
+	}
+
+	if targetAuthConfig == nil || !equality.Semantic.DeepEqual(targetAuthConfig.Data, sourceAuthConfig.Data) {


I would expect the syncer to be responsible for deciding whether or not it needs to do a write. Why test here?

As per my previous comment, I wanted to avoid an extra API call that the syncer does.

benluddy · 2024-11-20T15:14:16Z

pkg/operator/configobservation/auth/external_oidc.go

+	if cm == nil {
+		return fmt.Errorf("configmap is nil")
+	}


Nit: The caller is already handling "not found", so we never expect this to be called with a nil pointer. Would it be fine to let this panic on a nil pointer dereference since that would violate our assumptions about how it is used?

xingxingxia · 2024-11-22T09:49:26Z

From test result perspective, based on good pre-merge test results in https://issues.redhat.com/browse/OCPBUGS-44592?focusedId=26134688&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-26134688 , adding below label:
/label qe-approved

openshift-ci-robot · 2024-11-22T09:49:30Z

@liouk: This pull request references AUTH-541 which is a valid jira issue.

In response to this:

This PR does the following:

adds unit tests for the AuthMetadata config observer

adds a new config observer for direct OIDC; the config observer:

detects auth type OIDC and the structured auth config CM in openshift-config-managed

copies it to openshift-kube-apiserver where it will be used as a revisioned configmap and synced to a static file on all KAS nodes

enables OIDC config via the --authentication-config CLI flag of the KAS pods

modifies the AuthMetadata and WebhookTokenAuthenticator config observers to delete their respective resources when auth type OIDC is detected

Enhancement: openshift/enhancements#1632

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 · 2024-11-22T11:04:48Z

/retest

… None or OIDC

…type is OIDC

openshift-ci · 2024-12-03T15:37:45Z

@liouk: 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/e2e-aws-operator-disruptive-single-node	`6b9d211`	link	false	`/test e2e-aws-operator-disruptive-single-node`
ci/prow/e2e-aws-ovn-serial	`6b9d211`	link	true	`/test e2e-aws-ovn-serial`
ci/prow/e2e-gcp-operator-single-node	`6b9d211`	link	false	`/test e2e-gcp-operator-single-node`
ci/prow/e2e-aws-ovn-single-node	`6b9d211`	link	false	`/test e2e-aws-ovn-single-node`
ci/prow/k8s-e2e-gcp	`6b9d211`	link	true	`/test k8s-e2e-gcp`

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-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Oct 28, 2024

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 Oct 28, 2024

openshift-ci bot requested review from dgrisonnet and dinhxuanvu October 28, 2024 12:49

liouk mentioned this pull request Oct 29, 2024

AUTH-541: OIDC structured auth config openshift/cluster-authentication-operator#713

Open

liouk force-pushed the oidc-config-structured-auth branch 2 times, most recently from 48a956d to ef36c41 Compare October 30, 2024 14:01

liouk changed the title ~~WIP: AUTH-541: OIDC structured auth config~~ AUTH-541: OIDC structured auth config Oct 30, 2024

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 Oct 30, 2024

liouk mentioned this pull request Nov 5, 2024

WIP: AUTH-544: Extend e2e test to leverage new functionality openshift/cluster-authentication-operator#737

Open

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

liouk force-pushed the oidc-config-structured-auth branch from ef36c41 to 5cdfb5e Compare November 15, 2024 10:57

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

benluddy reviewed Nov 20, 2024

View reviewed changes

liouk force-pushed the oidc-config-structured-auth branch from 5cdfb5e to d98f147 Compare November 21, 2024 09:36

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Nov 22, 2024

liouk force-pushed the oidc-config-structured-auth branch from d98f147 to cabf5cf Compare November 29, 2024 12:54

liouk added 5 commits December 3, 2024 12:50

configobservation/auth: add unit tests for auth metadata config observer

b652ddb

configobservation/auth: add external OIDC config observer

261af6b

operator: add structured auth config as a revision configmap

89165ab

configobservation/auth: remove auth metadata config when auth type is…

89842b4

… None or OIDC

configobservation/auth: remove webhook token authenticator when auth …

6b9d211

…type is OIDC

liouk force-pushed the oidc-config-structured-auth branch from cabf5cf to 6b9d211 Compare December 3, 2024 11:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AUTH-541: OIDC structured auth config #1760

AUTH-541: OIDC structured auth config #1760

liouk commented Oct 28, 2024 •

edited

Loading

openshift-ci-robot commented Oct 28, 2024 •

edited by openshift-ci bot

Loading

openshift-ci bot commented Oct 28, 2024

openshift-ci-robot commented Oct 30, 2024 •

edited by openshift-ci bot

Loading

liouk commented Oct 31, 2024

liouk commented Oct 31, 2024

liouk commented Nov 1, 2024

liouk commented Nov 5, 2024

liouk commented Nov 15, 2024

liouk commented Nov 15, 2024

liouk commented Nov 18, 2024

liouk commented Nov 19, 2024

liouk commented Nov 20, 2024

benluddy Nov 20, 2024

liouk Nov 20, 2024

benluddy Nov 20, 2024

benluddy Nov 20, 2024

benluddy Nov 20, 2024

liouk Nov 20, 2024

benluddy Nov 20, 2024

liouk Nov 20, 2024

benluddy Nov 20, 2024

benluddy Nov 20, 2024

liouk Nov 21, 2024

benluddy Nov 20, 2024

liouk Nov 21, 2024

benluddy Nov 20, 2024

liouk Nov 21, 2024

benluddy Nov 20, 2024

liouk Nov 21, 2024

benluddy Nov 20, 2024

xingxingxia commented Nov 22, 2024

openshift-ci-robot commented Nov 22, 2024 •

edited by openshift-ci bot

Loading

liouk commented Nov 22, 2024

openshift-ci bot commented Dec 3, 2024

AUTH-541: OIDC structured auth config #1760

Are you sure you want to change the base?

AUTH-541: OIDC structured auth config #1760

Conversation

liouk commented Oct 28, 2024 • edited Loading

openshift-ci-robot commented Oct 28, 2024 • edited by openshift-ci bot Loading

openshift-ci bot commented Oct 28, 2024

openshift-ci-robot commented Oct 30, 2024 • edited by openshift-ci bot Loading

liouk commented Oct 31, 2024

liouk commented Oct 31, 2024

liouk commented Nov 1, 2024

liouk commented Nov 5, 2024

liouk commented Nov 15, 2024

liouk commented Nov 15, 2024

liouk commented Nov 18, 2024

liouk commented Nov 19, 2024

liouk commented Nov 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xingxingxia commented Nov 22, 2024

openshift-ci-robot commented Nov 22, 2024 • edited by openshift-ci bot Loading

liouk commented Nov 22, 2024

openshift-ci bot commented Dec 3, 2024

liouk commented Oct 28, 2024 •

edited

Loading

openshift-ci-robot commented Oct 28, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Oct 30, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Nov 22, 2024 •

edited by openshift-ci bot

Loading