diff --git a/pkg/agent/cluster/cluster.go b/pkg/agent/cluster/cluster.go index 5d6d4ac9dd1..89ce4ec7e25 100644 --- a/pkg/agent/cluster/cluster.go +++ b/pkg/agent/cluster/cluster.go @@ -78,7 +78,7 @@ func getTokenFromAPI() ([]byte, []byte, error) { } return secret.Data[coreV1.ServiceAccountRootCAKey], secret.Data[coreV1.ServiceAccountTokenKey], nil } - secret, err := serviceaccounttoken.EnsureSecretForServiceAccount(context.Background(), nil, k8s, sa) + secret, err := serviceaccounttoken.EnsureSecretForServiceAccount(context.Background(), nil, k8s, sa, "") if err != nil { return nil, nil, fmt.Errorf("failed to ensure secret for service account %s/%s: %w", namespace.System, "cattle", err) } diff --git a/pkg/capr/common.go b/pkg/capr/common.go index 6376870ad89..03951dcf916 100644 --- a/pkg/capr/common.go +++ b/pkg/capr/common.go @@ -25,6 +25,7 @@ import ( capicontrollers "github.com/rancher/rancher/pkg/generated/controllers/cluster.x-k8s.io/v1beta1" rkecontroller "github.com/rancher/rancher/pkg/generated/controllers/rke.cattle.io/v1" "github.com/rancher/rancher/pkg/serviceaccounttoken" + "github.com/rancher/rancher/pkg/utils" "github.com/rancher/wrangler/v3/pkg/condition" "github.com/rancher/wrangler/v3/pkg/data" corecontrollers "github.com/rancher/wrangler/v3/pkg/generated/controllers/core/v1" @@ -322,7 +323,7 @@ func GetPlanServiceAccountTokenSecret(secretClient corecontrollers.SecretControl if planSA == nil { return nil, false, fmt.Errorf("planSA was nil") } - secret, err := serviceaccounttoken.EnsureSecretForServiceAccount(context.Background(), secretClient.Cache(), k8s, planSA) + secret, err := serviceaccounttoken.EnsureSecretForServiceAccount(context.Background(), secretClient.Cache(), k8s, planSA, utils.FormatPrefix("local")) if err != nil { return nil, false, fmt.Errorf("error ensuring secret for service account [%s:%s]: %w", planSA.Namespace, planSA.Name, err) } diff --git a/pkg/controllers/capr/bootstrap/controller.go b/pkg/controllers/capr/bootstrap/controller.go index 53a42b2a3ff..4835b46458a 100644 --- a/pkg/controllers/capr/bootstrap/controller.go +++ b/pkg/controllers/capr/bootstrap/controller.go @@ -18,6 +18,7 @@ import ( "github.com/rancher/rancher/pkg/namespace" "github.com/rancher/rancher/pkg/serviceaccounttoken" "github.com/rancher/rancher/pkg/tls" + "github.com/rancher/rancher/pkg/utils" "github.com/rancher/rancher/pkg/wrangler" appcontrollers "github.com/rancher/wrangler/v3/pkg/generated/controllers/apps/v1" corecontrollers "github.com/rancher/wrangler/v3/pkg/generated/controllers/core/v1" @@ -120,7 +121,7 @@ func (h *handler) getBootstrapSecret(namespace, name string, envVars []corev1.En if err != nil { return nil, err } - secret, err := serviceaccounttoken.EnsureSecretForServiceAccount(context.Background(), h.secretCache, h.k8s, sa) + secret, err := serviceaccounttoken.EnsureSecretForServiceAccount(context.Background(), h.secretCache, h.k8s, sa, utils.FormatPrefix("local")) if err != nil { return nil, err } diff --git a/pkg/controllers/dashboard/apiservice/apiservice.go b/pkg/controllers/dashboard/apiservice/apiservice.go index aa3fe2c8ee1..e30e276c3eb 100644 --- a/pkg/controllers/dashboard/apiservice/apiservice.go +++ b/pkg/controllers/dashboard/apiservice/apiservice.go @@ -11,6 +11,7 @@ import ( "github.com/rancher/rancher/pkg/namespace" "github.com/rancher/rancher/pkg/serviceaccounttoken" "github.com/rancher/rancher/pkg/settings" + "github.com/rancher/rancher/pkg/utils" "github.com/rancher/rancher/pkg/wrangler" appscontrollers "github.com/rancher/wrangler/v3/pkg/generated/controllers/apps/v1" corev1controllers "github.com/rancher/wrangler/v3/pkg/generated/controllers/core/v1" @@ -148,7 +149,7 @@ func (h *handler) getToken(sa *corev1.ServiceAccount) (string, error) { } // create a secret-based token for the service account if one does not exist - secret, err := serviceaccounttoken.EnsureSecretForServiceAccount(h.ctx, h.secretsCache, h.k8s, sa) + secret, err := serviceaccounttoken.EnsureSecretForServiceAccount(h.ctx, h.secretsCache, h.k8s, sa, utils.FormatPrefix("local")) if err != nil { return "", fmt.Errorf("error ensuring secret for service account [%s:%s]: %w", sa.Namespace, sa.Name, err) } diff --git a/pkg/controllers/management/aks/aks_cluster_handler.go b/pkg/controllers/management/aks/aks_cluster_handler.go index 91ec5869cfb..7872fae8e3c 100644 --- a/pkg/controllers/management/aks/aks_cluster_handler.go +++ b/pkg/controllers/management/aks/aks_cluster_handler.go @@ -341,7 +341,7 @@ func (e *aksOperatorController) generateAndSetServiceAccount(cluster *apimgmtv3. } restConfig.Dial = clusterDialer - saToken, err := clusteroperator.GenerateSAToken(restConfig) + saToken, err := clusteroperator.GenerateSAToken(restConfig, cluster.Name) if err != nil { return cluster, fmt.Errorf("error generating service account token: %v", err) } @@ -422,7 +422,7 @@ func (e *aksOperatorController) generateSATokenWithPublicAPI(cluster *apimgmtv3. Timeout: 30 * time.Second, KeepAlive: 30 * time.Second, }).DialContext - serviceToken, err := clusteroperator.GenerateSAToken(restConfig) + serviceToken, err := clusteroperator.GenerateSAToken(restConfig, cluster.Name) if err != nil { *requiresTunnel = true var dnsError *net.DNSError diff --git a/pkg/controllers/management/clusteroperator/utils.go b/pkg/controllers/management/clusteroperator/utils.go index 2181daba1d2..7742ccacfd4 100644 --- a/pkg/controllers/management/clusteroperator/utils.go +++ b/pkg/controllers/management/clusteroperator/utils.go @@ -166,13 +166,13 @@ func (e *OperatorController) CheckCrdReady(cluster *mgmtv3.Cluster, clusterType return cluster, nil } -func GenerateSAToken(restConfig *rest.Config) (string, error) { +func GenerateSAToken(restConfig *rest.Config, clusterName string) (string, error) { clientSet, err := kubernetes.NewForConfig(restConfig) if err != nil { - return "", fmt.Errorf("error creating clientset: %v", err) + return "", fmt.Errorf("error creating clientset for cluster %s: %v", clusterName, err) } - return util.GenerateServiceAccountToken(clientSet) + return util.GenerateServiceAccountToken(clientSet, clusterName) } func addAdditionalCA(secretsCache wranglerv1.SecretCache, caCert string) (string, error) { diff --git a/pkg/controllers/management/eks/eks_cluster_handler.go b/pkg/controllers/management/eks/eks_cluster_handler.go index 5a74ad0e770..1c4c9317152 100644 --- a/pkg/controllers/management/eks/eks_cluster_handler.go +++ b/pkg/controllers/management/eks/eks_cluster_handler.go @@ -449,7 +449,7 @@ func (e *eksOperatorController) generateAndSetServiceAccount(cluster *mgmtv3.Clu return cluster, err } - saToken, err := clusteroperator.GenerateSAToken(restConfig) + saToken, err := clusteroperator.GenerateSAToken(restConfig, cluster.Name) if err != nil { return cluster, err } @@ -529,7 +529,7 @@ func (e *eksOperatorController) generateSATokenWithPublicAPI(cluster *mgmtv3.Clu } requiresTunnel := new(bool) - serviceToken, err := clusteroperator.GenerateSAToken(restConfig) + serviceToken, err := clusteroperator.GenerateSAToken(restConfig, cluster.Name) if err != nil { *requiresTunnel = true var dnsError *net.DNSError diff --git a/pkg/controllers/management/gke/gke_cluster_handler.go b/pkg/controllers/management/gke/gke_cluster_handler.go index 9a3947b6227..cd19c2baea5 100644 --- a/pkg/controllers/management/gke/gke_cluster_handler.go +++ b/pkg/controllers/management/gke/gke_cluster_handler.go @@ -365,7 +365,7 @@ func (e *gkeOperatorController) generateAndSetServiceAccount(cluster *mgmtv3.Clu return cluster, err } - saToken, err := clusteroperator.GenerateSAToken(restConfig) + saToken, err := clusteroperator.GenerateSAToken(restConfig, cluster.Name) if err != nil { return cluster, fmt.Errorf("error generating service account token: %w", err) } @@ -444,7 +444,7 @@ func (e *gkeOperatorController) generateSATokenWithPublicAPI(cluster *mgmtv3.Clu return "", nil, err } requiresTunnel := new(bool) - serviceToken, err := clusteroperator.GenerateSAToken(restConfig) + serviceToken, err := clusteroperator.GenerateSAToken(restConfig, cluster.Name) if err != nil { *requiresTunnel = true if strings.Contains(err.Error(), "dial tcp") { diff --git a/pkg/impersonation/impersonation.go b/pkg/impersonation/impersonation.go index 231cd346298..b7ba63f6a92 100644 --- a/pkg/impersonation/impersonation.go +++ b/pkg/impersonation/impersonation.go @@ -12,6 +12,7 @@ import ( v3 "github.com/rancher/rancher/pkg/generated/norman/management.cattle.io/v3" "github.com/rancher/rancher/pkg/serviceaccounttoken" "github.com/rancher/rancher/pkg/types/config" + "github.com/rancher/rancher/pkg/utils" corecontrollers "github.com/rancher/wrangler/v3/pkg/generated/controllers/core/v1" "github.com/sirupsen/logrus" corev1 "k8s.io/api/core/v1" @@ -104,7 +105,7 @@ func (i *Impersonator) SetUpImpersonation() (*corev1.ServiceAccount, error) { // GetToken accepts a service account and returns the service account's token. func (i *Impersonator) GetToken(sa *corev1.ServiceAccount) (string, error) { - secret, err := serviceaccounttoken.EnsureSecretForServiceAccount(context.Background(), i.secretsCache, i.clusterContext.K8sClient, sa) + secret, err := serviceaccounttoken.EnsureSecretForServiceAccount(context.Background(), i.secretsCache, i.clusterContext.K8sClient, sa, utils.FormatPrefix(i.clusterContext.ClusterName)) if err != nil { return "", fmt.Errorf("error getting secret: %w", err) } @@ -170,7 +171,7 @@ func (i *Impersonator) createServiceAccount(role *rbacv1.ClusterRole) (*corev1.S } } // create secret for service account if it was not automatically generated - _, err = serviceaccounttoken.EnsureSecretForServiceAccount(context.Background(), i.secretsCache, i.clusterContext.K8sClient, sa) + _, err = serviceaccounttoken.EnsureSecretForServiceAccount(context.Background(), i.secretsCache, i.clusterContext.K8sClient, sa, utils.FormatPrefix(i.clusterContext.ClusterName)) if err != nil { return nil, fmt.Errorf("impersonation: error ensuring secret for service account %s: %w", name, err) } diff --git a/pkg/kontainer-engine/drivers/aks/aks_driver.go b/pkg/kontainer-engine/drivers/aks/aks_driver.go index 5e61cc6af72..63b7f7f7978 100644 --- a/pkg/kontainer-engine/drivers/aks/aks_driver.go +++ b/pkg/kontainer-engine/drivers/aks/aks_driver.go @@ -1304,7 +1304,7 @@ func (d *Driver) PostCheck(ctx context.Context, info *types.ClusterInfo) (*types failureCount := 0 for { - info.ServiceAccountToken, err = util.GenerateServiceAccountToken(clientset) + info.ServiceAccountToken, err = util.GenerateServiceAccountToken(clientset, "") if err == nil { logrus.Info("[azurekubernetesservice] service account token generated successfully") diff --git a/pkg/kontainer-engine/drivers/eks/eks_driver.go b/pkg/kontainer-engine/drivers/eks/eks_driver.go index e09ec25bf3d..22c6a34d629 100644 --- a/pkg/kontainer-engine/drivers/eks/eks_driver.go +++ b/pkg/kontainer-engine/drivers/eks/eks_driver.go @@ -1175,7 +1175,7 @@ func (d *Driver) PostCheck(ctx context.Context, info *types.ClusterInfo) (*types logrus.Infof("[amazonelasticcontainerservice] Generating service account token") - info.ServiceAccountToken, err = util.GenerateServiceAccountToken(clientset) + info.ServiceAccountToken, err = util.GenerateServiceAccountToken(clientset, "") if err != nil { return nil, fmt.Errorf("error generating service account token: %v", err) } diff --git a/pkg/kontainer-engine/drivers/import/import_driver.go b/pkg/kontainer-engine/drivers/import/import_driver.go index 2492f9115da..d97ccfba1b8 100644 --- a/pkg/kontainer-engine/drivers/import/import_driver.go +++ b/pkg/kontainer-engine/drivers/import/import_driver.go @@ -163,7 +163,7 @@ func (d *Driver) PostCheck(ctx context.Context, info *types.ClusterInfo) (*types return nil, fmt.Errorf("failed to get Kubernetes server version: %v", err) } - info.ServiceAccountToken, err = util.GenerateServiceAccountToken(clientset) + info.ServiceAccountToken, err = util.GenerateServiceAccountToken(clientset, "") if err != nil { return nil, err diff --git a/pkg/kontainer-engine/drivers/rke/rke_driver.go b/pkg/kontainer-engine/drivers/rke/rke_driver.go index 121ce98ea40..b60c6706708 100644 --- a/pkg/kontainer-engine/drivers/rke/rke_driver.go +++ b/pkg/kontainer-engine/drivers/rke/rke_driver.go @@ -300,7 +300,7 @@ func (d *Driver) PostCheck(ctx context.Context, info *types.ClusterInfo) (*types continue } - token, err := util.GenerateServiceAccountToken(clientset) + token, err := util.GenerateServiceAccountToken(clientset, "") if err != nil { lastErr = err time.Sleep(2 * time.Second) diff --git a/pkg/kontainer-engine/drivers/util/utils.go b/pkg/kontainer-engine/drivers/util/utils.go index a478dc65d74..4db96bca6c9 100644 --- a/pkg/kontainer-engine/drivers/util/utils.go +++ b/pkg/kontainer-engine/drivers/util/utils.go @@ -5,6 +5,7 @@ import ( "fmt" "github.com/rancher/rancher/pkg/serviceaccounttoken" + "github.com/rancher/rancher/pkg/utils" rketypes "github.com/rancher/rke/types" "gopkg.in/yaml.v2" v1 "k8s.io/api/core/v1" @@ -25,7 +26,7 @@ const ( ) // GenerateServiceAccountToken generate a serviceAccountToken for clusterAdmin given a rest clientset -func GenerateServiceAccountToken(clientset kubernetes.Interface) (string, error) { +func GenerateServiceAccountToken(clientset kubernetes.Interface, clusterName string) (string, error) { _, err := clientset.CoreV1().Namespaces().Create(context.TODO(), &v1.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: cattleNamespace, @@ -95,7 +96,7 @@ func GenerateServiceAccountToken(clientset kubernetes.Interface) (string, error) if serviceAccount, err = clientset.CoreV1().ServiceAccounts(cattleNamespace).Get(context.Background(), serviceAccount.Name, metav1.GetOptions{}); err != nil { return "", fmt.Errorf("error getting service account: %w", err) } - secret, err := serviceaccounttoken.EnsureSecretForServiceAccount(context.Background(), nil, clientset, serviceAccount) + secret, err := serviceaccounttoken.EnsureSecretForServiceAccount(context.Background(), nil, clientset, serviceAccount, utils.FormatPrefix(clusterName)) if err != nil { return "", fmt.Errorf("error ensuring secret for service account: %w", err) } diff --git a/pkg/serviceaccounttoken/secret.go b/pkg/serviceaccounttoken/secret.go index b9f461f2ed5..80a6028f3ae 100644 --- a/pkg/serviceaccounttoken/secret.go +++ b/pkg/serviceaccounttoken/secret.go @@ -3,11 +3,12 @@ package serviceaccounttoken import ( "context" "fmt" + "sync" "time" corecontrollers "github.com/rancher/wrangler/v3/pkg/generated/controllers/core/v1" "github.com/sirupsen/logrus" - v1 "k8s.io/api/core/v1" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/wait" @@ -25,74 +26,104 @@ const ( // secretLister is an abstraction over any kind of secret lister. // The caller can use any cache or client it has available, whether that is from norman, wrangler, or client-go, // as long as it can wrap it in a simplified lambda with this signature. -type secretLister func(namespace string, selector labels.Selector) ([]*v1.Secret, error) +type secretLister func(namespace string, selector labels.Selector) ([]*corev1.Secret, error) + +var lockMap sync.Map + +func getLock(key string) *sync.Mutex { + actual, _ := lockMap.LoadOrStore(key, &sync.Mutex{}) + return actual.(*sync.Mutex) +} // EnsureSecretForServiceAccount gets or creates a service account token Secret for the provided Service Account. -// For k8s <1.24, the secret is automatically generated for the service account. For >=1.24, we need to generate it explicitly. -func EnsureSecretForServiceAccount(ctx context.Context, secretsCache corecontrollers.SecretCache, clientSet kubernetes.Interface, sa *v1.ServiceAccount) (*v1.Secret, error) { +// +// If lockPrefix is provided, this is used as a prefix for the lock to provide +// context for concurrent requests. No concurrent creates will be allowed for a +// service account. +// +// The lockPrefix should be of the same format as a generateName e.g. cluster- +// +// For example, if this is "cluster-1" and the ServiceAccount is +// "default:test-sa" subsequent requests to create a ServiceAccount with the +// same name in "cluster-1" will be blocked until the first request has +// completed. +// +// Note: This mutex is per-replica, currently there's no attempt to co-ordinate +// across replicas. +func EnsureSecretForServiceAccount(ctx context.Context, secretsCache corecontrollers.SecretCache, clientSet kubernetes.Interface, sa *corev1.ServiceAccount, lockPrefix string) (*corev1.Secret, error) { if sa == nil { return nil, fmt.Errorf("could not ensure secret for invalid service account") } + logrus.Tracef("EnsureSecretForServiceAccount for %s:%s", sa.Namespace, sa.Name) + + lockKey := fmt.Sprintf("%v%v-%v", lockPrefix, sa.Namespace, sa.Name) secretClient := clientSet.CoreV1().Secrets(sa.Namespace) var secretLister secretLister if secretsCache != nil { secretLister = secretsCache.List } else { - secretLister = func(_ string, selector labels.Selector) ([]*v1.Secret, error) { + secretLister = func(_ string, selector labels.Selector) ([]*corev1.Secret, error) { secretList, err := secretClient.List(ctx, metav1.ListOptions{ LabelSelector: selector.String(), }) if err != nil { return nil, err } - result := make([]*v1.Secret, len(secretList.Items)) + result := make([]*corev1.Secret, len(secretList.Items)) for i := range secretList.Items { result[i] = &secretList.Items[i] } return result, nil } } + secret, err := ServiceAccountSecret(ctx, sa, secretLister, secretClient) if err != nil { return nil, fmt.Errorf("error looking up secret for service account [%s:%s]: %w", sa.Namespace, sa.Name, err) } + if secret == nil { - sc := SecretTemplate(sa) - secret, err = secretClient.Create(ctx, sc, metav1.CreateOptions{}) + secret, err = createServiceAccountSecret(ctx, sa, secretLister, secretClient, lockKey) if err != nil { - return nil, fmt.Errorf("error ensuring secret for service account [%s:%s]: %w", sa.Namespace, sa.Name, err) + return nil, err } } - if len(secret.Data[v1.ServiceAccountTokenKey]) > 0 { + + if len(secret.Data[corev1.ServiceAccountTokenKey]) > 0 { return secret, nil } - logrus.Infof("EnsureSecretForServiceAccount: waiting for secret [%s] to be populated with token", secret.Name) + + logrus.Infof("EnsureSecretForServiceAccount: waiting for secret [%s:%s] for service account [%s:%s] to be populated with token", secret.Namespace, secret.Name, sa.Namespace, sa.Name) backoff := wait.Backoff{ Duration: 2 * time.Millisecond, Cap: 100 * time.Millisecond, Steps: 50, } + start := time.Now() err = wait.ExponentialBackoff(backoff, func() (bool, error) { + logrus.Tracef("Waiting for the secret with backoff for %s/%s", sa.GetNamespace(), sa.GetName()) var err error // use the secret client, rather than the secret getter, to circumvent the cache secret, err = secretClient.Get(ctx, secret.Name, metav1.GetOptions{}) if err != nil { return false, fmt.Errorf("error ensuring secret for service account [%s:%s]: %w", sa.Namespace, sa.Name, err) } - if len(secret.Data[v1.ServiceAccountTokenKey]) > 0 { + if len(secret.Data[corev1.ServiceAccountTokenKey]) > 0 { + logrus.Infof("EnsureSecretForServiceAccount: got the service account token for service account [%s:%s] in %s %s ", sa.GetNamespace(), sa.GetName(), time.Now().Sub(start), lockPrefix) return true, nil } return false, nil }) if err != nil { - return nil, fmt.Errorf("error ensuring secret for service account [%s:%s]: %w", sa.Namespace, sa.Name, err) + return nil, err // err is already wapped inside the Wait. } + return secret, nil } // SecretTemplate generate a template of service-account-token Secret for the provided Service Account. -func SecretTemplate(sa *v1.ServiceAccount) *v1.Secret { - return &v1.Secret{ +func SecretTemplate(sa *corev1.ServiceAccount) *corev1.Secret { + return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ GenerateName: serviceAccountSecretPrefix(sa), Namespace: sa.Namespace, @@ -111,33 +142,38 @@ func SecretTemplate(sa *v1.ServiceAccount) *v1.Secret { ServiceAccountSecretLabel: sa.Name, }, }, - Type: v1.SecretTypeServiceAccountToken, + Type: corev1.SecretTypeServiceAccountToken, } } // serviceAccountSecretPrefix returns the prefix that will be used to generate the secret for the given service account. -func serviceAccountSecretPrefix(sa *v1.ServiceAccount) string { +func serviceAccountSecretPrefix(sa *corev1.ServiceAccount) string { return fmt.Sprintf("%s-token-", sa.Name) } // ServiceAccountSecret returns the secret for the given Service Account. // If there are more than one, it returns the first. Can return a nil secret // and a nil error if no secret is found -func ServiceAccountSecret(ctx context.Context, sa *v1.ServiceAccount, secretLister secretLister, secretClient clientv1.SecretInterface) (*v1.Secret, error) { +func ServiceAccountSecret(ctx context.Context, sa *corev1.ServiceAccount, secretLister secretLister, secretClient clientv1.SecretInterface) (*corev1.Secret, error) { if sa == nil { return nil, fmt.Errorf("cannot get secret for nil service account") } + secrets, err := secretLister(sa.Namespace, labels.SelectorFromSet(map[string]string{ ServiceAccountSecretLabel: sa.Name, })) if err != nil { return nil, fmt.Errorf("could not get secrets for service account: %w", err) } + if len(secrets) < 1 { return nil, nil } - var result *v1.Secret + + var result *corev1.Secret + // There is an issue here - multiple calls could result in multiple attempts + // to delete secrets while the secret deletion is ongoing. for _, s := range secrets { if isSecretForServiceAccount(s, sa) { if result == nil { @@ -145,6 +181,7 @@ func ServiceAccountSecret(ctx context.Context, sa *v1.ServiceAccount, secretList } continue } + logrus.Warnf("EnsureSecretForServiceAccount: secret [%s:%s] is invalid for service account [%s], deleting", s.Namespace, s.Name, sa.Name) err = secretClient.Delete(ctx, s.Name, metav1.DeleteOptions{}) if err != nil { @@ -153,14 +190,43 @@ func ServiceAccountSecret(ctx context.Context, sa *v1.ServiceAccount, secretList logrus.Errorf("unable to delete secret [%s:%s]: %v", s.Namespace, s.Name, err) } } + return result, nil } -func isSecretForServiceAccount(secret *v1.Secret, sa *v1.ServiceAccount) bool { - if secret.Type != v1.SecretTypeServiceAccountToken { +func isSecretForServiceAccount(secret *corev1.Secret, sa *corev1.ServiceAccount) bool { + if secret.Type != corev1.SecretTypeServiceAccountToken { return false } annotations := secret.Annotations annotation := annotations[serviceAccountSecretAnnotation] + return sa.Name == annotation } + +func createServiceAccountSecret(ctx context.Context, sa *corev1.ServiceAccount, secretLister secretLister, secretClient clientv1.SecretInterface, lockKey string) (*corev1.Secret, error) { + mutex := getLock(lockKey) + mutex.Lock() + defer func(key string) { + mutex.Unlock() + lockMap.Delete(lockKey) + }(lockKey) + + // We could have been waiting for the Mutex to unlock in a parallel run of + // createServiceAccountSecret - check again for the secret existing. + secret, err := ServiceAccountSecret(ctx, sa, secretLister, secretClient) + if err != nil { + return nil, err + } + if secret != nil { + return secret, nil + } + + sc := SecretTemplate(sa) + secret, err = secretClient.Create(ctx, sc, metav1.CreateOptions{}) + if err != nil { + return nil, fmt.Errorf("error ensuring secret for service account [%s:%s]: %w", sa.Namespace, sa.Name, err) + } + + return secret, nil +} diff --git a/pkg/serviceaccounttoken/secret_test.go b/pkg/serviceaccounttoken/secret_test.go index 527ebd26b01..5874c2b4bc2 100644 --- a/pkg/serviceaccounttoken/secret_test.go +++ b/pkg/serviceaccounttoken/secret_test.go @@ -3,27 +3,30 @@ package serviceaccounttoken import ( "context" "fmt" + "sync" + "sync/atomic" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - v1 "k8s.io/api/core/v1" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/rand" "k8s.io/client-go/kubernetes/fake" k8stesting "k8s.io/client-go/testing" ) func TestEnsureSecretForServiceAccount(t *testing.T) { t.Parallel() - defaultWantSA := &v1.ServiceAccount{ + defaultWantSA := &corev1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{ Name: "test", Namespace: "default", }, } - defaultWantSecret := &v1.Secret{ + defaultWantSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "test-token-abcde", Namespace: "default", @@ -37,19 +40,19 @@ func TestEnsureSecretForServiceAccount(t *testing.T) { Data: map[string][]byte{ "token": []byte("abcde"), }, - Type: v1.SecretTypeServiceAccountToken, + Type: corev1.SecretTypeServiceAccountToken, } tests := []struct { name string - sa *v1.ServiceAccount - wantSA *v1.ServiceAccount - existingSecret *v1.Secret - wantSecret *v1.Secret + sa *corev1.ServiceAccount + wantSA *corev1.ServiceAccount + existingSecret *corev1.Secret + wantSecret *corev1.Secret wantErr bool }{ { name: "service account with no secret generates secret", - sa: &v1.ServiceAccount{ + sa: &corev1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{ Name: "test", Namespace: "default", @@ -60,14 +63,14 @@ func TestEnsureSecretForServiceAccount(t *testing.T) { }, { name: "service account with existing secret returns it", - sa: &v1.ServiceAccount{ + sa: &corev1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{ Name: "test", Namespace: "default", }, }, wantSA: defaultWantSA, - existingSecret: &v1.Secret{ + existingSecret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "test-token-abcde", Namespace: "default", @@ -81,7 +84,7 @@ func TestEnsureSecretForServiceAccount(t *testing.T) { Data: map[string][]byte{ "token": []byte("abcde"), }, - Type: v1.SecretTypeServiceAccountToken, + Type: corev1.SecretTypeServiceAccountToken, }, wantSecret: defaultWantSecret, }, @@ -91,7 +94,7 @@ func TestEnsureSecretForServiceAccount(t *testing.T) { }, { name: "service account with invalid secret is updated with new secret", - sa: &v1.ServiceAccount{ + sa: &corev1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{ Name: "test", Namespace: "default", @@ -102,14 +105,14 @@ func TestEnsureSecretForServiceAccount(t *testing.T) { }, { name: "secret of wrong type gets recreated", - sa: &v1.ServiceAccount{ + sa: &corev1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{ Name: "test", Namespace: "default", }, }, wantSA: defaultWantSA, - existingSecret: &v1.Secret{ + existingSecret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "test-token-xyz", Namespace: "default", @@ -123,20 +126,20 @@ func TestEnsureSecretForServiceAccount(t *testing.T) { Data: map[string][]byte{ "token": []byte("abcde"), }, - Type: v1.SecretTypeOpaque, + Type: corev1.SecretTypeOpaque, }, wantSecret: defaultWantSecret, }, { name: "secret for wrong service account type gets recreated", - sa: &v1.ServiceAccount{ + sa: &corev1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{ Name: "test", Namespace: "default", }, }, wantSA: defaultWantSA, - existingSecret: &v1.Secret{ + existingSecret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "test-token-xyz", Namespace: "default", @@ -150,7 +153,7 @@ func TestEnsureSecretForServiceAccount(t *testing.T) { Data: map[string][]byte{ "token": []byte("abcde"), }, - Type: v1.SecretTypeServiceAccountToken, + Type: corev1.SecretTypeServiceAccountToken, }, wantSecret: defaultWantSecret, }, @@ -171,7 +174,7 @@ func TestEnsureSecretForServiceAccount(t *testing.T) { k8sClient = fake.NewSimpleClientset(objs...) k8sClient.PrependReactor("create", "secrets", func(action k8stesting.Action) (bool, runtime.Object, error) { - ret := action.(k8stesting.CreateAction).GetObject().(*v1.Secret) + ret := action.(k8stesting.CreateAction).GetObject().(*corev1.Secret) ret.ObjectMeta.Name = ret.GenerateName + "abcde" ret.Data = map[string][]byte{ "token": []byte("abcde"), @@ -180,7 +183,7 @@ func TestEnsureSecretForServiceAccount(t *testing.T) { return true, ret, nil }, ) - got, gotErr := EnsureSecretForServiceAccount(context.Background(), nil, k8sClient, tt.sa) + got, gotErr := EnsureSecretForServiceAccount(context.Background(), nil, k8sClient, tt.sa, "") if tt.wantErr { assert.Error(t, gotErr) return @@ -199,13 +202,13 @@ func TestServiceAccountSecret(t *testing.T) { clientset *fake.Clientset fakeLister *fakeSecretLister } - baseSA := v1.ServiceAccount{ + baseSA := corev1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{ Name: "base-sa", Namespace: "test-ns", }, } - validSecret := v1.Secret{ + validSecret := corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "base-sa-secret", Namespace: "test-ns", @@ -216,9 +219,9 @@ func TestServiceAccountSecret(t *testing.T) { serviceAccountSecretAnnotation: baseSA.Name, }, }, - Type: v1.SecretTypeServiceAccountToken, + Type: corev1.SecretTypeServiceAccountToken, } - invalidSecretType := v1.Secret{ + invalidSecretType := corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "invalid-secret-type", Namespace: "test-ns", @@ -229,9 +232,9 @@ func TestServiceAccountSecret(t *testing.T) { serviceAccountSecretAnnotation: baseSA.Name, }, }, - Type: v1.SecretTypeOpaque, + Type: corev1.SecretTypeOpaque, } - invalidSecretAnnotation := v1.Secret{ + invalidSecretAnnotation := corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "invalid-secret-annotation", Namespace: "test-ns", @@ -242,13 +245,13 @@ func TestServiceAccountSecret(t *testing.T) { serviceAccountSecretAnnotation: "some-other-sa", }, }, - Type: v1.SecretTypeOpaque, + Type: corev1.SecretTypeOpaque, } tests := []struct { name string stateSetup func(testState) - inputSA *v1.ServiceAccount - wantSecret *v1.Secret + inputSA *corev1.ServiceAccount + wantSecret *corev1.Secret wantError bool }{ { @@ -268,7 +271,7 @@ func TestServiceAccountSecret(t *testing.T) { stateSetup: func(ts testState) { validSecondSecret := validSecret.DeepCopy() validSecondSecret.Name = "base-sa-secret-2" - ts.fakeLister.secrets = []*v1.Secret{&validSecret, validSecondSecret} + ts.fakeLister.secrets = []*corev1.Secret{&validSecret, validSecondSecret} }, wantError: false, wantSecret: &validSecret, @@ -277,7 +280,7 @@ func TestServiceAccountSecret(t *testing.T) { name: "test invalid secrets, none returned", inputSA: &baseSA, stateSetup: func(ts testState) { - ts.fakeLister.secrets = []*v1.Secret{&invalidSecretType, &invalidSecretAnnotation} + ts.fakeLister.secrets = []*corev1.Secret{&invalidSecretType, &invalidSecretAnnotation} ts.clientset.Tracker().Add(&invalidSecretType) ts.clientset.Tracker().Add(&invalidSecretAnnotation) }, @@ -288,7 +291,7 @@ func TestServiceAccountSecret(t *testing.T) { name: "test invalid secrets delete failure, valid still returned", inputSA: &baseSA, stateSetup: func(ts testState) { - ts.fakeLister.secrets = []*v1.Secret{&invalidSecretType, &invalidSecretAnnotation, &validSecret} + ts.fakeLister.secrets = []*corev1.Secret{&invalidSecretType, &invalidSecretAnnotation, &validSecret} ts.clientset.Tracker().Add(&invalidSecretType) // don't add the invalid annotation secret to the state, this will cause a not-found error on delete }, @@ -299,7 +302,7 @@ func TestServiceAccountSecret(t *testing.T) { name: "test valid + invalid secrets, only valid returned", inputSA: &baseSA, stateSetup: func(ts testState) { - ts.fakeLister.secrets = []*v1.Secret{&invalidSecretType, &invalidSecretAnnotation, &validSecret} + ts.fakeLister.secrets = []*corev1.Secret{&invalidSecretType, &invalidSecretAnnotation, &validSecret} ts.clientset.Tracker().Add(&invalidSecretType) ts.clientset.Tracker().Add(&invalidSecretAnnotation) }, @@ -310,7 +313,7 @@ func TestServiceAccountSecret(t *testing.T) { name: "test secret lister error", inputSA: &baseSA, stateSetup: func(ts testState) { - ts.fakeLister.secrets = []*v1.Secret{&invalidSecretType, &invalidSecretAnnotation, &validSecret} + ts.fakeLister.secrets = []*corev1.Secret{&invalidSecretType, &invalidSecretAnnotation, &validSecret} ts.fakeLister.err = fmt.Errorf("server unavailable") }, wantError: true, @@ -340,10 +343,200 @@ func TestServiceAccountSecret(t *testing.T) { } type fakeSecretLister struct { - secrets []*v1.Secret + secrets []*corev1.Secret err error } -func (f *fakeSecretLister) list(namespace string, selector labels.Selector) ([]*v1.Secret, error) { +func (f *fakeSecretLister) list(namespace string, selector labels.Selector) ([]*corev1.Secret, error) { return f.secrets, f.err } + +func TestEnsureSecretForServiceAccount_in_parallel(t *testing.T) { + managedSecrets := map[string]*corev1.Secret{ + "test-secret-1": &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-secret-1", + Namespace: "default", + Labels: map[string]string{ + ServiceAccountSecretLabel: "test", + }, + }, + Type: corev1.SecretTypeOpaque, + }, + "test-secret-2": &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-secret-2", + Namespace: "default", + Labels: map[string]string{ + ServiceAccountSecretLabel: "test", + }, + }, + Type: corev1.SecretTypeOpaque, + }, + } + + k8sClient := fake.NewSimpleClientset() + var m sync.Mutex + + k8sClient.PrependReactor("list", "secrets", + func(a k8stesting.Action) (bool, runtime.Object, error) { + m.Lock() + defer m.Unlock() + + secrets := &corev1.SecretList{} + for _, v := range managedSecrets { + secrets.Items = append(secrets.Items, *v) + } + + return true, secrets, nil + }, + ) + + k8sClient.PrependReactor("create", "secrets", + func(action k8stesting.Action) (bool, runtime.Object, error) { + m.Lock() + defer m.Unlock() + ret := action.(k8stesting.CreateAction).GetObject().(*corev1.Secret) + ret.ObjectMeta.Name = ret.GenerateName + rand.String(5) + ret.Data = map[string][]byte{ + "token": []byte("abcde"), + } + + managedSecrets[ret.ObjectMeta.Name] = ret + + return true, ret, nil + }, + ) + + k8sClient.PrependReactor("delete", "secrets", + func(action k8stesting.Action) (bool, runtime.Object, error) { + m.Lock() + defer m.Unlock() + deleteName := action.(k8stesting.DeleteAction).GetName() + delete(managedSecrets, deleteName) + + return true, nil, nil + }, + ) + + var wg sync.WaitGroup + for i := 0; i < 10; i++ { + wg.Add(1) + go func() { + defer wg.Done() + _, err := EnsureSecretForServiceAccount(context.Background(), nil, k8sClient, &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "default", + }, + }, "cluster-1-") + assert.NoError(t, err) + }() + } + + wg.Wait() + var remaining []string + lockMap.Range(func(key, value any) bool { + remaining = append(remaining, fmt.Sprintf("%v", key)) + return true + }) + assert.Empty(t, remaining) + assert.Len(t, managedSecrets, 1) +} + +func TestEnsureSecretForServiceAccount_in_parallel_avoids_deadlock(t *testing.T) { + managedSecrets := map[string]*corev1.Secret{ + "test-secret-1": &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-secret-1", + Namespace: "default", + Labels: map[string]string{ + ServiceAccountSecretLabel: "test", + }, + }, + Type: corev1.SecretTypeOpaque, + }, + "test-secret-2": &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-secret-2", + Namespace: "default", + Labels: map[string]string{ + ServiceAccountSecretLabel: "test", + }, + }, + Type: corev1.SecretTypeOpaque, + }, + } + + k8sClient := fake.NewSimpleClientset() + var m sync.Mutex + var listCount int64 + + k8sClient.PrependReactor("list", "secrets", + func(a k8stesting.Action) (bool, runtime.Object, error) { + m.Lock() + defer m.Unlock() + + secrets := &corev1.SecretList{} + if listCount > 0 { + for _, v := range managedSecrets { + secrets.Items = append(secrets.Items, *v) + } + } + + atomic.AddInt64(&listCount, 1) + return true, secrets, nil + }, + ) + + k8sClient.PrependReactor("create", "secrets", + func(action k8stesting.Action) (bool, runtime.Object, error) { + m.Lock() + defer m.Unlock() + ret := action.(k8stesting.CreateAction).GetObject().(*corev1.Secret) + ret.ObjectMeta.Name = ret.GenerateName + rand.String(5) + ret.Data = map[string][]byte{ + "token": []byte("abcde"), + } + + managedSecrets[ret.ObjectMeta.Name] = ret + + return true, ret, nil + }, + ) + + k8sClient.PrependReactor("delete", "secrets", + func(action k8stesting.Action) (bool, runtime.Object, error) { + m.Lock() + defer m.Unlock() + deleteName := action.(k8stesting.DeleteAction).GetName() + delete(managedSecrets, deleteName) + + return true, nil, nil + }, + ) + + var wg sync.WaitGroup + for i := 0; i < 10; i++ { + wg.Add(1) + go func() { + defer wg.Done() + _, err := EnsureSecretForServiceAccount(context.Background(), nil, k8sClient, &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "default", + }, + }, "cluster-1-") + assert.NoError(t, err) + }() + } + + wg.Wait() + var remaining []string + lockMap.Range(func(key, value any) bool { + remaining = append(remaining, fmt.Sprintf("%v", key)) + return true + }) + assert.Empty(t, remaining) + assert.Len(t, managedSecrets, 1) +} diff --git a/pkg/tunnelserver/peermanager.go b/pkg/tunnelserver/peermanager.go index f8afd75c9ef..61b396acbde 100644 --- a/pkg/tunnelserver/peermanager.go +++ b/pkg/tunnelserver/peermanager.go @@ -15,6 +15,7 @@ import ( "github.com/rancher/rancher/pkg/peermanager" "github.com/rancher/rancher/pkg/serviceaccounttoken" "github.com/rancher/rancher/pkg/settings" + "github.com/rancher/rancher/pkg/utils" "github.com/rancher/remotedialer" "github.com/rancher/wrangler/v3/pkg/data" corecontrollers "github.com/rancher/wrangler/v3/pkg/generated/controllers/core/v1" @@ -77,7 +78,7 @@ func getTokenFromToken(ctx context.Context, tokenBytes []byte) ([]byte, error) { return nil, err } - secret, err := serviceaccounttoken.EnsureSecretForServiceAccount(ctx, nil, client, sa) + secret, err := serviceaccounttoken.EnsureSecretForServiceAccount(ctx, nil, client, sa, utils.FormatPrefix("local")) if err != nil { return nil, err } diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index ff919fb9bb7..d61dce139f7 100644 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -17,3 +17,20 @@ func FormatResourceList(resources v1.ResourceList) string { sort.Strings(resourceStrings) return strings.Join(resourceStrings, ",") } + +// FormatPrefix converts the provided string into a form suitable for use as a +// generateName prefix. +// +// It does this by converting to lower-case and appending a "-" character. +func FormatPrefix(s string) string { + if s == "" { + return s + } + + s = strings.ToLower(s) + if !strings.HasSuffix(s, "-") { + s = s + "-" + } + + return s +} diff --git a/pkg/utils/utils_test.go b/pkg/utils/utils_test.go new file mode 100644 index 00000000000..33e71236a0c --- /dev/null +++ b/pkg/utils/utils_test.go @@ -0,0 +1,31 @@ +package utils + +import "testing" + +func TestFormatPrefix(t *testing.T) { + testStrings := []struct { + s string + want string + }{ + { + "example", "example-", + }, + { + "Test", "test-", + }, + { + "another-", "another-", + }, + { + "", "", + }, + } + + for _, tt := range testStrings { + t.Run(tt.s, func(t *testing.T) { + if v := FormatPrefix(tt.s); v != tt.want { + t.Errorf("FormatPrefix() got %v, want %v", v, tt.want) + } + }) + } +} diff --git a/tests/v2/integration/serviceaccount/serviceaccounttoken_test.go b/tests/v2/integration/serviceaccount/serviceaccounttoken_test.go new file mode 100644 index 00000000000..07808213be4 --- /dev/null +++ b/tests/v2/integration/serviceaccount/serviceaccounttoken_test.go @@ -0,0 +1,97 @@ +package integration + +import ( + "context" + "sync" + "testing" + "time" + + "github.com/rancher/rancher/pkg/serviceaccounttoken" + "github.com/rancher/shepherd/clients/rancher" + "github.com/rancher/shepherd/pkg/session" + "github.com/stretchr/testify/suite" + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/tools/clientcmd" +) + +type ServiceAccountSuite struct { + suite.Suite + client *rancher.Client + session *session.Session +} + +func (s *ServiceAccountSuite) TearDownSuite() { + s.session.Cleanup() +} + +func (s *ServiceAccountSuite) SetupSuite() { + testSession := session.NewSession() + s.session = testSession + + client, err := rancher.NewClient("", testSession) + s.Require().NoError(err) + s.client = client +} + +func (s *ServiceAccountSuite) TestSingleSecretForServiceAccount() { + localCluster, err := s.client.Management.Cluster.ByID("local") + s.Require().NoError(err) + s.Require().NotEmpty(localCluster) + localClusterKubeconfig, err := s.client.Management.Cluster.ActionGenerateKubeconfig(localCluster) + s.Require().NoError(err) + c, err := clientcmd.NewClientConfigFromBytes([]byte(localClusterKubeconfig.Config)) + s.Require().NoError(err) + cc, err := c.ClientConfig() + s.Require().NoError(err) + clientset, err := kubernetes.NewForConfig(cc) + s.Require().NoError(err) + + testNS := &v1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ns", + }, + } + testNS, err = clientset.CoreV1().Namespaces().Create(context.Background(), testNS, metav1.CreateOptions{}) + s.Require().NoError(err) + + serviceAccount := &v1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: testNS.Name, + }, + } + serviceAccount, err = clientset.CoreV1().ServiceAccounts(testNS.Name).Create(context.Background(), serviceAccount, metav1.CreateOptions{}) + s.Require().NoError(err) + + // mimic a scenario where multiple func calls for the same SA, and check the resulting Secrets + var wg sync.WaitGroup + for i := 0; i < 10; i++ { + wg.Add(1) + go func() { + defer wg.Done() + _, err := serviceaccounttoken.EnsureSecretForServiceAccount(context.Background(), nil, clientset, serviceAccount, "cluster-") + s.Require().NoError(err) + }() + } + + pollInterval := 500 * time.Millisecond + err = wait.Poll(pollInterval, 5*time.Second, func() (done bool, err error) { + secrets, err := clientset.CoreV1().Secrets(testNS.Name).List(context.Background(), metav1.ListOptions{}) + if err != nil { + return false, err + } + + return len(secrets.Items) > 0, nil + }) + + secrets, err := clientset.CoreV1().Secrets(testNS.Name).List(context.Background(), metav1.ListOptions{}) + s.Require().NoError(err) + s.Assert().Equal(1, len(secrets.Items)) +} + +func TestSATestSuite(t *testing.T) { + suite.Run(t, new(ServiceAccountSuite)) +}