Preserve ServiceAccount annotations during CSV update (#3610)

Fixes #3607

When a ClusterServiceVersion (CSV) is updated, the EnsureServiceAccount
function now preserves existing annotations on the ServiceAccount. This
prevents the loss of important annotations that may have been added by
users or other controllers.

The fix merges existing annotations from the old ServiceAccount with
annotations from the new ServiceAccount object, giving precedence to
the new annotations in case of conflicts.



Add unit tests for ServiceAccount annotation preservation

Added comprehensive unit tests for the EnsureServiceAccount function
to verify that annotations are properly preserved during updates.
The tests cover:
- Creating new service accounts
- Preserving existing annotations during updates
- Handling annotation conflicts (new annotations take precedence)
- Preserving secrets during updates
- Error handling scenarios

Signed-off-by: Tiger Kaovilai <[email protected]>

Author: kaovilai

pkg/controller/operators/catalog/step_ensurer.go

@@ -151,7 +151,7 @@ func (o *StepEnsurer) EnsureServiceAccount(namespace string, sa *corev1.ServiceA
 		return
 	}
 
-	// Carrying secrets through the service account update.
+	// Carrying secrets and annotations through the service account update.
 	preSa, getErr := o.kubeClient.KubernetesInterface().CoreV1().ServiceAccounts(namespace).Get(context.TODO(),
 		sa.Name,
 		metav1.GetOptions{})
@@ -162,6 +162,16 @@ func (o *StepEnsurer) EnsureServiceAccount(namespace string, sa *corev1.ServiceA
 	sa.Secrets = preSa.Secrets
 	sa.OwnerReferences = mergedOwnerReferences(preSa.OwnerReferences, sa.OwnerReferences)
 
+	// Merge annotations, giving precedence to the new ones.
+	if sa.Annotations == nil {
+		sa.Annotations = make(map[string]string)
+	}
+	for k, v := range preSa.Annotations {
+		if _, ok := sa.Annotations[k]; !ok {
+			sa.Annotations[k] = v
+		}
+	}
+
 	sa.SetNamespace(namespace)
 
 	// Use DeepDerivative to check if new SA is the same as the old SA. If no field is changed, we skip the update call.

pkg/controller/operators/catalog/step_ensurer_test.go

@@ -3,9 +3,21 @@ package catalog
 import (
 	"testing"
 
+	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
+	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/runtime"
+	fakedynamic "k8s.io/client-go/dynamic/fake"
+	k8sfake "k8s.io/client-go/kubernetes/fake"
+	k8stesting "k8s.io/client-go/testing"
+
+	"github.com/operator-framework/api/pkg/operators/v1alpha1"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned/fake"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient/operatorclientmocks"
 )
 
 func TestMergedOwnerReferences(t *testing.T) {
@@ -188,3 +200,227 @@ func TestMergedOwnerReferences(t *testing.T) {
 		})
 	}
 }
+
+func TestEnsureServiceAccount(t *testing.T) {
+	namespace := "test-namespace"
+	saName := "test-sa"
+
+	tests := []struct {
+		name                   string
+		existingServiceAccount *corev1.ServiceAccount
+		newServiceAccount      *corev1.ServiceAccount
+		expectedAnnotations    map[string]string
+		expectedStatus         v1alpha1.StepStatus
+		expectError            bool
+		createError            error
+		getError               error
+		updateError            error
+	}{
+		{
+			name: "create new service account",
+			newServiceAccount: &corev1.ServiceAccount{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      saName,
+					Namespace: namespace,
+					Annotations: map[string]string{
+						"new-annotation": "new-value",
+					},
+				},
+			},
+			expectedAnnotations: map[string]string{
+				"new-annotation": "new-value",
+			},
+			expectedStatus: v1alpha1.StepStatusCreated,
+		},
+		{
+			name: "update existing service account - preserve existing annotations",
+			existingServiceAccount: &corev1.ServiceAccount{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      saName,
+					Namespace: namespace,
+					Annotations: map[string]string{
+						"existing-annotation": "existing-value",
+						"override-annotation": "old-value",
+					},
+				},
+				Secrets: []corev1.ObjectReference{
+					{Name: "existing-secret"},
+				},
+			},
+			newServiceAccount: &corev1.ServiceAccount{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      saName,
+					Namespace: namespace,
+					Annotations: map[string]string{
+						"new-annotation":      "new-value",
+						"override-annotation": "new-value",
+					},
+				},
+			},
+			expectedAnnotations: map[string]string{
+				"existing-annotation": "existing-value",
+				"new-annotation":      "new-value",
+				"override-annotation": "new-value",
+			},
+			expectedStatus: v1alpha1.StepStatusPresent,
+			createError:    apierrors.NewAlreadyExists(corev1.Resource("serviceaccounts"), saName),
+		},
+		{
+			name: "update existing service account - no annotations on new SA",
+			existingServiceAccount: &corev1.ServiceAccount{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      saName,
+					Namespace: namespace,
+					Annotations: map[string]string{
+						"existing-annotation": "existing-value",
+					},
+				},
+			},
+			newServiceAccount: &corev1.ServiceAccount{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      saName,
+					Namespace: namespace,
+				},
+			},
+			expectedAnnotations: map[string]string{
+				"existing-annotation": "existing-value",
+			},
+			expectedStatus: v1alpha1.StepStatusPresent,
+			createError:    apierrors.NewAlreadyExists(corev1.Resource("serviceaccounts"), saName),
+		},
+		{
+			name: "update existing service account - preserve secrets",
+			existingServiceAccount: &corev1.ServiceAccount{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      saName,
+					Namespace: namespace,
+				},
+				Secrets: []corev1.ObjectReference{
+					{Name: "secret-1"},
+					{Name: "secret-2"},
+				},
+			},
+			newServiceAccount: &corev1.ServiceAccount{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      saName,
+					Namespace: namespace,
+				},
+			},
+			expectedAnnotations: map[string]string{},
+			expectedStatus:      v1alpha1.StepStatusPresent,
+			createError:         apierrors.NewAlreadyExists(corev1.Resource("serviceaccounts"), saName),
+		},
+		{
+			name: "create error - not already exists",
+			newServiceAccount: &corev1.ServiceAccount{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      saName,
+					Namespace: namespace,
+				},
+			},
+			createError: apierrors.NewInternalError(assert.AnError),
+			expectError: true,
+		},
+		{
+			name: "update error - get existing fails",
+			existingServiceAccount: &corev1.ServiceAccount{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      saName,
+					Namespace: namespace,
+				},
+			},
+			newServiceAccount: &corev1.ServiceAccount{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      saName,
+					Namespace: namespace,
+				},
+			},
+			createError: apierrors.NewAlreadyExists(corev1.Resource("serviceaccounts"), saName),
+			getError:    apierrors.NewInternalError(assert.AnError),
+			expectError: true,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			ctrl := gomock.NewController(t)
+			defer ctrl.Finish()
+
+			// Create mock client
+			mockClient := operatorclientmocks.NewMockClientInterface(ctrl)
+
+			// Create fake kubernetes client
+			var objects []runtime.Object
+			if tc.existingServiceAccount != nil {
+				objects = append(objects, tc.existingServiceAccount)
+			}
+
+			fakeClient := k8sfake.NewSimpleClientset(objects...)
+
+			// Setup expectations
+			mockClient.EXPECT().KubernetesInterface().Return(fakeClient).AnyTimes()
+
+			// Mock the create call
+			if tc.createError != nil {
+				// We need to intercept the create call and return the error
+				fakeClient.PrependReactor("create", "serviceaccounts", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) {
+					return true, nil, tc.createError
+				})
+			}
+
+			// Mock the get call if needed
+			if tc.getError != nil {
+				fakeClient.PrependReactor("get", "serviceaccounts", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) {
+					return true, nil, tc.getError
+				})
+			}
+
+			// Mock UpdateServiceAccount if the test expects an update
+			if tc.createError != nil && apierrors.IsAlreadyExists(tc.createError) && tc.getError == nil {
+				// Calculate expected SA after merge
+				expectedSA := tc.newServiceAccount.DeepCopy()
+				if tc.existingServiceAccount != nil {
+					expectedSA.Secrets = tc.existingServiceAccount.Secrets
+					// Merge annotations
+					if expectedSA.Annotations == nil {
+						expectedSA.Annotations = make(map[string]string)
+					}
+					for k, v := range tc.existingServiceAccount.Annotations {
+						if _, ok := expectedSA.Annotations[k]; !ok {
+							expectedSA.Annotations[k] = v
+						}
+					}
+				}
+				expectedSA.SetNamespace(namespace)
+
+				mockClient.EXPECT().UpdateServiceAccount(gomock.Any()).DoAndReturn(func(sa *corev1.ServiceAccount) (*corev1.ServiceAccount, error) {
+					// Verify the merged service account has the expected annotations
+					assert.Equal(t, tc.expectedAnnotations, sa.Annotations)
+					// Verify secrets were preserved if they existed
+					if tc.existingServiceAccount != nil && len(tc.existingServiceAccount.Secrets) > 0 {
+						assert.Equal(t, tc.existingServiceAccount.Secrets, sa.Secrets)
+					}
+					return sa, tc.updateError
+				}).MaxTimes(1)
+			}
+
+			// Create StepEnsurer
+			ensurer := &StepEnsurer{
+				kubeClient:    mockClient,
+				crClient:      fake.NewSimpleClientset(),
+				dynamicClient: fakedynamic.NewSimpleDynamicClient(runtime.NewScheme()),
+			}
+
+			// Execute EnsureServiceAccount
+			status, err := ensurer.EnsureServiceAccount(namespace, tc.newServiceAccount)
+
+			// Verify results
+			if tc.expectError {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+				assert.Equal(t, tc.expectedStatus, status)
+			}
+		})
+	}
+}