fix: generate config with only globalImageRegistryMirror set

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

@@ -6,7 +6,6 @@ package credentials
 import (
 	"bytes"
 	_ "embed"
-	"errors"
 	"fmt"
 	"net/url"
 	"path"
@@ -47,8 +46,6 @@ var (
 	)
 )
 
-var ErrCredentialsNotFound = errors.New("registry credentials not found")
-
 type providerConfig struct {
 	URL      string
 	Username string
@@ -61,6 +58,43 @@ func (c providerConfig) isCredentialsEmpty() bool {
 		c.Password == ""
 }
 
+func (c providerConfig) requiresStaticCredentials() (bool, error) {
+	registryHostWithPath, err := c.registryHostWithPath()
+	if err != nil {
+		return false, fmt.Errorf(
+			"failed to get registry host with path: %w",
+			err,
+		)
+	}
+
+	knownRegistryProvider, err := credentialprovider.URLMatchesKnownRegistryProvider(
+		registryHostWithPath,
+	)
+	if err != nil {
+		return false, fmt.Errorf(
+			"failed to check if registry matches a known registry provider: %w",
+			err,
+		)
+	}
+
+	// require static credentials if the registry provider is not known
+	return !knownRegistryProvider, nil
+}
+
+func (c providerConfig) registryHostWithPath() (string, error) {
+	registryURL, err := url.ParseRequestURI(c.URL)
+	if err != nil {
+		return "", fmt.Errorf("failed parsing registry URL: %w", err)
+	}
+
+	registryHostWithPath := registryURL.Host
+	if registryURL.Path != "" {
+		registryHostWithPath = path.Join(registryURL.Host, registryURL.Path)
+	}
+
+	return registryHostWithPath, nil
+}
+
 func templateFilesForImageCredentialProviderConfigs(
 	configs []providerConfig,
 ) ([]cabpkv1.File, error) {
@@ -121,27 +155,9 @@ func templateDynamicCredentialProviderConfig(
 	inputs := make([]templateInput, 0, len(configs))
 
 	for _, config := range configs {
-		registryURL, err := url.ParseRequestURI(config.URL)
-		if err != nil {
-			return nil, fmt.Errorf("failed parsing registry URL: %w", err)
-		}
-
-		registryHostWithPath := registryURL.Host
-		if registryURL.Path != "" {
-			registryHostWithPath = path.Join(registryURL.Host, registryURL.Path)
-		}
-
-		supportedProvider, err := credentialprovider.URLMatchesSupportedProvider(
-			registryHostWithPath,
-		)
+		registryHostWithPath, err := config.registryHostWithPath()
 		if err != nil {
-			return nil, fmt.Errorf(
-				"failed to check if registry matches a supported provider: %w",
-				err,
-			)
-		}
-		if config.isCredentialsEmpty() && !supportedProvider {
-			return nil, ErrCredentialsNotFound
+			return nil, err
 		}
 
 		providerBinary, providerArgs, providerAPIVersion, err := dynamicCredentialProvider(

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

@@ -184,13 +184,6 @@ credentialProviders:
 `,
 			},
 		},
-		{
-			name: "error for a registry with no credentials",
-			credentials: []providerConfig{{
-				URL: "https://registry.example.com",
-			}},
-			wantErr: ErrCredentialsNotFound,
-		},
 		{
 			name: "multiple registries",
 			credentials: []providerConfig{{

pkg/handlers/generic/mutation/imageregistries/credentials/credentialprovider/matcher.go

@@ -33,7 +33,7 @@ var (
 	}
 )
 
-func URLMatchesSupportedProvider(target string) (bool, error) {
+func URLMatchesKnownRegistryProvider(target string) (bool, error) {
 	urlGlobs := make([]string, 0, len(ecrURLGlobs)+len(gcrURLGlobs)+len(acrURLGlobs))
 	urlGlobs = append(urlGlobs, ecrURLGlobs...)
 	urlGlobs = append(urlGlobs, gcrURLGlobs...)

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

@@ -5,6 +5,7 @@ package credentials
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	corev1 "k8s.io/api/core/v1"
@@ -34,6 +35,8 @@ type imageRegistriesPatchHandler struct {
 	variableFieldPath []string
 }
 
+var ErrCredentialsNotFound = errors.New("registry credentials not found")
+
 func NewPatch(
 	cl ctrlclient.Client,
 ) *imageRegistriesPatchHandler {
@@ -70,15 +73,26 @@ func (h *imageRegistriesPatchHandler) Mutate(
 		"holderRef", holderRef,
 	)
 
-	imageRegistries, found, err := variables.Get[v1alpha1.ImageRegistries](
+	imageRegistries, imageRegistriesFound, err := variables.Get[v1alpha1.ImageRegistries](
 		vars,
 		h.variableName,
 		h.variableFieldPath...,
 	)
 	if err != nil {
 		return err
 	}
-	if !found {
+
+	// add credentials for global image registry mirror
+	globalMirror, globalMirrorFound, err := variables.Get[v1alpha1.GlobalImageRegistryMirror](
+		vars,
+		h.variableName,
+		mirrors.GlobalMirrorVariableName,
+	)
+	if err != nil {
+		return err
+	}
+
+	if !imageRegistriesFound && !globalMirrorFound {
 		log.V(5).Info("Image Registry Credentials variable not defined")
 		return nil
 	}
@@ -100,16 +114,8 @@ func (h *imageRegistriesPatchHandler) Mutate(
 			registryWithOptionalCredentials,
 		)
 	}
-	// add credentials for global image registry mirror
-	globalMirror, found, err := variables.Get[v1alpha1.GlobalImageRegistryMirror](
-		vars,
-		h.variableName,
-		mirrors.GlobalMirrorVariableName,
-	)
-	if err != nil {
-		return err
-	}
-	if found {
+
+	if globalMirrorFound {
 		mirrorCredentials, generateErr := mirrorConfigFromGlobalImageRegistryMirror(
 			ctx,
 			h.client,
@@ -125,6 +131,15 @@ func (h *imageRegistriesPatchHandler) Mutate(
 		)
 	}
 
+	needCredentials, err := needImageRegistryCredentialsConfiguration(registriesWithOptionalCredentials)
+	if err != nil {
+		return err
+	}
+	if !needCredentials {
+		log.V(5).Info("Only Global Registry Mirror is defined but credentials are not needed")
+		return nil
+	}
+
 	files, commands, generateErr := generateFilesAndCommands(
 		registriesWithOptionalCredentials,
 		clusterKey.Name,
@@ -359,3 +374,35 @@ func secretForImageRegistryCredentials(
 	err := c.Get(ctx, key, secret)
 	return secret, err
 }
+
+// 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,
+// 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
+// and this handler will skip generating any credential plugin related configuration.
+func needImageRegistryCredentialsConfiguration(configs []providerConfig) (bool, error) {
+	for _, config := range configs {
+		requiresStaticCredentials, err := config.requiresStaticCredentials()
+		if err != nil {
+			return false,
+				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)
+			}
+		}
+	}
+
+	return true, nil
+}

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

@@ -0,0 +1,102 @@
+// Copyright 2023 D2iQ, Inc. All rights reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package credentials
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_needImageRegistryCredentialsConfiguration(t *testing.T) {
+	t.Parallel()
+
+	testCases := []struct {
+		name    string
+		configs []providerConfig
+		need    bool
+		wantErr error
+	}{
+		{
+			name: "ECR credentials",
+			configs: []providerConfig{
+				{URL: "https://123456789.dkr.ecr.us-east-1.amazonaws.com"},
+			},
+			need: true,
+		},
+		{
+			name: "registry with static credentials",
+			configs: []providerConfig{{
+				URL:      "https://myregistry.com",
+				Username: "myuser",
+				Password: "mypassword",
+			}},
+			need: true,
+		},
+		{
+			name: "ECR mirror",
+			configs: []providerConfig{
+				{
+					URL:    "https://123456789.dkr.ecr.us-east-1.amazonaws.com",
+					Mirror: true,
+				},
+			},
+			need: true,
+		},
+		{
+			name: "mirror with static credentials",
+			configs: []providerConfig{{
+				URL:      "https://myregistry.com",
+				Username: "myuser",
+				Password: "mypassword",
+				Mirror:   true,
+			}},
+			need: true,
+		},
+		{
+			name: "mirror with no credentials",
+			configs: []providerConfig{{
+				URL:    "https://myregistry.com",
+				Mirror: true,
+			}},
+			need: false,
+		},
+		{
+			name: "a registry with static credentials and a mirror with no credentials",
+			configs: []providerConfig{
+				{
+					URL:      "https://myregistry.com",
+					Username: "myuser",
+					Password: "mypassword",
+					Mirror:   true,
+				},
+				{
+					URL:    "https://myregistry.com",
+					Mirror: true,
+				},
+			},
+			need: true,
+		},
+		{
+			name: "registry with missing credentials",
+			configs: []providerConfig{{
+				URL: "https://myregistry.com",
+			}},
+			need:    false,
+			wantErr: ErrCredentialsNotFound,
+		},
+	}
+
+	for idx := range testCases {
+		tt := testCases[idx]
+
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			need, err := needImageRegistryCredentialsConfiguration(tt.configs)
+			assert.ErrorIs(t, err, tt.wantErr)
+			assert.Equal(t, tt.need, need)
+		})
+	}
+}