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

… None or OIDC
openshift · Nov 15, 2024 · 41278f4 · 41278f4
1 parent 099a2bd
commit 41278f4
Show file tree

Hide file tree

Showing 2 changed files with 79 additions and 41 deletions.
diff --git 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
@@ -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{