From e910a911bcb55075714c19097ce08ec3071dbd21 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Mon, 7 Jul 2025 23:06:28 -0400 Subject: [PATCH] Preserve ServiceAccount annotations during CSV update 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. Signed-off-by: Tiger Kaovilai 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 --- .../operators/catalog/step_ensurer.go | 12 +- .../operators/catalog/step_ensurer_test.go | 236 ++++++++++++++++++ 2 files changed, 247 insertions(+), 1 deletion(-) diff --git a/pkg/controller/operators/catalog/step_ensurer.go b/pkg/controller/operators/catalog/step_ensurer.go index 3369aa6561..046f04b209 100644 --- a/pkg/controller/operators/catalog/step_ensurer.go +++ b/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. diff --git a/pkg/controller/operators/catalog/step_ensurer_test.go b/pkg/controller/operators/catalog/step_ensurer_test.go index 4bceee3077..233ba81397 100644 --- a/pkg/controller/operators/catalog/step_ensurer_test.go +++ b/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) + } + }) + } +}