From 93a1f7df4b91b8758774745f2a53fd68861e737d Mon Sep 17 00:00:00 2001
From: Hidehito Yabuuchi <hyab@preferred.jp>
Date: Mon, 5 Aug 2024 11:53:08 +0900
Subject: [PATCH] Fix image pull secret name

to prevent the issue in the following scenario:

1. An image pull secret is provisioned, and a Pod successfully pulls image and starts
2. ServiceAccount's annotation is changed, resulting in a change of the image pull secret name
3. The Pod attempts to pull the image again for some reason
4. The old image pull secret has been deleted, causing the Pod to fail in pulling the image
5. Since the Pod initially started successfully in step 1, the evictor does not recover the Pod
---
 README.md                                     | 15 ++++++++++
 internal/controller/config.go                 | 29 ++++++-------------
 internal/controller/metadata.go               |  2 ++
 .../serviceaccount_controller_test.go         |  4 +--
 4 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/README.md b/README.md
index 782bcdb..8adf476 100644
--- a/README.md
+++ b/README.md
@@ -113,6 +113,21 @@ rules:
    See also [Configure Service Accounts for Pods | Kubernetes](https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/)
 4. The pod will be able to pull container images from the registry
 
+## Image pull secret name
+
+By default, image pull secrets provisioner creates an image pull secret with the name `imagepullsecret-SERVICE-ACCOUNT-NAME`.
+If you want to use a different name, you can specify it in the ServiceAccount's annotation.
+
+```yaml
+apiVersion: v1
+kind: ServiceAccount
+metadata:
+  namespace: NAMESPACE
+  name: SERVICE-ACCOUNT-NAME
+  annotations:
+    imagepullsecrets.preferred.jp/secret-name: SECRET-NAME
+```
+
 ## Pod eviction
 
 Image pull secrets added to a ServiceAccount's `.imagePullSecrets` field do *not* apply to existing pods using the ServiceAccount.
diff --git a/internal/controller/config.go b/internal/controller/config.go
index d352a94..63fedfd 100644
--- a/internal/controller/config.go
+++ b/internal/controller/config.go
@@ -17,11 +17,8 @@ limitations under the License.
 package controller
 
 import (
-	"fmt"
-	"hash/fnv"
-
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/util/rand"
+	"k8s.io/apimachinery/pkg/util/validation"
 )
 
 // Helpers for config annotations.
@@ -50,23 +47,15 @@ func hasConfig(sa *corev1.ServiceAccount) bool {
 	return false
 }
 
-func configHash(sa *corev1.ServiceAccount) string {
-	hasher := fnv.New32a()
-
-	for _, key := range []string{
-		annotationKeyRegistry,
-		annotationKeyAudience,
-		annotationKeyAWSRoleARN,
-		annotationKeyGoogleWIDP,
-		annotationKeyGoogleSA,
-	} {
-		hasher.Write([]byte(sa.Annotations[key]))
+func secretName(sa *corev1.ServiceAccount) string {
+	if name, ok := sa.Annotations[annotationKeySecretName]; ok {
+		return name
 	}
 
-	return rand.SafeEncodeString(fmt.Sprint(hasher.Sum32()))
-}
+	name := "imagepullsecret-" + sa.GetName()
+	if len(name) > validation.DNS1123SubdomainMaxLength {
+		name = name[:validation.DNS1123SubdomainMaxLength]
+	}
 
-func secretName(sa *corev1.ServiceAccount) string {
-	// TODO: Consider name confliction with manual creation or other provisioning system.
-	return fmt.Sprintf("imagepullsecret-%s-%s", sa.GetName(), configHash(sa))
+	return name
 }
diff --git a/internal/controller/metadata.go b/internal/controller/metadata.go
index 4e3e9bd..a72cadd 100644
--- a/internal/controller/metadata.go
+++ b/internal/controller/metadata.go
@@ -31,6 +31,8 @@ const (
 	annotationKeyGoogleWIDP = metadataKeyPrefix + "googlecloud-workload-identity-provider"
 	annotationKeyGoogleSA   = metadataKeyPrefix + "googlecloud-service-account-email"
 
+	annotationKeySecretName = metadataKeyPrefix + "secret-name"
+
 	// Annotation for Secrets to store the expiration time.
 	annotationKeyExpiresAt = metadataKeyPrefix + "expires-at"
 
diff --git a/internal/controller/serviceaccount_controller_test.go b/internal/controller/serviceaccount_controller_test.go
index ac08a2d..3cf885a 100644
--- a/internal/controller/serviceaccount_controller_test.go
+++ b/internal/controller/serviceaccount_controller_test.go
@@ -208,9 +208,9 @@ var _ = Describe("ServiceAccountReconciler", func() {
 				outdated = &secrets.Items[0]
 			}).Should(Succeed())
 
-			// Change the config for image pull secret provisioning.
+			// Change the name of Secret to provision.
 			orig := sa.DeepCopy()
-			sa.Annotations["imagepullsecrets.preferred.jp/googlecloud-service-account-email"] = "other@example.iam.gserviceaccount.com"
+			sa.Annotations["imagepullsecrets.preferred.jp/secret-name"] = "imagepullsecret-2"
 			Expect(k8sClient.Patch(ctx, sa, client.StrategicMergeFrom(orig))).NotTo(HaveOccurred())
 
 			// Test that a new Secret is created and the outdated Secret is deleted.