Skip to content

Commit

Permalink
Revert "Merge pull request #770 from souleb/oci-for-deps-manager"
Browse files Browse the repository at this point in the history 
This reverts commit 0219905, reversing
changes made to f7006e9.
  • Loading branch information
Paulo Gomes committed Jul 15, 2022
1 parent 8cff49f commit f31dbc4
Show file tree
Hide file tree
Showing 16 changed files with 232 additions and 576 deletions.
128 changes: 32 additions & 96 deletions controllers/helmchart_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
kerrors "k8s.io/apimachinery/pkg/util/errors"
"k8s.io/apimachinery/pkg/util/uuid"
kuberecorder "k8s.io/client-go/tools/record"
ctrl "sigs.k8s.io/controller-runtime"
Expand Down Expand Up @@ -463,10 +462,9 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
loginOpts []helmreg.LoginOption
)

normalizedURL := repository.NormalizeURL(repo.Spec.URL)
// Construct the Getter options from the HelmRepository data
clientOpts := []helmgetter.Option{
helmgetter.WithURL(normalizedURL),
helmgetter.WithURL(repo.Spec.URL),
helmgetter.WithTimeout(repo.Spec.Timeout.Duration),
helmgetter.WithPassCredentialsAll(repo.Spec.PassCredentials),
}
Expand Down Expand Up @@ -494,7 +492,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
}
clientOpts = append(clientOpts, opts...)

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

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

// Initialize the chart repository
var chartRepo repository.Downloader
var chartRepo chart.Remote
switch repo.Spec.Type {
case sourcev1.HelmRepositoryTypeOCI:
if !helmreg.IsOCI(normalizedURL) {
err := fmt.Errorf("invalid OCI registry URL: %s", normalizedURL)
if !helmreg.IsOCI(repo.Spec.URL) {
err := fmt.Errorf("invalid OCI registry URL: %s", repo.Spec.URL)
return chartRepoConfigErrorReturn(err, obj)
}

// with this function call, we create a temporary file to store the credentials if needed.
// 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(loginOpts != nil)
registryClient, file, err := r.RegistryClientGenerator(loginOpts != nil)
if err != nil {
e := &serror.Event{
Err: fmt.Errorf("failed to construct Helm client: %w", err),
Expand All @@ -543,9 +541,9 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
return sreconcile.ResultEmpty, e
}

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

// Tell the chart repository to use the OCI client with the configured getter
clientOpts = append(clientOpts, helmgetter.WithRegistryClient(registryClient))
ociChartRepo, err := repository.NewOCIChartRepository(normalizedURL, repository.WithOCIGetter(r.Getters), repository.WithOCIGetterOptions(clientOpts), repository.WithOCIRegistryClient(registryClient))
ociChartRepo, err := repository.NewOCIChartRepository(repo.Spec.URL, repository.WithOCIGetter(r.Getters), repository.WithOCIGetterOptions(clientOpts), repository.WithOCIRegistryClient(registryClient))
if err != nil {
return chartRepoConfigErrorReturn(err, obj)
}
Expand All @@ -574,7 +572,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
}
}
default:
httpChartRepo, err := repository.NewChartRepository(normalizedURL, r.Storage.LocalPath(*repo.GetArtifact()), r.Getters, tlsConfig, clientOpts,
httpChartRepo, err := repository.NewChartRepository(repo.Spec.URL, r.Storage.LocalPath(*repo.GetArtifact()), r.Getters, tlsConfig, clientOpts,
repository.WithMemoryCache(r.Storage.LocalPath(*repo.GetArtifact()), r.Cache, r.TTL, func(event string) {
r.IncCacheEvents(event, obj.Name, obj.Namespace)
}))
Expand Down Expand Up @@ -687,15 +685,9 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj

// Setup dependency manager
dm := chart.NewDependencyManager(
chart.WithDownloaderCallback(r.namespacedChartRepositoryCallback(ctx, obj.GetName(), obj.GetNamespace())),
chart.WithRepositoryCallback(r.namespacedChartRepositoryCallback(ctx, obj.GetName(), obj.GetNamespace())),
)
defer func() {
err := dm.Clear()
if err != nil {
r.eventLogf(ctx, obj, corev1.EventTypeWarning, meta.FailedReason,
"dependency manager cleanup error: %s", err)
}
}()
defer dm.Clear()

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

// namespacedChartRepositoryCallback returns a chart.GetChartDownloaderCallback scoped to the given namespace.
// The returned callback returns a repository.Downloader configured with the retrieved v1beta1.HelmRepository,
// namespacedChartRepositoryCallback returns a chart.GetChartRepositoryCallback scoped to the given namespace.
// The returned callback returns a repository.ChartRepository configured with the retrieved v1beta1.HelmRepository,
// or a shim with defaults if no object could be found.
// The callback returns an object with a state, so the caller has to do the necessary cleanup.
func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Context, name, namespace string) chart.GetChartDownloaderCallback {
return func(url string) (repository.Downloader, error) {
var (
tlsConfig *tls.Config
loginOpts []helmreg.LoginOption
)
normalizedURL := repository.NormalizeURL(url)
func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Context, name, namespace string) chart.GetChartRepositoryCallback {
return func(url string) (*repository.ChartRepository, error) {
var tlsConfig *tls.Config
repo, err := r.resolveDependencyRepository(ctx, url, namespace)
if err != nil {
// Return Kubernetes client errors, but ignore others
Expand All @@ -947,7 +934,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
}
}
clientOpts := []helmgetter.Option{
helmgetter.WithURL(normalizedURL),
helmgetter.WithURL(repo.Spec.URL),
helmgetter.WithTimeout(repo.Spec.Timeout.Duration),
helmgetter.WithPassCredentialsAll(repo.Spec.PassCredentials),
}
Expand All @@ -961,77 +948,26 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
}
clientOpts = append(clientOpts, opts...)

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

// Build registryClient options from secret
loginOpt, err := registry.LoginOptionFromSecret(normalizedURL, *secret)
if err != nil {
return nil, fmt.Errorf("failed to create login options for HelmRepository '%s': %w", repo.Name, err)
}

loginOpts = append([]helmreg.LoginOption{}, loginOpt)
}

var chartRepo repository.Downloader
if helmreg.IsOCI(normalizedURL) {
registryClient, credentialsFile, err := r.RegistryClientGenerator(loginOpts != nil)
if err != nil {
return nil, fmt.Errorf("failed to create registry client for HelmRepository '%s': %w", repo.Name, err)
}

var errs []error
// Tell the chart repository to use the OCI client with the configured getter
clientOpts = append(clientOpts, helmgetter.WithRegistryClient(registryClient))
ociChartRepo, err := repository.NewOCIChartRepository(normalizedURL, repository.WithOCIGetter(r.Getters),
repository.WithOCIGetterOptions(clientOpts),
repository.WithOCIRegistryClient(registryClient),
repository.WithCredentialsFile(credentialsFile))
if err != nil {
errs = append(errs, fmt.Errorf("failed to create OCI chart repository for HelmRepository '%s': %w", repo.Name, err))
// clean up the credentialsFile
if credentialsFile != "" {
if err := os.Remove(credentialsFile); err != nil {
errs = append(errs, err)
}
}
return nil, kerrors.NewAggregate(errs)
}

// If login options are configured, use them to login to the registry
// The OCIGetter will later retrieve the stored credentials to pull the chart
if loginOpts != nil {
err = ociChartRepo.Login(loginOpts...)
if err != nil {
errs = append(errs, fmt.Errorf("failed to login to OCI chart repository for HelmRepository '%s': %w", repo.Name, err))
// clean up the credentialsFile
errs = append(errs, ociChartRepo.Clear())
return nil, kerrors.NewAggregate(errs)
}
}

chartRepo = ociChartRepo
} else {
httpChartRepo, err := repository.NewChartRepository(normalizedURL, "", r.Getters, tlsConfig, clientOpts)
if err != nil {
return nil, err
}

// Ensure that the cache key is the same as the artifact path
// otherwise don't enable caching. We don't want to cache indexes
// for repositories that are not reconciled by the source controller.
if repo.Status.Artifact != nil {
httpChartRepo.CachePath = r.Storage.LocalPath(*repo.GetArtifact())
httpChartRepo.SetMemCache(r.Storage.LocalPath(*repo.GetArtifact()), r.Cache, r.TTL, func(event string) {
r.IncCacheEvents(event, name, namespace)
})
}

chartRepo = httpChartRepo
chartRepo, err := repository.NewChartRepository(repo.Spec.URL, "", r.Getters, tlsConfig, clientOpts)
if err != nil {
return nil, err
}

// Ensure that the cache key is the same as the artifact path
// otherwise don't enable caching. We don't want to cache indexes
// for repositories that are not reconciled by the source controller.
if repo.Status.Artifact != nil {
chartRepo.CachePath = r.Storage.LocalPath(*repo.GetArtifact())
chartRepo.SetMemCache(r.Storage.LocalPath(*repo.GetArtifact()), r.Cache, r.TTL, func(event string) {
r.IncCacheEvents(event, name, namespace)
})
}
return chartRepo, nil
}
}
Expand Down
14 changes: 8 additions & 6 deletions controllers/helmchart_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,9 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) {
}))
},
},
//{
// name: "Error on transient build error",
//},
{
name: "Stalling on persistent build error",
source: &sourcev1.GitRepository{
Expand Down Expand Up @@ -1217,7 +1220,7 @@ func TestHelmChartReconciler_buildFromTarballArtifact(t *testing.T) {
assertFunc: func(g *WithT, build chart.Build) {
g.Expect(build.Name).To(Equal("helmchartwithdeps"))
g.Expect(build.Version).To(Equal("0.1.0"))
g.Expect(build.ResolvedDependencies).To(Equal(4))
g.Expect(build.ResolvedDependencies).To(Equal(3))
g.Expect(build.Path).To(BeARegularFile())
},
cleanFunc: func(g *WithT, build *chart.Build) {
Expand Down Expand Up @@ -1325,11 +1328,10 @@ func TestHelmChartReconciler_buildFromTarballArtifact(t *testing.T) {
g := NewWithT(t)

r := &HelmChartReconciler{
Client: fake.NewClientBuilder().Build(),
EventRecorder: record.NewFakeRecorder(32),
Storage: storage,
Getters: testGetters,
RegistryClientGenerator: registry.ClientGenerator,
Client: fake.NewClientBuilder().Build(),
EventRecorder: record.NewFakeRecorder(32),
Storage: storage,
Getters: testGetters,
}

obj := &sourcev1.HelmChart{
Expand Down
2 changes: 1 addition & 1 deletion controllers/helmrepository_controller_oci.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, obj *v1beta
if loginOpts != nil {
err = chartRepo.Login(loginOpts...)
if err != nil {
e := fmt.Errorf("failed to login to registry '%s': %w", obj.Spec.URL, err)
e := fmt.Errorf("failed to log into registry '%s': %w", obj.Spec.URL, err)
conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, e.Error())
result, retErr = ctrl.Result{}, e
return
Expand Down
3 changes: 0 additions & 3 deletions controllers/testdata/charts/helmchartwithdeps/Chart.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,3 @@ dependencies:
- name: grafana
version: ">=5.7.0"
repository: "https://grafana.github.io/helm-charts"
- name: podinfo
version: ">=6.1.*"
repository: "oci://ghcr.io/stefanprodan/charts"
6 changes: 3 additions & 3 deletions internal/helm/chart/builder_local_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ func TestLocalBuilder_Build(t *testing.T) {
reference Reference
buildOpts BuildOptions
valuesFiles []helmchart.File
repositories map[string]repository.Downloader
repositories map[string]*repository.ChartRepository
dependentChartPaths []string
wantValues chartutil.Values
wantVersion string
Expand Down Expand Up @@ -146,7 +146,7 @@ fullnameOverride: "full-foo-name-override"`),
{
name: "chart with dependencies",
reference: LocalReference{Path: "../testdata/charts/helmchartwithdeps"},
repositories: map[string]repository.Downloader{
repositories: map[string]*repository.ChartRepository{
"https://grafana.github.io/helm-charts/": mockRepo(),
},
dependentChartPaths: []string{"./../testdata/charts/helmchart"},
Expand All @@ -165,7 +165,7 @@ fullnameOverride: "full-foo-name-override"`),
{
name: "v1 chart with dependencies",
reference: LocalReference{Path: "../testdata/charts/helmchartwithdeps-v1"},
repositories: map[string]repository.Downloader{
repositories: map[string]*repository.ChartRepository{
"https://grafana.github.io/helm-charts/": mockRepo(),
},
dependentChartPaths: []string{"../testdata/charts/helmchart-v1"},
Expand Down
Loading

0 comments on commit f31dbc4

Please sign in to comment.