Skip to content

Commit 211bf24

Browse files
author
Paulo Gomes
committed
Revert "Merge pull request #770 from souleb/oci-for-deps-manager"
This reverts commit 0219905, reversing changes made to f7006e9.
1 parent 8cff49f commit 211bf24

16 files changed

+231
-578
lines changed

‎controllers/helmchart_controller.go

+32-96
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@ import (
3535
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3636
"k8s.io/apimachinery/pkg/runtime"
3737
"k8s.io/apimachinery/pkg/types"
38-
kerrors "k8s.io/apimachinery/pkg/util/errors"
3938
"k8s.io/apimachinery/pkg/util/uuid"
4039
kuberecorder "k8s.io/client-go/tools/record"
4140
ctrl "sigs.k8s.io/controller-runtime"
@@ -463,10 +462,9 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
463462
loginOpts []helmreg.LoginOption
464463
)
465464

466-
normalizedURL := repository.NormalizeURL(repo.Spec.URL)
467465
// Construct the Getter options from the HelmRepository data
468466
clientOpts := []helmgetter.Option{
469-
helmgetter.WithURL(normalizedURL),
467+
helmgetter.WithURL(repo.Spec.URL),
470468
helmgetter.WithTimeout(repo.Spec.Timeout.Duration),
471469
helmgetter.WithPassCredentialsAll(repo.Spec.PassCredentials),
472470
}
@@ -494,7 +492,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
494492
}
495493
clientOpts = append(clientOpts, opts...)
496494

497-
tlsConfig, err = getter.TLSClientConfigFromSecret(*secret, normalizedURL)
495+
tlsConfig, err = getter.TLSClientConfigFromSecret(*secret, repo.Spec.URL)
498496
if err != nil {
499497
e := &serror.Event{
500498
Err: fmt.Errorf("failed to create TLS client config with secret data: %w", err),
@@ -506,7 +504,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
506504
}
507505

508506
// Build registryClient options from secret
509-
loginOpt, err := registry.LoginOptionFromSecret(normalizedURL, *secret)
507+
loginOpt, err := registry.LoginOptionFromSecret(repo.Spec.URL, *secret)
510508
if err != nil {
511509
e := &serror.Event{
512510
Err: fmt.Errorf("failed to configure Helm client with secret data: %w", err),
@@ -521,19 +519,19 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
521519
}
522520

523521
// Initialize the chart repository
524-
var chartRepo repository.Downloader
522+
var chartRepo chart.Remote
525523
switch repo.Spec.Type {
526524
case sourcev1.HelmRepositoryTypeOCI:
527-
if !helmreg.IsOCI(normalizedURL) {
528-
err := fmt.Errorf("invalid OCI registry URL: %s", normalizedURL)
525+
if !helmreg.IsOCI(repo.Spec.URL) {
526+
err := fmt.Errorf("invalid OCI registry URL: %s", repo.Spec.URL)
529527
return chartRepoConfigErrorReturn(err, obj)
530528
}
531529

532530
// with this function call, we create a temporary file to store the credentials if needed.
533531
// this is needed because otherwise the credentials are stored in ~/.docker/config.json.
534532
// TODO@souleb: remove this once the registry move to Oras v2
535533
// or rework to enable reusing credentials to avoid the unneccessary handshake operations
536-
registryClient, credentialsFile, err := r.RegistryClientGenerator(loginOpts != nil)
534+
registryClient, file, err := r.RegistryClientGenerator(loginOpts != nil)
537535
if err != nil {
538536
e := &serror.Event{
539537
Err: fmt.Errorf("failed to construct Helm client: %w", err),
@@ -543,9 +541,9 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
543541
return sreconcile.ResultEmpty, e
544542
}
545543

546-
if credentialsFile != "" {
544+
if file != "" {
547545
defer func() {
548-
if err := os.Remove(credentialsFile); err != nil {
546+
if err := os.Remove(file); err != nil {
549547
r.eventLogf(ctx, obj, corev1.EventTypeWarning, meta.FailedReason,
550548
"failed to delete temporary credentials file: %s", err)
551549
}
@@ -554,7 +552,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
554552

555553
// Tell the chart repository to use the OCI client with the configured getter
556554
clientOpts = append(clientOpts, helmgetter.WithRegistryClient(registryClient))
557-
ociChartRepo, err := repository.NewOCIChartRepository(normalizedURL, repository.WithOCIGetter(r.Getters), repository.WithOCIGetterOptions(clientOpts), repository.WithOCIRegistryClient(registryClient))
555+
ociChartRepo, err := repository.NewOCIChartRepository(repo.Spec.URL, repository.WithOCIGetter(r.Getters), repository.WithOCIGetterOptions(clientOpts), repository.WithOCIRegistryClient(registryClient))
558556
if err != nil {
559557
return chartRepoConfigErrorReturn(err, obj)
560558
}
@@ -574,7 +572,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
574572
}
575573
}
576574
default:
577-
httpChartRepo, err := repository.NewChartRepository(normalizedURL, r.Storage.LocalPath(*repo.GetArtifact()), r.Getters, tlsConfig, clientOpts,
575+
httpChartRepo, err := repository.NewChartRepository(repo.Spec.URL, r.Storage.LocalPath(*repo.GetArtifact()), r.Getters, tlsConfig, clientOpts,
578576
repository.WithMemoryCache(r.Storage.LocalPath(*repo.GetArtifact()), r.Cache, r.TTL, func(event string) {
579577
r.IncCacheEvents(event, obj.Name, obj.Namespace)
580578
}))
@@ -687,15 +685,9 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj
687685

688686
// Setup dependency manager
689687
dm := chart.NewDependencyManager(
690-
chart.WithDownloaderCallback(r.namespacedChartRepositoryCallback(ctx, obj.GetName(), obj.GetNamespace())),
688+
chart.WithRepositoryCallback(r.namespacedChartRepositoryCallback(ctx, obj.GetName(), obj.GetNamespace())),
691689
)
692-
defer func() {
693-
err := dm.Clear()
694-
if err != nil {
695-
r.eventLogf(ctx, obj, corev1.EventTypeWarning, meta.FailedReason,
696-
"dependency manager cleanup error: %s", err)
697-
}
698-
}()
690+
defer dm.Clear()
699691

700692
// Configure builder options, including any previously cached chart
701693
opts := chart.BuildOptions{
@@ -922,17 +914,12 @@ func (r *HelmChartReconciler) garbageCollect(ctx context.Context, obj *sourcev1.
922914
return nil
923915
}
924916

925-
// namespacedChartRepositoryCallback returns a chart.GetChartDownloaderCallback scoped to the given namespace.
926-
// The returned callback returns a repository.Downloader configured with the retrieved v1beta1.HelmRepository,
917+
// namespacedChartRepositoryCallback returns a chart.GetChartRepositoryCallback scoped to the given namespace.
918+
// The returned callback returns a repository.ChartRepository configured with the retrieved v1beta1.HelmRepository,
927919
// or a shim with defaults if no object could be found.
928-
// The callback returns an object with a state, so the caller has to do the necessary cleanup.
929-
func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Context, name, namespace string) chart.GetChartDownloaderCallback {
930-
return func(url string) (repository.Downloader, error) {
931-
var (
932-
tlsConfig *tls.Config
933-
loginOpts []helmreg.LoginOption
934-
)
935-
normalizedURL := repository.NormalizeURL(url)
920+
func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Context, name, namespace string) chart.GetChartRepositoryCallback {
921+
return func(url string) (*repository.ChartRepository, error) {
922+
var tlsConfig *tls.Config
936923
repo, err := r.resolveDependencyRepository(ctx, url, namespace)
937924
if err != nil {
938925
// Return Kubernetes client errors, but ignore others
@@ -947,7 +934,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
947934
}
948935
}
949936
clientOpts := []helmgetter.Option{
950-
helmgetter.WithURL(normalizedURL),
937+
helmgetter.WithURL(repo.Spec.URL),
951938
helmgetter.WithTimeout(repo.Spec.Timeout.Duration),
952939
helmgetter.WithPassCredentialsAll(repo.Spec.PassCredentials),
953940
}
@@ -961,77 +948,26 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
961948
}
962949
clientOpts = append(clientOpts, opts...)
963950

964-
tlsConfig, err = getter.TLSClientConfigFromSecret(*secret, normalizedURL)
951+
tlsConfig, err = getter.TLSClientConfigFromSecret(*secret, repo.Spec.URL)
965952
if err != nil {
966953
return nil, fmt.Errorf("failed to create TLS client config for HelmRepository '%s': %w", repo.Name, err)
967954
}
968-
969-
// Build registryClient options from secret
970-
loginOpt, err := registry.LoginOptionFromSecret(normalizedURL, *secret)
971-
if err != nil {
972-
return nil, fmt.Errorf("failed to create login options for HelmRepository '%s': %w", repo.Name, err)
973-
}
974-
975-
loginOpts = append([]helmreg.LoginOption{}, loginOpt)
976955
}
977956

978-
var chartRepo repository.Downloader
979-
if helmreg.IsOCI(normalizedURL) {
980-
registryClient, credentialsFile, err := r.RegistryClientGenerator(loginOpts != nil)
981-
if err != nil {
982-
return nil, fmt.Errorf("failed to create registry client for HelmRepository '%s': %w", repo.Name, err)
983-
}
984-
985-
var errs []error
986-
// Tell the chart repository to use the OCI client with the configured getter
987-
clientOpts = append(clientOpts, helmgetter.WithRegistryClient(registryClient))
988-
ociChartRepo, err := repository.NewOCIChartRepository(normalizedURL, repository.WithOCIGetter(r.Getters),
989-
repository.WithOCIGetterOptions(clientOpts),
990-
repository.WithOCIRegistryClient(registryClient),
991-
repository.WithCredentialsFile(credentialsFile))
992-
if err != nil {
993-
errs = append(errs, fmt.Errorf("failed to create OCI chart repository for HelmRepository '%s': %w", repo.Name, err))
994-
// clean up the credentialsFile
995-
if credentialsFile != "" {
996-
if err := os.Remove(credentialsFile); err != nil {
997-
errs = append(errs, err)
998-
}
999-
}
1000-
return nil, kerrors.NewAggregate(errs)
1001-
}
1002-
1003-
// If login options are configured, use them to login to the registry
1004-
// The OCIGetter will later retrieve the stored credentials to pull the chart
1005-
if loginOpts != nil {
1006-
err = ociChartRepo.Login(loginOpts...)
1007-
if err != nil {
1008-
errs = append(errs, fmt.Errorf("failed to login to OCI chart repository for HelmRepository '%s': %w", repo.Name, err))
1009-
// clean up the credentialsFile
1010-
errs = append(errs, ociChartRepo.Clear())
1011-
return nil, kerrors.NewAggregate(errs)
1012-
}
1013-
}
1014-
1015-
chartRepo = ociChartRepo
1016-
} else {
1017-
httpChartRepo, err := repository.NewChartRepository(normalizedURL, "", r.Getters, tlsConfig, clientOpts)
1018-
if err != nil {
1019-
return nil, err
1020-
}
1021-
1022-
// Ensure that the cache key is the same as the artifact path
1023-
// otherwise don't enable caching. We don't want to cache indexes
1024-
// for repositories that are not reconciled by the source controller.
1025-
if repo.Status.Artifact != nil {
1026-
httpChartRepo.CachePath = r.Storage.LocalPath(*repo.GetArtifact())
1027-
httpChartRepo.SetMemCache(r.Storage.LocalPath(*repo.GetArtifact()), r.Cache, r.TTL, func(event string) {
1028-
r.IncCacheEvents(event, name, namespace)
1029-
})
1030-
}
1031-
1032-
chartRepo = httpChartRepo
957+
chartRepo, err := repository.NewChartRepository(repo.Spec.URL, "", r.Getters, tlsConfig, clientOpts)
958+
if err != nil {
959+
return nil, err
1033960
}
1034961

962+
// Ensure that the cache key is the same as the artifact path
963+
// otherwise don't enable caching. We don't want to cache indexes
964+
// for repositories that are not reconciled by the source controller.
965+
if repo.Status.Artifact != nil {
966+
chartRepo.CachePath = r.Storage.LocalPath(*repo.GetArtifact())
967+
chartRepo.SetMemCache(r.Storage.LocalPath(*repo.GetArtifact()), r.Cache, r.TTL, func(event string) {
968+
r.IncCacheEvents(event, name, namespace)
969+
})
970+
}
1035971
return chartRepo, nil
1036972
}
1037973
}

‎controllers/helmchart_controller_test.go

+8-6
Original file line numberDiff line numberDiff line change
@@ -411,6 +411,9 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) {
411411
}))
412412
},
413413
},
414+
//{
415+
// name: "Error on transient build error",
416+
//},
414417
{
415418
name: "Stalling on persistent build error",
416419
source: &sourcev1.GitRepository{
@@ -1217,7 +1220,7 @@ func TestHelmChartReconciler_buildFromTarballArtifact(t *testing.T) {
12171220
assertFunc: func(g *WithT, build chart.Build) {
12181221
g.Expect(build.Name).To(Equal("helmchartwithdeps"))
12191222
g.Expect(build.Version).To(Equal("0.1.0"))
1220-
g.Expect(build.ResolvedDependencies).To(Equal(4))
1223+
g.Expect(build.ResolvedDependencies).To(Equal(3))
12211224
g.Expect(build.Path).To(BeARegularFile())
12221225
},
12231226
cleanFunc: func(g *WithT, build *chart.Build) {
@@ -1325,11 +1328,10 @@ func TestHelmChartReconciler_buildFromTarballArtifact(t *testing.T) {
13251328
g := NewWithT(t)
13261329

13271330
r := &HelmChartReconciler{
1328-
Client: fake.NewClientBuilder().Build(),
1329-
EventRecorder: record.NewFakeRecorder(32),
1330-
Storage: storage,
1331-
Getters: testGetters,
1332-
RegistryClientGenerator: registry.ClientGenerator,
1331+
Client: fake.NewClientBuilder().Build(),
1332+
EventRecorder: record.NewFakeRecorder(32),
1333+
Storage: storage,
1334+
Getters: testGetters,
13331335
}
13341336

13351337
obj := &sourcev1.HelmChart{

‎controllers/helmrepository_controller_oci.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -326,7 +326,7 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, obj *v1beta
326326
if loginOpts != nil {
327327
err = chartRepo.Login(loginOpts...)
328328
if err != nil {
329-
e := fmt.Errorf("failed to login to registry '%s': %w", obj.Spec.URL, err)
329+
e := fmt.Errorf("failed to log into registry '%s': %w", obj.Spec.URL, err)
330330
conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, e.Error())
331331
result, retErr = ctrl.Result{}, e
332332
return

‎controllers/testdata/charts/helmchartwithdeps/Chart.yaml

-3
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,3 @@ dependencies:
3131
- name: grafana
3232
version: ">=5.7.0"
3333
repository: "https://grafana.github.io/helm-charts"
34-
- name: podinfo
35-
version: ">=6.1.*"
36-
repository: "oci://ghcr.io/stefanprodan/charts"

‎internal/helm/chart/builder_local_test.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ func TestLocalBuilder_Build(t *testing.T) {
6767
reference Reference
6868
buildOpts BuildOptions
6969
valuesFiles []helmchart.File
70-
repositories map[string]repository.Downloader
70+
repositories map[string]*repository.ChartRepository
7171
dependentChartPaths []string
7272
wantValues chartutil.Values
7373
wantVersion string
@@ -146,7 +146,7 @@ fullnameOverride: "full-foo-name-override"`),
146146
{
147147
name: "chart with dependencies",
148148
reference: LocalReference{Path: "../testdata/charts/helmchartwithdeps"},
149-
repositories: map[string]repository.Downloader{
149+
repositories: map[string]*repository.ChartRepository{
150150
"https://grafana.github.io/helm-charts/": mockRepo(),
151151
},
152152
dependentChartPaths: []string{"./../testdata/charts/helmchart"},
@@ -165,7 +165,7 @@ fullnameOverride: "full-foo-name-override"`),
165165
{
166166
name: "v1 chart with dependencies",
167167
reference: LocalReference{Path: "../testdata/charts/helmchartwithdeps-v1"},
168-
repositories: map[string]repository.Downloader{
168+
repositories: map[string]*repository.ChartRepository{
169169
"https://grafana.github.io/helm-charts/": mockRepo(),
170170
},
171171
dependentChartPaths: []string{"../testdata/charts/helmchart-v1"},

0 commit comments

Comments
 (0)