Skip to content

Commit 4086c25

Browse files
aryan9600aryan9600
committed
helmrepo: allow OCI helmrepos to connect to insecure registries
Signed-off-by: Sanskar Jaiswal <[email protected]>
1 parent 6e78779 commit 4086c25

File tree

7 files changed

+70
-45
lines changed

7 files changed

+70
-45
lines changed

‎internal/controller/helmchart_controller.go

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ type HelmChartReconciler struct {
144144
// and an optional file name.
145145
// The file is used to store the registry client credentials.
146146
// The caller is responsible for deleting the file.
147-
type RegistryClientGeneratorFunc func(tlsConfig *tls.Config, isLogin bool) (*helmreg.Client, string, error)
147+
type RegistryClientGeneratorFunc func(tlsConfig *tls.Config, isLogin, insecure bool) (*helmreg.Client, string, error)
148148

149149
func (r *HelmChartReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager) error {
150150
return r.SetupWithManagerAndOptions(ctx, mgr, HelmChartReconcilerOptions{})
@@ -555,7 +555,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
555555
// this is needed because otherwise the credentials are stored in ~/.docker/config.json.
556556
// TODO@souleb: remove this once the registry move to Oras v2
557557
// or rework to enable reusing credentials to avoid the unneccessary handshake operations
558-
registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.TlsConfig, clientOpts.MustLoginToRegistry())
558+
registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.TlsConfig, clientOpts.MustLoginToRegistry(), repo.Spec.Insecure)
559559
if err != nil {
560560
e := serror.NewGeneric(
561561
fmt.Errorf("failed to construct Helm client: %w", err),
@@ -593,11 +593,17 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
593593

594594
// Tell the chart repository to use the OCI client with the configured getter
595595
getterOpts = append(getterOpts, helmgetter.WithRegistryClient(registryClient))
596-
ociChartRepo, err := repository.NewOCIChartRepository(normalizedURL,
596+
chartRepoOpts := []repository.OCIChartRepositoryOption{
597597
repository.WithOCIGetter(r.Getters),
598598
repository.WithOCIGetterOptions(getterOpts),
599599
repository.WithOCIRegistryClient(registryClient),
600-
repository.WithVerifiers(verifiers))
600+
repository.WithVerifiers(verifiers),
601+
}
602+
if repo.Spec.Insecure {
603+
chartRepoOpts = append(chartRepoOpts, repository.WithInsecureHTTP())
604+
}
605+
606+
ociChartRepo, err := repository.NewOCIChartRepository(normalizedURL, chartRepoOpts...)
601607
if err != nil {
602608
return chartRepoConfigErrorReturn(err, obj)
603609
}
@@ -1010,7 +1016,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
10101016

10111017
var chartRepo repository.Downloader
10121018
if helmreg.IsOCI(normalizedURL) {
1013-
registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.TlsConfig, clientOpts.MustLoginToRegistry())
1019+
registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.TlsConfig, clientOpts.MustLoginToRegistry(), obj.Spec.Insecure)
10141020
if err != nil {
10151021
return nil, fmt.Errorf("failed to create registry client: %w", err)
10161022
}

‎internal/controller/helmchart_controller_test.go

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
"errors"
2424
"fmt"
2525
"io"
26+
"net"
2627
"net/http"
2728
"os"
2829
"path"
@@ -32,6 +33,7 @@ import (
3233
"testing"
3334
"time"
3435

36+
"github.com/foxcpp/go-mockdns"
3537
. "github.com/onsi/gomega"
3638
coptions "github.com/sigstore/cosign/v2/cmd/cosign/cli/options"
3739
"github.com/sigstore/cosign/v2/cmd/cosign/cli/sign"
@@ -1295,6 +1297,7 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
12951297
Timeout: &metav1.Duration{Duration: timeout},
12961298
Provider: helmv1.GenericOCIProvider,
12971299
Type: helmv1.HelmRepositoryTypeOCI,
1300+
Insecure: true,
12981301
},
12991302
}
13001303
obj := &helmv1.HelmChart{
@@ -1314,12 +1317,14 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
13141317
}
13151318
got, err := r.buildFromHelmRepository(context.TODO(), obj, repository, &b)
13161319

1317-
g.Expect(err != nil).To(Equal(tt.wantErr != nil))
13181320
if tt.wantErr != nil {
1321+
g.Expect(err).To(HaveOccurred())
13191322
g.Expect(reflect.TypeOf(err).String()).To(Equal(reflect.TypeOf(tt.wantErr).String()))
13201323
g.Expect(err.Error()).To(ContainSubstring(tt.wantErr.Error()))
1324+
} else {
1325+
g.Expect(err).ToNot(HaveOccurred())
1326+
g.Expect(got).To(Equal(tt.want))
13211327
}
1322-
g.Expect(got).To(Equal(tt.want))
13231328

13241329
if tt.assertFunc != nil {
13251330
tt.assertFunc(g, obj, b)
@@ -1333,6 +1338,14 @@ func TestHelmChartReconciler_buildFromTarballArtifact(t *testing.T) {
13331338

13341339
tmpDir := t.TempDir()
13351340

1341+
// Unpatch the changes we make to the default DNS resolver in `setupRegistryServer()`.
1342+
// This is required because the changes somehow also cause remote lookups to fail and
1343+
// this test tests functionality related to remote dependencies.
1344+
mockdns.UnpatchNet(net.DefaultResolver)
1345+
defer func() {
1346+
testRegistryServer.dnsServer.PatchNet(net.DefaultResolver)
1347+
}()
1348+
13361349
storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords)
13371350
g.Expect(err).ToNot(HaveOccurred())
13381351

@@ -2430,9 +2443,6 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
24302443

24312444
workspaceDir := t.TempDir()
24322445

2433-
if tt.insecure {
2434-
tt.registryOpts.disableDNSMocking = true
2435-
}
24362446
server, err := setupRegistryServer(ctx, workspaceDir, tt.registryOpts)
24372447
g.Expect(err).NotTo(HaveOccurred())
24382448
t.Cleanup(func() {
@@ -2457,6 +2467,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
24572467
Type: helmv1.HelmRepositoryTypeOCI,
24582468
Provider: helmv1.GenericOCIProvider,
24592469
URL: fmt.Sprintf("oci://%s/testrepo", server.registryHost),
2470+
Insecure: tt.insecure,
24602471
},
24612472
}
24622473

@@ -2726,9 +2737,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T
27262737
g := NewWithT(t)
27272738

27282739
tmpDir := t.TempDir()
2729-
server, err := setupRegistryServer(ctx, tmpDir, registryOptions{
2730-
disableDNSMocking: true,
2731-
})
2740+
server, err := setupRegistryServer(ctx, tmpDir, registryOptions{})
27322741
g.Expect(err).ToNot(HaveOccurred())
27332742
t.Cleanup(func() {
27342743
server.Close()
@@ -2871,6 +2880,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T
28712880
Timeout: &metav1.Duration{Duration: timeout},
28722881
Provider: helmv1.GenericOCIProvider,
28732882
Type: helmv1.HelmRepositoryTypeOCI,
2883+
Insecure: true,
28742884
},
28752885
}
28762886

@@ -2925,7 +2935,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T
29252935
Upload: true,
29262936
SkipConfirmation: true,
29272937
TlogUpload: false,
2928-
Registry: coptions.RegistryOptions{Keychain: oci.Anonymous{}, AllowInsecure: true},
2938+
Registry: coptions.RegistryOptions{Keychain: oci.Anonymous{}, AllowHTTPRegistry: true},
29292939
},
29302940
[]string{fmt.Sprintf("%s/testrepo/%s:%s", server.registryHost, metadata.Name, metadata.Version)})
29312941
g.Expect(err).ToNot(HaveOccurred())

‎internal/controller/suite_test.go

Lines changed: 15 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -127,11 +127,6 @@ type registryOptions struct {
127127
withBasicAuth bool
128128
withTLS bool
129129
withClientCertAuth bool
130-
// Allow disbaling DNS mocking since Helm OCI doesn't yet suppot
131-
// insecure OCI registries, which means we need Docker's automatic
132-
// connection downgrading if the registry is hosted on localhost.
133-
// Once Helm OCI supports insecure registries, we can get rid of this.
134-
disableDNSMocking bool
135130
}
136131

137132
func setupRegistryServer(ctx context.Context, workspaceDir string, opts registryOptions) (*registryClientTestServer, error) {
@@ -158,27 +153,23 @@ func setupRegistryServer(ctx context.Context, workspaceDir string, opts registry
158153
return nil, fmt.Errorf("failed to get free port: %s", err)
159154
}
160155

161-
server.registryHost = fmt.Sprintf("localhost:%d", port)
162-
163156
// Change the registry host to a host which is not localhost and
164157
// mock DNS to map example.com to 127.0.0.1.
165158
// This is required because Docker enforces HTTP if the registry
166159
// is hosted on localhost/127.0.0.1.
167-
if !opts.disableDNSMocking {
168-
server.registryHost = fmt.Sprintf("example.com:%d", port)
169-
// Disable DNS server logging as it is extremely chatty.
170-
dnsLog := log.Default()
171-
dnsLog.SetOutput(io.Discard)
172-
server.dnsServer, err = mockdns.NewServerWithLogger(map[string]mockdns.Zone{
173-
"example.com.": {
174-
A: []string{"127.0.0.1"},
175-
},
176-
}, dnsLog, false)
177-
if err != nil {
178-
return nil, err
179-
}
180-
server.dnsServer.PatchNet(net.DefaultResolver)
160+
server.registryHost = fmt.Sprintf("example.com:%d", port)
161+
// Disable DNS server logging as it is extremely chatty.
162+
dnsLog := log.Default()
163+
dnsLog.SetOutput(io.Discard)
164+
server.dnsServer, err = mockdns.NewServerWithLogger(map[string]mockdns.Zone{
165+
"example.com.": {
166+
A: []string{"127.0.0.1"},
167+
},
168+
}, dnsLog, false)
169+
if err != nil {
170+
return nil, err
181171
}
172+
server.dnsServer.PatchNet(net.DefaultResolver)
182173

183174
config.HTTP.Addr = fmt.Sprintf(":%d", port)
184175
config.HTTP.DrainTimeout = time.Duration(10) * time.Second
@@ -219,6 +210,8 @@ func setupRegistryServer(ctx context.Context, workspaceDir string, opts registry
219210
return nil, fmt.Errorf("failed to create TLS configured HTTP client: %s", err)
220211
}
221212
clientOpts = append(clientOpts, helmreg.ClientOptHTTPClient(httpClient))
213+
} else {
214+
clientOpts = append(clientOpts, helmreg.ClientOptPlainHTTP())
222215
}
223216

224217
// setup logger options
@@ -312,8 +305,7 @@ func TestMain(m *testing.M) {
312305
panic(fmt.Sprintf("failed to create workspace directory: %v", err))
313306
}
314307
testRegistryServer, err = setupRegistryServer(ctx, testWorkspaceDir, registryOptions{
315-
withBasicAuth: true,
316-
disableDNSMocking: true,
308+
withBasicAuth: true,
317309
})
318310
if err != nil {
319311
panic(fmt.Sprintf("Failed to create a test registry server: %v", err))

‎internal/helm/getter/client_opts.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,6 @@ func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmReposit
7474
helmgetter.WithURL(url),
7575
helmgetter.WithTimeout(obj.GetTimeout()),
7676
helmgetter.WithPassCredentialsAll(obj.Spec.PassCredentials),
77-
helmgetter.WithPlainHTTP(obj.Spec.Insecure),
7877
},
7978
}
8079
ociRepo := obj.Spec.Type == helmv1.HelmRepositoryTypeOCI

‎internal/helm/getter/client_opts_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ func TestGetClientOpts(t *testing.T) {
6868
},
6969
afterFunc: func(t *WithT, hcOpts *ClientOpts) {
7070
t.Expect(hcOpts.TlsConfig).ToNot(BeNil())
71-
t.Expect(len(hcOpts.GetterOpts)).To(Equal(5))
71+
t.Expect(len(hcOpts.GetterOpts)).To(Equal(4))
7272
},
7373
},
7474
{
@@ -85,7 +85,7 @@ func TestGetClientOpts(t *testing.T) {
8585
},
8686
afterFunc: func(t *WithT, hcOpts *ClientOpts) {
8787
t.Expect(hcOpts.TlsConfig).ToNot(BeNil())
88-
t.Expect(len(hcOpts.GetterOpts)).To(Equal(5))
88+
t.Expect(len(hcOpts.GetterOpts)).To(Equal(4))
8989
},
9090
err: ErrDeprecatedTLSConfig,
9191
},

‎internal/helm/registry/client.go

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ import (
2929
// ClientGenerator generates a registry client and a temporary credential file.
3030
// The client is meant to be used for a single reconciliation.
3131
// The file is meant to be used for a single reconciliation and deleted after.
32-
func ClientGenerator(tlsConfig *tls.Config, isLogin bool) (*registry.Client, string, error) {
32+
func ClientGenerator(tlsConfig *tls.Config, isLogin, insecureHTTP bool) (*registry.Client, string, error) {
3333
if isLogin {
3434
// create a temporary file to store the credentials
3535
// 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
3939
}
4040

4141
var errs []error
42-
rClient, err := newClient(credentialsFile.Name(), tlsConfig)
42+
rClient, err := newClient(credentialsFile.Name(), tlsConfig, insecureHTTP)
4343
if err != nil {
4444
errs = append(errs, err)
4545
// attempt to delete the temporary file
@@ -54,17 +54,20 @@ func ClientGenerator(tlsConfig *tls.Config, isLogin bool) (*registry.Client, str
5454
return rClient, credentialsFile.Name(), nil
5555
}
5656

57-
rClient, err := newClient("", tlsConfig)
57+
rClient, err := newClient("", tlsConfig, insecureHTTP)
5858
if err != nil {
5959
return nil, "", err
6060
}
6161
return rClient, "", nil
6262
}
6363

64-
func newClient(credentialsFile string, tlsConfig *tls.Config) (*registry.Client, error) {
64+
func newClient(credentialsFile string, tlsConfig *tls.Config, insecureHTTP bool) (*registry.Client, error) {
6565
opts := []registry.ClientOption{
6666
registry.ClientOptWriter(io.Discard),
6767
}
68+
if insecureHTTP {
69+
opts = append(opts, registry.ClientOptPlainHTTP())
70+
}
6871
if tlsConfig != nil {
6972
opts = append(opts, registry.ClientOptHTTPClient(&http.Client{
7073
Transport: &http.Transport{

‎internal/helm/repository/oci_chart_repository.go

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,9 @@ type OCIChartRepository struct {
7575

7676
// verifiers is a list of verifiers to use when verifying a chart.
7777
verifiers []oci.Verifier
78+
79+
// insecureHTTP indicates that the chart is hosted on an insecure registry.
80+
insecure bool
7881
}
7982

8083
// OCIChartRepositoryOption is a function that can be passed to NewOCIChartRepository
@@ -89,6 +92,13 @@ func WithVerifiers(verifiers []oci.Verifier) OCIChartRepositoryOption {
8992
}
9093
}
9194

95+
func WithInsecureHTTP() OCIChartRepositoryOption {
96+
return func(r *OCIChartRepository) error {
97+
r.insecure = true
98+
return nil
99+
}
100+
}
101+
92102
// WithOCIRegistryClient returns a ChartRepositoryOption that will set the registry client
93103
func WithOCIRegistryClient(client RegistryClient) OCIChartRepositoryOption {
94104
return func(r *OCIChartRepository) error {
@@ -358,7 +368,12 @@ func (r *OCIChartRepository) VerifyChart(ctx context.Context, chart *repo.ChartV
358368
return fmt.Errorf("chart '%s' has no downloadable URLs", chart.Name)
359369
}
360370

361-
ref, err := name.ParseReference(strings.TrimPrefix(chart.URLs[0], fmt.Sprintf("%s://", registry.OCIScheme)))
371+
var nameOpts []name.Option
372+
if r.insecure {
373+
nameOpts = append(nameOpts, name.Insecure)
374+
}
375+
376+
ref, err := name.ParseReference(strings.TrimPrefix(chart.URLs[0], fmt.Sprintf("%s://", registry.OCIScheme)), nameOpts...)
362377
if err != nil {
363378
return fmt.Errorf("invalid chart reference: %s", err)
364379
}

0 commit comments

Comments
 (0)