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) + } + }) + } +}