Allow to unlink deleted secrets from a service account

Author: mfojtik

pkg/cmd/cli/secrets/link_secret_to_obj.go

@@ -127,7 +127,7 @@ func (o LinkSecretOptions) LinkSecrets() error {
 // linkSecretsToServiceAccount links secrets to the service account, either as pull secrets, mount secrets, or both.
 func (o LinkSecretOptions) linkSecretsToServiceAccount(serviceaccount *kapi.ServiceAccount) error {
 	updated := false
-	newSecrets, failLater, err := o.GetSecrets()
+	newSecrets, hasNotFound, err := o.GetSecrets(false)
 	if err != nil {
 		return err
 	}
@@ -154,7 +154,7 @@ func (o LinkSecretOptions) linkSecretsToServiceAccount(serviceaccount *kapi.Serv
 		return err
 	}
 
-	if failLater {
+	if hasNotFound {
 		return errors.New("Some secrets could not be linked")
 	}
 

pkg/cmd/cli/secrets/options.go

@@ -8,6 +8,7 @@ import (
 	"os"
 
 	kapi "k8s.io/kubernetes/pkg/api"
+	kerrors "k8s.io/kubernetes/pkg/api/errors"
 	"k8s.io/kubernetes/pkg/api/meta"
 	client "k8s.io/kubernetes/pkg/client/unversioned"
 	kcmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
@@ -144,9 +145,10 @@ func (o SecretOptions) GetOut() io.Writer {
 }
 
 // GetSecrets Return a list of secret objects in the default namespace
-func (o SecretOptions) GetSecrets() ([]*kapi.Secret, bool, error) {
+// If allowNonExisting is set to true, we will return the non-existing secrets as well.
+func (o SecretOptions) GetSecrets(allowNonExisting bool) ([]*kapi.Secret, bool, error) {
 	secrets := []*kapi.Secret{}
-	failLater := false
+	hasNotFound := false
 
 	for _, secretName := range o.SecretNames {
 		r := resource.NewBuilder(o.Mapper, o.Typer, o.ClientMapper, kapi.Codecs.UniversalDecoder()).
@@ -159,11 +161,23 @@ func (o SecretOptions) GetSecrets() ([]*kapi.Secret, bool, error) {
 		}
 		obj, err := r.Object()
 		if err != nil {
-			fmt.Fprintf(os.Stderr, "secrets \"%s\" not found\n", secretName)
-			// Missing secrets are non-fatal but the command should not return
-			// success.
-			failLater = true
-			continue
+			// If the secret is not found it means it was deleted but we want still to allow to
+			// unlink a removed secret from the service account
+			if kerrors.IsNotFound(err) {
+				fmt.Fprintf(os.Stderr, "secret %q not found\n", secretName)
+				hasNotFound = true
+				if allowNonExisting {
+					obj = &kapi.Secret{
+						ObjectMeta: kapi.ObjectMeta{
+							Name: secretName,
+						},
+					}
+				} else {
+					continue
+				}
+			} else if err != nil {
+				return nil, false, err
+			}
 		}
 		switch t := obj.(type) {
 		case *kapi.Secret:
@@ -177,5 +191,5 @@ func (o SecretOptions) GetSecrets() ([]*kapi.Secret, bool, error) {
 		return nil, false, errors.New("No valid secrets found")
 	}
 
-	return secrets, failLater, nil
+	return secrets, hasNotFound, nil
 }

pkg/cmd/cli/secrets/unlink_secret_from_object.go

@@ -74,20 +74,23 @@ func (o UnlinkSecretOptions) UnlinkSecrets() error {
 func (o UnlinkSecretOptions) unlinkSecretsFromServiceAccount(serviceaccount *kapi.ServiceAccount) error {
 	// All of the requested secrets must be present in either the Mount or Pull secrets
 	// If any of them are not present, we'll return an error and push no changes.
-	rmSecrets, failLater, err := o.GetSecrets()
+	rmSecrets, hasNotFound, err := o.GetSecrets(true)
 	if err != nil {
 		return err
 	}
 	rmSecretNames := o.GetSecretNames(rmSecrets)
 
 	newMountSecrets := []kapi.ObjectReference{}
 	newPullSecrets := []kapi.LocalObjectReference{}
+	updated := false
 
 	// Check the mount secrets
 	for _, secret := range serviceaccount.Secrets {
 		if !rmSecretNames.Has(secret.Name) {
 			// Copy this back in, since it doesn't match the ones we're removing
 			newMountSecrets = append(newMountSecrets, secret)
+		} else {
+			updated = true
 		}
 	}
 
@@ -96,20 +99,24 @@ func (o UnlinkSecretOptions) unlinkSecretsFromServiceAccount(serviceaccount *kap
 		if !rmSecretNames.Has(imagePullSecret.Name) {
 			// Copy this back in, since it doesn't match the one we're removing
 			newPullSecrets = append(newPullSecrets, imagePullSecret)
+		} else {
+			updated = true
 		}
 	}
 
-	// Save the updated Secret lists back to the server
-	serviceaccount.Secrets = newMountSecrets
-	serviceaccount.ImagePullSecrets = newPullSecrets
-	_, err = o.ClientInterface.ServiceAccounts(o.Namespace).Update(serviceaccount)
-	if err != nil {
-		return err
-	}
-
-	if failLater {
-		return errors.New("Some secrets could not be unlinked")
+	if updated {
+		// Save the updated Secret lists back to the server
+		serviceaccount.Secrets = newMountSecrets
+		serviceaccount.ImagePullSecrets = newPullSecrets
+		_, err = o.ClientInterface.ServiceAccounts(o.Namespace).Update(serviceaccount)
+		if err != nil {
+			return err
+		}
+		if hasNotFound {
+			return fmt.Errorf("Unlinked deleted secrets from %s/%s service account", o.Namespace, serviceaccount.Name)
+		}
+		return nil
+	} else {
+		return errors.New("No valid secrets found or secrets not linked to service account")
 	}
-
-	return nil
 }

test/cmd/secrets.sh

@@ -105,16 +105,16 @@ os::cmd::expect_failure 'oc get serviceaccounts/deployer -o yaml |grep -q basica
 os::cmd::expect_success 'oc secrets link deployer basicauth'
 
 # Removing a non-existent secret should warn but succeed and change nothing
-os::cmd::expect_failure_and_text 'oc secrets unlink deployer foobar' 'secrets "foobar" not found'
+os::cmd::expect_failure_and_text 'oc secrets unlink deployer foobar' 'secret "foobar" not found'
 
 # Make sure that removing an existent and non-existent secret succeeds but warns about the non-existent one
-os::cmd::expect_failure_and_text 'oc secrets unlink deployer foobar basicauth' 'secrets "foobar" not found'
+os::cmd::expect_failure_and_text 'oc secrets unlink deployer foobar basicauth' 'secret "foobar" not found'
 # Make sure that the existing secret is removed
 os::cmd::expect_failure 'oc get serviceaccounts/deployer -o yaml |grep -q basicauth'
 
 # Make sure that removing a real but unlinked secret succeeds
 # https://github.com/openshift/origin/pull/9234#discussion_r70832486
-os::cmd::expect_success 'oc secrets unlink deployer basicauth'
+os::cmd::expect_failure_and_text 'oc secrets unlink deployer basicauth', 'No valid secrets found or secrets not linked to service account'
 
 # Make sure that it succeeds if *any* of the secrets are linked
 # https://github.com/openshift/origin/pull/9234#discussion_r70832486