fix: skip imageRegistries with no credentials

Author: dkoshkin

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

@@ -131,19 +131,19 @@ func (h *imageRegistriesPatchHandler) Mutate(
 		)
 	}
 
-	needCredentials, err := needImageRegistryCredentialsConfiguration(
+	registriesThatNeedConfiguration, err := providerConfigsThatNeedConfiguration(
 		registriesWithOptionalCredentials,
 	)
 	if err != nil {
 		return err
 	}
-	if !needCredentials {
-		log.V(5).Info("Only Global Registry Mirror is defined but credentials are not needed")
+	if len(registriesThatNeedConfiguration) == 0 {
+		log.V(5).Info("Image registry credentials are not needed")
 		return nil
 	}
 
 	files, commands, generateErr := generateFilesAndCommands(
-		registriesWithOptionalCredentials,
+		registriesThatNeedConfiguration,
 		clusterKey.Name,
 	)
 	if generateErr != nil {
@@ -185,7 +185,7 @@ func (h *imageRegistriesPatchHandler) Mutate(
 				return err
 			}
 
-			err = createSecretIfNeeded(ctx, h.client, registriesWithOptionalCredentials, cluster)
+			err = createSecretIfNeeded(ctx, h.client, registriesThatNeedConfiguration, cluster)
 			if err != nil {
 				return err
 			}
@@ -243,7 +243,7 @@ func (h *imageRegistriesPatchHandler) Mutate(
 				return err
 			}
 
-			err = createSecretIfNeeded(ctx, h.client, registriesWithOptionalCredentials, cluster)
+			err = createSecretIfNeeded(ctx, h.client, registriesThatNeedConfiguration, cluster)
 			if err != nil {
 				return err
 			}
@@ -444,30 +444,30 @@ func createSecretIfNeeded(
 // and if that is not the case we assume the users missed setting static credentials and return an error.
 // However, in addition to passing credentials with the globalImageRegistryMirror variable,
 // it can also be used to only set Containerd mirror configuration,
-// in that case it valid for static credentials to not be set and will return false, no error
+// in which case it is valid for static credentials to not be set and will be skipped, no error
 // and this handler will skip generating any credential plugin related configuration.
-func needImageRegistryCredentialsConfiguration(configs []providerConfig) (bool, error) {
-	var needConfiguration bool
+func providerConfigsThatNeedConfiguration(configs []providerConfig) ([]providerConfig, error) {
+	var needConfiguration []providerConfig //nolint:prealloc // We don't know the size of the slice yet.
 	for _, config := range configs {
 		requiresStaticCredentials, err := config.requiresStaticCredentials()
 		if err != nil {
-			return false,
+			return nil,
 				fmt.Errorf("error determining if Image Registry is a supported provider: %w", err)
 		}
 		// verify the credentials are actually set if the plugin requires static credentials
 		if config.isCredentialsEmpty() && requiresStaticCredentials {
 			if config.Mirror || config.HasCACert {
-				// not setting credentials for a mirror is valid
-				// not setting credentials for a registry with a CA cert is valid
+				// not setting credentials for a mirror is valid, but won't need any configuration
+				// not setting credentials for a registry with a CA cert is valid, but won't need any configuration
 				continue
 			}
-			return false, fmt.Errorf(
+			return nil, fmt.Errorf(
 				"invalid image registry: %s: %w",
 				config.URL,
 				ErrCredentialsNotFound,
 			)
 		}
-		needConfiguration = true
+		needConfiguration = append(needConfiguration, config)
 	}
 
 	return needConfiguration, nil

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

@@ -33,21 +33,23 @@ const (
 	validSecretName = "myregistry-credentials"
 )
 
-func Test_needImageRegistryCredentialsConfiguration(t *testing.T) {
+func Test_providerConfigsThatNeedConfiguration(t *testing.T) {
 	t.Parallel()
 
 	testCases := []struct {
-		name    string
-		configs []providerConfig
-		need    bool
-		wantErr error
+		name     string
+		configs  []providerConfig
+		expected []providerConfig
+		wantErr  error
 	}{
 		{
 			name: "ECR registry with no credentials",
 			configs: []providerConfig{
 				{URL: "https://123456789.dkr.ecr.us-east-1.amazonaws.com"},
 			},
-			need: true,
+			expected: []providerConfig{
+				{URL: "https://123456789.dkr.ecr.us-east-1.amazonaws.com"},
+			},
 		},
 		{
 			name: "registry with static credentials",
@@ -56,17 +58,22 @@ func Test_needImageRegistryCredentialsConfiguration(t *testing.T) {
 				Username: "myuser",
 				Password: "mypassword",
 			}},
-			need: true,
+			expected: []providerConfig{{
+				URL:      "https://myregistry.com",
+				Username: "myuser",
+				Password: "mypassword",
+			}},
 		},
 		{
 			name: "ECR mirror with no credentials",
-			configs: []providerConfig{
-				{
-					URL:    "https://123456789.dkr.ecr.us-east-1.amazonaws.com",
-					Mirror: true,
-				},
-			},
-			need: true,
+			configs: []providerConfig{{
+				URL:    "https://123456789.dkr.ecr.us-east-1.amazonaws.com",
+				Mirror: true,
+			}},
+			expected: []providerConfig{{
+				URL:    "https://123456789.dkr.ecr.us-east-1.amazonaws.com",
+				Mirror: true,
+			}},
 		},
 		{
 			name: "mirror with static credentials",
@@ -76,15 +83,20 @@ func Test_needImageRegistryCredentialsConfiguration(t *testing.T) {
 				Password: "mypassword",
 				Mirror:   true,
 			}},
-			need: true,
+			expected: []providerConfig{{
+				URL:      "https://mymirror.com",
+				Username: "myuser",
+				Password: "mypassword",
+				Mirror:   true,
+			}},
 		},
 		{
 			name: "mirror with no credentials",
 			configs: []providerConfig{{
 				URL:    "https://mymirror.com",
 				Mirror: true,
 			}},
-			need: false,
+			expected: nil,
 		},
 		{
 			name: "a registry with static credentials and a mirror with no credentials",
@@ -100,7 +112,14 @@ func Test_needImageRegistryCredentialsConfiguration(t *testing.T) {
 					Mirror: true,
 				},
 			},
-			need: true,
+			expected: []providerConfig{
+				{
+					URL:      "https://myregistry.com",
+					Username: "myuser",
+					Password: "mypassword",
+					Mirror:   false,
+				},
+			},
 		},
 		{
 			name: "a registry with missing credentials and a mirror with no credentials",
@@ -114,15 +133,13 @@ func Test_needImageRegistryCredentialsConfiguration(t *testing.T) {
 					Mirror: true,
 				},
 			},
-			need:    false,
 			wantErr: ErrCredentialsNotFound,
 		},
 		{
 			name: "registry with missing credentials",
 			configs: []providerConfig{{
 				URL: "https://myregistry.com",
 			}},
-			need:    false,
 			wantErr: ErrCredentialsNotFound,
 		},
 		{
@@ -131,7 +148,6 @@ func Test_needImageRegistryCredentialsConfiguration(t *testing.T) {
 				URL:       "https://myregistry.com",
 				HasCACert: true,
 			}},
-			need: false,
 		},
 		{
 			name: "mirror with missing credentials but with a CA",
@@ -140,7 +156,6 @@ func Test_needImageRegistryCredentialsConfiguration(t *testing.T) {
 				HasCACert: true,
 				Mirror:    true,
 			}},
-			need: false,
 		},
 	}
 
@@ -150,9 +165,9 @@ func Test_needImageRegistryCredentialsConfiguration(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
 
-			need, err := needImageRegistryCredentialsConfiguration(tt.configs)
+			expected, err := providerConfigsThatNeedConfiguration(tt.configs)
 			require.ErrorIs(t, err, tt.wantErr)
-			assert.Equal(t, tt.need, need)
+			assert.Equal(t, tt.expected, expected)
 		})
 	}
 }