From 3247b8785044bdca040a185ce401161be8f421c0 Mon Sep 17 00:00:00 2001 From: Dimitri Koshkin Date: Thu, 13 Jun 2024 08:48:49 -0700 Subject: [PATCH] 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?**: New unit tests and brought up a Nutanix cluster with `docker.io` creds. **Special notes for your reviewer**: --- .../credentials/credentials_secret.go | 63 +++-- .../credentials/credentials_secret_test.go | 230 ++++++++++++++++++ .../imageregistries/credentials/inject.go | 12 +- .../static-credential-provider.json.gotmpl | 8 +- 4 files changed, 290 insertions(+), 23 deletions(-) create mode 100644 pkg/handlers/generic/mutation/imageregistries/credentials/credentials_secret_test.go diff --git a/pkg/handlers/generic/mutation/imageregistries/credentials/credentials_secret.go b/pkg/handlers/generic/mutation/imageregistries/credentials/credentials_secret.go index a592e3ad6..6e8302335 100644 --- a/pkg/handlers/generic/mutation/imageregistries/credentials/credentials_secret.go +++ b/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) } diff --git a/pkg/handlers/generic/mutation/imageregistries/credentials/credentials_secret_test.go b/pkg/handlers/generic/mutation/imageregistries/credentials/credentials_secret_test.go new file mode 100644 index 000000000..ac8aeb708 --- /dev/null +++ b/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) + }) + } +} diff --git a/pkg/handlers/generic/mutation/imageregistries/credentials/inject.go b/pkg/handlers/generic/mutation/imageregistries/credentials/inject.go index fb903250e..ecd5215cb 100644 --- a/pkg/handlers/generic/mutation/imageregistries/credentials/inject.go +++ b/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 } diff --git a/pkg/handlers/generic/mutation/imageregistries/credentials/templates/static-credential-provider.json.gotmpl b/pkg/handlers/generic/mutation/imageregistries/credentials/templates/static-credential-provider.json.gotmpl index 1064fa093..600fecf05 100644 --- a/pkg/handlers/generic/mutation/imageregistries/credentials/templates/static-credential-provider.json.gotmpl +++ b/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 }} } }