Skip to content

Commit 0219905

Browse files
darkowlzzdarkowlzz
authored
Merge pull request #770 from souleb/oci-for-deps-manager
Enable Umbrella Chart with dependencies from OCI repositories
2 parents f7006e9 + 361b975 commit 0219905

16 files changed

+584
-238
lines changed

‎controllers/helmchart_controller.go

+96-32
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ 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"
3839
"k8s.io/apimachinery/pkg/util/uuid"
3940
kuberecorder "k8s.io/client-go/tools/record"
4041
ctrl "sigs.k8s.io/controller-runtime"
@@ -461,9 +462,10 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
461462
loginOpts []helmreg.LoginOption
462463
)
463464

465+
normalizedURL := repository.NormalizeURL(repo.Spec.URL)
464466
// Construct the Getter options from the HelmRepository data
465467
clientOpts := []helmgetter.Option{
466-
helmgetter.WithURL(repo.Spec.URL),
468+
helmgetter.WithURL(normalizedURL),
467469
helmgetter.WithTimeout(repo.Spec.Timeout.Duration),
468470
helmgetter.WithPassCredentialsAll(repo.Spec.PassCredentials),
469471
}
@@ -491,7 +493,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
491493
}
492494
clientOpts = append(clientOpts, opts...)
493495

494-
tlsConfig, err = getter.TLSClientConfigFromSecret(*secret, repo.Spec.URL)
496+
tlsConfig, err = getter.TLSClientConfigFromSecret(*secret, normalizedURL)
495497
if err != nil {
496498
e := &serror.Event{
497499
Err: fmt.Errorf("failed to create TLS client config with secret data: %w", err),
@@ -503,7 +505,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
503505
}
504506

505507
// Build registryClient options from secret
506-
loginOpt, err := registry.LoginOptionFromSecret(repo.Spec.URL, *secret)
508+
loginOpt, err := registry.LoginOptionFromSecret(normalizedURL, *secret)
507509
if err != nil {
508510
e := &serror.Event{
509511
Err: fmt.Errorf("failed to configure Helm client with secret data: %w", err),
@@ -518,19 +520,19 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
518520
}
519521

520522
// Initialize the chart repository
521-
var chartRepo chart.Remote
523+
var chartRepo repository.Downloader
522524
switch repo.Spec.Type {
523525
case sourcev1.HelmRepositoryTypeOCI:
524-
if !helmreg.IsOCI(repo.Spec.URL) {
525-
err := fmt.Errorf("invalid OCI registry URL: %s", repo.Spec.URL)
526+
if !helmreg.IsOCI(normalizedURL) {
527+
err := fmt.Errorf("invalid OCI registry URL: %s", normalizedURL)
526528
return chartRepoConfigErrorReturn(err, obj)
527529
}
528530

529531
// with this function call, we create a temporary file to store the credentials if needed.
530532
// this is needed because otherwise the credentials are stored in ~/.docker/config.json.
531533
// TODO@souleb: remove this once the registry move to Oras v2
532534
// or rework to enable reusing credentials to avoid the unneccessary handshake operations
533-
registryClient, file, err := r.RegistryClientGenerator(loginOpts != nil)
535+
registryClient, credentialsFile, err := r.RegistryClientGenerator(loginOpts != nil)
534536
if err != nil {
535537
e := &serror.Event{
536538
Err: fmt.Errorf("failed to construct Helm client: %w", err),
@@ -540,9 +542,9 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
540542
return sreconcile.ResultEmpty, e
541543
}
542544

543-
if file != "" {
545+
if credentialsFile != "" {
544546
defer func() {
545-
if err := os.Remove(file); err != nil {
547+
if err := os.Remove(credentialsFile); err != nil {
546548
r.eventLogf(ctx, obj, corev1.EventTypeWarning, meta.FailedReason,
547549
"failed to delete temporary credentials file: %s", err)
548550
}
@@ -551,7 +553,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
551553

552554
// Tell the chart repository to use the OCI client with the configured getter
553555
clientOpts = append(clientOpts, helmgetter.WithRegistryClient(registryClient))
554-
ociChartRepo, err := repository.NewOCIChartRepository(repo.Spec.URL, repository.WithOCIGetter(r.Getters), repository.WithOCIGetterOptions(clientOpts), repository.WithOCIRegistryClient(registryClient))
556+
ociChartRepo, err := repository.NewOCIChartRepository(normalizedURL, repository.WithOCIGetter(r.Getters), repository.WithOCIGetterOptions(clientOpts), repository.WithOCIRegistryClient(registryClient))
555557
if err != nil {
556558
return chartRepoConfigErrorReturn(err, obj)
557559
}
@@ -571,7 +573,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
571573
}
572574
}
573575
default:
574-
httpChartRepo, err := repository.NewChartRepository(repo.Spec.URL, r.Storage.LocalPath(*repo.GetArtifact()), r.Getters, tlsConfig, clientOpts,
576+
httpChartRepo, err := repository.NewChartRepository(normalizedURL, r.Storage.LocalPath(*repo.GetArtifact()), r.Getters, tlsConfig, clientOpts,
575577
repository.WithMemoryCache(r.Storage.LocalPath(*repo.GetArtifact()), r.Cache, r.TTL, func(event string) {
576578
r.IncCacheEvents(event, obj.Name, obj.Namespace)
577579
}))
@@ -684,9 +686,15 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj
684686

685687
// Setup dependency manager
686688
dm := chart.NewDependencyManager(
687-
chart.WithRepositoryCallback(r.namespacedChartRepositoryCallback(ctx, obj.GetName(), obj.GetNamespace())),
689+
chart.WithDownloaderCallback(r.namespacedChartRepositoryCallback(ctx, obj.GetName(), obj.GetNamespace())),
688690
)
689-
defer dm.Clear()
691+
defer func() {
692+
err := dm.Clear()
693+
if err != nil {
694+
r.eventLogf(ctx, obj, corev1.EventTypeWarning, meta.FailedReason,
695+
"dependency manager cleanup error: %s", err)
696+
}
697+
}()
690698

691699
// Configure builder options, including any previously cached chart
692700
opts := chart.BuildOptions{
@@ -913,12 +921,17 @@ func (r *HelmChartReconciler) garbageCollect(ctx context.Context, obj *sourcev1.
913921
return nil
914922
}
915923

916-
// namespacedChartRepositoryCallback returns a chart.GetChartRepositoryCallback scoped to the given namespace.
917-
// The returned callback returns a repository.ChartRepository configured with the retrieved v1beta1.HelmRepository,
924+
// namespacedChartRepositoryCallback returns a chart.GetChartDownloaderCallback scoped to the given namespace.
925+
// The returned callback returns a repository.Downloader configured with the retrieved v1beta1.HelmRepository,
918926
// or a shim with defaults if no object could be found.
919-
func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Context, name, namespace string) chart.GetChartRepositoryCallback {
920-
return func(url string) (*repository.ChartRepository, error) {
921-
var tlsConfig *tls.Config
927+
// The callback returns an object with a state, so the caller has to do the necessary cleanup.
928+
func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Context, name, namespace string) chart.GetChartDownloaderCallback {
929+
return func(url string) (repository.Downloader, error) {
930+
var (
931+
tlsConfig *tls.Config
932+
loginOpts []helmreg.LoginOption
933+
)
934+
normalizedURL := repository.NormalizeURL(url)
922935
repo, err := r.resolveDependencyRepository(ctx, url, namespace)
923936
if err != nil {
924937
// Return Kubernetes client errors, but ignore others
@@ -933,7 +946,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
933946
}
934947
}
935948
clientOpts := []helmgetter.Option{
936-
helmgetter.WithURL(repo.Spec.URL),
949+
helmgetter.WithURL(normalizedURL),
937950
helmgetter.WithTimeout(repo.Spec.Timeout.Duration),
938951
helmgetter.WithPassCredentialsAll(repo.Spec.PassCredentials),
939952
}
@@ -947,26 +960,77 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
947960
}
948961
clientOpts = append(clientOpts, opts...)
949962

950-
tlsConfig, err = getter.TLSClientConfigFromSecret(*secret, repo.Spec.URL)
963+
tlsConfig, err = getter.TLSClientConfigFromSecret(*secret, normalizedURL)
951964
if err != nil {
952965
return nil, fmt.Errorf("failed to create TLS client config for HelmRepository '%s': %w", repo.Name, err)
953966
}
954-
}
955967

956-
chartRepo, err := repository.NewChartRepository(repo.Spec.URL, "", r.Getters, tlsConfig, clientOpts)
957-
if err != nil {
958-
return nil, err
968+
// Build registryClient options from secret
969+
loginOpt, err := registry.LoginOptionFromSecret(normalizedURL, *secret)
970+
if err != nil {
971+
return nil, fmt.Errorf("failed to create login options for HelmRepository '%s': %w", repo.Name, err)
972+
}
973+
974+
loginOpts = append([]helmreg.LoginOption{}, loginOpt)
959975
}
960976

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

‎controllers/helmchart_controller_test.go

+6-8
Original file line numberDiff line numberDiff line change
@@ -411,9 +411,6 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) {
411411
}))
412412
},
413413
},
414-
//{
415-
// name: "Error on transient build error",
416-
//},
417414
{
418415
name: "Stalling on persistent build error",
419416
source: &sourcev1.GitRepository{
@@ -1070,7 +1067,7 @@ func TestHelmChartReconciler_buildFromTarballArtifact(t *testing.T) {
10701067
assertFunc: func(g *WithT, build chart.Build) {
10711068
g.Expect(build.Name).To(Equal("helmchartwithdeps"))
10721069
g.Expect(build.Version).To(Equal("0.1.0"))
1073-
g.Expect(build.ResolvedDependencies).To(Equal(3))
1070+
g.Expect(build.ResolvedDependencies).To(Equal(4))
10741071
g.Expect(build.Path).To(BeARegularFile())
10751072
},
10761073
cleanFunc: func(g *WithT, build *chart.Build) {
@@ -1178,10 +1175,11 @@ func TestHelmChartReconciler_buildFromTarballArtifact(t *testing.T) {
11781175
g := NewWithT(t)
11791176

11801177
r := &HelmChartReconciler{
1181-
Client: fake.NewClientBuilder().Build(),
1182-
EventRecorder: record.NewFakeRecorder(32),
1183-
Storage: storage,
1184-
Getters: testGetters,
1178+
Client: fake.NewClientBuilder().Build(),
1179+
EventRecorder: record.NewFakeRecorder(32),
1180+
Storage: storage,
1181+
Getters: testGetters,
1182+
RegistryClientGenerator: registry.ClientGenerator,
11851183
}
11861184

11871185
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 log into registry '%s': %w", obj.Spec.URL, err)
329+
e := fmt.Errorf("failed to login to 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,3 +31,6 @@ 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.ChartRepository
70+
repositories map[string]repository.Downloader
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.ChartRepository{
149+
repositories: map[string]repository.Downloader{
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.ChartRepository{
168+
repositories: map[string]repository.Downloader{
169169
"https://grafana.github.io/helm-charts/": mockRepo(),
170170
},
171171
dependentChartPaths: []string{"../testdata/charts/helmchart-v1"},

0 commit comments

Comments
 (0)