From 4b230f305e2bdda65bd607da5c06bcff2dc7c82a Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Wed, 22 Nov 2023 19:46:07 +0530 Subject: [PATCH] helmrepo: allow OCI helmrepos to connect to insecure registries Signed-off-by: Sanskar Jaiswal --- internal/controller/helmchart_controller.go | 14 +++++-- .../controller/helmchart_controller_test.go | 28 +++++++++----- .../helmrepository_controller_oci.go | 4 +- .../helmrepository_controller_oci_test.go | 4 +- internal/controller/suite_test.go | 38 ++++++++----------- internal/helm/getter/client_opts.go | 1 - internal/helm/getter/client_opts_test.go | 4 +- internal/helm/registry/client.go | 11 ++++-- .../helm/repository/oci_chart_repository.go | 17 ++++++++- 9 files changed, 72 insertions(+), 49 deletions(-) diff --git a/internal/controller/helmchart_controller.go b/internal/controller/helmchart_controller.go index f840a85bc..8e5cbc796 100644 --- a/internal/controller/helmchart_controller.go +++ b/internal/controller/helmchart_controller.go @@ -548,7 +548,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * // this is needed because otherwise the credentials are stored in ~/.docker/config.json. // TODO@souleb: remove this once the registry move to Oras v2 // or rework to enable reusing credentials to avoid the unneccessary handshake operations - registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.TlsConfig, clientOpts.MustLoginToRegistry()) + registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.TlsConfig, clientOpts.MustLoginToRegistry(), repo.Spec.Insecure) if err != nil { e := serror.NewGeneric( fmt.Errorf("failed to construct Helm client: %w", err), @@ -586,11 +586,17 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * // Tell the chart repository to use the OCI client with the configured getter getterOpts = append(getterOpts, helmgetter.WithRegistryClient(registryClient)) - ociChartRepo, err := repository.NewOCIChartRepository(normalizedURL, + chartRepoOpts := []repository.OCIChartRepositoryOption{ repository.WithOCIGetter(r.Getters), repository.WithOCIGetterOptions(getterOpts), repository.WithOCIRegistryClient(registryClient), - repository.WithVerifiers(verifiers)) + repository.WithVerifiers(verifiers), + } + if repo.Spec.Insecure { + chartRepoOpts = append(chartRepoOpts, repository.WithInsecureHTTP()) + } + + ociChartRepo, err := repository.NewOCIChartRepository(normalizedURL, chartRepoOpts...) if err != nil { return chartRepoConfigErrorReturn(err, obj) } @@ -1003,7 +1009,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont var chartRepo repository.Downloader if helmreg.IsOCI(normalizedURL) { - registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.TlsConfig, clientOpts.MustLoginToRegistry()) + registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.TlsConfig, clientOpts.MustLoginToRegistry(), obj.Spec.Insecure) if err != nil { return nil, fmt.Errorf("failed to create registry client: %w", err) } diff --git a/internal/controller/helmchart_controller_test.go b/internal/controller/helmchart_controller_test.go index 796bfdcd4..0457f83b7 100644 --- a/internal/controller/helmchart_controller_test.go +++ b/internal/controller/helmchart_controller_test.go @@ -23,6 +23,7 @@ import ( "errors" "fmt" "io" + "net" "net/http" "os" "path" @@ -32,6 +33,7 @@ import ( "testing" "time" + "github.com/foxcpp/go-mockdns" . "github.com/onsi/gomega" coptions "github.com/sigstore/cosign/v2/cmd/cosign/cli/options" "github.com/sigstore/cosign/v2/cmd/cosign/cli/sign" @@ -1294,6 +1296,7 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) { Timeout: &metav1.Duration{Duration: timeout}, Provider: helmv1.GenericOCIProvider, Type: helmv1.HelmRepositoryTypeOCI, + Insecure: true, }, } obj := &helmv1.HelmChart{ @@ -1313,12 +1316,14 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) { } got, err := r.buildFromHelmRepository(context.TODO(), obj, repository, &b) - g.Expect(err != nil).To(Equal(tt.wantErr != nil)) if tt.wantErr != nil { + g.Expect(err).To(HaveOccurred()) g.Expect(reflect.TypeOf(err).String()).To(Equal(reflect.TypeOf(tt.wantErr).String())) g.Expect(err.Error()).To(ContainSubstring(tt.wantErr.Error())) + } else { + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(got).To(Equal(tt.want)) } - g.Expect(got).To(Equal(tt.want)) if tt.assertFunc != nil { tt.assertFunc(g, obj, b) @@ -1332,6 +1337,14 @@ func TestHelmChartReconciler_buildFromTarballArtifact(t *testing.T) { tmpDir := t.TempDir() + // Unpatch the changes we make to the default DNS resolver in `setupRegistryServer()`. + // This is required because the changes somehow also cause remote lookups to fail and + // this test tests functionality related to remote dependencies. + mockdns.UnpatchNet(net.DefaultResolver) + defer func() { + testRegistryServer.dnsServer.PatchNet(net.DefaultResolver) + }() + storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords) g.Expect(err).ToNot(HaveOccurred()) @@ -2429,9 +2442,6 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { workspaceDir := t.TempDir() - if tt.insecure { - tt.registryOpts.disableDNSMocking = true - } server, err := setupRegistryServer(ctx, workspaceDir, tt.registryOpts) g.Expect(err).NotTo(HaveOccurred()) t.Cleanup(func() { @@ -2456,6 +2466,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { Type: helmv1.HelmRepositoryTypeOCI, Provider: helmv1.GenericOCIProvider, URL: fmt.Sprintf("oci://%s/testrepo", server.registryHost), + Insecure: tt.insecure, }, } @@ -2725,9 +2736,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T g := NewWithT(t) tmpDir := t.TempDir() - server, err := setupRegistryServer(ctx, tmpDir, registryOptions{ - disableDNSMocking: true, - }) + server, err := setupRegistryServer(ctx, tmpDir, registryOptions{}) g.Expect(err).ToNot(HaveOccurred()) t.Cleanup(func() { server.Close() @@ -2870,6 +2879,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T Timeout: &metav1.Duration{Duration: timeout}, Provider: helmv1.GenericOCIProvider, Type: helmv1.HelmRepositoryTypeOCI, + Insecure: true, }, } @@ -2924,7 +2934,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T Upload: true, SkipConfirmation: true, TlogUpload: false, - Registry: coptions.RegistryOptions{Keychain: oci.Anonymous{}, AllowInsecure: true}, + Registry: coptions.RegistryOptions{Keychain: oci.Anonymous{}, AllowHTTPRegistry: true}, }, []string{fmt.Sprintf("%s/testrepo/%s:%s", server.registryHost, metadata.Name, metadata.Version)}) g.Expect(err).ToNot(HaveOccurred()) diff --git a/internal/controller/helmrepository_controller_oci.go b/internal/controller/helmrepository_controller_oci.go index e25eaf4fd..9306c1564 100644 --- a/internal/controller/helmrepository_controller_oci.go +++ b/internal/controller/helmrepository_controller_oci.go @@ -92,7 +92,7 @@ type HelmRepositoryOCIReconciler struct { // and an optional file name. // The file is used to store the registry client credentials. // The caller is responsible for deleting the file. -type RegistryClientGeneratorFunc func(tlsConfig *tls.Config, isLogin bool) (*helmreg.Client, string, error) +type RegistryClientGeneratorFunc func(tlsConfig *tls.Config, isLogin, insecure bool) (*helmreg.Client, string, error) func (r *HelmRepositoryOCIReconciler) SetupWithManager(mgr ctrl.Manager) error { return r.SetupWithManagerAndOptions(mgr, HelmRepositoryReconcilerOptions{}) @@ -332,7 +332,7 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, sp *patch.S } // Create registry client and login if needed. - registryClient, file, err := r.RegistryClientGenerator(clientOpts.TlsConfig, clientOpts.MustLoginToRegistry()) + registryClient, file, err := r.RegistryClientGenerator(clientOpts.TlsConfig, clientOpts.MustLoginToRegistry(), obj.Spec.Insecure) if err != nil { e := fmt.Errorf("failed to create registry client: %w", err) conditions.MarkFalse(obj, meta.ReadyCondition, meta.FailedReason, e.Error()) diff --git a/internal/controller/helmrepository_controller_oci_test.go b/internal/controller/helmrepository_controller_oci_test.go index 1d5361c91..ee812d2f9 100644 --- a/internal/controller/helmrepository_controller_oci_test.go +++ b/internal/controller/helmrepository_controller_oci_test.go @@ -376,9 +376,6 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { WithStatusSubresource(&helmv1.HelmRepository{}) workspaceDir := t.TempDir() - if tt.insecure { - tt.registryOpts.disableDNSMocking = true - } server, err := setupRegistryServer(ctx, workspaceDir, tt.registryOpts) g.Expect(err).NotTo(HaveOccurred()) t.Cleanup(func() { @@ -396,6 +393,7 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { Type: helmv1.HelmRepositoryTypeOCI, Provider: helmv1.GenericOCIProvider, URL: fmt.Sprintf("oci://%s", server.registryHost), + Insecure: tt.insecure, }, } diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index faa775d8a..0bb9e0db3 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -128,11 +128,6 @@ type registryOptions struct { withBasicAuth bool withTLS bool withClientCertAuth bool - // Allow disbaling DNS mocking since Helm OCI doesn't yet suppot - // insecure OCI registries, which means we need Docker's automatic - // connection downgrading if the registry is hosted on localhost. - // Once Helm OCI supports insecure registries, we can get rid of this. - disableDNSMocking bool } func setupRegistryServer(ctx context.Context, workspaceDir string, opts registryOptions) (*registryClientTestServer, error) { @@ -159,27 +154,23 @@ func setupRegistryServer(ctx context.Context, workspaceDir string, opts registry return nil, fmt.Errorf("failed to get free port: %s", err) } - server.registryHost = fmt.Sprintf("localhost:%d", port) - // Change the registry host to a host which is not localhost and // mock DNS to map example.com to 127.0.0.1. // This is required because Docker enforces HTTP if the registry // is hosted on localhost/127.0.0.1. - if !opts.disableDNSMocking { - server.registryHost = fmt.Sprintf("example.com:%d", port) - // Disable DNS server logging as it is extremely chatty. - dnsLog := log.Default() - dnsLog.SetOutput(io.Discard) - server.dnsServer, err = mockdns.NewServerWithLogger(map[string]mockdns.Zone{ - "example.com.": { - A: []string{"127.0.0.1"}, - }, - }, dnsLog, false) - if err != nil { - return nil, err - } - server.dnsServer.PatchNet(net.DefaultResolver) + server.registryHost = fmt.Sprintf("example.com:%d", port) + // Disable DNS server logging as it is extremely chatty. + dnsLog := log.Default() + dnsLog.SetOutput(io.Discard) + server.dnsServer, err = mockdns.NewServerWithLogger(map[string]mockdns.Zone{ + "example.com.": { + A: []string{"127.0.0.1"}, + }, + }, dnsLog, false) + if err != nil { + return nil, err } + server.dnsServer.PatchNet(net.DefaultResolver) config.HTTP.Addr = fmt.Sprintf(":%d", port) config.HTTP.DrainTimeout = time.Duration(10) * time.Second @@ -220,6 +211,8 @@ func setupRegistryServer(ctx context.Context, workspaceDir string, opts registry return nil, fmt.Errorf("failed to create TLS configured HTTP client: %s", err) } clientOpts = append(clientOpts, helmreg.ClientOptHTTPClient(httpClient)) + } else { + clientOpts = append(clientOpts, helmreg.ClientOptPlainHTTP()) } // setup logger options @@ -313,8 +306,7 @@ func TestMain(m *testing.M) { panic(fmt.Sprintf("failed to create workspace directory: %v", err)) } testRegistryServer, err = setupRegistryServer(ctx, testWorkspaceDir, registryOptions{ - withBasicAuth: true, - disableDNSMocking: true, + withBasicAuth: true, }) if err != nil { panic(fmt.Sprintf("Failed to create a test registry server: %v", err)) diff --git a/internal/helm/getter/client_opts.go b/internal/helm/getter/client_opts.go index e028c51bb..ce7620ca1 100644 --- a/internal/helm/getter/client_opts.go +++ b/internal/helm/getter/client_opts.go @@ -74,7 +74,6 @@ func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmReposit helmgetter.WithURL(url), helmgetter.WithTimeout(obj.Spec.Timeout.Duration), helmgetter.WithPassCredentialsAll(obj.Spec.PassCredentials), - helmgetter.WithPlainHTTP(obj.Spec.Insecure), }, } ociRepo := obj.Spec.Type == helmv1.HelmRepositoryTypeOCI diff --git a/internal/helm/getter/client_opts_test.go b/internal/helm/getter/client_opts_test.go index dd286722b..91bcd32f8 100644 --- a/internal/helm/getter/client_opts_test.go +++ b/internal/helm/getter/client_opts_test.go @@ -68,7 +68,7 @@ func TestGetClientOpts(t *testing.T) { }, afterFunc: func(t *WithT, hcOpts *ClientOpts) { t.Expect(hcOpts.TlsConfig).ToNot(BeNil()) - t.Expect(len(hcOpts.GetterOpts)).To(Equal(5)) + t.Expect(len(hcOpts.GetterOpts)).To(Equal(4)) }, }, { @@ -85,7 +85,7 @@ func TestGetClientOpts(t *testing.T) { }, afterFunc: func(t *WithT, hcOpts *ClientOpts) { t.Expect(hcOpts.TlsConfig).ToNot(BeNil()) - t.Expect(len(hcOpts.GetterOpts)).To(Equal(5)) + t.Expect(len(hcOpts.GetterOpts)).To(Equal(4)) }, err: ErrDeprecatedTLSConfig, }, diff --git a/internal/helm/registry/client.go b/internal/helm/registry/client.go index 7ac0d3d0b..8f2b315c2 100644 --- a/internal/helm/registry/client.go +++ b/internal/helm/registry/client.go @@ -29,7 +29,7 @@ import ( // ClientGenerator generates a registry client and a temporary credential file. // The client is meant to be used for a single reconciliation. // The file is meant to be used for a single reconciliation and deleted after. -func ClientGenerator(tlsConfig *tls.Config, isLogin bool) (*registry.Client, string, error) { +func ClientGenerator(tlsConfig *tls.Config, isLogin, insecureHTTP bool) (*registry.Client, string, error) { if isLogin { // create a temporary file to store the credentials // this is needed because otherwise the credentials are stored in ~/.docker/config.json. @@ -39,7 +39,7 @@ func ClientGenerator(tlsConfig *tls.Config, isLogin bool) (*registry.Client, str } var errs []error - rClient, err := newClient(credentialsFile.Name(), tlsConfig) + rClient, err := newClient(credentialsFile.Name(), tlsConfig, insecureHTTP) if err != nil { errs = append(errs, err) // attempt to delete the temporary file @@ -54,17 +54,20 @@ func ClientGenerator(tlsConfig *tls.Config, isLogin bool) (*registry.Client, str return rClient, credentialsFile.Name(), nil } - rClient, err := newClient("", tlsConfig) + rClient, err := newClient("", tlsConfig, insecureHTTP) if err != nil { return nil, "", err } return rClient, "", nil } -func newClient(credentialsFile string, tlsConfig *tls.Config) (*registry.Client, error) { +func newClient(credentialsFile string, tlsConfig *tls.Config, insecureHTTP bool) (*registry.Client, error) { opts := []registry.ClientOption{ registry.ClientOptWriter(io.Discard), } + if insecureHTTP { + opts = append(opts, registry.ClientOptPlainHTTP()) + } if tlsConfig != nil { opts = append(opts, registry.ClientOptHTTPClient(&http.Client{ Transport: &http.Transport{ diff --git a/internal/helm/repository/oci_chart_repository.go b/internal/helm/repository/oci_chart_repository.go index 6a119183b..d1244e7c7 100644 --- a/internal/helm/repository/oci_chart_repository.go +++ b/internal/helm/repository/oci_chart_repository.go @@ -75,6 +75,9 @@ type OCIChartRepository struct { // verifiers is a list of verifiers to use when verifying a chart. verifiers []oci.Verifier + + // insecureHTTP indicates that the chart is hosted on an insecure registry. + insecure bool } // OCIChartRepositoryOption is a function that can be passed to NewOCIChartRepository @@ -89,6 +92,13 @@ func WithVerifiers(verifiers []oci.Verifier) OCIChartRepositoryOption { } } +func WithInsecureHTTP() OCIChartRepositoryOption { + return func(r *OCIChartRepository) error { + r.insecure = true + return nil + } +} + // WithOCIRegistryClient returns a ChartRepositoryOption that will set the registry client func WithOCIRegistryClient(client RegistryClient) OCIChartRepositoryOption { return func(r *OCIChartRepository) error { @@ -358,7 +368,12 @@ func (r *OCIChartRepository) VerifyChart(ctx context.Context, chart *repo.ChartV return fmt.Errorf("chart '%s' has no downloadable URLs", chart.Name) } - ref, err := name.ParseReference(strings.TrimPrefix(chart.URLs[0], fmt.Sprintf("%s://", registry.OCIScheme))) + var nameOpts []name.Option + if r.insecure { + nameOpts = append(nameOpts, name.Insecure) + } + + ref, err := name.ParseReference(strings.TrimPrefix(chart.URLs[0], fmt.Sprintf("%s://", registry.OCIScheme)), nameOpts...) if err != nil { return fmt.Errorf("invalid chart reference: %s", err) }