diff --git a/pkg/operator/internalimageregistry/cleanup_controller.go b/pkg/operator/internalimageregistry/cleanup_controller.go index 355e4c581..db9c78e08 100644 --- a/pkg/operator/internalimageregistry/cleanup_controller.go +++ b/pkg/operator/internalimageregistry/cleanup_controller.go @@ -121,6 +121,8 @@ func (c *imagePullSecretCleanupController) cleanup(ctx context.Context) error { // cleanup the refs in the service account if len(imagePullSecretName) != 0 { + serviceAccount = serviceAccount.DeepCopy() + var secretRefs []corev1.ObjectReference for _, secretRef := range serviceAccount.Secrets { if secretRef.Name != imagePullSecretName { @@ -137,6 +139,11 @@ func (c *imagePullSecretCleanupController) cleanup(ctx context.Context) error { } serviceAccount.ImagePullSecrets = imagePullSecretRefs + // Clean up the annotation reference to the image pull secret from service account + if annotationValue, ok := serviceAccount.Annotations["openshift.io/internal-registry-pull-secret-ref"]; ok && annotationValue == imagePullSecretName { + delete(serviceAccount.Annotations, "openshift.io/internal-registry-pull-secret-ref") + } + _, err = c.kubeClient.CoreV1().ServiceAccounts(serviceAccount.Namespace).Update(ctx, serviceAccount, metav1.UpdateOptions{}) if err != nil { var statusErr *errors.StatusError diff --git a/pkg/operator/internalimageregistry/cleanup_controller_test.go b/pkg/operator/internalimageregistry/cleanup_controller_test.go new file mode 100644 index 000000000..0b2db2417 --- /dev/null +++ b/pkg/operator/internalimageregistry/cleanup_controller_test.go @@ -0,0 +1,180 @@ +package internalimageregistry + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/openshift/library-go/pkg/controller/factory" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" + corelistersv1 "k8s.io/client-go/listers/core/v1" + "k8s.io/client-go/rest" + "k8s.io/client-go/tools/cache" +) + +const ( + testNamespace = "test" + testServiceAccountName = "builder" + testImagePullSecretName = "builder-dockercfg-abc12" + internalRegistryPullRef = "openshift.io/internal-registry-pull-secret-ref" +) + +func TestCleanupServiceAccountUpdateConflictDoesNotMutateListerCache(t *testing.T) { + cachedServiceAccount := newServiceAccountWithInternalRegistryPullSecret( + testNamespace, + testServiceAccountName, + testImagePullSecretName, + ) + + expectedServiceAccount := cachedServiceAccount.DeepCopy() + + indexer := newServiceAccountIndexer(t, cachedServiceAccount) + + controller := newCleanupControllerForTest( + newKubeClientWithServiceAccountUpdateConflict(t), + corelistersv1.NewServiceAccountLister(indexer), + corelistersv1.NewSecretLister(newEmptyIndexer()), + corelistersv1.NewPodLister(newEmptyIndexer()), + ) + + err := controller.cleanup(context.Background()) + if !errors.Is(err, factory.SyntheticRequeueError) { + t.Fatalf("expected SyntheticRequeueError, got %v", err) + } + + assertServiceAccountHasInternalRegistryPullSecret(t, indexer, expectedServiceAccount) +} + +func newKubeClientWithServiceAccountUpdateConflict(t *testing.T) *kubernetes.Clientset { + t.Helper() + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPut && strings.Contains(r.URL.Path, "/serviceaccounts/") { + status := metav1.Status{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "Status", + }, + Status: metav1.StatusFailure, + Message: "conflict", + Reason: metav1.StatusReasonConflict, + Code: http.StatusConflict, + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusConflict) + if err := json.NewEncoder(w).Encode(status); err != nil { + t.Errorf("failed to encode conflict status: %v", err) + } + return + } + + w.WriteHeader(http.StatusNotFound) + })) + + t.Cleanup(server.Close) + + client, err := kubernetes.NewForConfig(&rest.Config{Host: server.URL}) + if err != nil { + t.Fatalf("failed to create kubernetes client: %v", err) + } + + return client +} + +func newServiceAccountWithInternalRegistryPullSecret(namespace, name, secretName string) *corev1.ServiceAccount { + return &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Annotations: map[string]string{ + internalRegistryPullRef: secretName, + }, + }, + ImagePullSecrets: []corev1.LocalObjectReference{ + {Name: secretName}, + }, + Secrets: []corev1.ObjectReference{ + {Name: secretName}, + }, + } +} + +func newServiceAccountIndexer(t *testing.T, serviceAccount *corev1.ServiceAccount) cache.Indexer { + t.Helper() + + indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{ + cache.NamespaceIndex: cache.MetaNamespaceIndexFunc, + }) + if err := indexer.Add(serviceAccount); err != nil { + t.Fatalf("failed to add service account to indexer: %v", err) + } + + return indexer +} + +func newEmptyIndexer() cache.Indexer { + return cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{ + cache.NamespaceIndex: cache.MetaNamespaceIndexFunc, + }) +} + +func newCleanupControllerForTest( + kubeClient *kubernetes.Clientset, + serviceAccountLister corelistersv1.ServiceAccountLister, + secretLister corelistersv1.SecretLister, + podLister corelistersv1.PodLister, +) *imagePullSecretCleanupController { + return &imagePullSecretCleanupController{ + kubeClient: kubeClient, + serviceAccountLister: serviceAccountLister, + secretLister: secretLister, + podLister: podLister, + } +} + +func assertServiceAccountHasInternalRegistryPullSecret(t *testing.T, indexer cache.Indexer, expected *corev1.ServiceAccount) { + t.Helper() + + cached, exists, err := indexer.GetByKey(expected.Namespace + "/" + expected.Name) + if err != nil { + t.Fatalf("failed to get cached service account: %v", err) + } + if !exists { + t.Fatal("expected cached service account to exist") + } + + cachedServiceAccount := cached.(*corev1.ServiceAccount) + + if got := cachedServiceAccount.Annotations[internalRegistryPullRef]; got != expected.Annotations[internalRegistryPullRef] { + t.Fatalf( + "expected cached annotation %q to remain %q, got %q", + internalRegistryPullRef, + expected.Annotations[internalRegistryPullRef], + got, + ) + } + + if len(cachedServiceAccount.ImagePullSecrets) != len(expected.ImagePullSecrets) { + t.Fatalf("expected %d image pull secret references, got %d", len(expected.ImagePullSecrets), len(cachedServiceAccount.ImagePullSecrets)) + } + for i, expectedRef := range expected.ImagePullSecrets { + if cachedServiceAccount.ImagePullSecrets[i].Name != expectedRef.Name { + t.Fatalf("expected image pull secret reference %q, got %q", expectedRef.Name, cachedServiceAccount.ImagePullSecrets[i].Name) + } + } + + if len(cachedServiceAccount.Secrets) != len(expected.Secrets) { + t.Fatalf("expected %d secret references, got %d", len(expected.Secrets), len(cachedServiceAccount.Secrets)) + } + for i, expectedRef := range expected.Secrets { + if cachedServiceAccount.Secrets[i].Name != expectedRef.Name { + t.Fatalf("expected secret reference %q, got %q", expectedRef.Name, cachedServiceAccount.Secrets[i].Name) + } + } +}