Merge pull request #5565 from crossplane/backport-5558-to-release-1.15

[Backport release-1.15] fix(sa): Merge image pull secrets created by other controllers
crossplane · Apr 9, 2024 · 432e163 · 432e163
2 parents 3f61489 + faded50
commit 432e163
Show file tree

Hide file tree

Showing 4 changed files with 123 additions and 2 deletions.
diff --git a/internal/controller/pkg/revision/runtime_function.go b/internal/controller/pkg/revision/runtime_function.go
@@ -134,7 +134,7 @@ func (h *FunctionHooks) Post(ctx context.Context, pkg runtime.Object, pr v1.Pack
 	// `deploymentTemplate.spec.template.spec.serviceAccountName` in the
 	// DeploymentRuntimeConfig.
 	if sa.Name == d.Spec.Template.Spec.ServiceAccountName {
-		if err := h.client.Apply(ctx, sa); err != nil {
+		if err := applySA(ctx, h.client, sa); err != nil {
 			return errors.Wrap(err, errApplyFunctionSA)
 		}
 	}

diff --git a/internal/controller/pkg/revision/runtime_function_test.go b/internal/controller/pkg/revision/runtime_function_test.go
@@ -398,6 +398,56 @@ func TestFunctionPostHook(t *testing.T) {
 				},
 			},
 		},
+		"SuccessWithExtraSecret": {
+			reason: "Should not return error if successfully applied service account with additional secret.",
+			args: args{
+				pkg: &pkgmetav1beta1.Function{},
+				rev: &v1beta1.FunctionRevision{
+					Spec: v1beta1.FunctionRevisionSpec{
+						PackageRevisionSpec: v1.PackageRevisionSpec{
+							Package:      functionImage,
+							DesiredState: v1.PackageRevisionActive,
+						},
+					},
+				},
+				manifests: &MockManifestBuilder{
+					ServiceAccountFn: func(_ ...ServiceAccountOverride) *corev1.ServiceAccount {
+						return &corev1.ServiceAccount{}
+					},
+					DeploymentFn: func(_ string, _ ...DeploymentOverride) *appsv1.Deployment {
+						return &appsv1.Deployment{}
+					},
+				},
+				client: &test.MockClient{
+					MockGet: func(_ context.Context, _ client.ObjectKey, obj client.Object) error {
+						if sa, ok := obj.(*corev1.ServiceAccount); ok {
+							sa.ImagePullSecrets = []corev1.LocalObjectReference{{Name: "test_secret"}}
+						}
+						return nil
+					},
+					MockPatch: func(_ context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) error {
+						if d, ok := obj.(*appsv1.Deployment); ok {
+							d.Status.Conditions = []appsv1.DeploymentCondition{{
+								Type:   appsv1.DeploymentAvailable,
+								Status: corev1.ConditionTrue,
+							}}
+							return nil
+						}
+						return nil
+					},
+				},
+			},
+			want: want{
+				rev: &v1beta1.FunctionRevision{
+					Spec: v1beta1.FunctionRevisionSpec{
+						PackageRevisionSpec: v1.PackageRevisionSpec{
+							Package:      functionImage,
+							DesiredState: v1.PackageRevisionActive,
+						},
+					},
+				},
+			},
+		},
 		"SuccessfulWithExternallyManagedSA": {
 			reason: "Should be successful without creating an SA, when the SA is managed externally",
 			args: args{

diff --git a/internal/controller/pkg/revision/runtime_provider.go b/internal/controller/pkg/revision/runtime_provider.go
@@ -24,6 +24,7 @@ import (
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/crossplane/crossplane-runtime/pkg/errors"
@@ -154,7 +155,7 @@ func (h *ProviderHooks) Post(ctx context.Context, pkg runtime.Object, pr v1.Pack
 	// `deploymentTemplate.spec.template.spec.serviceAccountName` in the
 	// DeploymentRuntimeConfig.
 	if sa.Name == d.Spec.Template.Spec.ServiceAccountName {
-		if err := h.client.Apply(ctx, sa); err != nil {
+		if err := applySA(ctx, h.client, sa); err != nil {
 			return errors.Wrap(err, errApplyProviderSA)
 		}
 	}
@@ -292,3 +293,23 @@ func getProviderImage(pm *pkgmetav1.Provider, pr v1.PackageRevisionWithRuntime,
 
 	return ref.Name(), nil
 }
+
+// applySA creates/updates a ServiceAccount and includes any image pull secrets
+// that have been added by external controllers.
+func applySA(ctx context.Context, cl resource.ClientApplicator, sa *corev1.ServiceAccount) error {
+	oldSa := &corev1.ServiceAccount{}
+	if err := cl.Get(ctx, types.NamespacedName{Name: sa.Name, Namespace: sa.Namespace}, oldSa); err == nil {
+		// Add pull secrets created by other controllers
+		existingSecrets := make(map[string]bool)
+		for _, secret := range sa.ImagePullSecrets {
+			existingSecrets[secret.Name] = true
+		}
+
+		for _, secret := range oldSa.ImagePullSecrets {
+			if !existingSecrets[secret.Name] {
+				sa.ImagePullSecrets = append(sa.ImagePullSecrets, secret)
+			}
+		}
+	}
+	return cl.Apply(ctx, sa)
+}
diff --git a/internal/controller/pkg/revision/runtime_provider_test.go b/internal/controller/pkg/revision/runtime_provider_test.go
@@ -470,6 +470,56 @@ func TestProviderPostHook(t *testing.T) {
 				},
 			},
 		},
+		"SuccessWithExtraSecret": {
+			reason: "Should not return error if successfully applied service account with additional secret.",
+			args: args{
+				pkg: &pkgmetav1.Provider{},
+				rev: &v1.ProviderRevision{
+					Spec: v1.ProviderRevisionSpec{
+						PackageRevisionSpec: v1.PackageRevisionSpec{
+							Package:      providerImage,
+							DesiredState: v1.PackageRevisionActive,
+						},
+					},
+				},
+				manifests: &MockManifestBuilder{
+					ServiceAccountFn: func(_ ...ServiceAccountOverride) *corev1.ServiceAccount {
+						return &corev1.ServiceAccount{}
+					},
+					DeploymentFn: func(_ string, _ ...DeploymentOverride) *appsv1.Deployment {
+						return &appsv1.Deployment{}
+					},
+				},
+				client: &test.MockClient{
+					MockGet: func(_ context.Context, _ client.ObjectKey, obj client.Object) error {
+						if sa, ok := obj.(*corev1.ServiceAccount); ok {
+							sa.ImagePullSecrets = []corev1.LocalObjectReference{{Name: "test_secret"}}
+						}
+						return nil
+					},
+					MockPatch: func(_ context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) error {
+						if d, ok := obj.(*appsv1.Deployment); ok {
+							d.Status.Conditions = []appsv1.DeploymentCondition{{
+								Type:   appsv1.DeploymentAvailable,
+								Status: corev1.ConditionTrue,
+							}}
+							return nil
+						}
+						return nil
+					},
+				},
+			},
+			want: want{
+				rev: &v1.ProviderRevision{
+					Spec: v1.ProviderRevisionSpec{
+						PackageRevisionSpec: v1.PackageRevisionSpec{
+							Package:      providerImage,
+							DesiredState: v1.PackageRevisionActive,
+						},
+					},
+				},
+			},
+		},
 		"SuccessfulWithExternallyManagedSA": {
 			reason: "Should be successful without creating an SA, when the SA is managed externally",
 			args: args{