[release 4.3] Bug 1779282: Use OwnerRefs to clean up SA pull secrets

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

@@ -324,7 +324,7 @@ func (e *DockercfgController) syncServiceAccount(key string) error {
 		return nil
 	}
 	if !needsDockercfgSecret(obj.(*v1.ServiceAccount)) {
-		return nil
+		return e.syncDockercfgOwnerRefs(obj.(*v1.ServiceAccount))
 	}
 
 	serviceAccount := obj.(*v1.ServiceAccount).DeepCopyObject().(*v1.ServiceAccount)
@@ -503,12 +503,81 @@ func (e *DockercfgController) createDockerPullSecret(serviceAccount *v1.ServiceA
 		return nil, false, err
 	}
 	dockercfgSecret.Data[v1.DockerConfigKey] = dockercfgContent
+	blockDeletion := false
+	ownerRef := metav1.NewControllerRef(tokenSecret, v1.SchemeGroupVersion.WithKind("Secret"))
+	ownerRef.BlockOwnerDeletion = &blockDeletion
+	dockercfgSecret.SetOwnerReferences([]metav1.OwnerReference{*ownerRef})
 
 	// Save the secret
 	createdSecret, err := e.client.CoreV1().Secrets(tokenSecret.Namespace).Create(dockercfgSecret)
 	return createdSecret, err == nil, err
 }
 
+func (e *DockercfgController) syncDockercfgOwnerRefs(serviceAccount *v1.ServiceAccount) error {
+	for _, secretRef := range serviceAccount.Secrets {
+		secret, exists, err := e.secretCache.GetByKey(fmt.Sprintf("%s/%s", secretRef.Namespace, secretRef.Name))
+		if err != nil {
+			return err
+		}
+		if !exists {
+			continue
+		}
+		err = e.syncDockercfgOwner(secret.(*v1.Secret))
+		if err != nil {
+			return err
+		}
+	}
+	for _, secretRef := range serviceAccount.ImagePullSecrets {
+		secret, exists, err := e.secretCache.GetByKey(fmt.Sprintf("%s/%s", serviceAccount.Namespace, secretRef.Name))
+		if err != nil {
+			return err
+		}
+		if !exists {
+			continue
+		}
+		err = e.syncDockercfgOwner(secret.(*v1.Secret))
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func (e *DockercfgController) syncDockercfgOwner(pullSecret *v1.Secret) error {
+	if pullSecret.Type != v1.SecretTypeDockercfg {
+		return nil
+	}
+	tokenName := pullSecret.Annotations[ServiceAccountTokenSecretNameKey]
+	// If there is no token name, this pull secret was likely linked by a user.
+	// No further work is needed.
+	if len(tokenName) == 0 {
+		return nil
+	}
+	// Make sure the token exists
+	tokenSecret, exists, err := e.secretCache.GetByKey(fmt.Sprintf("%s/%s", pullSecret.Namespace, tokenName))
+	if err != nil {
+		return err
+	}
+	if !exists {
+		// If the pull secret exists, and the token does not, delete the pull secret.
+		klog.V(4).Infof("Deleting pull secret %s/%s because its associated token %s/%s is missing", pullSecret.Namespace, pullSecret.Name, pullSecret.Namespace, tokenName)
+		return e.client.CoreV1().Secrets(pullSecret.Namespace).Delete(pullSecret.Name, &metav1.DeleteOptions{})
+	}
+	tokenSecretObj := tokenSecret.(*v1.Secret)
+	// If the pull secret has an owner reference to its associated token, no further work is needed.
+	if metav1.IsControlledBy(pullSecret, tokenSecretObj) {
+		return nil
+	}
+	pullSecret = pullSecret.DeepCopy()
+	blockDeletion := false
+	ownerRef := metav1.NewControllerRef(tokenSecretObj, v1.SchemeGroupVersion.WithKind("Secret"))
+	ownerRef.BlockOwnerDeletion = &blockDeletion
+	pullSecret.SetOwnerReferences([]metav1.OwnerReference{*ownerRef})
+	klog.V(4).Infof("Adding token %s/%s as the owner of pull secret %s/%s", pullSecret.Namespace, tokenName, pullSecret.Namespace, pullSecret.Name)
+	_, err = e.client.CoreV1().Secrets(pullSecret.Namespace).Update(pullSecret)
+	return err
+}
+
 func getGeneratedDockercfgSecretNames(serviceAccount *v1.ServiceAccount) (sets.String, sets.String) {
 	mountableDockercfgSecrets := sets.String{}
 	imageDockercfgPullSecrets := sets.String{}

pkg/serviceaccounts/controllers/deleted_token_secrets.go

@@ -4,17 +4,15 @@ import (
 	"fmt"
 	"time"
 
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/klog"
 
-	"k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/fields"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	informers "k8s.io/client-go/informers/core/v1"
 	kclientset "k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/tools/cache"
-	api "k8s.io/kubernetes/pkg/apis/core"
 )
 
 // DockercfgTokenDeletedControllerOptions contains options for the DockercfgTokenDeletedController
@@ -80,39 +78,24 @@ func (e *DockercfgTokenDeletedController) secretDeleted(obj interface{}) {
 	if !ok {
 		return
 	}
-
-	dockercfgSecrets, err := e.findDockercfgSecrets(tokenSecret)
+	dockercfgSecrets, err := findDockercfgSecrets(e.client, tokenSecret)
 	if err != nil {
 		klog.Error(err)
 		return
 	}
 	if len(dockercfgSecrets) == 0 {
 		return
 	}
-
 	// remove the reference token secrets
 	for _, dockercfgSecret := range dockercfgSecrets {
+		if metav1.IsControlledBy(dockercfgSecret, tokenSecret) {
+			// If the docker pull secret is owned by its associated token, let garbage collection take care of it.
+			klog.V(5).Infof("Ignoring deletion of pull secret %s/%s because it should be removed via garbage collection", dockercfgSecret.Namespace, dockercfgSecret.Name)
+			continue
+		}
+		klog.V(4).Infof("Deleting pull secret %s/%s because its associated token %s/%s has been deleted", dockercfgSecret.Namespace, dockercfgSecret.Name, tokenSecret.Namespace, tokenSecret.Name)
 		if err := e.client.CoreV1().Secrets(dockercfgSecret.Namespace).Delete(dockercfgSecret.Name, nil); (err != nil) && !apierrors.IsNotFound(err) {
 			utilruntime.HandleError(err)
 		}
 	}
 }
-
-// findDockercfgSecret checks all the secrets in the namespace to see if the token secret has any existing dockercfg secrets that reference it
-func (e *DockercfgTokenDeletedController) findDockercfgSecrets(tokenSecret *v1.Secret) ([]*v1.Secret, error) {
-	dockercfgSecrets := []*v1.Secret{}
-
-	options := metav1.ListOptions{FieldSelector: fields.OneTermEqualSelector(api.SecretTypeField, string(v1.SecretTypeDockercfg)).String()}
-	potentialSecrets, err := e.client.CoreV1().Secrets(tokenSecret.Namespace).List(options)
-	if err != nil {
-		return nil, err
-	}
-
-	for i, currSecret := range potentialSecrets.Items {
-		if currSecret.Annotations[ServiceAccountTokenSecretNameKey] == tokenSecret.Name {
-			dockercfgSecrets = append(dockercfgSecrets, &potentialSecrets.Items[i])
-		}
-	}
-
-	return dockercfgSecrets, nil
-}

pkg/serviceaccounts/controllers/deleted_token_secrets_test.go

@@ -5,7 +5,7 @@ import (
 	"reflect"
 	"testing"
 
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/fields"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -39,7 +39,12 @@ func regularSecretReferences() []v1.ObjectReference {
 
 // tokenSecretReferences is used by a service account that references a ServiceAccountToken secret
 func tokenSecretReferences() []v1.ObjectReference {
-	return []v1.ObjectReference{{Name: "token-secret-1"}}
+	return []v1.ObjectReference{
+		{
+			Name: "token-secret-1",
+			UID:  "23456",
+		},
+	}
 }
 
 // addTokenSecretReference adds a reference to the ServiceAccountToken that will be created
@@ -85,6 +90,37 @@ func createdDockercfgSecret() *v1.Secret {
 	}
 }
 
+// createdDockercfgSecretWithOwnerRef returns the ServiceAccountToken secret posted when creating a new token secret, including an owner reference.
+// Named "default-token-fplln", since that is the first generated name after rand.Seed(1)
+func createdDockercfgSecretWithOwnerRef() *v1.Secret {
+	isController := true
+	gv := v1.SchemeGroupVersion
+	return &v1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "default-dockercfg-fplln",
+			Namespace: "default",
+			Annotations: map[string]string{
+				v1.ServiceAccountNameKey:         "default",
+				v1.ServiceAccountUIDKey:          "12345",
+				ServiceAccountTokenSecretNameKey: "token-secret-1",
+			},
+			OwnerReferences: []metav1.OwnerReference{
+				{
+					APIVersion: gv.String(),
+					Kind:       "Secret",
+					Name:       "token-secret-1",
+					UID:        "23456",
+					Controller: &isController,
+				},
+			},
+		},
+		Type: v1.SecretTypeDockercfg,
+		Data: map[string][]byte{
+			v1.DockerConfigKey: []byte(`{"docker-registry.default.svc.cluster.local":{"Username":"serviceaccount","Password":"ABC","Email":"serviceaccount@example.org"}}`),
+		},
+	}
+}
+
 // opaqueSecret returns a persisted non-ServiceAccountToken secret named "regular-secret-1"
 func opaqueSecret() *v1.Secret {
 	return &v1.Secret{
@@ -163,7 +199,7 @@ func TestTokenDeletion(t *testing.T) {
 			},
 		},
 		"deleted token secret with serviceaccount without reference": {
-			ClientObjects: []runtime.Object{serviceAccount(addTokenSecretReference(tokenSecretReferences()), imagePullSecretReferences()), createdDockercfgSecret()},
+			ClientObjects: []runtime.Object{serviceAccount(addTokenSecretReference(tokenSecretReferences()), imagePullSecretReferences()), createdDockercfgSecret(), serviceAccountTokenSecret()},
 
 			DeletedSecret: serviceAccountTokenSecret(),
 			ExpectedActions: []clientgotesting.Action{
@@ -174,6 +210,16 @@ func TestTokenDeletion(t *testing.T) {
 				clientgotesting.NewDeleteAction(schema.GroupVersionResource{Resource: "secrets", Version: "v1"}, "default", "default-dockercfg-fplln"),
 			},
 		},
+		"deleted token secret with pull secret owner reference": {
+			ClientObjects: []runtime.Object{serviceAccount(addTokenSecretReference(tokenSecretReferences()), imagePullSecretReferences()), createdDockercfgSecretWithOwnerRef(), serviceAccountTokenSecret()},
+			DeletedSecret: serviceAccountTokenSecret(),
+			ExpectedActions: []clientgotesting.Action{
+				clientgotesting.NewListAction(
+					schema.GroupVersionResource{Resource: "secrets", Version: "v1"},
+					schema.GroupVersionKind{Kind: "Secret", Version: "v1"},
+					"default", metav1.ListOptions{FieldSelector: dockercfgSecretFieldSelector.String()}),
+			},
+		},
 	}
 
 	for k, tc := range testcases {

pkg/serviceaccounts/controllers/pull_secrets.go

@@ -0,0 +1,29 @@
+package controllers
+
+import (
+	corev1 "k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/fields"
+	"k8s.io/client-go/kubernetes"
+	api "k8s.io/kubernetes/pkg/apis/core"
+)
+
+// findDockercfgSecret checks all the secrets in the namespace to see if the token secret has any existing dockercfg secrets that reference it
+func findDockercfgSecrets(client kubernetes.Interface, tokenSecret *corev1.Secret) ([]*corev1.Secret, error) {
+	dockercfgSecrets := []*corev1.Secret{}
+
+	options := metav1.ListOptions{FieldSelector: fields.OneTermEqualSelector(api.SecretTypeField, string(v1.SecretTypeDockercfg)).String()}
+	potentialSecrets, err := client.CoreV1().Secrets(tokenSecret.Namespace).List(options)
+	if err != nil {
+		return nil, err
+	}
+
+	for i, currSecret := range potentialSecrets.Items {
+		if currSecret.Annotations[ServiceAccountTokenSecretNameKey] == tokenSecret.Name {
+			dockercfgSecrets = append(dockercfgSecrets, &potentialSecrets.Items[i])
+		}
+	}
+
+	return dockercfgSecrets, nil
+}