Remove association between ServiceAccounts and token Secrets

If a token Secret is associated to a ServiceAccount via its
"secrets" field, K8s 1.27 interprets this as an auto-generated
token and issues a client-side warning:

"Use tokens from the TokenRequest API or manually created
secret-based tokens instead of auto-generated secret-based tokens."

While we no longer use auto-generated Secrets, we do still set the
"secrets" field with the manually created Secret which causes the
warning to be generated for every API call issued using a rest config
created with the Secret's token.

We don't need to set the "secrets" field - we can locate a
ServiceAccount's associated Secret via the
"kubernetes.io/service-account.name" annotation on the Secret.

For migration, also remove the "secrets" field from existing
ServiceAccounts.

As a further safeguard on upgrade, ignore the warning via the
warning handler.

Signed-off-by: Tom Pantelis <tompantelis@gmail.com>

cmd/subctl/root.go

@@ -43,7 +43,8 @@ import (
 type suppressWarnings struct{}
 
 func (suppressWarnings) HandleWarningHeader(code int, agent, message string) {
-	if code == 299 && strings.Contains(message, "would violate PodSecurity") {
+	if code == 299 && (strings.Contains(message, "would violate PodSecurity") ||
+		strings.Contains(message, "instead of auto-generated secret-based tokens")) {
 		return
 	}
 

internal/rbac/rbac.go

@@ -20,40 +20,34 @@ package rbac
 
 import (
 	"context"
-	"fmt"
-	"strings"
 
 	"github.com/pkg/errors"
+	corev1 "k8s.io/api/core/v1"
 	v1 "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"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/kubernetes"
 )
 
-// maxGeneratedNameLength is the maximum generated length for a token, excluding the random suffix
-// See k8s.io/apiserver/pkg/storage/names.
-const maxGeneratedNameLength = 63 - 5
-
 func GetClientTokenSecret(ctx context.Context, kubeClient kubernetes.Interface, namespace, serviceAccountName string) (*v1.Secret, error) {
-	sa, err := kubeClient.CoreV1().ServiceAccounts(namespace).Get(ctx, serviceAccountName, metav1.GetOptions{})
+	saSecrets, err := kubeClient.CoreV1().Secrets(namespace).List(ctx, metav1.ListOptions{
+		FieldSelector: fields.OneTermEqualSelector("type", string(corev1.SecretTypeServiceAccountToken)).String(),
+	})
 	if err != nil {
-		return nil, errors.Wrapf(err, "ServiceAccount %s get failed", serviceAccountName)
-	}
-
-	if len(sa.Secrets) < 1 {
-		return nil, fmt.Errorf("ServiceAccount %s does not have any secret", sa.Name)
-	}
-
-	tokenPrefix := fmt.Sprintf("%s-token-", serviceAccountName)
-	if len(tokenPrefix) > maxGeneratedNameLength {
-		tokenPrefix = tokenPrefix[:maxGeneratedNameLength]
+		return nil, errors.Wrapf(err, "failed to list secrets of type %q in namespace %q",
+			corev1.SecretTypeServiceAccountToken, namespace)
 	}
 
-	for _, secret := range sa.Secrets {
-		if strings.HasPrefix(secret.Name, tokenPrefix) {
-			//nolint:wrapcheck // No need to wrap here
-			return kubeClient.CoreV1().Secrets(namespace).Get(ctx, secret.Name, metav1.GetOptions{})
+	for i := range saSecrets.Items {
+		if saSecrets.Items[i].Annotations[corev1.ServiceAccountNameKey] == serviceAccountName {
+			return &saSecrets.Items[i], nil
 		}
 	}
 
-	return nil, fmt.Errorf("ServiceAccount %s does not have a secret of type token", serviceAccountName)
+	return nil, apierrors.NewNotFound(schema.GroupResource{
+		Group:    corev1.SchemeGroupVersion.Group,
+		Resource: "secrets",
+	}, serviceAccountName)
 }

pkg/broker/ensure.go

@@ -20,11 +20,9 @@ package broker
 
 import (
 	"context"
-	"time"
 
 	"github.com/pkg/errors"
 	"github.com/submariner-io/subctl/internal/component"
-	"github.com/submariner-io/subctl/internal/constants"
 	"github.com/submariner-io/subctl/internal/rbac"
 	"github.com/submariner-io/subctl/pkg/gateway"
 	"github.com/submariner-io/subctl/pkg/namespace"
@@ -39,7 +37,6 @@ import (
 	rbacv1 "k8s.io/api/rbac/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/kubernetes"
 )
 
@@ -88,9 +85,7 @@ func Ensure(ctx context.Context, crdUpdater crd.Updater, kubeClient kubernetes.I
 		return errors.Wrap(err, "error creating broker role")
 	}
 
-	_, err = WaitForClientToken(ctx, kubeClient, constants.SubmarinerBrokerAdminSA, brokerNS)
-
-	return err
+	return nil
 }
 
 // CreateSAForCluster creates a new SA for each new cluster joined and binds it to the submariner cluster role.
@@ -107,8 +102,8 @@ func CreateSAForCluster(ctx context.Context, kubeClient kubernetes.Interface, cl
 		return nil, errors.Wrap(err, "error binding sa to cluster role")
 	}
 
-	clientToken, err := WaitForClientToken(ctx, kubeClient, saName, inNamespace)
-	if err != nil && !apierrors.IsAlreadyExists(err) {
+	clientToken, err := rbac.GetClientTokenSecret(ctx, kubeClient, inNamespace, saName)
+	if err != nil {
 		return nil, errors.Wrap(err, "error getting cluster sa token")
 	}
 
@@ -137,37 +132,6 @@ func createBrokerAdministratorRoleAndSA(ctx context.Context, kubeClient kubernet
 	return nil
 }
 
-func WaitForClientToken(ctx context.Context, kubeClient kubernetes.Interface, submarinerBrokerSA, inNamespace string) (*v1.Secret, error) {
-	// wait for the client token to be ready, while implementing
-	// exponential backoff pattern, it will wait a total of:
-	// sum(n=0..9, 1.2^n * 5) seconds, = 130 seconds
-	backoff := wait.Backoff{
-		Steps:    10,
-		Duration: 5 * time.Second,
-		Factor:   1.2,
-		Jitter:   1,
-	}
-
-	var secret *v1.Secret
-	var lastErr error
-
-	err := wait.ExponentialBackoff(backoff, func() (bool, error) {
-		secret, lastErr = rbac.GetClientTokenSecret(ctx, kubeClient, inNamespace, submarinerBrokerSA)
-		if lastErr != nil {
-			//nolint:nilerr // Intentional - the error is propagated via the outer-scoped var 'lastErr'
-			return false, nil
-		}
-
-		return true, nil
-	})
-
-	if wait.Interrupted(err) {
-		return nil, lastErr //nolint:wrapcheck // No need to wrap here
-	}
-
-	return secret, err //nolint:wrapcheck // No need to wrap here
-}
-
 //nolint:wrapcheck // No need to wrap here
 func CreateOrUpdateClusterBrokerRole(ctx context.Context, kubeClient kubernetes.Interface, inNamespace string) (bool, error) {
 	return role.EnsureFromYAML(ctx, kubeClient, inNamespace, embeddedyamls.Config_broker_broker_client_role_yaml)
@@ -196,7 +160,7 @@ func CreateOrUpdateBrokerAdminRoleBinding(ctx context.Context, kubeClient kubern
 //nolint:wrapcheck // No need to wrap here
 func CreateNewBrokerSA(ctx context.Context, kubeClient kubernetes.Interface, submarinerBrokerSA, inNamespace string) (err error) {
 	sa := NewBrokerSA(submarinerBrokerSA)
-	_, err = serviceaccount.Ensure(ctx, kubeClient, inNamespace, sa, true)
+	_, err = serviceaccount.Ensure(ctx, kubeClient, inNamespace, sa)
 
 	return err
 }

pkg/serviceaccount/ensure.go

@@ -21,18 +21,19 @@ package serviceaccount
 import (
 	"context"
 	"fmt"
-	"strings"
+	"time"
 
 	"github.com/pkg/errors"
 	"github.com/submariner-io/admiral/pkg/resource"
-	resourceutil "github.com/submariner-io/subctl/pkg/resource"
+	"github.com/submariner-io/admiral/pkg/util"
+	"github.com/submariner-io/subctl/internal/rbac"
 	"github.com/submariner-io/subctl/pkg/secret"
 	"github.com/submariner-io/submariner-operator/pkg/embeddedyamls"
 	corev1 "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"
-	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/kubernetes"
 )
 
@@ -50,7 +51,7 @@ func ensureFromYAML(ctx context.Context, kubeClient kubernetes.Interface, namesp
 		return nil, err //nolint:wrapcheck // No need to wrap errors here.
 	}
 
-	err = ensure(ctx, kubeClient, namespace, sa, true)
+	err = ensure(ctx, kubeClient, namespace, sa)
 	if err != nil {
 		return nil, err
 	}
@@ -59,26 +60,20 @@ func ensureFromYAML(ctx context.Context, kubeClient kubernetes.Interface, namesp
 }
 
 //nolint:wrapcheck // No need to wrap errors here.
-func ensure(ctx context.Context, kubeClient kubernetes.Interface, namespace string, sa *corev1.ServiceAccount, onlyCreate bool) error {
-	if onlyCreate {
-		_, err := kubeClient.CoreV1().ServiceAccounts(namespace).Get(ctx, sa.Name, metav1.GetOptions{})
-
-		if err == nil || !apierrors.IsNotFound(err) {
-			return err
-		}
-	}
-
-	_, err := resourceutil.CreateOrUpdate(ctx, resource.ForServiceAccount(kubeClient, namespace), sa)
+func ensure(ctx context.Context, kubeClient kubernetes.Interface, namespace string, sa *corev1.ServiceAccount) error {
+	_, err := util.CreateOrUpdate(ctx, resource.ForServiceAccount(kubeClient, namespace), sa,
+		func(existing runtime.Object) (runtime.Object, error) {
+			existing.(*corev1.ServiceAccount).Secrets = nil
+			return existing, nil
+		})
 
 	return err
 }
 
 //nolint:wrapcheck // No need to wrap errors here.
-func Ensure(ctx context.Context,
-	kubeClient kubernetes.Interface, namespace string, sa *corev1.ServiceAccount, onlyCreate bool) (
-	*corev1.ServiceAccount, error,
-) {
-	err := ensure(ctx, kubeClient, namespace, sa, onlyCreate)
+func Ensure(ctx context.Context, kubeClient kubernetes.Interface, namespace string, sa *corev1.ServiceAccount,
+) (*corev1.ServiceAccount, error) {
+	err := ensure(ctx, kubeClient, namespace, sa)
 	if err != nil {
 		return nil, err
 	}
@@ -108,86 +103,56 @@ func EnsureFromYAML(ctx context.Context, kubeClient kubernetes.Interface, namesp
 }
 
 func EnsureSecretFromSA(ctx context.Context, client kubernetes.Interface, saName, namespace string) (*corev1.Secret, error) {
-	sa, err := client.CoreV1().ServiceAccounts(namespace).Get(ctx, saName, metav1.GetOptions{})
-	if err != nil {
-		return nil, errors.Wrapf(err, "failed to get ServiceAccount %s/%s", namespace, saName)
-	}
-
-	saSecret := getSecretFromSA(ctx, client, sa)
-
-	if saSecret != nil {
+	saSecret, err := rbac.GetClientTokenSecret(ctx, client, namespace, saName)
+	if err == nil {
 		return saSecret, nil
 	}
 
-	// We couldn't find right secret from this SA, search all Secrets
-	saSecret, err = getSecretForSA(ctx, client, sa)
-	if err != nil && !apierrors.IsNotFound(err) {
-		return nil, err
+	if !apierrors.IsNotFound(err) {
+		return nil, err //nolint:wrapcheck // No need to wrap
 	}
 
-	if err != nil {
-		newSecret := &corev1.Secret{
-			ObjectMeta: metav1.ObjectMeta{
-				GenerateName: fmt.Sprintf("%s-token-", sa.Name),
-				Namespace:    namespace,
-				Annotations: map[string]string{
-					corev1.ServiceAccountNameKey: saName,
-					createdByAnnotation:          creatorName,
-				},
+	newSecret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			GenerateName: fmt.Sprintf("%s-token-", saName),
+			Namespace:    namespace,
+			Annotations: map[string]string{
+				corev1.ServiceAccountNameKey: saName,
+				createdByAnnotation:          creatorName,
 			},
-			Type: corev1.SecretTypeServiceAccountToken,
-		}
-
-		saSecret, err = secret.Ensure(ctx, client, newSecret.Namespace, newSecret)
-		if err != nil {
-			return nil, errors.Wrapf(err, "failed to create secret for ServiceAccount %v", saName)
-		}
-	}
-
-	secretRef := corev1.ObjectReference{
-		Name: saSecret.Name,
+		},
+		Type: corev1.SecretTypeServiceAccountToken,
 	}
 
-	sa.Secrets = append(sa.Secrets, secretRef)
-	err = ensure(ctx, client, namespace, sa, false)
-
+	saSecret, err = secret.Ensure(ctx, client, newSecret.Namespace, newSecret)
 	if err != nil {
-		return nil, errors.Wrapf(err, "failed to update ServiceAccount %v with Secret reference %v", saName, secretRef.Name)
+		return nil, errors.Wrapf(err, "failed to create secret for service account %q", saName)
 	}
 
-	return saSecret, nil
-}
+	// Ensure the token has been generated for the secret.
+	backoff := wait.Backoff{
+		Steps:    30,
+		Duration: 100 * time.Millisecond,
+		Factor:   1.3,
+		Jitter:   1,
+	}
 
-func getSecretFromSA(ctx context.Context, client kubernetes.Interface, sa *corev1.ServiceAccount) *corev1.Secret {
-	secretNamePrefix := fmt.Sprintf("%s-token-", sa.Name)
-	for _, saSecretRef := range sa.Secrets {
-		if strings.HasPrefix(saSecretRef.Name, secretNamePrefix) {
-			saSecret, _ := client.CoreV1().Secrets(sa.Namespace).Get(ctx, saSecretRef.Name, metav1.GetOptions{})
-			if saSecret.Annotations[corev1.ServiceAccountNameKey] == sa.Name && saSecret.Type == corev1.SecretTypeServiceAccountToken {
-				return saSecret
-			}
+	err = wait.ExponentialBackoff(backoff, func() (bool, error) {
+		saSecret, err = client.CoreV1().Secrets(namespace).Get(ctx, saSecret.Name, metav1.GetOptions{})
+		if err != nil {
+			return false, errors.Wrapf(err, "error getting secret %q", saSecret.Name)
 		}
-	}
 
-	return nil
-}
+		if len(saSecret.Data["token"]) == 0 {
+			return false, nil
+		}
 
-func getSecretForSA(ctx context.Context, client kubernetes.Interface, sa *corev1.ServiceAccount) (*corev1.Secret, error) {
-	saSecrets, err := client.CoreV1().Secrets(sa.Namespace).List(ctx, metav1.ListOptions{
-		FieldSelector: fields.OneTermEqualSelector("type", "kubernetes.io/service-account-token").String(),
+		return true, nil
 	})
-	if err != nil {
-		return nil, errors.Wrapf(err, "failed to get secrets of type service-account-token in %v", sa.Namespace)
-	}
 
-	for i := 0; i < len(saSecrets.Items); i++ {
-		if saSecrets.Items[i].Annotations[corev1.ServiceAccountNameKey] == sa.Name {
-			return &saSecrets.Items[i], nil
-		}
+	if wait.Interrupted(err) {
+		return nil, fmt.Errorf("the token was not generated for secret %q", saSecret.Name)
 	}
 
-	return nil, apierrors.NewNotFound(schema.GroupResource{
-		Group:    corev1.SchemeGroupVersion.Group,
-		Resource: "secrets",
-	}, sa.Name)
+	return saSecret, err //nolint:wrapcheck // No need to wrap here
 }

pkg/serviceaccount/ensure_test.go

@@ -71,7 +71,7 @@ var _ = Describe("EnsureFromYAML", func() {
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "test-sa",
 				},
-			}, false)
+			})
 			Expect(err).To(Succeed())
 			assertServiceAccount()