From abb1e9ccb4926db7016891186c2f3439e6ac5867 Mon Sep 17 00:00:00 2001 From: Dimitri Koshkin Date: Wed, 14 Feb 2024 19:56:54 -0800 Subject: [PATCH] fix: handle globalImageRegistryMirror with no credentials --- .../credential_provider_config_files.go | 61 +++++++---- .../credential_provider_config_files_test.go | 7 -- .../imageregistries/credentials/inject.go | 36 +++++++ .../credentials/inject_test.go | 102 ++++++++++++++++++ 4 files changed, 176 insertions(+), 30 deletions(-) create mode 100644 pkg/handlers/generic/mutation/imageregistries/credentials/inject_test.go diff --git a/pkg/handlers/generic/mutation/imageregistries/credentials/credential_provider_config_files.go b/pkg/handlers/generic/mutation/imageregistries/credentials/credential_provider_config_files.go index 496488817..ff3fce2f2 100644 --- a/pkg/handlers/generic/mutation/imageregistries/credentials/credential_provider_config_files.go +++ b/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,42 @@ func (c providerConfig) isCredentialsEmpty() bool { c.Password == "" } +func (c providerConfig) isSupportedProvider() (bool, error) { + registryHostWithPath, err := c.registryHostWithPath() + if err != nil { + return false, fmt.Errorf( + "failed to get registry host with path: %w", + err, + ) + } + + supportedProvider, err := credentialprovider.URLMatchesSupportedProvider( + registryHostWithPath, + ) + if err != nil { + return false, fmt.Errorf( + "failed to check if registry matches a supported provider: %w", + err, + ) + } + + return supportedProvider, 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 +154,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( diff --git a/pkg/handlers/generic/mutation/imageregistries/credentials/credential_provider_config_files_test.go b/pkg/handlers/generic/mutation/imageregistries/credentials/credential_provider_config_files_test.go index abaaebc92..cf95732f1 100644 --- a/pkg/handlers/generic/mutation/imageregistries/credentials/credential_provider_config_files_test.go +++ b/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{{ diff --git a/pkg/handlers/generic/mutation/imageregistries/credentials/inject.go b/pkg/handlers/generic/mutation/imageregistries/credentials/inject.go index 130524d44..a8f8ec5f6 100644 --- a/pkg/handlers/generic/mutation/imageregistries/credentials/inject.go +++ b/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 { @@ -128,6 +131,15 @@ func (h *imageRegistriesPatchHandler) Mutate( ) } + needCredentials, err := needImageRegistryCredentials(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, @@ -362,3 +374,27 @@ func secretForImageRegistryCredentials( err := c.Get(ctx, key, secret) return secret, err } + +// needImageRegistryCredentials will return true if all the providerConfigs have valid credentials +// will return false if there is only a single providerConfig for a mirror with no credentials +// otherwise will return an error +func needImageRegistryCredentials(configs []providerConfig) (bool, error) { + for _, config := range configs { + supportedProvider, err := config.isSupportedProvider() + if err != nil { + return false, + fmt.Errorf("error determining if Image Registry is a supported provider: %w", err) + } + if config.isCredentialsEmpty() && !supportedProvider { + 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 +} diff --git a/pkg/handlers/generic/mutation/imageregistries/credentials/inject_test.go b/pkg/handlers/generic/mutation/imageregistries/credentials/inject_test.go new file mode 100644 index 000000000..1005afeb1 --- /dev/null +++ b/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_needImageRegistryCredentials(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 := needImageRegistryCredentials(tt.configs) + assert.ErrorIs(t, err, tt.wantErr) + assert.Equal(t, tt.need, need) + }) + } +}