From c5506e7a573b83a7c85d326d75caef371812fbde Mon Sep 17 00:00:00 2001 From: Vishal Thapar <5137689+vthapar@users.noreply.github.com> Date: Tue, 21 Jun 2022 15:37:52 +0530 Subject: [PATCH 1/3] Create secrets for serviceaccounts K8s 1.24+ doesn't autocreate secret for serviceaccount. This breaks submariner deployments which expects secret to be created and associated to the corresponding serviceaccount. The fix ensures the Secret of type service-account-token exists, is annotated with ServiceAccount.Name, and is added to Secrets list in the ServiceAccount. Fixes #102 Signed-off-by: Vishal Thapar <5137689+vthapar@users.noreply.github.com> --- pkg/broker/ensure.go | 17 +++- pkg/serviceaccount/ensure.go | 138 +++++++++++++++++++++++++++++- pkg/serviceaccount/ensure_test.go | 4 +- 3 files changed, 151 insertions(+), 8 deletions(-) diff --git a/pkg/broker/ensure.go b/pkg/broker/ensure.go index 0d7b4770f..c0380b1e6 100644 --- a/pkg/broker/ensure.go +++ b/pkg/broker/ensure.go @@ -30,6 +30,7 @@ import ( "github.com/submariner-io/subctl/pkg/gateway" "github.com/submariner-io/subctl/pkg/namespace" "github.com/submariner-io/subctl/pkg/role" + "github.com/submariner-io/subctl/pkg/serviceaccount" "github.com/submariner-io/submariner-operator/pkg/crd" "github.com/submariner-io/submariner-operator/pkg/lighthouse" "github.com/submariner-io/submariner-operator/pkg/names" @@ -204,6 +205,18 @@ func CreateNewBrokerRoleBinding(kubeClient kubernetes.Interface, serviceAccount, // nolint:wrapcheck // No need to wrap here func CreateNewBrokerSA(kubeClient kubernetes.Interface, submarinerBrokerSA, inNamespace string) (brokerSA *v1.ServiceAccount, err error) { - return kubeClient.CoreV1().ServiceAccounts(inNamespace).Create( - context.TODO(), NewBrokerSA(submarinerBrokerSA), metav1.CreateOptions{}) + sa := NewBrokerSA(submarinerBrokerSA) + _, err = serviceaccount.Ensure(kubeClient, inNamespace, sa) + + if err != nil { + return nil, err + } + + _, err = serviceaccount.EnsureSecretFromSA(kubeClient, sa.Name, inNamespace) + + if err != nil { + return nil, errors.Wrap(err, "failed to get secret for broker SA") + } + + return kubeClient.CoreV1().ServiceAccounts(inNamespace).Get(context.TODO(), sa.Name, metav1.GetOptions{}) } diff --git a/pkg/serviceaccount/ensure.go b/pkg/serviceaccount/ensure.go index 6ca69ea40..62b617137 100644 --- a/pkg/serviceaccount/ensure.go +++ b/pkg/serviceaccount/ensure.go @@ -20,28 +20,158 @@ package serviceaccount import ( "context" + "fmt" + "math/rand" + "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/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/client-go/kubernetes" ) -// EnsureFromYAML creates the given service account. +const ( + createdByAnnotation = "kubernetes.io/created-by" + creatorName = "subctl" +) + +// ensureFromYAML creates the given service account. // nolint:wrapcheck // No need to wrap errors here. -func EnsureFromYAML(kubeClient kubernetes.Interface, namespace, yaml string) (bool, error) { +func ensureFromYAML(kubeClient kubernetes.Interface, namespace, yaml string) (*corev1.ServiceAccount, error) { sa := &corev1.ServiceAccount{} err := embeddedyamls.GetObject(yaml, sa) if err != nil { - return false, err + return nil, err } - return Ensure(kubeClient, namespace, sa) + _, err = Ensure(kubeClient, namespace, sa) + if err != nil { + return nil, err + } + + return sa, err } // nolint:wrapcheck // No need to wrap errors here. func Ensure(kubeClient kubernetes.Interface, namespace string, sa *corev1.ServiceAccount) (bool, error) { return resourceutil.CreateOrUpdate(context.TODO(), resource.ForServiceAccount(kubeClient, namespace), sa) } + +// EnsureFromYAML creates the given service account and secret for it. +func EnsureFromYAML(kubeClient kubernetes.Interface, namespace, yaml string) (bool, error) { + sa, err := ensureFromYAML(kubeClient, namespace, yaml) + if err != nil { + return false, errors.Wrap(err, "error provisioning the ServiceAccount resource") + } + + saSecret, err := EnsureSecretFromSA(kubeClient, sa.Name, namespace) + if err != nil { + return false, errors.Wrap(err, "error creating secret for ServiceAccount resource") + } + + return sa != nil && saSecret != nil, nil +} + +func EnsureSecretFromSA(client kubernetes.Interface, saName, namespace string) (*corev1.Secret, error) { + sa, err := client.CoreV1().ServiceAccounts(namespace).Get(context.TODO(), saName, metav1.GetOptions{}) + if err != nil { + return nil, errors.Wrapf(err, "failed to get ServiceAccount %s/%s", namespace, saName) + } + + saSecret := getSecretFromSA(client, sa) + + if saSecret != nil { + return saSecret, nil + } + + // We couldn't find right secret from this SA, search all Secrets + saSecret, err = getSecretForSA(client, sa) + if err != nil && !apierrors.IsNotFound(err) { + return nil, err + } + + if err != nil { + newSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("%s-token-%s", sa.Name, generateRandomString(5)), + Namespace: namespace, + Annotations: map[string]string{ + corev1.ServiceAccountNameKey: saName, + createdByAnnotation: creatorName, + }, + }, + Type: corev1.SecretTypeServiceAccountToken, + } + + saSecret, err = secret.Ensure(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, + } + + sa.Secrets = append(sa.Secrets, secretRef) + _, err = Ensure(client, namespace, sa) + + if err != nil { + return nil, errors.Wrapf(err, "failed to update ServiceAccount %v with Secret reference %v", saName, secretRef.Name) + } + + return saSecret, nil +} + +func getSecretFromSA(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(context.TODO(), saSecretRef.Name, metav1.GetOptions{}) + if saSecret.Annotations[corev1.ServiceAccountNameKey] == sa.Name && saSecret.Type == corev1.SecretTypeServiceAccountToken { + return saSecret + } + } + } + + return nil +} + +func getSecretForSA(client kubernetes.Interface, sa *corev1.ServiceAccount) (*corev1.Secret, error) { + saSecrets, err := client.CoreV1().Secrets(sa.Namespace).List(context.TODO(), metav1.ListOptions{ + FieldSelector: fields.OneTermEqualSelector("type", "kubernetes.io/service-account-token").String(), + }) + 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 + } + } + + return nil, apierrors.NewNotFound(schema.GroupResource{ + Group: corev1.SchemeGroupVersion.Group, + Resource: "secrets", + }, sa.Name) +} + +// nolint:gosec // we need a pseudo random string for name. +func generateRandomString(length int) string { + rand.Seed(time.Now().UnixNano()) + + s := make([]byte, length) + rand.Read(s) + + return fmt.Sprintf("%x", s)[:length] +} diff --git a/pkg/serviceaccount/ensure_test.go b/pkg/serviceaccount/ensure_test.go index 29a6d6db2..3d90712f0 100644 --- a/pkg/serviceaccount/ensure_test.go +++ b/pkg/serviceaccount/ensure_test.go @@ -62,7 +62,7 @@ var _ = Describe("EnsureFromYAML", func() { }) When("the ServiceAccount already exists", func() { - It("should not update it", func() { + It("should not return any error", func() { _, err := serviceaccount.Ensure(client, namespace, &corev1.ServiceAccount{ TypeMeta: metav1.TypeMeta{ Kind: "ServiceAccount", @@ -76,7 +76,7 @@ var _ = Describe("EnsureFromYAML", func() { assertServiceAccount() created, err := serviceaccount.EnsureFromYAML(client, namespace, roleYAML) - Expect(created).To(BeFalse()) + Expect(created).To(BeTrue()) Expect(err).To(Succeed()) }) }) From 9b88e19d692dd4e4cbe5dca440fd1c54d78d0f62 Mon Sep 17 00:00:00 2001 From: Vishal Thapar <5137689+vthapar@users.noreply.github.com> Date: Mon, 27 Jun 2022 19:33:31 +0530 Subject: [PATCH 2/3] Fix issue with duplicate secret Signed-off-by: Vishal Thapar <5137689+vthapar@users.noreply.github.com> --- pkg/broker/ensure.go | 2 +- pkg/serviceaccount/ensure.go | 13 ++++++++++--- pkg/serviceaccount/ensure_test.go | 2 +- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/pkg/broker/ensure.go b/pkg/broker/ensure.go index c0380b1e6..5fc60e391 100644 --- a/pkg/broker/ensure.go +++ b/pkg/broker/ensure.go @@ -206,7 +206,7 @@ func CreateNewBrokerRoleBinding(kubeClient kubernetes.Interface, serviceAccount, // nolint:wrapcheck // No need to wrap here func CreateNewBrokerSA(kubeClient kubernetes.Interface, submarinerBrokerSA, inNamespace string) (brokerSA *v1.ServiceAccount, err error) { sa := NewBrokerSA(submarinerBrokerSA) - _, err = serviceaccount.Ensure(kubeClient, inNamespace, sa) + _, err = serviceaccount.Ensure(kubeClient, inNamespace, sa, true) if err != nil { return nil, err diff --git a/pkg/serviceaccount/ensure.go b/pkg/serviceaccount/ensure.go index 62b617137..5be5f13af 100644 --- a/pkg/serviceaccount/ensure.go +++ b/pkg/serviceaccount/ensure.go @@ -53,7 +53,7 @@ func ensureFromYAML(kubeClient kubernetes.Interface, namespace, yaml string) (*c return nil, err } - _, err = Ensure(kubeClient, namespace, sa) + _, err = Ensure(kubeClient, namespace, sa, true) if err != nil { return nil, err } @@ -62,7 +62,14 @@ func ensureFromYAML(kubeClient kubernetes.Interface, namespace, yaml string) (*c } // nolint:wrapcheck // No need to wrap errors here. -func Ensure(kubeClient kubernetes.Interface, namespace string, sa *corev1.ServiceAccount) (bool, error) { +func Ensure(kubeClient kubernetes.Interface, namespace string, sa *corev1.ServiceAccount, onlyCreate bool) (bool, error) { + if onlyCreate { + _, err := kubeClient.CoreV1().ServiceAccounts(namespace).Get(context.TODO(), sa.Name, metav1.GetOptions{}) + if err == nil { + return true, nil + } + } + return resourceutil.CreateOrUpdate(context.TODO(), resource.ForServiceAccount(kubeClient, namespace), sa) } @@ -123,7 +130,7 @@ func EnsureSecretFromSA(client kubernetes.Interface, saName, namespace string) ( } sa.Secrets = append(sa.Secrets, secretRef) - _, err = Ensure(client, namespace, sa) + _, err = Ensure(client, namespace, sa, false) if err != nil { return nil, errors.Wrapf(err, "failed to update ServiceAccount %v with Secret reference %v", saName, secretRef.Name) diff --git a/pkg/serviceaccount/ensure_test.go b/pkg/serviceaccount/ensure_test.go index 3d90712f0..f7df47f9f 100644 --- a/pkg/serviceaccount/ensure_test.go +++ b/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() From 8a4308cf7eec1664330b2a469b408d9880f70875 Mon Sep 17 00:00:00 2001 From: Vishal Thapar <5137689+vthapar@users.noreply.github.com> Date: Wed, 29 Jun 2022 22:08:48 +0530 Subject: [PATCH 3/3] Refactor serviceaccount.Ensure Refactor serviceaccount.Ensure so that creation of secret is transparent to caller. Signed-off-by: Vishal Thapar <5137689+vthapar@users.noreply.github.com> --- pkg/broker/ensure.go | 20 +++++--------------- pkg/serviceaccount/ensure.go | 31 +++++++++++++++++++++++++------ 2 files changed, 30 insertions(+), 21 deletions(-) diff --git a/pkg/broker/ensure.go b/pkg/broker/ensure.go index 5fc60e391..575b4387f 100644 --- a/pkg/broker/ensure.go +++ b/pkg/broker/ensure.go @@ -91,7 +91,7 @@ func Ensure(crdUpdater crd.Updater, kubeClient kubernetes.Interface, componentAr func createBrokerClusterRoleAndDefaultSA(kubeClient kubernetes.Interface, inNamespace string) error { // Create the a default SA for cluster access (backwards compatibility with documentation) - _, err := CreateNewBrokerSA(kubeClient, submarinerBrokerClusterDefaultSA, inNamespace) + err := CreateNewBrokerSA(kubeClient, submarinerBrokerClusterDefaultSA, inNamespace) if err != nil && !apierrors.IsAlreadyExists(err) { return errors.Wrap(err, "error creating the default broker service account") } @@ -115,7 +115,7 @@ func createBrokerClusterRoleAndDefaultSA(kubeClient kubernetes.Interface, inName func CreateSAForCluster(kubeClient kubernetes.Interface, clusterID, inNamespace string) (*v1.Secret, error) { saName := names.ForClusterSA(clusterID) - _, err := CreateNewBrokerSA(kubeClient, saName, inNamespace) + err := CreateNewBrokerSA(kubeClient, saName, inNamespace) if err != nil && !apierrors.IsAlreadyExists(err) { return nil, errors.Wrap(err, "error creating cluster sa") } @@ -135,7 +135,7 @@ func CreateSAForCluster(kubeClient kubernetes.Interface, clusterID, inNamespace func createBrokerAdministratorRoleAndSA(kubeClient kubernetes.Interface, inNamespace string) error { // Create the SA we need for the managing the broker (from subctl, etc..). - _, err := CreateNewBrokerSA(kubeClient, constants.SubmarinerBrokerAdminSA, inNamespace) + err := CreateNewBrokerSA(kubeClient, constants.SubmarinerBrokerAdminSA, inNamespace) if err != nil && !apierrors.IsAlreadyExists(err) { return errors.Wrap(err, "error creating the broker admin service account") } @@ -204,19 +204,9 @@ func CreateNewBrokerRoleBinding(kubeClient kubernetes.Interface, serviceAccount, } // nolint:wrapcheck // No need to wrap here -func CreateNewBrokerSA(kubeClient kubernetes.Interface, submarinerBrokerSA, inNamespace string) (brokerSA *v1.ServiceAccount, err error) { +func CreateNewBrokerSA(kubeClient kubernetes.Interface, submarinerBrokerSA, inNamespace string) (err error) { sa := NewBrokerSA(submarinerBrokerSA) _, err = serviceaccount.Ensure(kubeClient, inNamespace, sa, true) - if err != nil { - return nil, err - } - - _, err = serviceaccount.EnsureSecretFromSA(kubeClient, sa.Name, inNamespace) - - if err != nil { - return nil, errors.Wrap(err, "failed to get secret for broker SA") - } - - return kubeClient.CoreV1().ServiceAccounts(inNamespace).Get(context.TODO(), sa.Name, metav1.GetOptions{}) + return err } diff --git a/pkg/serviceaccount/ensure.go b/pkg/serviceaccount/ensure.go index 5be5f13af..b933c75d2 100644 --- a/pkg/serviceaccount/ensure.go +++ b/pkg/serviceaccount/ensure.go @@ -53,7 +53,7 @@ func ensureFromYAML(kubeClient kubernetes.Interface, namespace, yaml string) (*c return nil, err } - _, err = Ensure(kubeClient, namespace, sa, true) + err = ensure(kubeClient, namespace, sa, true) if err != nil { return nil, err } @@ -62,15 +62,34 @@ func ensureFromYAML(kubeClient kubernetes.Interface, namespace, yaml string) (*c } // nolint:wrapcheck // No need to wrap errors here. -func Ensure(kubeClient kubernetes.Interface, namespace string, sa *corev1.ServiceAccount, onlyCreate bool) (bool, error) { +func ensure(kubeClient kubernetes.Interface, namespace string, sa *corev1.ServiceAccount, onlyCreate bool) error { if onlyCreate { _, err := kubeClient.CoreV1().ServiceAccounts(namespace).Get(context.TODO(), sa.Name, metav1.GetOptions{}) - if err == nil { - return true, nil + + if err == nil || !apierrors.IsNotFound(err) { + return err } } - return resourceutil.CreateOrUpdate(context.TODO(), resource.ForServiceAccount(kubeClient, namespace), sa) + _, err := resourceutil.CreateOrUpdate(context.TODO(), resource.ForServiceAccount(kubeClient, namespace), sa) + + return err +} + +// nolint:wrapcheck // No need to wrap errors here. +func Ensure(kubeClient kubernetes.Interface, namespace string, sa *corev1.ServiceAccount, onlyCreate bool) (*corev1.ServiceAccount, error) { + err := ensure(kubeClient, namespace, sa, onlyCreate) + if err != nil { + return nil, err + } + + _, err = EnsureSecretFromSA(kubeClient, sa.Name, namespace) + + if err != nil { + return nil, errors.Wrap(err, "failed to get secret for broker SA") + } + + return kubeClient.CoreV1().ServiceAccounts(namespace).Get(context.TODO(), sa.Name, metav1.GetOptions{}) } // EnsureFromYAML creates the given service account and secret for it. @@ -130,7 +149,7 @@ func EnsureSecretFromSA(client kubernetes.Interface, saName, namespace string) ( } sa.Secrets = append(sa.Secrets, secretRef) - _, err = Ensure(client, namespace, sa, false) + err = ensure(client, namespace, sa, false) if err != nil { return nil, errors.Wrapf(err, "failed to update ServiceAccount %v with Secret reference %v", saName, secretRef.Name)