controllers: Fix helmchart values file merge test

controllers/helmchart_controller_test.go

@@ -25,7 +25,6 @@ import (
 	"path"
 	"path/filepath"
 	"strings"
-	"testing"
 	"time"
 
 	"github.com/fluxcd/pkg/apis/meta"
@@ -732,6 +731,7 @@ var _ = Describe("HelmChartReconciler", func() {
 			}, timeout, interval).Should(BeTrue())
 			helmChart, err := loader.Load(storage.LocalPath(*now.Status.Artifact))
 			Expect(err).NotTo(HaveOccurred())
+			Expect(helmChart.Values).ToNot(BeNil())
 			Expect(helmChart.Values["testDefault"]).To(BeTrue())
 			Expect(helmChart.Values["testOverride"]).To(BeFalse())
 
@@ -828,8 +828,17 @@ var _ = Describe("HelmChartReconciler", func() {
 				got := &sourcev1.HelmChart{}
 				Eventually(func() bool {
 					_ = k8sClient.Get(context.Background(), key, got)
-					return got.Status.Artifact.Checksum != updated.Status.Artifact.Checksum &&
-						storage.ArtifactExist(*got.Status.Artifact)
+					// Since a lot of chart updates took place above, checking
+					// artifact checksum isn't the most reliable way to find out
+					// if the artifact was changed due to the current update.
+					// Use status condition to be sure.
+					for _, condn := range got.Status.Conditions {
+						if strings.Contains(condn.Message, "with merged values files [./testdata/charts/helmchart/override.yaml]") &&
+							storage.ArtifactExist(*got.Status.Artifact) {
+							return true
+						}
+					}
+					return false
 				}, timeout, interval).Should(BeTrue())
 				f, err := os.Stat(storage.LocalPath(*got.Status.Artifact))
 				Expect(err).NotTo(HaveOccurred())
@@ -1326,26 +1335,3 @@ var _ = Describe("HelmChartReconciler", func() {
 		})
 	})
 })
-
-func Test_validHelmChartName(t *testing.T) {
-	tests := []struct {
-		name      string
-		chart     string
-		expectErr bool
-	}{
-		{"valid", "drupal", false},
-		{"valid dash", "nginx-lego", false},
-		{"valid dashes", "aws-cluster-autoscaler", false},
-		{"valid alphanum", "ng1nx-leg0", false},
-		{"invalid slash", "artifactory/invalid", true},
-		{"invalid dot", "in.valid", true},
-		{"invalid uppercase", "inValid", true},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			if err := validHelmChartName(tt.chart); (err != nil) != tt.expectErr {
-				t.Errorf("validHelmChartName() error = %v, expectErr %v", err, tt.expectErr)
-			}
-		})
-	}
-}

controllers/helmrepository_controller.go

@@ -17,14 +17,14 @@ limitations under the License.
 package controllers
 
 import (
-	"bytes"
 	"context"
 	"fmt"
 	"net/url"
+	"os"
 	"time"
 
 	"github.com/go-logr/logr"
-	"helm.sh/helm/v3/pkg/getter"
+	helmgetter "helm.sh/helm/v3/pkg/getter"
 	corev1 "k8s.io/api/core/v1"
 	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -37,15 +37,15 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
-	"sigs.k8s.io/yaml"
 
 	"github.com/fluxcd/pkg/apis/meta"
 	"github.com/fluxcd/pkg/runtime/events"
 	"github.com/fluxcd/pkg/runtime/metrics"
 	"github.com/fluxcd/pkg/runtime/predicates"
 
 	sourcev1 "github.com/fluxcd/source-controller/api/v1beta1"
-	"github.com/fluxcd/source-controller/internal/helm"
+	"github.com/fluxcd/source-controller/internal/helm/getter"
+	"github.com/fluxcd/source-controller/internal/helm/repository"
 )
 
 // +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=helmrepositories,verbs=get;list;watch;create;update;patch;delete
@@ -58,7 +58,7 @@ type HelmRepositoryReconciler struct {
 	client.Client
 	Scheme                *runtime.Scheme
 	Storage               *Storage
-	Getters               getter.Providers
+	Getters               helmgetter.Providers
 	EventRecorder         kuberecorder.EventRecorder
 	ExternalEventRecorder *events.Recorder
 	MetricsRecorder       *metrics.Recorder
@@ -170,96 +170,108 @@ func (r *HelmRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 	return ctrl.Result{RequeueAfter: repository.GetInterval().Duration}, nil
 }
 
-func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, repository sourcev1.HelmRepository) (sourcev1.HelmRepository, error) {
-	clientOpts := []getter.Option{
-		getter.WithURL(repository.Spec.URL),
-		getter.WithTimeout(repository.Spec.Timeout.Duration),
-		getter.WithPassCredentialsAll(repository.Spec.PassCredentials),
+func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, repo sourcev1.HelmRepository) (sourcev1.HelmRepository, error) {
+	clientOpts := []helmgetter.Option{
+		helmgetter.WithURL(repo.Spec.URL),
+		helmgetter.WithTimeout(repo.Spec.Timeout.Duration),
+		helmgetter.WithPassCredentialsAll(repo.Spec.PassCredentials),
 	}
-	if repository.Spec.SecretRef != nil {
+	if repo.Spec.SecretRef != nil {
 		name := types.NamespacedName{
-			Namespace: repository.GetNamespace(),
-			Name:      repository.Spec.SecretRef.Name,
+			Namespace: repo.GetNamespace(),
+			Name:      repo.Spec.SecretRef.Name,
 		}
 
 		var secret corev1.Secret
 		err := r.Client.Get(ctx, name, &secret)
 		if err != nil {
 			err = fmt.Errorf("auth secret error: %w", err)
-			return sourcev1.HelmRepositoryNotReady(repository, sourcev1.AuthenticationFailedReason, err.Error()), err
+			return sourcev1.HelmRepositoryNotReady(repo, sourcev1.AuthenticationFailedReason, err.Error()), err
 		}
 
-		opts, cleanup, err := helm.ClientOptionsFromSecret(secret)
+		authDir, err := os.MkdirTemp("", repo.Kind+"-"+repo.Namespace+"-"+repo.Name+"-")
+		if err != nil {
+			err = fmt.Errorf("failed to create temporary working directory for credentials: %w", err)
+			return sourcev1.HelmRepositoryNotReady(repo, sourcev1.AuthenticationFailedReason, err.Error()), err
+		}
+		defer os.RemoveAll(authDir)
+
+		opts, err := getter.ClientOptionsFromSecret(authDir, secret)
 		if err != nil {
 			err = fmt.Errorf("auth options error: %w", err)
-			return sourcev1.HelmRepositoryNotReady(repository, sourcev1.AuthenticationFailedReason, err.Error()), err
+			return sourcev1.HelmRepositoryNotReady(repo, sourcev1.AuthenticationFailedReason, err.Error()), err
 		}
-		defer cleanup()
 		clientOpts = append(clientOpts, opts...)
 	}
 
-	chartRepo, err := helm.NewChartRepository(repository.Spec.URL, r.Getters, clientOpts)
+	chartRepo, err := repository.NewChartRepository(repo.Spec.URL, "", r.Getters, clientOpts)
 	if err != nil {
 		switch err.(type) {
 		case *url.Error:
-			return sourcev1.HelmRepositoryNotReady(repository, sourcev1.URLInvalidReason, err.Error()), err
+			return sourcev1.HelmRepositoryNotReady(repo, sourcev1.URLInvalidReason, err.Error()), err
 		default:
-			return sourcev1.HelmRepositoryNotReady(repository, sourcev1.IndexationFailedReason, err.Error()), err
+			return sourcev1.HelmRepositoryNotReady(repo, sourcev1.IndexationFailedReason, err.Error()), err
 		}
 	}
-	if err := chartRepo.DownloadIndex(); err != nil {
+	checksum, err := chartRepo.CacheIndex()
+	if err != nil {
 		err = fmt.Errorf("failed to download repository index: %w", err)
-		return sourcev1.HelmRepositoryNotReady(repository, sourcev1.IndexationFailedReason, err.Error()), err
+		return sourcev1.HelmRepositoryNotReady(repo, sourcev1.IndexationFailedReason, err.Error()), err
 	}
+	defer chartRepo.RemoveCache()
 
-	indexBytes, err := yaml.Marshal(&chartRepo.Index)
-	if err != nil {
-		return sourcev1.HelmRepositoryNotReady(repository, sourcev1.StorageOperationFailedReason, err.Error()), err
-	}
-	hash := r.Storage.Checksum(bytes.NewReader(indexBytes))
-	artifact := r.Storage.NewArtifactFor(repository.Kind,
-		repository.ObjectMeta.GetObjectMeta(),
-		hash,
-		fmt.Sprintf("index-%s.yaml", hash))
-	// return early on unchanged index
-	if apimeta.IsStatusConditionTrue(repository.Status.Conditions, meta.ReadyCondition) && repository.GetArtifact().HasRevision(artifact.Revision) {
-		if artifact.URL != repository.GetArtifact().URL {
-			r.Storage.SetArtifactURL(repository.GetArtifact())
-			repository.Status.URL = r.Storage.SetHostname(repository.Status.URL)
+	artifact := r.Storage.NewArtifactFor(repo.Kind,
+		repo.ObjectMeta.GetObjectMeta(),
+		"",
+		fmt.Sprintf("index-%s.yaml", checksum))
+
+	// Return early on unchanged index
+	if apimeta.IsStatusConditionTrue(repo.Status.Conditions, meta.ReadyCondition) &&
+		(repo.GetArtifact() != nil && repo.GetArtifact().Checksum == checksum) {
+		if artifact.URL != repo.GetArtifact().URL {
+			r.Storage.SetArtifactURL(repo.GetArtifact())
+			repo.Status.URL = r.Storage.SetHostname(repo.Status.URL)
 		}
-		return repository, nil
+		return repo, nil
+	}
+
+	// Load the cached repository index to ensure it passes validation
+	if err := chartRepo.LoadFromCache(); err != nil {
+		return sourcev1.HelmRepositoryNotReady(repo, sourcev1.IndexationFailedReason, err.Error()), err
 	}
+	// The repository checksum is the SHA256 of the loaded bytes, after sorting
+	artifact.Revision = chartRepo.Checksum
+	chartRepo.Unload()
 
-	// create artifact dir
+	// Create artifact dir
 	err = r.Storage.MkdirAll(artifact)
 	if err != nil {
 		err = fmt.Errorf("unable to create repository index directory: %w", err)
-		return sourcev1.HelmRepositoryNotReady(repository, sourcev1.StorageOperationFailedReason, err.Error()), err
+		return sourcev1.HelmRepositoryNotReady(repo, sourcev1.StorageOperationFailedReason, err.Error()), err
 	}
 
-	// acquire lock
+	// Acquire lock
 	unlock, err := r.Storage.Lock(artifact)
 	if err != nil {
 		err = fmt.Errorf("unable to acquire lock: %w", err)
-		return sourcev1.HelmRepositoryNotReady(repository, sourcev1.StorageOperationFailedReason, err.Error()), err
+		return sourcev1.HelmRepositoryNotReady(repo, sourcev1.StorageOperationFailedReason, err.Error()), err
 	}
 	defer unlock()
 
-	// save artifact to storage
-	if err := r.Storage.AtomicWriteFile(&artifact, bytes.NewReader(indexBytes), 0644); err != nil {
-		err = fmt.Errorf("unable to write repository index file: %w", err)
-		return sourcev1.HelmRepositoryNotReady(repository, sourcev1.StorageOperationFailedReason, err.Error()), err
+	// Save artifact to storage
+	if err = r.Storage.CopyFromPath(&artifact, chartRepo.CachePath); err != nil {
+		return sourcev1.HelmRepositoryNotReady(repo, sourcev1.StorageOperationFailedReason, err.Error()), err
 	}
 
-	// update index symlink
+	// Update index symlink
 	indexURL, err := r.Storage.Symlink(artifact, "index.yaml")
 	if err != nil {
 		err = fmt.Errorf("storage error: %w", err)
-		return sourcev1.HelmRepositoryNotReady(repository, sourcev1.StorageOperationFailedReason, err.Error()), err
+		return sourcev1.HelmRepositoryNotReady(repo, sourcev1.StorageOperationFailedReason, err.Error()), err
 	}
 
 	message := fmt.Sprintf("Fetched revision: %s", artifact.Revision)
-	return sourcev1.HelmRepositoryReady(repository, artifact, indexURL, sourcev1.IndexationSucceededReason, message), nil
+	return sourcev1.HelmRepositoryReady(repo, artifact, indexURL, sourcev1.IndexationSucceededReason, message), nil
 }
 
 func (r *HelmRepositoryReconciler) reconcileDelete(ctx context.Context, repository sourcev1.HelmRepository) (ctrl.Result, error) {

go.mod

@@ -29,6 +29,7 @@ require (
 	github.com/minio/minio-go/v7 v7.0.10
 	github.com/onsi/ginkgo v1.16.4
 	github.com/onsi/gomega v1.14.0
+	github.com/otiai10/copy v1.7.0
 	github.com/spf13/pflag v1.0.5
 	golang.org/x/crypto v0.0.0-20210421170649-83a5a9bb288b
 	golang.org/x/net v0.0.0-20210825183410-e898025ed96a // indirect

go.sum

@@ -752,6 +752,13 @@ github.com/openzipkin-contrib/zipkin-go-opentracing v0.4.5/go.mod h1:/wsWhb9smxS
 github.com/openzipkin/zipkin-go v0.1.6/go.mod h1:QgAqvLzwWbR/WpD4A3cGpPtJrZXNIiJc5AZX7/PBEpw=
 github.com/openzipkin/zipkin-go v0.2.1/go.mod h1:NaW6tEwdmWMaCDZzg8sh+IBNOxHMPnhQw8ySjnjRyN4=
 github.com/openzipkin/zipkin-go v0.2.2/go.mod h1:NaW6tEwdmWMaCDZzg8sh+IBNOxHMPnhQw8ySjnjRyN4=
+github.com/otiai10/copy v1.7.0 h1:hVoPiN+t+7d2nzzwMiDHPSOogsWAStewq3TwU05+clE=
+github.com/otiai10/copy v1.7.0/go.mod h1:rmRl6QPdJj6EiUqXQ/4Nn2lLXoNQjFCQbbNrxgc/t3U=
+github.com/otiai10/curr v0.0.0-20150429015615-9b4961190c95/go.mod h1:9qAhocn7zKJG+0mI8eUu6xqkFDYS2kb2saOteoSB3cE=
+github.com/otiai10/curr v1.0.0/go.mod h1:LskTG5wDwr8Rs+nNQ+1LlxRjAtTZZjtJW4rMXl6j4vs=
+github.com/otiai10/mint v1.3.0/go.mod h1:F5AjcsTsWUqX+Na9fpHb52P8pcRX2CI6A3ctIT91xUo=
+github.com/otiai10/mint v1.3.3 h1:7JgpsBaN0uMkyju4tbYHu0mnM55hNKVYLsXmwr15NQI=
+github.com/otiai10/mint v1.3.3/go.mod h1:/yxELlJQ0ufhjUwhshSj+wFjZ78CnZ48/1wtmBH1OTc=
 github.com/pact-foundation/pact-go v1.0.4/go.mod h1:uExwJY4kCzNPcHRj+hCR/HBbOOIwwtUjcrb0b5/5kLM=
 github.com/pascaldekloe/goe v0.0.0-20180627143212-57f6aae5913c/go.mod h1:lzWF7FIEvWOWxwDKqyGYQf6ZUaNfKdP144TG7ZOy1lc=
 github.com/pborman/uuid v1.2.0/go.mod h1:X/NO0urCmaxf9VXbdlT7C2Yzkj2IKimNn4k+gtPdI/k=