Merge pull request #160 from enj/enj/i/rv_track

Bug 1733336: Correct race between config sync loop and resource syncer

Author: openshift-merge-robot

Makefile

@@ -84,7 +84,7 @@ verify-commits:
 #   make test-unit
 #   make test-unit WHAT=pkg/build TESTFLAGS=-v
 test-unit:
-	GOTEST_FLAGS="$(TESTFLAGS)" hack/test-go.sh $(WHAT) $(TESTS)
+	GOTEST_FLAGS="$(TESTFLAGS)" GODEBUG=tls13 KUBERNETES_SERVICE_PORT_HTTPS=443 hack/test-go.sh $(WHAT) $(TESTS)
 .PHONY: test-unit
 
 test-sec:

pkg/operator2/branding.go

@@ -5,7 +5,6 @@ import (
 
 	"gopkg.in/yaml.v2"
 
-	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
@@ -68,10 +67,6 @@ func (c *authOperator) handleBrandingTemplates(configTemplates configv1.OAuthTem
 	return &templates, nil
 }
 
-func (c *authOperator) handleOCPBrandingSecret() (*corev1.Secret, error) {
-	return c.secrets.Secrets(targetNamespace).Get(ocpBrandingSecretName, metav1.GetOptions{})
-}
-
 func (c *authOperator) getConsoleBranding() (string, error) {
 	cm, err := c.configMaps.ConfigMaps(targetNamespace).Get(consoleConfigMapLocalName, metav1.GetOptions{})
 	if errors.IsNotFound(err) {

pkg/operator2/configsync.go

@@ -12,55 +12,57 @@ import (
 	"github.com/openshift/library-go/pkg/operator/resourcesynccontroller"
 )
 
-func (c *authOperator) handleConfigSync(data *configSyncData) ([]string, error) {
-	// TODO we probably need listers
-	configMapClient := c.configMaps.ConfigMaps(targetNamespace)
-	secretClient := c.secrets.Secrets(targetNamespace)
+func (c *authOperator) handleConfigSync(data *configSyncData) error {
+	// TODO this has too much boilerplate
 
-	configMaps, err := configMapClient.List(metav1.ListOptions{})
-	if err != nil {
-		return nil, fmt.Errorf("error listing configMaps: %v", err)
+	inUseConfigMapNames := sets.NewString()
+	inUseSecretNames := sets.NewString()
+
+	for _, dest := range sets.StringKeySet(data.idpConfigMaps).List() {
+		syncOrDie(c.resourceSyncer.SyncConfigMap, dest, data.idpConfigMaps[dest].src)
+		inUseConfigMapNames.Insert(dest)
+	}
+	for _, dest := range sets.StringKeySet(data.idpSecrets).List() {
+		syncOrDie(c.resourceSyncer.SyncSecret, dest, data.idpSecrets[dest].src)
+		inUseSecretNames.Insert(dest)
+	}
+	for _, dest := range sets.StringKeySet(data.tplSecrets).List() {
+		syncOrDie(c.resourceSyncer.SyncSecret, dest, data.tplSecrets[dest].src)
+		inUseSecretNames.Insert(dest)
 	}
 
-	secrets, err := secretClient.List(metav1.ListOptions{})
+	// TODO we probably need listers
+
+	configMaps, err := c.configMaps.ConfigMaps(targetNamespace).List(metav1.ListOptions{})
 	if err != nil {
-		return nil, fmt.Errorf("error listing secrets: %v", err)
+		return err
 	}
-
+	configMapNames := sets.NewString()
 	prefixConfigMapNames := sets.NewString()
-	prefixSecretNames := sets.NewString()
-	resourceVersionsAll := map[string]string{}
-
-	// TODO this has too much boilerplate
-
 	for _, cm := range configMaps.Items {
+		configMapNames.Insert(cm.Name)
 		if strings.HasPrefix(cm.Name, userConfigPrefix) {
 			prefixConfigMapNames.Insert(cm.Name)
-			resourceVersionsAll[cm.Name] = cm.GetResourceVersion()
 		}
 	}
+	if !configMapNames.IsSuperset(inUseConfigMapNames) {
+		return fmt.Errorf("config maps %s in %s not synced", inUseConfigMapNames.Difference(configMapNames).List(), targetNamespace)
+	}
 
+	secrets, err := c.secrets.Secrets(targetNamespace).List(metav1.ListOptions{})
+	if err != nil {
+		return err
+	}
+	secretNames := sets.NewString()
+	prefixSecretNames := sets.NewString()
 	for _, secret := range secrets.Items {
+		secretNames.Insert(secret.Name)
 		if strings.HasPrefix(secret.Name, userConfigPrefix) {
 			prefixSecretNames.Insert(secret.Name)
-			resourceVersionsAll[secret.Name] = secret.GetResourceVersion()
 		}
 	}
-
-	inUseConfigMapNames := sets.NewString()
-	inUseSecretNames := sets.NewString()
-
-	for dest, src := range data.idpConfigMaps {
-		syncOrDie(c.resourceSyncer.SyncConfigMap, dest, src.src)
-		inUseConfigMapNames.Insert(dest)
-	}
-	for dest, src := range data.idpSecrets {
-		syncOrDie(c.resourceSyncer.SyncSecret, dest, src.src)
-		inUseSecretNames.Insert(dest)
-	}
-	for dest, src := range data.tplSecrets {
-		syncOrDie(c.resourceSyncer.SyncSecret, dest, src.src)
-		inUseSecretNames.Insert(dest)
+	if !secretNames.IsSuperset(inUseSecretNames) {
+		return fmt.Errorf("secrets %s in %s not synced", inUseSecretNames.Difference(secretNames).List(), targetNamespace)
 	}
 
 	notInUseConfigMapNames := prefixConfigMapNames.Difference(inUseConfigMapNames)
@@ -76,18 +78,7 @@ func (c *authOperator) handleConfigSync(data *configSyncData) ([]string, error)
 		syncOrDie(c.resourceSyncer.SyncSecret, dest, "")
 	}
 
-	// only get the resource versions of the elements in use
-	var resourceVersionsInUse []string
-
-	for name := range inUseConfigMapNames {
-		resourceVersionsInUse = append(resourceVersionsInUse, resourceVersionsAll[name])
-	}
-
-	for name := range inUseSecretNames {
-		resourceVersionsInUse = append(resourceVersionsInUse, resourceVersionsAll[name])
-	}
-
-	return resourceVersionsInUse, nil
+	return nil
 }
 
 type configSyncData struct {