Repository: scylladb/scylla-operator
Pull request #935 (2022-02-01 14:22:59)

[v1.7] Keep custom annotations on ServiceAccounts
Backport of #933 



Patch:
diff --git a/pkg/controller/scyllacluster/resource.go b/pkg/controller/scyllacluster/resource.go
index a68abb306..b11537fdf 100644
--- a/pkg/controller/scyllacluster/resource.go
+++ b/pkg/controller/scyllacluster/resource.go
@@ -12,9 +12,11 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/api/policy/v1beta1"
 	rbacv1 "k8s.io/api/rbac/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
+	corev1listers "k8s.io/client-go/listers/core/v1"
 	"k8s.io/utils/pointer"
 )
 
@@ -648,17 +650,45 @@ func stringOrDefault(str, def string) string {
 	return def
 }
 
-func MakeServiceAccount(sc *scyllav1.ScyllaCluster) *corev1.ServiceAccount {
+func MakeServiceAccount(sc *scyllav1.ScyllaCluster, serviceAccounts map[string]*corev1.ServiceAccount, serviceAccountLister corev1listers.ServiceAccountLister) (*corev1.ServiceAccount, error) {
+	annotations := map[string]string{}
+	name := naming.MemberServiceAccountNameForScyllaCluster(sc.Name)
+
+	existing, found := serviceAccounts[name]
+	if !found {
+		// In 1.7.0 we started managing ServiceAccounts. Because previously these were created by users
+		// there's a high chance that they don't match our selector, hence are not available in serviceAccounts.
+		// Ref: https://github.com/scylladb/scylla-operator/issues/932
+		//
+		// Cache fetch happens only once - when SA is being adopted, or it's missing.
+		// It can be removed in 1.8.0 because all SAs will be already adopted, and they will match our selector.
+		// TODO: https://github.com/scylladb/scylla-operator/issues/934
+		var err error
+		existing, err = serviceAccountLister.ServiceAccounts(sc.Namespace).Get(name)
+		if err != nil && !apierrors.IsNotFound(err) {
+			return nil, fmt.Errorf("can't get %s service account: %w", naming.ManualRef(sc.Namespace, name), err)
+		}
+	}
+
+	if existing != nil {
+		// We need to copy annotations stored in user SA because these may control integrations with k8s provider services
+		// like IAM management.
+		for k, v := range existing.Annotations {
+			annotations[k] = v
+		}
+	}
+
 	return &corev1.ServiceAccount{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      naming.MemberServiceAccountNameForScyllaCluster(sc.Name),
+			Name:      name,
 			Namespace: sc.Namespace,
 			OwnerReferences: []metav1.OwnerReference{
 				*metav1.NewControllerRef(sc, controllerGVK),
 			},
-			Labels: naming.ClusterLabels(sc),
+			Labels:      naming.ClusterLabels(sc),
+			Annotations: annotations,
 		},
-	}
+	}, nil
 }
 
 func MakeRoleBinding(sc *scyllav1.ScyllaCluster) *rbacv1.RoleBinding {
diff --git a/pkg/controller/scyllacluster/sync_serviceaccounts.go b/pkg/controller/scyllacluster/sync_serviceaccounts.go
index d98ff3505..72d05a3cf 100644
--- a/pkg/controller/scyllacluster/sync_serviceaccounts.go
+++ b/pkg/controller/scyllacluster/sync_serviceaccounts.go
@@ -16,9 +16,10 @@ func (scc *Controller) syncServiceAccounts(
 	sc *scyllav1.ScyllaCluster,
 	serviceAccounts map[string]*corev1.ServiceAccount,
 ) error {
-	var err error
-
-	requiredServiceAccount := MakeServiceAccount(sc)
+	requiredServiceAccount, err := MakeServiceAccount(sc, serviceAccounts, scc.serviceAccountLister)
+	if err != nil {
+		return fmt.Errorf("can't make service account(s): %w", err)
+	}
 
 	// Delete any excessive ServiceAccounts.
 	// Delete has to be the fist action to avoid getting stuck on quota.
diff --git a/pkg/resourceapply/core.go b/pkg/resourceapply/core.go
index 43320fd2d..ce25c4cfc 100644
--- a/pkg/resourceapply/core.go
+++ b/pkg/resourceapply/core.go
@@ -3,6 +3,7 @@ package resourceapply
 import (
 	"context"
 	"fmt"
+	"strings"
 
 	"github.com/scylladb/scylla-operator/pkg/naming"
 	corev1 "k8s.io/api/core/v1"
@@ -237,11 +238,26 @@ func ApplyServiceAccount(
 	}
 
 	requiredCopy := required.DeepCopy()
+
+	// Users may need to add custom annotations to integrate with k8s provider services.
+	// These should be filtered out before hash is computed.
+	customAnnotations := map[string]string{}
+	for key, value := range requiredCopy.Annotations {
+		if !strings.HasPrefix(key, "scylla-operator.scylladb.com/") {
+			customAnnotations[key] = value
+			delete(requiredCopy.Annotations, key)
+		}
+	}
+
 	err := SetHashAnnotation(requiredCopy)
 	if err != nil {
 		return nil, false, err
 	}
 
+	for key, value := range customAnnotations {
+		requiredCopy.Annotations[key] = value
+	}
+
 	existing, err := lister.ServiceAccounts(requiredCopy.Namespace).Get(requiredCopy.Name)
 	if err != nil {
 		if !apierrors.IsNotFound(err) {


Test patch:
diff --git a/pkg/resourceapply/core_test.go b/pkg/resourceapply/core_test.go
index bdbe2b786..8442028d4 100644
--- a/pkg/resourceapply/core_test.go
+++ b/pkg/resourceapply/core_test.go
@@ -1166,6 +1166,66 @@ func TestApplyServiceAccount(t *testing.T) {
 			expectedErr:     fmt.Errorf(`serviceAccount "default/test" isn't controlled by us`),
 			expectedEvents:  []string{`Warning UpdateServiceAccountFailed Failed to update ServiceAccount default/test: serviceAccount "default/test" isn't controlled by us`},
 		},
+		{
+			name: "annotations not starting with our prefix are ignored during hashing and kept in the object",
+			existing: []runtime.Object{
+				func() *corev1.ServiceAccount {
+					sa := newSAWithControllerRef()
+					utilruntime.Must(SetHashAnnotation(sa))
+					sa.Annotations["custom-annotation"] = "custom-value"
+					return sa
+				}(),
+			},
+			required: func() *corev1.ServiceAccount {
+				sa := newSAWithControllerRef()
+				sa.Annotations = map[string]string{
+					"custom-annotation": "custom-value",
+				}
+				return sa
+			}(),
+			forceOwnership: true,
+			expectedSA: func() *corev1.ServiceAccount {
+				sa := newSAWithControllerRef()
+				utilruntime.Must(SetHashAnnotation(sa))
+				sa.Annotations["custom-annotation"] = "custom-value"
+				return sa
+			}(),
+			expectedChanged: false,
+			expectedErr:     nil,
+			expectedEvents:  nil,
+		},
+		{
+			name: "annotations starting with our prefix are accounted during hash compute",
+			existing: []runtime.Object{
+				func() *corev1.ServiceAccount {
+					sa := newSAWithControllerRef()
+					utilruntime.Must(SetHashAnnotation(sa))
+					sa.Annotations["custom-annotation"] = "custom-value"
+					return sa
+				}(),
+			},
+			required: func() *corev1.ServiceAccount {
+				sa := newSAWithControllerRef()
+				sa.Annotations = map[string]string{
+					"custom-annotation":                   "custom-value",
+					"scylla-operator.scylladb.com/blabla": "123",
+				}
+				return sa
+			}(),
+			forceOwnership: true,
+			expectedSA: func() *corev1.ServiceAccount {
+				sa := newSAWithControllerRef()
+				sa.Annotations = map[string]string{
+					"scylla-operator.scylladb.com/blabla": "123",
+				}
+				utilruntime.Must(SetHashAnnotation(sa))
+				sa.Annotations["custom-annotation"] = "custom-value"
+				return sa
+			}(),
+			expectedChanged: true,
+			expectedErr:     nil,
+			expectedEvents:  []string{"Normal ServiceAccountUpdated ServiceAccount default/test updated"},
+		},
 	}
 
 	for _, tc := range tt {
diff --git a/test/e2e/set/scyllacluster/scyllacluster_sa.go b/test/e2e/set/scyllacluster/scyllacluster_sa.go
index 8db74dfd5..b132bf33a 100644
--- a/test/e2e/set/scyllacluster/scyllacluster_sa.go
+++ b/test/e2e/set/scyllacluster/scyllacluster_sa.go
@@ -35,6 +35,9 @@ var _ = g.Describe("ScyllaCluster", func() {
 		sa, err := f.KubeClient().CoreV1().ServiceAccounts(f.Namespace()).Create(ctx, &corev1.ServiceAccount{
 			ObjectMeta: metav1.ObjectMeta{
 				Name: fmt.Sprintf("%s-member", sc.Name),
+				Annotations: map[string]string{
+					"user-annotation": "123",
+				},
 			},
 		}, metav1.CreateOptions{})
 		o.Expect(err).NotTo(o.HaveOccurred())
@@ -85,6 +88,7 @@ var _ = g.Describe("ScyllaCluster", func() {
 		}, utils.WaitForStateOptions{})
 		o.Expect(err).NotTo(o.HaveOccurred())
 		o.Expect(sa.OwnerReferences).To(o.HaveLen(1))
+		o.Expect(sa.Annotations).To(o.HaveKeyWithValue("user-annotation", "123"))
 
 		framework.By("Waiting for the RoleBinding to be adopted")
 		waitCtx2, waitCtx2Cancel := utils.ContextForRollout(ctx, sc)