From a1b1224136024143e1d059a7bf4e9a0bd8575bb3 Mon Sep 17 00:00:00 2001 From: Dimitri Koshkin Date: Sun, 2 Mar 2025 13:24:21 -0800 Subject: [PATCH 1/2] fix: correctly handle multiple registries with the same Host Write out a single CA file for multiple registries with the same url.Host. Return an error if the CA content for those registries do not match. --- .../mutation/mirrors/containerd_files.go | 68 ++++++++++++++++--- .../mutation/mirrors/containerd_files_test.go | 63 ++++++++++++++++- 2 files changed, 119 insertions(+), 12 deletions(-) diff --git a/pkg/handlers/generic/mutation/mirrors/containerd_files.go b/pkg/handlers/generic/mutation/mirrors/containerd_files.go index b0aedce3e..4363af2ff 100644 --- a/pkg/handlers/generic/mutation/mirrors/containerd_files.go +++ b/pkg/handlers/generic/mutation/mirrors/containerd_files.go @@ -9,6 +9,7 @@ import ( "fmt" "net/url" "path" + "slices" "strings" "text/template" @@ -139,21 +140,17 @@ func generateRegistryCACertFiles( var files []cabpkv1.File //nolint:prealloc // We don't know the size of the slice yet. - for _, config := range configs { - if config.CASecretName == "" { - continue - } - - registryCACertPathOnRemote, err := config.filePathFromURL() - if err != nil { - return nil, fmt.Errorf("failed generating CA certificate file path from URL: %w", err) - } + filesToGenerate, err := registryCACertFiles(configs) + if err != nil { + return nil, err + } + for _, file := range filesToGenerate { files = append(files, cabpkv1.File{ - Path: registryCACertPathOnRemote, + Path: file.path, Permissions: "0600", ContentFrom: &cabpkv1.FileSource{ Secret: cabpkv1.SecretFileSource{ - Name: config.CASecretName, + Name: file.caSecretName, Key: secretKeyForCACert, }, }, @@ -189,3 +186,52 @@ func formatURLForContainerd(uri string) (string, error) { // using path.Join on all elements incorrectly drops a "/" from "https://" return fmt.Sprintf("%s/%s", mirror, mirrorPath), nil } + +type containerdConfigFile struct { + path string + url string + caSecretName string + caCert string +} + +// registryCACertFiles returns a list of CA certificate files +// that should be generated for the given containerd configurations. +// If any of the provided configurations share the same url.Host only a single file will be generated. +// An error will be returned, if the CA certificate content for the same URL.Host do not match. +func registryCACertFiles(configs []containerdConfig) ([]containerdConfigFile, error) { + filesToGenerate := make([]containerdConfigFile, 0) + for _, config := range configs { + // Skip if CA certificate is not provided. + if config.CASecretName == "" { + continue + } + registryCACertPathOnRemote, err := config.filePathFromURL() + if err != nil { + return nil, fmt.Errorf("failed generating CA certificate file path from URL: %w", err) + } + + foundIndex := slices.IndexFunc(filesToGenerate, func(f containerdConfigFile) bool { + return registryCACertPathOnRemote == f.path + }) + // File not already found and needs to be generated. + if foundIndex == -1 { + filesToGenerate = append(filesToGenerate, containerdConfigFile{ + path: registryCACertPathOnRemote, + url: config.URL, + caSecretName: config.CASecretName, + caCert: config.CACert, + }) + continue + } + // File is already in the list, check if the CA certificate content matches. + if config.CACert != filesToGenerate[foundIndex].caCert { + return nil, fmt.Errorf( + "CA certificate content for %q does not match one for %q", + config.URL, + filesToGenerate[foundIndex].url, + ) + } + } + + return filesToGenerate, nil +} diff --git a/pkg/handlers/generic/mutation/mirrors/containerd_files_test.go b/pkg/handlers/generic/mutation/mirrors/containerd_files_test.go index b2abc61cf..6a69b1067 100644 --- a/pkg/handlers/generic/mutation/mirrors/containerd_files_test.go +++ b/pkg/handlers/generic/mutation/mirrors/containerd_files_test.go @@ -4,6 +4,7 @@ package mirrors import ( + "fmt" "testing" "github.com/stretchr/testify/assert" @@ -156,6 +157,7 @@ func Test_generateRegistryCACertFiles(t *testing.T) { name string configs []containerdConfig want []cabpkv1.File + wantErr error }{ { name: "ECR mirror image registry with no CA certificate", @@ -173,6 +175,39 @@ func Test_generateRegistryCACertFiles(t *testing.T) { { URL: "https://registry.example.com", CASecretName: "my-registry-credentials-secret", + CACert: "-----BEGIN CERTIFICATE-----", + Mirror: true, + }, + }, + want: []cabpkv1.File{ + { + Path: "/etc/containerd/certs.d/registry.example.com/ca.crt", + Owner: "", + Permissions: "0600", + Encoding: "", + Append: false, + ContentFrom: &cabpkv1.FileSource{ + Secret: cabpkv1.SecretFileSource{ + Name: "my-registry-credentials-secret", + Key: "ca.crt", + }, + }, + }, + }, + }, + { + name: "Mirror image registry and registry with the same CA certificate", + configs: []containerdConfig{ + { + URL: "https://registry.example.com/mirror", + CASecretName: "my-registry-credentials-secret", + CACert: "-----BEGIN CERTIFICATE-----", + Mirror: true, + }, + { + URL: "https://registry.example.com/library", + CASecretName: "my-registry-credentials-secret", + CACert: "-----BEGIN CERTIFICATE-----", Mirror: true, }, }, @@ -192,13 +227,39 @@ func Test_generateRegistryCACertFiles(t *testing.T) { }, }, }, + { + name: "Mirror image registry and registry with different CA certificates", + configs: []containerdConfig{ + { + URL: "https://registry.example.com/mirror", + CASecretName: "my-registry-credentials-secret", + CACert: "-----BEGIN CERTIFICATE-----", + Mirror: true, + }, + { + URL: "https://registry.example.com/library", + CASecretName: "my-registry-credentials-secret", + CACert: "-----BEGIN CERTIFICATE----------END CERTIFICATE-----", + Mirror: true, + }, + }, + wantErr: fmt.Errorf( + "CA certificate content for \"https://registry.example.com/library\" " + + "does not match one for \"https://registry.example.com/mirror\"", + ), + }, } for idx := range tests { tt := tests[idx] t.Run(tt.name, func(t *testing.T) { t.Parallel() file, err := generateRegistryCACertFiles(tt.configs) - require.NoError(t, err) + if tt.wantErr != nil { + assert.Equal(t, tt.wantErr.Error(), err.Error()) + return + } else { + require.NoError(t, err) + } assert.Equal(t, tt.want, file) }) } From 20958344b437a24df2fcf85e3199ff90a2786703 Mon Sep 17 00:00:00 2001 From: Jimmi Dyson Date: Mon, 3 Mar 2025 10:35:00 +0000 Subject: [PATCH 2/2] fixup! refactor: Use sentinel error and lo.Find --- .../mutation/mirrors/containerd_files.go | 56 ++++++++++++------- .../mutation/mirrors/containerd_files_test.go | 41 +++++++------- 2 files changed, 57 insertions(+), 40 deletions(-) diff --git a/pkg/handlers/generic/mutation/mirrors/containerd_files.go b/pkg/handlers/generic/mutation/mirrors/containerd_files.go index 4363af2ff..0af0aa279 100644 --- a/pkg/handlers/generic/mutation/mirrors/containerd_files.go +++ b/pkg/handlers/generic/mutation/mirrors/containerd_files.go @@ -6,13 +6,14 @@ package mirrors import ( "bytes" _ "embed" + "errors" "fmt" "net/url" "path" - "slices" "strings" "text/template" + "github.com/samber/lo" cabpkv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1beta1" "github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/pkg/common" @@ -194,12 +195,17 @@ type containerdConfigFile struct { caCert string } +var ErrConflictingRegistryCACertificates = errors.New( + "conflicting CA certificate specified for registry host", +) + // registryCACertFiles returns a list of CA certificate files // that should be generated for the given containerd configurations. // If any of the provided configurations share the same url.Host only a single file will be generated. // An error will be returned, if the CA certificate content for the same URL.Host do not match. func registryCACertFiles(configs []containerdConfig) ([]containerdConfigFile, error) { - filesToGenerate := make([]containerdConfigFile, 0) + filesToGenerate := make([]containerdConfigFile, 0, len(configs)) + for _, config := range configs { // Skip if CA certificate is not provided. if config.CASecretName == "" { @@ -210,27 +216,35 @@ func registryCACertFiles(configs []containerdConfig) ([]containerdConfigFile, er return nil, fmt.Errorf("failed generating CA certificate file path from URL: %w", err) } - foundIndex := slices.IndexFunc(filesToGenerate, func(f containerdConfigFile) bool { - return registryCACertPathOnRemote == f.path - }) - // File not already found and needs to be generated. - if foundIndex == -1 { - filesToGenerate = append(filesToGenerate, containerdConfigFile{ - path: registryCACertPathOnRemote, - url: config.URL, - caSecretName: config.CASecretName, - caCert: config.CACert, - }) + existingFileToGenerate, existing := lo.Find( + filesToGenerate, + func(f containerdConfigFile) bool { + return registryCACertPathOnRemote == f.path + }, + ) + // File exists and needs to be checked for conflicts. + if existing { + if config.CACert != existingFileToGenerate.caCert { + return nil, fmt.Errorf( + "%w: %q (from secrets %q and %q)", + ErrConflictingRegistryCACertificates, + config.URL, + config.CASecretName, + existingFileToGenerate.caSecretName, + ) + } + + // File already found with matching content - skipping. continue } - // File is already in the list, check if the CA certificate content matches. - if config.CACert != filesToGenerate[foundIndex].caCert { - return nil, fmt.Errorf( - "CA certificate content for %q does not match one for %q", - config.URL, - filesToGenerate[foundIndex].url, - ) - } + + // File not already found and needs to be generated. + filesToGenerate = append(filesToGenerate, containerdConfigFile{ + path: registryCACertPathOnRemote, + url: config.URL, + caSecretName: config.CASecretName, + caCert: config.CACert, + }) } return filesToGenerate, nil diff --git a/pkg/handlers/generic/mutation/mirrors/containerd_files_test.go b/pkg/handlers/generic/mutation/mirrors/containerd_files_test.go index 6a69b1067..9b6314214 100644 --- a/pkg/handlers/generic/mutation/mirrors/containerd_files_test.go +++ b/pkg/handlers/generic/mutation/mirrors/containerd_files_test.go @@ -4,7 +4,6 @@ package mirrors import ( - "fmt" "testing" "github.com/stretchr/testify/assert" @@ -41,7 +40,6 @@ func Test_generateContainerdDefaultHostsFile(t *testing.T) { override_path = true `, }, - wantErr: nil, }, { name: "ECR mirror image registry with a path and no CA certificate", @@ -64,7 +62,6 @@ func Test_generateContainerdDefaultHostsFile(t *testing.T) { override_path = true `, }, - wantErr: nil, }, { name: "Mirror image registry with a CA and an image registry with no CA certificate", @@ -92,7 +89,6 @@ func Test_generateContainerdDefaultHostsFile(t *testing.T) { override_path = true `, }, - wantErr: nil, }, { name: "Mirror image registry with a CA and an image registry with a CA", @@ -125,7 +121,6 @@ func Test_generateContainerdDefaultHostsFile(t *testing.T) { override_path = true `, }, - wantErr: nil, }, { name: "Image registry with a CA", @@ -136,8 +131,6 @@ func Test_generateContainerdDefaultHostsFile(t *testing.T) { }, }, want: nil, - - wantErr: nil, }, } for idx := range tests { @@ -208,7 +201,7 @@ func Test_generateRegistryCACertFiles(t *testing.T) { URL: "https://registry.example.com/library", CASecretName: "my-registry-credentials-secret", CACert: "-----BEGIN CERTIFICATE-----", - Mirror: true, + Mirror: false, }, }, want: []cabpkv1.File{ @@ -240,13 +233,28 @@ func Test_generateRegistryCACertFiles(t *testing.T) { URL: "https://registry.example.com/library", CASecretName: "my-registry-credentials-secret", CACert: "-----BEGIN CERTIFICATE----------END CERTIFICATE-----", - Mirror: true, + Mirror: false, + }, + }, + wantErr: ErrConflictingRegistryCACertificates, + }, + { + name: "Multiple image registries with different CA certificates", + configs: []containerdConfig{ + { + URL: "https://registry.example.com/mirror", + CASecretName: "my-registry-credentials-secret", + CACert: "-----BEGIN CERTIFICATE-----", + Mirror: false, + }, + { + URL: "https://registry.example.com/library", + CASecretName: "my-registry-credentials-secret", + CACert: "-----BEGIN CERTIFICATE----------END CERTIFICATE-----", + Mirror: false, }, }, - wantErr: fmt.Errorf( - "CA certificate content for \"https://registry.example.com/library\" " + - "does not match one for \"https://registry.example.com/mirror\"", - ), + wantErr: ErrConflictingRegistryCACertificates, }, } for idx := range tests { @@ -254,12 +262,7 @@ func Test_generateRegistryCACertFiles(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() file, err := generateRegistryCACertFiles(tt.configs) - if tt.wantErr != nil { - assert.Equal(t, tt.wantErr.Error(), err.Error()) - return - } else { - require.NoError(t, err) - } + require.ErrorIs(t, err, tt.wantErr) assert.Equal(t, tt.want, file) }) }