From a8132d35f5e9e32c6bdfc0e6b35d6af8f00b8465 Mon Sep 17 00:00:00 2001 From: Adam Kaplan Date: Wed, 29 Jan 2020 10:03:24 -0500 Subject: [PATCH] Bug 1765294: Use OwnerRefs to clean up SA pull secrets Update the service account pull secret controller to add owner references to generated pull secrets. This ensures pull secrets are deleted when the associated token is deleted. --- .../controllers/create_dockercfg_secrets.go | 71 ++++++++++++++++++- .../controllers/deleted_token_secrets.go | 33 +++------ .../controllers/deleted_token_secrets_test.go | 52 +++++++++++++- .../controllers/pull_secrets.go | 29 ++++++++ 4 files changed, 156 insertions(+), 29 deletions(-) create mode 100644 pkg/serviceaccounts/controllers/pull_secrets.go diff --git a/pkg/serviceaccounts/controllers/create_dockercfg_secrets.go b/pkg/serviceaccounts/controllers/create_dockercfg_secrets.go index 094282612..04fd0f02d 100644 --- a/pkg/serviceaccounts/controllers/create_dockercfg_secrets.go +++ b/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{} diff --git a/pkg/serviceaccounts/controllers/deleted_token_secrets.go b/pkg/serviceaccounts/controllers/deleted_token_secrets.go index f83ecdffe..d0b682057 100644 --- a/pkg/serviceaccounts/controllers/deleted_token_secrets.go +++ b/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,8 +78,7 @@ 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 @@ -89,30 +86,16 @@ func (e *DockercfgTokenDeletedController) secretDeleted(obj interface{}) { 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 -} diff --git a/pkg/serviceaccounts/controllers/deleted_token_secrets_test.go b/pkg/serviceaccounts/controllers/deleted_token_secrets_test.go index ee8bb70da..e11f81c45 100644 --- a/pkg/serviceaccounts/controllers/deleted_token_secrets_test.go +++ b/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 { diff --git a/pkg/serviceaccounts/controllers/pull_secrets.go b/pkg/serviceaccounts/controllers/pull_secrets.go new file mode 100644 index 000000000..1deb261aa --- /dev/null +++ b/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 +}