Skip to content

Commit 3247b87

Browse files
dkoshkindkoshkin
authored
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. -->
1 parent 5cdac0b commit 3247b87

File tree

4 files changed

+290
-23
lines changed

4 files changed

+290
-23
lines changed

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

Lines changed: 49 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -31,21 +31,19 @@ var (
3131
)
3232
)
3333

34-
func generateCredentialsSecretFile(configs []providerConfig, clusterName string) []cabpkv1.File {
35-
if len(configs) == 0 {
34+
func generateCredentialsSecretFile(configs []providerConfig, clusterName string) *cabpkv1.File {
35+
if !configsRequireStaticCredentials(configs) {
3636
return nil
3737
}
38-
return []cabpkv1.File{
39-
{
40-
Path: kubeletStaticCredentialProviderCredentialsOnRemote,
41-
ContentFrom: &cabpkv1.FileSource{
42-
Secret: cabpkv1.SecretFileSource{
43-
Name: credentialSecretName(clusterName),
44-
Key: secretKeyForStaticCredentialProviderConfig,
45-
},
38+
return &cabpkv1.File{
39+
Path: kubeletStaticCredentialProviderCredentialsOnRemote,
40+
ContentFrom: &cabpkv1.FileSource{
41+
Secret: cabpkv1.SecretFileSource{
42+
Name: credentialSecretName(clusterName),
43+
Key: secretKeyForStaticCredentialProviderConfig,
4644
},
47-
Permissions: "0600",
4845
},
46+
Permissions: "0600",
4947
}
5048
}
5149

@@ -54,7 +52,7 @@ func generateCredentialsSecretFile(configs []providerConfig, clusterName string)
5452
func generateCredentialsSecret(
5553
configs []providerConfig, clusterName, namespace string,
5654
) (*corev1.Secret, error) {
57-
if len(configs) == 0 {
55+
if !configsRequireStaticCredentials(configs) {
5856
return nil, nil
5957
}
6058

@@ -90,8 +88,19 @@ func kubeletStaticCredentialProviderSecretContents(configs []providerConfig) (st
9088
Password string
9189
}
9290

93-
inputs := make([]templateInput, 0, len(configs))
91+
var inputs []templateInput //nolint:prealloc // We don't know the size of the slice yet.
9492
for _, config := range configs {
93+
requiresStaticCredentials, err := config.requiresStaticCredentials()
94+
if err != nil {
95+
return "", fmt.Errorf(
96+
"error determining if Image Registry is a supported provider: %w",
97+
err,
98+
)
99+
}
100+
if !requiresStaticCredentials {
101+
continue
102+
}
103+
95104
registryURL, err := url.ParseRequestURI(config.URL)
96105
if err != nil {
97106
return "", fmt.Errorf("failed parsing registry URL: %w", err)
@@ -102,6 +111,19 @@ func kubeletStaticCredentialProviderSecretContents(configs []providerConfig) (st
102111
Username: config.Username,
103112
Password: config.Password,
104113
})
114+
115+
// Preserve special handling of "registry-1.docker.io" and add "docker.io" as an alias.
116+
if registryURL.Host == "registry-1.docker.io" {
117+
inputs = append(inputs, templateInput{
118+
RegistryHost: "docker.io",
119+
Username: config.Username,
120+
Password: config.Password,
121+
})
122+
}
123+
}
124+
125+
if len(inputs) == 0 {
126+
return "", nil
105127
}
106128

107129
var b bytes.Buffer
@@ -113,6 +135,19 @@ func kubeletStaticCredentialProviderSecretContents(configs []providerConfig) (st
113135
return strings.TrimSpace(b.String()), nil
114136
}
115137

138+
func configsRequireStaticCredentials(configs []providerConfig) bool {
139+
for _, config := range configs {
140+
requiresStaticCredentials, err := config.requiresStaticCredentials()
141+
if err != nil {
142+
return false
143+
}
144+
if requiresStaticCredentials {
145+
return true
146+
}
147+
}
148+
return false
149+
}
150+
116151
func credentialSecretName(clusterName string) string {
117-
return fmt.Sprintf("%s-credential-provider-response", clusterName)
152+
return fmt.Sprintf("%s-static-credential-provider-response", clusterName)
118153
}

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

Lines changed: 230 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,230 @@
1+
// Copyright 2024 Nutanix. All rights reserved.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
package credentials
5+
6+
import (
7+
"testing"
8+
9+
"github.com/stretchr/testify/assert"
10+
"github.com/stretchr/testify/require"
11+
corev1 "k8s.io/api/core/v1"
12+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
13+
cabpkv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1beta1"
14+
)
15+
16+
func Test_generateCredentialsSecretFile(t *testing.T) {
17+
t.Parallel()
18+
19+
tests := []struct {
20+
name string
21+
configs []providerConfig
22+
clusterName string
23+
wantFile *cabpkv1.File
24+
}{
25+
{
26+
name: "empty configs, expect no file",
27+
configs: nil,
28+
clusterName: "test-cluster",
29+
wantFile: nil,
30+
},
31+
{
32+
name: "config with no static credentials, expect no file",
33+
configs: []providerConfig{
34+
{URL: "https://123456789.dkr.ecr.us-east-1.amazonaws.com"},
35+
},
36+
clusterName: "test-cluster",
37+
wantFile: nil,
38+
},
39+
{
40+
name: "config with static credentials, expect a file",
41+
configs: []providerConfig{
42+
{
43+
URL: "https://myregistry.com",
44+
Username: "myuser",
45+
Password: "mypassword",
46+
},
47+
},
48+
clusterName: "test-cluster",
49+
wantFile: &cabpkv1.File{
50+
Path: "/etc/kubernetes/static-image-credentials.json",
51+
Permissions: "0600",
52+
ContentFrom: &cabpkv1.FileSource{
53+
Secret: cabpkv1.SecretFileSource{
54+
Name: "test-cluster-static-credential-provider-response",
55+
Key: "static-credential-provider",
56+
},
57+
},
58+
},
59+
},
60+
}
61+
62+
for idx := range tests {
63+
tt := tests[idx]
64+
t.Run(tt.name, func(t *testing.T) {
65+
t.Parallel()
66+
67+
gotFile := generateCredentialsSecretFile(tt.configs, tt.clusterName)
68+
assert.Equal(t, tt.wantFile, gotFile)
69+
})
70+
}
71+
}
72+
73+
func Test_generateCredentialsSecret(t *testing.T) {
74+
t.Parallel()
75+
76+
tests := []struct {
77+
name string
78+
configs []providerConfig
79+
clusterName string
80+
namespace string
81+
wantSecret *corev1.Secret
82+
}{
83+
{
84+
name: "empty configs, expect no Secret",
85+
configs: nil,
86+
clusterName: "test-cluster",
87+
namespace: "test-namespace",
88+
wantSecret: nil,
89+
},
90+
{
91+
name: "config with no static credentials, expect no Secret",
92+
configs: []providerConfig{
93+
{URL: "https://123456789.dkr.ecr.us-east-1.amazonaws.com"},
94+
},
95+
clusterName: "test-cluster",
96+
namespace: "test-namespace",
97+
wantSecret: nil,
98+
},
99+
{
100+
name: "config with static credentials, expect a Secret",
101+
configs: []providerConfig{
102+
{
103+
URL: "https://myregistry.com",
104+
Username: "myuser",
105+
Password: "mypassword",
106+
},
107+
},
108+
clusterName: "test-cluster",
109+
namespace: "test-namespace",
110+
wantSecret: &corev1.Secret{
111+
TypeMeta: metav1.TypeMeta{
112+
APIVersion: "v1",
113+
Kind: "Secret",
114+
},
115+
ObjectMeta: metav1.ObjectMeta{
116+
Name: "test-cluster-static-credential-provider-response",
117+
Namespace: "test-namespace",
118+
Labels: map[string]string{
119+
"cluster.x-k8s.io/cluster-name": "test-cluster",
120+
"clusterctl.cluster.x-k8s.io/move": "",
121+
},
122+
},
123+
StringData: map[string]string{
124+
"static-credential-provider": `{
125+
"kind":"CredentialProviderResponse",
126+
"apiVersion":"credentialprovider.kubelet.k8s.io/v1",
127+
"cacheKeyType":"Image",
128+
"cacheDuration":"0s",
129+
"auth":{
130+
"myregistry.com": {"username": "myuser", "password": "mypassword"}
131+
}
132+
}`,
133+
},
134+
Type: corev1.SecretTypeOpaque,
135+
},
136+
},
137+
}
138+
139+
for idx := range tests {
140+
tt := tests[idx]
141+
t.Run(tt.name, func(t *testing.T) {
142+
t.Parallel()
143+
144+
gotSecret, err := generateCredentialsSecret(tt.configs, tt.clusterName, tt.namespace)
145+
require.NoError(t, err)
146+
assert.Equal(t, tt.wantSecret, gotSecret)
147+
})
148+
}
149+
}
150+
151+
func Test_kubeletStaticCredentialProviderSecretContents(t *testing.T) {
152+
t.Parallel()
153+
154+
tests := []struct {
155+
name string
156+
configs []providerConfig
157+
wantContents string
158+
}{
159+
{
160+
name: "config with no static credentials, expect empty string",
161+
configs: []providerConfig{
162+
{URL: "https://123456789.dkr.ecr.us-east-1.amazonaws.com"},
163+
},
164+
wantContents: "",
165+
},
166+
{
167+
name: "config with 'registry-1.docker.io', expect it to also add 'docker.io'",
168+
configs: []providerConfig{
169+
{
170+
URL: "https://registry-1.docker.io",
171+
Username: "myuser",
172+
Password: "mypassword",
173+
},
174+
},
175+
wantContents: `{
176+
"kind":"CredentialProviderResponse",
177+
"apiVersion":"credentialprovider.kubelet.k8s.io/v1",
178+
"cacheKeyType":"Image",
179+
"cacheDuration":"0s",
180+
"auth":{
181+
"registry-1.docker.io": {"username": "myuser", "password": "mypassword"},
182+
"docker.io": {"username": "myuser", "password": "mypassword"}
183+
}
184+
}`,
185+
},
186+
{
187+
name: "multiple configs with some static credentials, expect a string with multiple entries",
188+
configs: []providerConfig{
189+
{
190+
URL: "https://myregistry.com",
191+
Username: "myuser",
192+
Password: "mypassword",
193+
},
194+
{URL: "https://123456789.dkr.ecr.us-east-1.amazonaws.com"},
195+
{
196+
URL: "https://registry-1.docker.io",
197+
Username: "myuser",
198+
Password: "mypassword",
199+
},
200+
{
201+
URL: "https://anotherregistry.com",
202+
Username: "anotheruser",
203+
Password: "anotherpassword",
204+
},
205+
},
206+
wantContents: `{
207+
"kind":"CredentialProviderResponse",
208+
"apiVersion":"credentialprovider.kubelet.k8s.io/v1",
209+
"cacheKeyType":"Image",
210+
"cacheDuration":"0s",
211+
"auth":{
212+
"myregistry.com": {"username": "myuser", "password": "mypassword"},
213+
"registry-1.docker.io": {"username": "myuser", "password": "mypassword"},
214+
"docker.io": {"username": "myuser", "password": "mypassword"},
215+
"anotherregistry.com": {"username": "anotheruser", "password": "anotherpassword"}
216+
}
217+
}`,
218+
},
219+
}
220+
for idx := range tests {
221+
tt := tests[idx]
222+
t.Run(tt.name, func(t *testing.T) {
223+
t.Parallel()
224+
225+
gotContents, err := kubeletStaticCredentialProviderSecretContents(tt.configs)
226+
require.NoError(t, err)
227+
assert.Equal(t, tt.wantContents, gotContents)
228+
})
229+
}
230+
}

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

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -344,9 +344,15 @@ func generateFilesAndCommands(
344344
)
345345
}
346346
files = append(files, imageCredentialProviderConfigFiles...)
347-
files = append(
348-
files,
349-
generateCredentialsSecretFile(registriesWithOptionalCredentials, clusterName)...)
347+
348+
credentialSecretFile := generateCredentialsSecretFile(
349+
registriesWithOptionalCredentials,
350+
clusterName,
351+
)
352+
if credentialSecretFile != nil {
353+
files = append(files, *credentialSecretFile)
354+
}
355+
350356
return files, commands, err
351357
}
352358

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

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,8 @@
44
"cacheKeyType":"Image",
55
"cacheDuration":"0s",
66
"auth":{
7-
{{- range . }}
8-
{{- if .RegistryHost }}
9-
{{ printf "%q" .RegistryHost }}: {"username": {{ printf "%q" .Username }}, "password": {{ printf "%q" .Password }}}{{ if eq .RegistryHost "registry-1.docker.io" }},
10-
"docker.io": {"username": {{ printf "%q" .Username }}, "password": {{ printf "%q" .Password }}}
7+
{{- range $i, $config := . }}{{ if $i }},{{ end}}
8+
{{ printf "%q" $config.RegistryHost }}: {"username": {{ printf "%q" $config.Username }}, "password": {{ printf "%q" $config.Password }}}
119
{{- end }}
12-
{{- end }}
13-
{{- end }}
1410
}
1511
}

0 commit comments

Comments
 (0)