fix: Always create manual long lived token (#19970)

Signed-off-by: Max Gautier <mg@max.gautier.name>

go.mod

@@ -103,7 +103,6 @@ require (
 	k8s.io/api v0.32.2
 	k8s.io/apiextensions-apiserver v0.32.2
 	k8s.io/apimachinery v0.32.2
-	k8s.io/apiserver v0.32.2
 	k8s.io/client-go v0.32.2
 	k8s.io/code-generator v0.32.2
 	k8s.io/klog/v2 v2.130.1
@@ -277,6 +276,7 @@ require (
 	gopkg.in/gomail.v2 v2.0.0-20160411212932-81ebce5c23df // indirect
 	gopkg.in/inf.v0 v0.9.1 // indirect
 	gopkg.in/warnings.v0 v0.1.2 // indirect
+	k8s.io/apiserver v0.32.2 // indirect
 	k8s.io/cli-runtime v0.32.2 // indirect
 	k8s.io/component-base v0.32.2 // indirect
 	k8s.io/component-helpers v0.32.2 // indirect

util/clusterauth/clusterauth.go

@@ -2,7 +2,6 @@ package clusterauth
 
 import (
 	"context"
-	"encoding/json"
 	"fmt"
 	"strings"
 	"time"
@@ -13,7 +12,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/types"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/kubernetes"
 
@@ -25,6 +23,7 @@ const (
 	ArgoCDManagerServiceAccount     = "argocd-manager"
 	ArgoCDManagerClusterRole        = "argocd-manager-role"
 	ArgoCDManagerClusterRoleBinding = "argocd-manager-role-binding"
+	SATokenSecretSuffix             = "-long-lived-token"
 )
 
 // ArgoCDManagerPolicyRules are the policies to give argocd-manager
@@ -237,7 +236,7 @@ func GetServiceAccountBearerToken(clientset kubernetes.Interface, ns string, sa
 			return false, fmt.Errorf("failed to get secret %q for serviceaccount %q: %w", secretName, sa, err)
 		}
 
-		_, ok := secret.Data["token"]
+		_, ok := secret.Data[corev1.ServiceAccountTokenKey]
 		if !ok {
 			return false, nil
 		}
@@ -248,59 +247,21 @@ func GetServiceAccountBearerToken(clientset kubernetes.Interface, ns string, sa
 		return "", fmt.Errorf("failed to get token for serviceaccount %q: %w", sa, err)
 	}
 
-	return string(secret.Data["token"]), nil
+	return string(secret.Data[corev1.ServiceAccountTokenKey]), nil
 }
 
-// getOrCreateServiceAccountTokenSecret will check if a ServiceAccount
-// already has a kubernetes.io/service-account-token secret associated
-// with it or creates one if the ServiceAccount doesn't have one. This
-// was added to help add k8s v1.24+ clusters.
-func getOrCreateServiceAccountTokenSecret(clientset kubernetes.Interface, sa, ns string) (string, error) {
-	// Wait for sa to have secret, but don't wait too
-	// long for 1.24+ clusters
-	var serviceAccount *corev1.ServiceAccount
-	err := wait.PollUntilContextTimeout(context.Background(), 500*time.Millisecond, 30*time.Second, true, func(ctx context.Context) (bool, error) {
-		ctx, cancel := context.WithTimeout(ctx, common.ClusterAuthRequestTimeout)
-		defer cancel()
-		var getErr error
-		serviceAccount, getErr = clientset.CoreV1().ServiceAccounts(ns).Get(ctx, sa, metav1.GetOptions{})
-		if getErr != nil {
-			return false, fmt.Errorf("failed to get serviceaccount %q: %w", sa, getErr)
-		}
-		return true, nil
-	})
-	if err != nil && !wait.Interrupted(err) {
-		return "", fmt.Errorf("failed to get serviceaccount token secret: %w", err)
-	}
-	if serviceAccount == nil {
-		log.Errorf("Unexpected nil serviceaccount '%s/%s' with no error returned", ns, sa)
-		return "", fmt.Errorf("failed to create serviceaccount token secret: nil serviceaccount returned for '%s/%s' with no error", ns, sa)
-	}
-
-	outerCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
-	defer cancel()
-	for _, s := range serviceAccount.Secrets {
-		innerCtx, cancel := context.WithTimeout(outerCtx, common.ClusterAuthRequestTimeout)
-		defer cancel()
-		existingSecret, err := clientset.CoreV1().Secrets(ns).Get(innerCtx, s.Name, metav1.GetOptions{})
-		if err != nil {
-			return "", fmt.Errorf("failed to retrieve secret %q: %w", s.Name, err)
-		}
-		if existingSecret.Type == corev1.SecretTypeServiceAccountToken {
-			return existingSecret.Name, nil
-		}
-	}
-
-	return createServiceAccountToken(clientset, serviceAccount)
-}
-
-func createServiceAccountToken(clientset kubernetes.Interface, serviceAccount *corev1.ServiceAccount) (string, error) {
+// getOrCreateServiceAccountTokenSecret will create a
+// kubernetes.io/service-account-token secret associated with a
+// ServiceAccount named '<service account name>-long-lived-token', or
+// use the existing one with that name.
+// This was added to help add k8s v1.24+ clusters.
+func getOrCreateServiceAccountTokenSecret(clientset kubernetes.Interface, serviceaccount, namespace string) (string, error) {
 	secret := &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
-			GenerateName: serviceAccount.Name + "-token-",
-			Namespace:    serviceAccount.Namespace,
+			Name:      serviceaccount + SATokenSecretSuffix,
+			Namespace: namespace,
 			Annotations: map[string]string{
-				corev1.ServiceAccountNameKey: serviceAccount.Name,
+				corev1.ServiceAccountNameKey: serviceaccount,
 			},
 		},
 		Type: corev1.SecretTypeServiceAccountToken,
@@ -308,24 +269,15 @@ func createServiceAccountToken(clientset kubernetes.Interface, serviceAccount *c
 
 	ctx, cancel := context.WithTimeout(context.Background(), common.ClusterAuthRequestTimeout)
 	defer cancel()
-	secret, err := clientset.CoreV1().Secrets(serviceAccount.Namespace).Create(ctx, secret, metav1.CreateOptions{})
-	if err != nil {
-		return "", fmt.Errorf("failed to create secret for serviceaccount %q: %w", serviceAccount.Name, err)
-	}
+	_, err := clientset.CoreV1().Secrets(namespace).Create(ctx, secret, metav1.CreateOptions{})
 
-	log.Infof("Created bearer token secret for ServiceAccount %q", serviceAccount.Name)
-	serviceAccount.Secrets = []corev1.ObjectReference{{
-		Name:      secret.Name,
-		Namespace: secret.Namespace,
-	}}
-	patch, err := json.Marshal(serviceAccount)
-	if err != nil {
-		return "", fmt.Errorf("failed marshaling patch for serviceaccount %q: %w", serviceAccount.Name, err)
-	}
-
-	_, err = clientset.CoreV1().ServiceAccounts(serviceAccount.Namespace).Patch(ctx, serviceAccount.Name, types.StrategicMergePatchType, patch, metav1.PatchOptions{})
-	if err != nil {
-		return "", fmt.Errorf("failed to patch serviceaccount %q with bearer token secret: %w", serviceAccount.Name, err)
+	switch {
+	case apierrors.IsAlreadyExists(err):
+		log.Infof("Using existing bearer token secret %q for ServiceAccount %q", secret.Name, serviceaccount)
+	case err != nil:
+		return "", fmt.Errorf("failed to create secret %q for serviceaccount %q: %w", secret.Name, serviceaccount, err)
+	default:
+		log.Infof("Created bearer token secret %q for ServiceAccount %q", secret.Name, serviceaccount)
 	}
 
 	return secret.Name, nil

util/clusterauth/clusterauth_test.go

@@ -1,6 +1,7 @@
 package clusterauth
 
 import (
+	"errors"
 	"os"
 	"testing"
 	"time"
@@ -13,7 +14,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	"k8s.io/apiserver/pkg/storage/names"
 	"k8s.io/client-go/kubernetes/fake"
 	kubetesting "k8s.io/client-go/testing"
 	"sigs.k8s.io/yaml"
@@ -106,13 +106,36 @@ func TestCreateServiceAccount(t *testing.T) {
 	})
 }
 
+func _MockK8STokenController(objects kubetesting.ObjectTracker) kubetesting.ReactionFunc {
+	return (func(action kubetesting.Action) (bool, runtime.Object, error) {
+		secret, ok := action.(kubetesting.CreateAction).GetObject().(*corev1.Secret)
+		if !ok {
+			return false, nil, nil
+		}
+		_, err := objects.Get(schema.GroupVersionResource{Version: "v1", Resource: "serviceaccounts"},
+			secret.Namespace,
+			secret.Annotations[corev1.ServiceAccountNameKey],
+			metav1.GetOptions{})
+		if err != nil {
+			return false, nil, nil
+		}
+		if secret.Data == nil {
+			secret.Data = map[string][]byte{}
+		}
+		if secret.Data[corev1.ServiceAccountTokenKey] == nil {
+			secret.Data[corev1.ServiceAccountTokenKey] = []byte(testToken)
+		}
+		return false, secret, nil
+	})
+}
+
 func TestInstallClusterManagerRBAC(t *testing.T) {
 	ns := &corev1.Namespace{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "test",
 		},
 	}
-	secret := &corev1.Secret{
+	legacyAutoSecret := &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "sa-secret",
 			Namespace: "test",
@@ -129,25 +152,39 @@ func TestInstallClusterManagerRBAC(t *testing.T) {
 		},
 		Secrets: []corev1.ObjectReference{
 			{
-				Kind:            secret.GetObjectKind().GroupVersionKind().Kind,
-				APIVersion:      secret.APIVersion,
-				Name:            secret.GetName(),
-				Namespace:       secret.GetNamespace(),
-				UID:             secret.GetUID(),
-				ResourceVersion: secret.GetResourceVersion(),
+				Kind:            legacyAutoSecret.GetObjectKind().GroupVersionKind().Kind,
+				APIVersion:      legacyAutoSecret.APIVersion,
+				Name:            legacyAutoSecret.GetName(),
+				Namespace:       legacyAutoSecret.GetNamespace(),
+				UID:             legacyAutoSecret.GetUID(),
+				ResourceVersion: legacyAutoSecret.GetResourceVersion(),
 			},
 		},
 	}
+	longLivedSecret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      sa.Name + SATokenSecretSuffix,
+			Namespace: "test",
+			Annotations: map[string]string{
+				corev1.ServiceAccountNameKey: sa.Name,
+			},
+		},
+		Type: corev1.SecretTypeServiceAccountToken,
+		Data: map[string][]byte{
+			"token": []byte("barfoo"),
+		},
+	}
 
 	t.Run("Cluster Scope - Success", func(t *testing.T) {
-		cs := fake.NewClientset(ns, secret, sa)
+		cs := fake.NewClientset(ns, legacyAutoSecret, sa)
+		cs.PrependReactor("create", "secrets", _MockK8STokenController(cs.Tracker()))
 		token, err := InstallClusterManagerRBAC(cs, "test", nil, testBearerTokenTimeout)
 		require.NoError(t, err)
-		assert.Equal(t, "foobar", token)
+		assert.Equal(t, testToken, token)
 	})
 
 	t.Run("Cluster Scope - Missing data in secret", func(t *testing.T) {
-		nsecret := secret.DeepCopy()
+		nsecret := legacyAutoSecret.DeepCopy()
 		nsecret.Data = make(map[string][]byte)
 		cs := fake.NewClientset(ns, nsecret, sa)
 		token, err := InstallClusterManagerRBAC(cs, "test", nil, testBearerTokenTimeout)
@@ -156,14 +193,15 @@ func TestInstallClusterManagerRBAC(t *testing.T) {
 	})
 
 	t.Run("Namespace Scope - Success", func(t *testing.T) {
-		cs := fake.NewClientset(ns, secret, sa)
+		cs := fake.NewClientset(ns, sa, longLivedSecret)
+		cs.PrependReactor("create", "secrets", _MockK8STokenController(cs.Tracker()))
 		token, err := InstallClusterManagerRBAC(cs, "test", []string{"nsa"}, testBearerTokenTimeout)
 		require.NoError(t, err)
-		assert.Equal(t, "foobar", token)
+		assert.Equal(t, "barfoo", token)
 	})
 
 	t.Run("Namespace Scope - Missing data in secret", func(t *testing.T) {
-		nsecret := secret.DeepCopy()
+		nsecret := legacyAutoSecret.DeepCopy()
 		nsecret.Data = make(map[string][]byte)
 		cs := fake.NewClientset(ns, nsecret, sa)
 		token, err := InstallClusterManagerRBAC(cs, "test", []string{"nsa"}, testBearerTokenTimeout)
@@ -242,10 +280,6 @@ func TestGetServiceAccountBearerToken(t *testing.T) {
 			Name:      dockercfgSecret.Name,
 			Namespace: dockercfgSecret.Namespace,
 		},
-		{
-			Name:      tokenSecret.Name,
-			Namespace: tokenSecret.Namespace,
-		},
 	}
 	kubeclientset := fake.NewClientset(sa, dockercfgSecret, tokenSecret)
 
@@ -260,40 +294,60 @@ func Test_getOrCreateServiceAccountTokenSecret_NoSecretForSA(t *testing.T) {
 			Name: "kube-system",
 		},
 	}
-	saWithoutSecret := &corev1.ServiceAccount{
+	sa := &corev1.ServiceAccount{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      ArgoCDManagerServiceAccount,
 			Namespace: ns.Name,
 		},
 	}
-	cs := fake.NewClientset(ns, saWithoutSecret)
-	cs.PrependReactor("create", "secrets",
-		func(a kubetesting.Action) (handled bool, ret runtime.Object, err error) {
-			s, ok := a.(kubetesting.CreateAction).GetObject().(*corev1.Secret)
-			if !ok {
-				return
-			}
+	manualSecret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      ArgoCDManagerServiceAccount + SATokenSecretSuffix,
+			Namespace: ns.Name,
+			Annotations: map[string]string{
+				corev1.ServiceAccountNameKey: sa.Name,
+			},
+		},
+		Type: corev1.SecretTypeServiceAccountToken,
+	}
 
-			if s.Name == "" && s.GenerateName != "" {
-				s.SetName(names.SimpleNameGenerator.GenerateName(s.GenerateName))
-			}
+	assertOnlyOneTokenExists := func(t *testing.T, cs *fake.Clientset) {
+		t.Helper()
+		got, err := getOrCreateServiceAccountTokenSecret(cs, ArgoCDManagerServiceAccount, ns.Name)
+		require.NoError(t, err)
+		assert.Equal(t, ArgoCDManagerServiceAccount+SATokenSecretSuffix, got)
 
-			s.Data = make(map[string][]byte)
-			s.Data["token"] = []byte("fake-token")
+		list, err := cs.Tracker().List(schema.GroupVersionResource{Version: "v1", Resource: "secrets"},
+			schema.GroupVersionKind{Version: "v1", Kind: "Secret"}, ns.Name, metav1.ListOptions{})
+		require.NoError(t, err)
+		secretList, ok := list.(*corev1.SecretList)
+		require.True(t, ok)
+		assert.Len(t, secretList.Items, 1)
+		obj, err := cs.Tracker().Get(schema.GroupVersionResource{Version: "v1", Resource: "serviceaccounts"},
+			ns.Name, ArgoCDManagerServiceAccount)
+		require.NoError(t, err, "ServiceAccount %s not found but was expected to be found", ArgoCDManagerServiceAccount)
 
-			return
+		assert.Empty(t, obj.(*corev1.ServiceAccount).Secrets, 0)
+	}
+	t.Run("Token secret exists", func(t *testing.T) {
+		cs := fake.NewClientset(ns, sa, manualSecret)
+		assertOnlyOneTokenExists(t, cs)
+	})
+
+	t.Run("Token secret does not exist", func(t *testing.T) {
+		cs := fake.NewClientset(ns, sa)
+		assertOnlyOneTokenExists(t, cs)
+	})
+
+	t.Run("Error on secret creation", func(t *testing.T) {
+		cs := fake.NewClientset(ns, sa)
+		cs.PrependReactor("create", "secrets", func(kubetesting.Action) (handled bool, ret runtime.Object, err error) {
+			return true, &corev1.Secret{}, errors.New("testing error case")
 		})
-
-	got, err := getOrCreateServiceAccountTokenSecret(cs, ArgoCDManagerServiceAccount, ns.Name)
-	require.NoError(t, err)
-	assert.Contains(t, got, "argocd-manager-token-")
-
-	obj, err := cs.Tracker().Get(schema.GroupVersionResource{Version: "v1", Resource: "serviceaccounts"},
-		ns.Name, ArgoCDManagerServiceAccount)
-	require.NoError(t, err, "ServiceAccount %s not found but was expected to be found", ArgoCDManagerServiceAccount)
-
-	sa := obj.(*corev1.ServiceAccount)
-	assert.Len(t, sa.Secrets, 1)
+		got, err := getOrCreateServiceAccountTokenSecret(cs, ArgoCDManagerServiceAccount, ns.Name)
+		require.Error(t, err)
+		assert.Empty(t, got)
+	})
 }
 
 func Test_getOrCreateServiceAccountTokenSecret_SAHasSecret(t *testing.T) {
@@ -335,7 +389,7 @@ func Test_getOrCreateServiceAccountTokenSecret_SAHasSecret(t *testing.T) {
 
 	got, err := getOrCreateServiceAccountTokenSecret(cs, ArgoCDManagerServiceAccount, ns.Name)
 	require.NoError(t, err)
-	assert.Equal(t, "sa-secret", got)
+	assert.Equal(t, ArgoCDManagerServiceAccount+SATokenSecretSuffix, got)
 
 	obj, err := cs.Tracker().Get(schema.GroupVersionResource{Version: "v1", Resource: "serviceaccounts"},
 		ns.Name, ArgoCDManagerServiceAccount)

util/clusterauth/testdata/argocd-manager-sa-token.yaml

@@ -9,10 +9,9 @@ metadata:
     kubernetes.io/service-account.name: argocd-manager
     kubernetes.io/service-account.uid: 91dd37cf-8d92-11e9-a091-d65f2ae7fa8d
   creationTimestamp: "2019-06-13T04:30:24Z"
-  generateName: argocd-manager-token-
-  name: argocd-manager-token-tj79r
+  name: argocd-manager-long-lived-token
   namespace: kube-system
   resourceVersion: "133010"
   selfLink: /api/v1/namespaces/kube-system/secrets/argocd-manager-token-tj79r
   uid: f657d67e-8d93-11e9-a091-d65f2ae7fa8d
-type: kubernetes.io/service-account-token
+type: kubernetes.io/service-account-token

util/clusterauth/testdata/argocd-manager-sa.yaml

@@ -7,5 +7,3 @@ metadata:
   resourceVersion: "133015"
   selfLink: /api/v1/namespaces/kube-system/serviceaccounts/argocd-manager
   uid: 91dd37cf-8d92-11e9-a091-d65f2ae7fa8d
-secrets:
-- name: argocd-manager-token-tj79r