Merge pull request #19788 from mfojtik/sa-10-fix-docker-registry-secrets

sa: make docker registry service controller check all secrets

Author: openshift-merge-robot

pkg/serviceaccounts/controllers/docker_registry_service.go

@@ -103,7 +103,7 @@ type DockerRegistryServiceController struct {
 	serviceLister  listers.ServiceLister
 	servicesSynced func() bool
 
-	syncRegistryLocationHandler func(key string) error
+	syncRegistryLocationHandler func() error
 
 	secretCache       cache.Store
 	secretsSynced     func() bool
@@ -115,6 +115,11 @@ type DockerRegistryServiceController struct {
 	secretsToUpdate       workqueue.RateLimitingInterface
 
 	dockerURLsInitialized chan struct{}
+
+	// initialSecretsCheckDone is used to indicate that the controller should perform a full resync of all secrets
+	// regardless of whether the registry location changed or not. This check is usually done on controller start
+	// to verify the content of dockercfg entries in secrets
+	initialSecretsCheckDone bool
 }
 
 // Runs controller loops and returns immediately
@@ -190,7 +195,7 @@ func (e *DockerRegistryServiceController) watchForDockerURLChanges() {
 		}
 		defer e.registryLocationQueue.Done(key)
 
-		if err := e.syncRegistryLocationHandler(key.(string)); err == nil {
+		if err := e.syncRegistryLocationHandler(); err == nil {
 			// this means the request was successfully handled.  We should "forget" the item so that any retry
 			// later on is reset
 			e.registryLocationQueue.Forget(key)
@@ -238,18 +243,19 @@ func getDockerRegistryLocations(lister listers.ServiceLister, location serviceLo
 }
 
 // syncRegistryLocationChange goes through all service account dockercfg secrets and updates them to point at a new docker-registry location
-func (e *DockerRegistryServiceController) syncRegistryLocationChange(key string) error {
+func (e *DockerRegistryServiceController) syncRegistryLocationChange() error {
 	newLocations := e.getDockerRegistryLocations()
 	newDockerRegistryLocations := sets.NewString(newLocations...)
 	existingURLs := e.getRegistryURLs()
-	if existingURLs.Equal(newDockerRegistryLocations) {
+	if existingURLs.Equal(newDockerRegistryLocations) && e.initialSecretsCheckDone {
 		glog.V(4).Infof("No effective update: %v", newDockerRegistryLocations)
 		return nil
 	}
 
 	// make sure that new dockercfg secrets get the correct locations
 	e.dockercfgController.SetDockerURLs(newDockerRegistryLocations.List()...)
 	e.setRegistryURLs(newDockerRegistryLocations.List()...)
+	e.initialSecretsCheckDone = true
 
 	// we've changed the docker registry URL.  Add items to the work queue for all known secrets
 	// new secrets will already get the updated value.

pkg/serviceaccounts/controllers/docker_registry_service_test.go

@@ -59,11 +59,25 @@ func controllerSetup(startingObjects []runtime.Object, t *testing.T, stopCh <-ch
 			DockerURLsInitialized: make(chan struct{}),
 		},
 	)
+	controller.initialSecretsCheckDone = true
 	controller.secretsSynced = func() bool { return true }
 	return kubeclient, fakeWatch, controller, informerFactory
 }
 
-func wrapHandler(indicator chan bool, handler func(string) error, t *testing.T) func(string) error {
+func wrapHandler(indicator chan bool, handler func() error, t *testing.T) func() error {
+	return func() error {
+		defer func() { indicator <- true }()
+
+		err := handler()
+		if err != nil {
+			t.Errorf("unexpected error: %v", err)
+		}
+
+		return err
+	}
+}
+
+func wrapStringHandler(indicator chan bool, handler func(string) error, t *testing.T) func(string) error {
 	return func(key string) error {
 		defer func() { indicator <- true }()
 
@@ -129,7 +143,8 @@ func TestUpdateNewStyleSecret(t *testing.T) {
 
 	kubeclient, fakeWatch, controller, informerFactory := controllerSetup([]runtime.Object{newStyleDockercfgSecret}, t, stopChannel)
 	controller.syncRegistryLocationHandler = wrapHandler(received, controller.syncRegistryLocationChange, t)
-	controller.syncSecretHandler = wrapHandler(updatedSecret, controller.syncSecretUpdate, t)
+	controller.syncSecretHandler = wrapStringHandler(updatedSecret, controller.syncSecretUpdate, t)
+	controller.initialSecretsCheckDone = false
 	informerFactory.Start(stopChannel)
 	go controller.Run(5, stopChannel)
 
@@ -139,6 +154,9 @@ func TestUpdateNewStyleSecret(t *testing.T) {
 	case <-time.After(time.Duration(45 * time.Second)):
 		t.Fatalf("failed to become ready")
 	}
+	if controller.initialSecretsCheckDone != false {
+		t.Fatalf("initialSecretsCheckDone should be false")
+	}
 
 	fakeWatch.Modify(registryService)
 	t.Log("Waiting to reach syncRegistryLocationHandler")
@@ -147,6 +165,12 @@ func TestUpdateNewStyleSecret(t *testing.T) {
 	case <-time.After(time.Duration(45 * time.Second)):
 		t.Fatalf("failed to call into syncRegistryLocationHandler")
 	}
+
+	// after this point the secrets should be added to the queue and initial check should be done.
+	if controller.initialSecretsCheckDone != true {
+		t.Fatalf("initialSecretsCheckDone should be true")
+	}
+
 	t.Log("Waiting to update secret")
 	select {
 	case <-updatedSecret:
@@ -216,9 +240,10 @@ func TestUpdateOldStyleSecretWithKey(t *testing.T) {
 		Data: map[string][]byte{v1.DockerConfigKey: dockercfgContent},
 	}
 
-	kubeclient, fakeWatch, controller, informerFactory := controllerSetup([]runtime.Object{oldStyleDockercfgSecret}, t, stopChannel)
+	kubeclient, _, controller, informerFactory := controllerSetup([]runtime.Object{registryService, oldStyleDockercfgSecret}, t, stopChannel)
 	controller.syncRegistryLocationHandler = wrapHandler(received, controller.syncRegistryLocationChange, t)
-	controller.syncSecretHandler = wrapHandler(updatedSecret, controller.syncSecretUpdate, t)
+	controller.syncSecretHandler = wrapStringHandler(updatedSecret, controller.syncSecretUpdate, t)
+	controller.initialSecretsCheckDone = false
 	informerFactory.Start(stopChannel)
 	go controller.Run(5, stopChannel)
 
@@ -229,8 +254,6 @@ func TestUpdateOldStyleSecretWithKey(t *testing.T) {
 		t.Fatalf("failed to become ready")
 	}
 
-	fakeWatch.Modify(registryService)
-
 	t.Log("Waiting to reach syncRegistryLocationHandler")
 	select {
 	case <-received:
@@ -309,7 +332,7 @@ func TestUpdateOldStyleSecretWithoutKey(t *testing.T) {
 		return true, tokenSecret, nil
 	})
 	controller.syncRegistryLocationHandler = wrapHandler(received, controller.syncRegistryLocationChange, t)
-	controller.syncSecretHandler = wrapHandler(updatedSecret, controller.syncSecretUpdate, t)
+	controller.syncSecretHandler = wrapStringHandler(updatedSecret, controller.syncSecretUpdate, t)
 	informerFactory.Start(stopChannel)
 	go controller.Run(5, stopChannel)
 
@@ -400,17 +423,18 @@ func TestClearSecretAndRecreate(t *testing.T) {
 
 	kubeclient, fakeWatch, controller, informerFactory := controllerSetup([]runtime.Object{registryService, oldStyleDockercfgSecret}, t, stopChannel)
 	controller.syncRegistryLocationHandler = wrapHandler(received, controller.syncRegistryLocationChange, t)
-	controller.syncSecretHandler = wrapHandler(updatedSecret, controller.syncSecretUpdate, t)
+	controller.syncSecretHandler = wrapStringHandler(updatedSecret, controller.syncSecretUpdate, t)
 	informerFactory.Start(stopChannel)
 	go controller.Run(5, stopChannel)
 
 	t.Log("Waiting for ready")
 	select {
 	case <-controller.dockerURLsInitialized:
 	case <-time.After(time.Duration(45 * time.Second)):
-		t.Fatalf("failed to become ready")
+		t.Fatalf("failed waiting for dockerURLsInitialized")
 	}
 
+	t.Logf("deleting %s service", registryService.Name)
 	fakeWatch.Delete(registryService)
 
 	t.Log("Waiting for first update")
@@ -419,6 +443,7 @@ func TestClearSecretAndRecreate(t *testing.T) {
 	case <-time.After(time.Duration(45 * time.Second)):
 		t.Fatalf("failed to call into syncRegistryLocationHandler")
 	}
+
 	t.Log("Waiting to update secret")
 	select {
 	case <-updatedSecret:
@@ -449,6 +474,8 @@ func TestClearSecretAndRecreate(t *testing.T) {
 	}
 
 	kubeclient.ClearActions()
+
+	t.Logf("adding %s service", registryService.Name)
 	fakeWatch.Add(registryService)
 
 	t.Log("Waiting for second update")
@@ -457,6 +484,7 @@ func TestClearSecretAndRecreate(t *testing.T) {
 	case <-time.After(time.Duration(45 * time.Second)):
 		t.Fatalf("failed to call into syncRegistryLocationHandler")
 	}
+
 	t.Log("Waiting to update secret")
 	select {
 	case <-updatedSecret: