From 41278f440df85518a565c442874df9eb0019c61c Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Thu, 10 Oct 2024 11:24:56 +0200 Subject: [PATCH] configobservation/auth: remove auth metadata config when auth type is None or OIDC --- .../configobservation/auth/auth_metadata.go | 66 ++++++++++--------- .../auth/auth_metadata_test.go | 54 ++++++++++++--- 2 files changed, 79 insertions(+), 41 deletions(-) diff --git a/pkg/operator/configobservation/auth/auth_metadata.go b/pkg/operator/configobservation/auth/auth_metadata.go index a3309eb4c2..9dae7247ea 100644 --- a/pkg/operator/configobservation/auth/auth_metadata.go +++ b/pkg/operator/configobservation/auth/auth_metadata.go @@ -8,6 +8,7 @@ import ( configv1 "github.com/openshift/api/config/v1" "github.com/openshift/cluster-kube-apiserver-operator/pkg/operator/configobservation" + "github.com/openshift/cluster-kube-apiserver-operator/pkg/operator/operatorclient" "github.com/openshift/library-go/pkg/operator/configobserver" "github.com/openshift/library-go/pkg/operator/events" "github.com/openshift/library-go/pkg/operator/resourcesynccontroller" @@ -39,7 +40,7 @@ func ObserveAuthMetadata(genericListers configobserver.Listers, recorder events. } observedConfig := map[string]interface{}{} - authConfigNoDefaults, err := listers.AuthConfigLister.Get("cluster") + authConfig, err := listers.AuthConfigLister.Get("cluster") if errors.IsNotFound(err) { klog.Warningf("authentications.config.openshift.io/cluster: not found") return observedConfig, errs @@ -49,34 +50,45 @@ func ObserveAuthMetadata(genericListers configobserver.Listers, recorder events. return prevObservedConfig, errs } - authConfig := defaultAuthConfig(authConfigNoDefaults) - var ( sourceNamespace string sourceConfigMap string - statusConfigMap string ) - specConfigMap := authConfig.Spec.OAuthMetadata.Name + switch authConfig.Spec.Type { + case configv1.AuthenticationTypeIntegratedOAuth, "": + specConfigMap := authConfig.Spec.OAuthMetadata.Name + statusConfigMap := authConfig.Status.IntegratedOAuthMetadata.Name + if len(statusConfigMap) == 0 { + klog.V(5).Infof("no integrated oauth metadata configmap observed from status") + } - // TODO: Add a case here for the KeyCloak type. - switch { - case len(authConfig.Status.IntegratedOAuthMetadata.Name) > 0 && authConfig.Spec.Type == configv1.AuthenticationTypeIntegratedOAuth: - statusConfigMap = authConfig.Status.IntegratedOAuthMetadata.Name - default: - klog.V(5).Infof("no integrated oauth metadata configmap observed from status") - } + // Spec configMap takes precedence over Status. + switch { + case len(specConfigMap) > 0: + sourceConfigMap = specConfigMap + sourceNamespace = configNamespace + case len(statusConfigMap) > 0: + sourceConfigMap = statusConfigMap + sourceNamespace = managedNamespace + default: + klog.V(5).Infof("no authentication config metadata specified") + } + + case configv1.AuthenticationTypeNone: + // no oauth metadata is served; do not set anything as source + // in order to delete the configmap and unset oauthMetadataFile + + case configv1.AuthenticationTypeOIDC: + if _, err := listers.ConfigmapLister_.ConfigMaps(operatorclient.TargetNamespace).Get(AuthConfigCMName); errors.IsNotFound(err) { + // auth-config does not exist in target namespace yet; do not remove oauth metadata until it's there + return prevObservedConfig, errs + } else if err != nil { + return prevObservedConfig, append(errs, err) + } - // Spec configMap takes precedence over Status. - switch { - case len(specConfigMap) > 0: - sourceConfigMap = specConfigMap - sourceNamespace = configNamespace - case len(statusConfigMap) > 0: - sourceConfigMap = statusConfigMap - sourceNamespace = managedNamespace - default: - klog.V(5).Infof("no authentication config metadata specified") + // no oauth metadata is served; do not set anything as source + // in order to delete the configmap and unset oauthMetadataFile } // Sync the user or status-specified configMap to the well-known resting place that corresponds to the oauthMetadataFile path. @@ -109,13 +121,3 @@ func ObserveAuthMetadata(genericListers configobserver.Listers, recorder events. return observedConfig, errs } - -func defaultAuthConfig(authConfig *configv1.Authentication) *configv1.Authentication { - out := authConfig.DeepCopy() // do not mutate informer cache - - if len(out.Spec.Type) == 0 { - out.Spec.Type = configv1.AuthenticationTypeIntegratedOAuth - } - - return out -} diff --git a/pkg/operator/configobservation/auth/auth_metadata_test.go b/pkg/operator/configobservation/auth/auth_metadata_test.go index 93a5d164b4..640a1a8bf6 100644 --- a/pkg/operator/configobservation/auth/auth_metadata_test.go +++ b/pkg/operator/configobservation/auth/auth_metadata_test.go @@ -33,7 +33,7 @@ func TestObserveAuthMetadata(t *testing.T) { cmIndexer cache.Indexer existingConfig map[string]interface{} - existingConfigMap *corev1.ConfigMap + authConfigMap *corev1.ConfigMap authSpec *configv1.AuthenticationSpec statusMetadataName string syncerError error @@ -144,13 +144,25 @@ func TestObserveAuthMetadata(t *testing.T) { }, }, statusMetadataName: "metadata-from-status", - expectedConfig: baseAuthMetadataConfig, + expectedConfig: nil, expectedSynced: map[string]string{ - // FIXME should be delete - "configmap/oauth-metadata.openshift-kube-apiserver": "configmap/metadata-from-spec.openshift-config", + "configmap/oauth-metadata.openshift-kube-apiserver": "DELETE", }, expectErrors: false, }, + { + name: "metadata from spec with auth type OIDC but auth-config missing", + authSpec: &configv1.AuthenticationSpec{ + Type: configv1.AuthenticationTypeOIDC, + OAuthMetadata: configv1.ConfigMapNameReference{ + Name: "metadata-from-spec", + }, + }, + statusMetadataName: "metadata-from-status", + expectedConfig: nil, + expectedSynced: map[string]string{}, + expectErrors: false, + }, { name: "metadata from spec with auth type OIDC", authSpec: &configv1.AuthenticationSpec{ @@ -159,11 +171,16 @@ func TestObserveAuthMetadata(t *testing.T) { Name: "metadata-from-spec", }, }, + authConfigMap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auth-config", + Namespace: "openshift-kube-apiserver", + }, + }, statusMetadataName: "metadata-from-status", - expectedConfig: baseAuthMetadataConfig, + expectedConfig: nil, expectedSynced: map[string]string{ - // FIXME should be delete - "configmap/oauth-metadata.openshift-kube-apiserver": "configmap/metadata-from-spec.openshift-config", + "configmap/oauth-metadata.openshift-kube-apiserver": "DELETE", }, expectErrors: false, }, @@ -212,6 +229,19 @@ func TestObserveAuthMetadata(t *testing.T) { }, expectErrors: false, }, + { + name: "metadata from status with auth type OIDC but auth-config missing", + authSpec: &configv1.AuthenticationSpec{ + Type: configv1.AuthenticationTypeOIDC, + OAuthMetadata: configv1.ConfigMapNameReference{ + Name: "", + }, + }, + statusMetadataName: "metadata-from-status", + expectedConfig: nil, + expectedSynced: map[string]string{}, + expectErrors: false, + }, { name: "metadata from status with auth type OIDC", authSpec: &configv1.AuthenticationSpec{ @@ -220,6 +250,12 @@ func TestObserveAuthMetadata(t *testing.T) { Name: "", }, }, + authConfigMap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auth-config", + Namespace: "openshift-kube-apiserver", + }, + }, statusMetadataName: "metadata-from-status", expectedConfig: nil, expectedSynced: map[string]string{ @@ -256,8 +292,8 @@ func TestObserveAuthMetadata(t *testing.T) { tt.cmIndexer = cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) } - if tt.existingConfigMap != nil { - tt.cmIndexer.Add(tt.existingConfigMap) + if tt.authConfigMap != nil { + tt.cmIndexer.Add(tt.authConfigMap) } listers := configobservation.Listers{