fix: static credentials Secret generation (#717)

**What problem does this PR solve?**:
While working in this package I was confused why `createSecretIfNeeded`
was called that since it always generated a Secret.
Turns out there was a bug in these files that it never checked if the
passed in `configs []providerConfig` even have static credentials. Added
some failing unit tests and fixed the logic to skip generating the
Secret and the corresponding file if not needed.
While writing the tests, I also noticed that the templating was not
generating valid JSON for multiple registries. To keep that separator
logic simple I moved the `docker.io` alias into Go code.

**Which issue(s) this PR fixes**:
Fixes #

**How Has This Been Tested?**:
<!--
Please describe the tests that you ran to verify your changes.
Provide output from the tests and any manual steps needed to replicate
the tests.
-->
New unit tests and brought up a Nutanix cluster with `docker.io` creds.

**Special notes for your reviewer**:
<!--
Use this to provide any additional information to the reviewers.
This may include:
- Best way to review the PR.
- Where the author wants the most review attention on.
- etc.
-->

pkg/handlers/generic/mutation/imageregistries/credentials/credentials_secret.go

@@ -31,21 +31,19 @@ var (
 	)
 )
 
-func generateCredentialsSecretFile(configs []providerConfig, clusterName string) []cabpkv1.File {
-	if len(configs) == 0 {
+func generateCredentialsSecretFile(configs []providerConfig, clusterName string) *cabpkv1.File {
+	if !configsRequireStaticCredentials(configs) {
 		return nil
 	}
-	return []cabpkv1.File{
-		{
-			Path: kubeletStaticCredentialProviderCredentialsOnRemote,
-			ContentFrom: &cabpkv1.FileSource{
-				Secret: cabpkv1.SecretFileSource{
-					Name: credentialSecretName(clusterName),
-					Key:  secretKeyForStaticCredentialProviderConfig,
-				},
+	return &cabpkv1.File{
+		Path: kubeletStaticCredentialProviderCredentialsOnRemote,
+		ContentFrom: &cabpkv1.FileSource{
+			Secret: cabpkv1.SecretFileSource{
+				Name: credentialSecretName(clusterName),
+				Key:  secretKeyForStaticCredentialProviderConfig,
 			},
-			Permissions: "0600",
 		},
+		Permissions: "0600",
 	}
 }
 
@@ -54,7 +52,7 @@ func generateCredentialsSecretFile(configs []providerConfig, clusterName string)
 func generateCredentialsSecret(
 	configs []providerConfig, clusterName, namespace string,
 ) (*corev1.Secret, error) {
-	if len(configs) == 0 {
+	if !configsRequireStaticCredentials(configs) {
 		return nil, nil
 	}
 
@@ -90,8 +88,19 @@ func kubeletStaticCredentialProviderSecretContents(configs []providerConfig) (st
 		Password     string
 	}
 
-	inputs := make([]templateInput, 0, len(configs))
+	var inputs []templateInput //nolint:prealloc // We don't know the size of the slice yet.
 	for _, config := range configs {
+		requiresStaticCredentials, err := config.requiresStaticCredentials()
+		if err != nil {
+			return "", fmt.Errorf(
+				"error determining if Image Registry is a supported provider: %w",
+				err,
+			)
+		}
+		if !requiresStaticCredentials {
+			continue
+		}
+
 		registryURL, err := url.ParseRequestURI(config.URL)
 		if err != nil {
 			return "", fmt.Errorf("failed parsing registry URL: %w", err)
@@ -102,6 +111,19 @@ func kubeletStaticCredentialProviderSecretContents(configs []providerConfig) (st
 			Username:     config.Username,
 			Password:     config.Password,
 		})
+
+		// Preserve special handling of "registry-1.docker.io" and add "docker.io" as an alias.
+		if registryURL.Host == "registry-1.docker.io" {
+			inputs = append(inputs, templateInput{
+				RegistryHost: "docker.io",
+				Username:     config.Username,
+				Password:     config.Password,
+			})
+		}
+	}
+
+	if len(inputs) == 0 {
+		return "", nil
 	}
 
 	var b bytes.Buffer
@@ -113,6 +135,19 @@ func kubeletStaticCredentialProviderSecretContents(configs []providerConfig) (st
 	return strings.TrimSpace(b.String()), nil
 }
 
+func configsRequireStaticCredentials(configs []providerConfig) bool {
+	for _, config := range configs {
+		requiresStaticCredentials, err := config.requiresStaticCredentials()
+		if err != nil {
+			return false
+		}
+		if requiresStaticCredentials {
+			return true
+		}
+	}
+	return false
+}
+
 func credentialSecretName(clusterName string) string {
-	return fmt.Sprintf("%s-credential-provider-response", clusterName)
+	return fmt.Sprintf("%s-static-credential-provider-response", clusterName)
 }

pkg/handlers/generic/mutation/imageregistries/credentials/credentials_secret_test.go

@@ -0,0 +1,230 @@
+// Copyright 2024 Nutanix. All rights reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package credentials
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	cabpkv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1beta1"
+)
+
+func Test_generateCredentialsSecretFile(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name        string
+		configs     []providerConfig
+		clusterName string
+		wantFile    *cabpkv1.File
+	}{
+		{
+			name:        "empty configs, expect no file",
+			configs:     nil,
+			clusterName: "test-cluster",
+			wantFile:    nil,
+		},
+		{
+			name: "config with no static credentials, expect no file",
+			configs: []providerConfig{
+				{URL: "https://123456789.dkr.ecr.us-east-1.amazonaws.com"},
+			},
+			clusterName: "test-cluster",
+			wantFile:    nil,
+		},
+		{
+			name: "config with static credentials, expect a file",
+			configs: []providerConfig{
+				{
+					URL:      "https://myregistry.com",
+					Username: "myuser",
+					Password: "mypassword",
+				},
+			},
+			clusterName: "test-cluster",
+			wantFile: &cabpkv1.File{
+				Path:        "/etc/kubernetes/static-image-credentials.json",
+				Permissions: "0600",
+				ContentFrom: &cabpkv1.FileSource{
+					Secret: cabpkv1.SecretFileSource{
+						Name: "test-cluster-static-credential-provider-response",
+						Key:  "static-credential-provider",
+					},
+				},
+			},
+		},
+	}
+
+	for idx := range tests {
+		tt := tests[idx]
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			gotFile := generateCredentialsSecretFile(tt.configs, tt.clusterName)
+			assert.Equal(t, tt.wantFile, gotFile)
+		})
+	}
+}
+
+func Test_generateCredentialsSecret(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name        string
+		configs     []providerConfig
+		clusterName string
+		namespace   string
+		wantSecret  *corev1.Secret
+	}{
+		{
+			name:        "empty configs, expect no Secret",
+			configs:     nil,
+			clusterName: "test-cluster",
+			namespace:   "test-namespace",
+			wantSecret:  nil,
+		},
+		{
+			name: "config with no static credentials, expect no Secret",
+			configs: []providerConfig{
+				{URL: "https://123456789.dkr.ecr.us-east-1.amazonaws.com"},
+			},
+			clusterName: "test-cluster",
+			namespace:   "test-namespace",
+			wantSecret:  nil,
+		},
+		{
+			name: "config with static credentials, expect a Secret",
+			configs: []providerConfig{
+				{
+					URL:      "https://myregistry.com",
+					Username: "myuser",
+					Password: "mypassword",
+				},
+			},
+			clusterName: "test-cluster",
+			namespace:   "test-namespace",
+			wantSecret: &corev1.Secret{
+				TypeMeta: metav1.TypeMeta{
+					APIVersion: "v1",
+					Kind:       "Secret",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-cluster-static-credential-provider-response",
+					Namespace: "test-namespace",
+					Labels: map[string]string{
+						"cluster.x-k8s.io/cluster-name":    "test-cluster",
+						"clusterctl.cluster.x-k8s.io/move": "",
+					},
+				},
+				StringData: map[string]string{
+					"static-credential-provider": `{
+  "kind":"CredentialProviderResponse",
+  "apiVersion":"credentialprovider.kubelet.k8s.io/v1",
+  "cacheKeyType":"Image",
+  "cacheDuration":"0s",
+  "auth":{
+    "myregistry.com": {"username": "myuser", "password": "mypassword"}
+  }
+}`,
+				},
+				Type: corev1.SecretTypeOpaque,
+			},
+		},
+	}
+
+	for idx := range tests {
+		tt := tests[idx]
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			gotSecret, err := generateCredentialsSecret(tt.configs, tt.clusterName, tt.namespace)
+			require.NoError(t, err)
+			assert.Equal(t, tt.wantSecret, gotSecret)
+		})
+	}
+}
+
+func Test_kubeletStaticCredentialProviderSecretContents(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name         string
+		configs      []providerConfig
+		wantContents string
+	}{
+		{
+			name: "config with no static credentials, expect empty string",
+			configs: []providerConfig{
+				{URL: "https://123456789.dkr.ecr.us-east-1.amazonaws.com"},
+			},
+			wantContents: "",
+		},
+		{
+			name: "config with 'registry-1.docker.io', expect it to also add 'docker.io'",
+			configs: []providerConfig{
+				{
+					URL:      "https://registry-1.docker.io",
+					Username: "myuser",
+					Password: "mypassword",
+				},
+			},
+			wantContents: `{
+  "kind":"CredentialProviderResponse",
+  "apiVersion":"credentialprovider.kubelet.k8s.io/v1",
+  "cacheKeyType":"Image",
+  "cacheDuration":"0s",
+  "auth":{
+    "registry-1.docker.io": {"username": "myuser", "password": "mypassword"},
+    "docker.io": {"username": "myuser", "password": "mypassword"}
+  }
+}`,
+		},
+		{
+			name: "multiple configs with some static credentials, expect a string with multiple entries",
+			configs: []providerConfig{
+				{
+					URL:      "https://myregistry.com",
+					Username: "myuser",
+					Password: "mypassword",
+				},
+				{URL: "https://123456789.dkr.ecr.us-east-1.amazonaws.com"},
+				{
+					URL:      "https://registry-1.docker.io",
+					Username: "myuser",
+					Password: "mypassword",
+				},
+				{
+					URL:      "https://anotherregistry.com",
+					Username: "anotheruser",
+					Password: "anotherpassword",
+				},
+			},
+			wantContents: `{
+  "kind":"CredentialProviderResponse",
+  "apiVersion":"credentialprovider.kubelet.k8s.io/v1",
+  "cacheKeyType":"Image",
+  "cacheDuration":"0s",
+  "auth":{
+    "myregistry.com": {"username": "myuser", "password": "mypassword"},
+    "registry-1.docker.io": {"username": "myuser", "password": "mypassword"},
+    "docker.io": {"username": "myuser", "password": "mypassword"},
+    "anotherregistry.com": {"username": "anotheruser", "password": "anotherpassword"}
+  }
+}`,
+		},
+	}
+	for idx := range tests {
+		tt := tests[idx]
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			gotContents, err := kubeletStaticCredentialProviderSecretContents(tt.configs)
+			require.NoError(t, err)
+			assert.Equal(t, tt.wantContents, gotContents)
+		})
+	}
+}

pkg/handlers/generic/mutation/imageregistries/credentials/inject.go

@@ -344,9 +344,15 @@ func generateFilesAndCommands(
 		)
 	}
 	files = append(files, imageCredentialProviderConfigFiles...)
-	files = append(
-		files,
-		generateCredentialsSecretFile(registriesWithOptionalCredentials, clusterName)...)
+
+	credentialSecretFile := generateCredentialsSecretFile(
+		registriesWithOptionalCredentials,
+		clusterName,
+	)
+	if credentialSecretFile != nil {
+		files = append(files, *credentialSecretFile)
+	}
+
 	return files, commands, err
 }
 

pkg/handlers/generic/mutation/imageregistries/credentials/templates/static-credential-provider.json.gotmpl

@@ -4,12 +4,8 @@
   "cacheKeyType":"Image",
   "cacheDuration":"0s",
   "auth":{
-{{- range . }}
-  {{- if .RegistryHost }}
-    {{ printf "%q" .RegistryHost }}: {"username": {{ printf "%q" .Username }}, "password": {{ printf "%q" .Password }}}{{ if eq .RegistryHost "registry-1.docker.io" }},
-    "docker.io": {"username": {{ printf "%q" .Username }}, "password": {{ printf "%q" .Password }}}
+  {{- range $i, $config := . }}{{ if $i }},{{ end}}
+    {{ printf "%q" $config.RegistryHost }}: {"username": {{ printf "%q" $config.Username }}, "password": {{ printf "%q" $config.Password }}}
   {{- end }}
-  {{- end }}
-{{- end }}
   }
 }