fix: image registries with no credentials but with a CA (#927)

**What problem does this PR solve?**:
Passing `imageRegistries` with a CA but without credentials is still a
valid configuration and should not error.

**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.
-->

**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.
-->

Author: dkoshkin

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

@@ -11,6 +11,7 @@ import (
 	"path"
 	"text/template"
 
+	corev1 "k8s.io/api/core/v1"
 	credentialproviderv1 "k8s.io/kubelet/pkg/apis/credentialprovider/v1"
 	cabpkv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1beta1"
 
@@ -28,6 +29,8 @@ const (
 	kubeletDynamicCredentialProviderConfigOnRemote = "/etc/kubernetes/dynamic-credential-provider-config.yaml"
 
 	azureCloudConfigFilePath = "/etc/kubernetes/azure.json"
+
+	secretKeyForCACert = "ca.crt"
 )
 
 var (
@@ -47,10 +50,11 @@ var (
 )
 
 type providerConfig struct {
-	URL      string
-	Username string
-	Password string
-	Mirror   bool
+	URL       string
+	Username  string
+	Password  string
+	HasCACert bool
+	Mirror    bool
 }
 
 func (c providerConfig) isCredentialsEmpty() bool {
@@ -249,3 +253,12 @@ func fileFromTemplate(
 		Permissions: "0600",
 	}, nil
 }
+
+func secretHasCACert(secret *corev1.Secret) bool {
+	if secret == nil {
+		return false
+	}
+
+	_, ok := secret.Data[secretKeyForCACert]
+	return ok
+}

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

@@ -116,7 +116,7 @@ func (h *imageRegistriesPatchHandler) Mutate(
 	}
 
 	if globalMirrorErr == nil {
-		mirrorCredentials, generateErr := mirrorConfigFromGlobalImageRegistryMirror(
+		mirrorCredentials, generateErr := mirrorWithOptionalCredentialsFromGlobalImageRegistryMirror(
 			ctx,
 			h.client,
 			globalMirror,
@@ -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
 			}
@@ -332,12 +332,13 @@ func registryWithOptionalCredentialsFromImageRegistryCredentials(
 	if secret != nil {
 		registryWithOptionalCredentials.Username = string(secret.Data["username"])
 		registryWithOptionalCredentials.Password = string(secret.Data["password"])
+		registryWithOptionalCredentials.HasCACert = secretHasCACert(secret)
 	}
 
 	return registryWithOptionalCredentials, nil
 }
 
-func mirrorConfigFromGlobalImageRegistryMirror(
+func mirrorWithOptionalCredentialsFromGlobalImageRegistryMirror(
 	ctx context.Context,
 	c ctrlclient.Client,
 	mirror v1alpha1.GlobalImageRegistryMirror,
@@ -365,6 +366,7 @@ func mirrorConfigFromGlobalImageRegistryMirror(
 	if secret != nil {
 		mirrorCredentials.Username = string(secret.Data["username"])
 		mirrorCredentials.Password = string(secret.Data["password"])
+		mirrorCredentials.HasCACert = secretHasCACert(secret)
 	}
 
 	return mirrorCredentials, nil
@@ -438,31 +440,35 @@ func createSecretIfNeeded(
 // This handler reads input from two user provided variables: globalImageRegistryMirror and imageRegistries.
 // We expect if imageRegistries is set it will either have static credentials
 // or be for a registry where the credential plugin returns the credentials, ie ECR, GCR, ACR, etc,
+// or have no credentials set but to contain a CA cert,
 // 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) {
+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 {
-			// not setting credentials for a mirror is valid
-			// but if it's the only configuration then return false here and exit the handler early
-			if config.Mirror {
-				if len(configs) == 1 {
-					return false, nil
-				}
-			} else {
-				return false, fmt.Errorf("invalid image registry: %s: %w", config.URL, ErrCredentialsNotFound)
+			if config.Mirror || config.HasCACert {
+				// 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 nil, fmt.Errorf(
+				"invalid image registry: %s: %w",
+				config.URL,
+				ErrCredentialsNotFound,
+			)
 		}
+		needConfiguration = append(needConfiguration, config)
 	}
 
-	return true, nil
+	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 credentials",
+			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,35 +58,45 @@ 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",
-			configs: []providerConfig{
-				{
-					URL:    "https://123456789.dkr.ecr.us-east-1.amazonaws.com",
-					Mirror: true,
-				},
-			},
-			need: true,
+			name: "ECR mirror with no credentials",
+			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",
 			configs: []providerConfig{{
-				URL:      "https://myregistry.com",
+				URL:      "https://mymirror.com",
+				Username: "myuser",
+				Password: "mypassword",
+				Mirror:   true,
+			}},
+			expected: []providerConfig{{
+				URL:      "https://mymirror.com",
 				Username: "myuser",
 				Password: "mypassword",
 				Mirror:   true,
 			}},
-			need: true,
 		},
 		{
 			name: "mirror with no credentials",
 			configs: []providerConfig{{
-				URL:    "https://myregistry.com",
+				URL:    "https://mymirror.com",
 				Mirror: true,
 			}},
-			need: false,
+			expected: nil,
 		},
 		{
 			name: "a registry with static credentials and a mirror with no credentials",
@@ -93,23 +105,58 @@ func Test_needImageRegistryCredentialsConfiguration(t *testing.T) {
 					URL:      "https://myregistry.com",
 					Username: "myuser",
 					Password: "mypassword",
-					Mirror:   true,
+					Mirror:   false,
 				},
+				{
+					URL:    "https://mymirror.com",
+					Mirror: 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",
+			configs: []providerConfig{
 				{
 					URL:    "https://myregistry.com",
+					Mirror: false,
+				},
+				{
+					URL:    "https://mymirror.com",
 					Mirror: true,
 				},
 			},
-			need: true,
+			wantErr: ErrCredentialsNotFound,
 		},
 		{
 			name: "registry with missing credentials",
 			configs: []providerConfig{{
 				URL: "https://myregistry.com",
 			}},
-			need:    false,
 			wantErr: ErrCredentialsNotFound,
 		},
+		{
+			name: "registry with missing credentials but with a CA",
+			configs: []providerConfig{{
+				URL:       "https://myregistry.com",
+				HasCACert: true,
+			}},
+		},
+		{
+			name: "mirror with missing credentials but with a CA",
+			configs: []providerConfig{{
+				URL:       "https://mymirror.com",
+				HasCACert: true,
+				Mirror:    true,
+			}},
+		},
 	}
 
 	for idx := range testCases {
@@ -118,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)
 		})
 	}
 }