OCPBUGS-54780: Fix issue where console operator orphans custom logo configmaps in openshift-console namespace (#978)

* OCPBUGS-54780: Fix issue where console operator orphans custom logo configmaps in openshift-console namespace

* improve debug logs for custom logo sync

Author: TheRealJon

pkg/console/operator/operator.go

@@ -102,8 +102,9 @@ type trackables struct {
 	// used to keep track of OLM capability
 	isOLMDisabled bool
 	// track organization ID and mail
-	organizationID string
-	accountMail    string
+	organizationID       string
+	accountMail          string
+	customLogoConfigMaps []string
 }
 
 func NewConsoleOperator(

pkg/console/operator/sync_v400.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"net/url"
 	"os"
+	"slices"
 	"strings"
 
 	// kube
@@ -559,30 +560,39 @@ func (co *consoleOperator) SyncCustomLogos(operatorConfig *operatorv1.Console) (
 		aggregatedError error
 		err             error
 		reason          string
+		newSyncedLogos  []string
 	)
 	for _, logo := range operatorConfig.Spec.Customization.Logos {
 		for _, theme := range logo.Themes {
 			logoToSync := theme.Source.ConfigMap
-			err, reason = co.UpdateCustomLogoSyncSource(logoToSync)
-			if err != nil {
+			if err, reason = co.ValidateCustomLogo(logoToSync); err != nil {
 				if aggregatedError == nil {
-					aggregatedError = fmt.Errorf("One or more errors were encountered while syncing custom logos:\n  - %v, %s", logoToSync, err.Error())
+					aggregatedError = fmt.Errorf("error syncing custom logos:  - Invalid config: %v, %s", logoToSync, err.Error())
 				} else {
-					aggregatedError = fmt.Errorf("%s\n  - %v, %s", aggregatedError.Error(), logoToSync, err.Error())
+					aggregatedError = fmt.Errorf("%s  - %v, %s", aggregatedError.Error(), logoToSync, err.Error())
 				}
+			} else {
+				newSyncedLogos = append(newSyncedLogos, logoToSync.Name)
 			}
 		}
 	}
 	if aggregatedError != nil {
 		return aggregatedError, reason
 	}
-	return nil, ""
+	slices.Sort(newSyncedLogos)
+	return co.UpdateCustomLogoSyncSources(newSyncedLogos)
 }
 
 // TODO remove deprecated CustomLogoFile API
 func (co *consoleOperator) SyncCustomLogoConfigMap(operatorConfig *operatorv1.Console) (error, string) {
 	var customLogoRef = operatorv1.ConfigMapFileReference(operatorConfig.Spec.Customization.CustomLogoFile)
-	return co.UpdateCustomLogoSyncSource(&customLogoRef)
+	klog.V(4).Infof("[SyncCustomLogoConfigMap] syncing customLogoFile, Name: %s, Key: %s", customLogoRef.Name, customLogoRef.Key)
+	err, reason := co.ValidateCustomLogo(&customLogoRef)
+	if err != nil {
+		klog.V(4).Infof("[SyncCustomLogoConfigMap] failed to sync customLogoFile, %v", err)
+		return err, reason
+	}
+	return co.UpdateCustomLogoSyncSources([]string{customLogoRef.Name})
 }
 
 func (co *consoleOperator) ValidateOAuthServingCertConfigMap(ctx context.Context) (oauthServingCert *corev1.ConfigMap, reason string, err error) {
@@ -600,35 +610,54 @@ func (co *consoleOperator) ValidateOAuthServingCertConfigMap(ctx context.Context
 }
 
 // on each pass of the operator sync loop, we need to check the
-// operator config for a custom logo.  If this has been set, then
-// we notify the resourceSyncer that it needs to start watching this
-// configmap in its own sync loop.  Note that the resourceSyncer's actual
+// operator config for custom logos.  If this has been set, then
+// we notify the resourceSyncer that it needs to start watching the associated
+// configmaps in its own sync loop.  Note that the resourceSyncer's actual
 // sync loop will run later.  Our operator is waiting to receive
-// the copied configmap into the console namespace for a future
+// the copied configmaps into the console namespace for a future
 // sync loop to mount into the console deployment.
-func (c *consoleOperator) UpdateCustomLogoSyncSource(cmRef *operatorv1.ConfigMapFileReference) (error, string) {
-	// validate first, to avoid a broken volume mount & a crashlooping console
-	err, reason := c.ValidateCustomLogo(cmRef)
-	if err != nil {
-		return err, reason
+func (co *consoleOperator) UpdateCustomLogoSyncSources(configMapNames []string) (error, string) {
+	klog.V(4).Info("[UpdateCustomLogoSyncSources] syncing custom logo configmap resources")
+	klog.V(4).Infof("%#v", configMapNames)
+
+	errors := []string{}
+	if len(co.trackables.customLogoConfigMaps) > 0 {
+		klog.V(4).Info("[UpdateCustomLogoSyncSources] unsyncing custom logo configmap resources from previous sync loop...")
+		for _, configMapName := range co.trackables.customLogoConfigMaps {
+			err := co.UpdateCustomLogoSyncSource(configMapName, true)
+			if err != nil {
+				errors = append(errors, err.Error())
+			}
+		}
+
+		if len(errors) > 0 {
+			msg := fmt.Sprintf("error syncing custom logo configmap resources:\n%v", errors)
+			klog.V(4).Infof("[UpdateCustomLogoSyncSources] %s", msg)
+			return fmt.Errorf(msg), "FailedResourceSync"
+		}
 	}
 
-	source := resourcesynccontroller.ResourceLocation{}
-	logoConfigMapName := cmRef.Name
+	if len(configMapNames) > 0 {
+		// If the new list of synced configmaps is different than the last sync, we need to update the
+		// resource syncer with the new list, and re
+		klog.V(4).Infof("[UpdateCustomLogoSyncSources] syncing new custom logo configmap resources...")
+		for _, configMapName := range configMapNames {
+			err := co.UpdateCustomLogoSyncSource(configMapName, false)
+			if err != nil {
+				errors = append(errors, err.Error())
+			}
+		}
 
-	if logoConfigMapName != "" {
-		source.Name = logoConfigMapName
-		source.Namespace = api.OpenShiftConfigNamespace
-	}
-	// if no custom logo provided, sync an empty source to delete
-	err = c.resourceSyncer.SyncConfigMap(
-		resourcesynccontroller.ResourceLocation{Namespace: api.OpenShiftConsoleNamespace, Name: cmRef.Name},
-		source,
-	)
-	if err != nil {
-		return err, "FailedResourceSync"
+		if len(errors) > 0 {
+			msg := fmt.Sprintf("error syncing custom logo configmap resources:\n%v", errors)
+			klog.V(4).Infof("[UpdateCustomLogoSyncSources] %s", msg)
+			return fmt.Errorf(msg), "FailedResourceSync"
+		}
 	}
 
+	co.trackables.customLogoConfigMaps = configMapNames
+
+	klog.V(4).Info("[UpdateCustomLogoSyncSources] done")
 	return nil, ""
 }
 
@@ -637,34 +666,57 @@ func (co *consoleOperator) ValidateCustomLogo(logoFileRef *operatorv1.ConfigMapF
 	logoImageKey := logoFileRef.Key
 
 	if (len(logoConfigMapName) == 0) != (len(logoImageKey) == 0) {
-		klog.V(4).Infoln("custom logo filename or key have not been set")
-		return customerrors.NewCustomLogoError("either custom logo filename or key have not been set"), "KeyOrFilenameInvalid"
+		msg := "custom logo filename or key have not been set"
+		klog.V(4).Infof("[ValidateCustomLogo] %s", msg)
+		return customerrors.NewCustomLogoError(msg), "KeyOrFilenameInvalid"
 	}
 	// fine if nothing set, but don't mount it
 	if len(logoConfigMapName) == 0 {
-		klog.V(4).Infoln("no custom logo configured")
+		klog.V(4).Infoln("[ValidateCustomLogo] no custom logo configured")
 		return nil, ""
 	}
 	logoConfigMap, err := co.configNSConfigMapLister.ConfigMaps(api.OpenShiftConfigNamespace).Get(logoConfigMapName)
 	// If we 404, the logo file may not have been created yet.
 	if err != nil {
-		klog.V(4).Infof("failed to get ConfigMap %v, %v", logoConfigMapName, err)
-		return customerrors.NewCustomLogoError(fmt.Sprintf("failed to get ConfigMap %v, %v", logoConfigMapName, err)), "FailedGet"
+		msg := fmt.Sprintf("failed to get ConfigMap %v, %v", logoConfigMapName, err)
+		klog.V(4).Infof("[ValidateCustomLogo] %s", msg)
+		return customerrors.NewCustomLogoError(msg), "FailedGet"
 	}
 
 	_, imageDataFound := logoConfigMap.BinaryData[logoImageKey]
 	if !imageDataFound {
 		_, imageDataFound = logoConfigMap.Data[logoImageKey]
 	}
 	if !imageDataFound {
-		klog.V(4).Infoln("custom logo file exists but no image provided")
-		return customerrors.NewCustomLogoError("custom logo file exists but no image provided"), "NoImageProvided"
+		msg := "custom logo file exists but no image provided"
+		klog.V(4).Infof("[ValidateCustomLogo] %s", msg)
+		return customerrors.NewCustomLogoError(msg), "NoImageProvided"
 	}
 
-	klog.V(4).Infof("custom logo %s ok to mount", logoConfigMapName)
+	klog.V(4).Infof("[ValidateCustomLogo] custom logo %s ok to mount", logoConfigMapName)
 	return nil, ""
 }
 
+func (co *consoleOperator) UpdateCustomLogoSyncSource(targetName string, unsync bool) error {
+	source := resourcesynccontroller.ResourceLocation{}
+	if !unsync {
+		source.Name = targetName
+		source.Namespace = api.OpenShiftConfigNamespace
+	}
+
+	target := resourcesynccontroller.ResourceLocation{
+		Namespace: api.OpenShiftConsoleNamespace,
+		Name:      targetName,
+	}
+
+	if unsync {
+		klog.V(4).Infof("[UpdateCustomLogoSyncSource] unsyncing %s", targetName)
+	} else {
+		klog.V(4).Infof("[UpdateCustomLogoSyncSource] syncing %s", targetName)
+	}
+	return co.resourceSyncer.SyncConfigMap(target, source)
+}
+
 func (co *consoleOperator) GetAvailablePlugins(enabledPluginsNames []string) []*v1.ConsolePlugin {
 	var availablePlugins []*v1.ConsolePlugin
 	for _, pluginName := range utilsub.RemoveDuplicateStr(enabledPluginsNames) {

test/e2e/brand_customization_test.go

@@ -115,22 +115,21 @@ func TestCustomBrand(t *testing.T) {
 			t.Fatalf("could not clear customizations from operator config (%s)", err)
 		}
 
-		// TODO re-enable this check once https://issues.redhat.com/browse/OCPBUGS-54780 is fixed
-		// 	t.Logf("ensure that the %s configmap is no longer synced to 'openshift-console' namespace", suite.customLogoConfigMapName)
-		// 	err = wait.Poll(pollFrequency, pollStandardMax, func() (stop bool, err error) {
-		// 		_, err = framework.GetCustomLogoConfigMap(client, suite.customLogoConfigMapName)
-		// 		if apiErrors.IsNotFound(err) {
-		// 			return true, nil
-		// 		}
-		// 		if err != nil {
-		// 			return false, err
-		// 		}
-		// 		// Try until timeout
-		// 		return false, nil
-		// 	})
-		// 	if err != nil {
-		// 		t.Fatalf("timed out checking for configmap '%s' in 'openshift-console'", suite.customLogoConfigMapName)
-		// 	}
+		t.Logf("ensure that the %s configmap is no longer synced to 'openshift-console' namespace", suite.customLogoConfigMapName)
+		err = wait.Poll(pollFrequency, pollStandardMax, func() (stop bool, err error) {
+			_, err = framework.GetCustomLogoConfigMap(client, suite.customLogoConfigMapName)
+			if apiErrors.IsNotFound(err) {
+				return true, nil
+			}
+			if err != nil {
+				return false, err
+			}
+			// Try until timeout
+			return false, nil
+		})
+		if err != nil {
+			t.Fatalf("configmap '%s' was orphaned in 'openshift-console' namespace", suite.customLogoConfigMapName)
+		}
 	}
 }