controllers: more tidying of wiring

Dealing with some loose ends around making observations, and code
style.

The loaded byes of a chart are used as a revision to ensure e.g.
periodic builds with unstable ordering of items do not trigger a false
positive.

Signed-off-by: Hidde Beydals <hello@hidde.co>

controllers/helmchart_controller.go

@@ -22,13 +22,12 @@ import (
 	"net/url"
 	"os"
 	"path/filepath"
-	"regexp"
 	"strings"
 	"time"
 
 	securejoin "github.com/cyphar/filepath-securejoin"
 	"github.com/go-logr/logr"
-	extgetter "helm.sh/helm/v3/pkg/getter"
+	helmgetter "helm.sh/helm/v3/pkg/getter"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	apimeta "k8s.io/apimachinery/pkg/api/meta"
@@ -69,7 +68,7 @@ type HelmChartReconciler struct {
 	client.Client
 	Scheme                *runtime.Scheme
 	Storage               *Storage
-	Getters               extgetter.Providers
+	Getters               helmgetter.Providers
 	EventRecorder         kuberecorder.EventRecorder
 	ExternalEventRecorder *events.Recorder
 	MetricsRecorder       *metrics.Recorder
@@ -199,7 +198,7 @@ func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 	}
 
 	// Create working directory
-	workDir, err := os.MkdirTemp("", chart.Kind + "-" + chart.Namespace + "-" + chart.Name + "-")
+	workDir, err := os.MkdirTemp("", chart.Kind+"-"+chart.Namespace+"-"+chart.Name+"-")
 	if err != nil {
 		err = fmt.Errorf("failed to create temporary working directory: %w", err)
 		chart = sourcev1.HelmChartNotReady(*chart.DeepCopy(), sourcev1.ChartPullFailedReason, err.Error())
@@ -216,21 +215,6 @@ func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 	var reconcileErr error
 	switch typedSource := source.(type) {
 	case *sourcev1.HelmRepository:
-		// TODO: move this to a validation webhook once the discussion around
-		//  certificates has settled: https://github.com/fluxcd/image-reflector-controller/issues/69
-		if err := validHelmChartName(chart.Spec.Chart); err != nil {
-			reconciledChart = sourcev1.HelmChartNotReady(chart, sourcev1.ChartPullFailedReason, err.Error())
-			log.Error(err, "validation failed")
-			if err := r.updateStatus(ctx, req, reconciledChart.Status); err != nil {
-				log.Info(fmt.Sprintf("%v", reconciledChart.Status))
-				log.Error(err, "unable to update status")
-				return ctrl.Result{Requeue: true}, err
-			}
-			r.event(ctx, reconciledChart, events.EventSeverityError, err.Error())
-			r.recordReadiness(ctx, reconciledChart)
-			// Do not requeue as there is no chance on recovery.
-			return ctrl.Result{Requeue: false}, nil
-		}
 		reconciledChart, reconcileErr = r.fromHelmRepository(ctx, *typedSource, *chart.DeepCopy(), workDir, changed)
 	case *sourcev1.GitRepository, *sourcev1.Bucket:
 		reconciledChart, reconcileErr = r.fromTarballArtifact(ctx, *typedSource.GetArtifact(), *chart.DeepCopy(),
@@ -309,10 +293,10 @@ func (r *HelmChartReconciler) getSource(ctx context.Context, chart sourcev1.Helm
 func (r *HelmChartReconciler) fromHelmRepository(ctx context.Context, repo sourcev1.HelmRepository, c sourcev1.HelmChart,
 	workDir string, force bool) (sourcev1.HelmChart, error) {
 	// Configure Index getter options
-	clientOpts := []extgetter.Option{
+	clientOpts := []helmgetter.Option{
-		extgetter.WithURL(repo.Spec.URL),
+		helmgetter.WithURL(repo.Spec.URL),
-		extgetter.WithTimeout(repo.Spec.Timeout.Duration),
+		helmgetter.WithTimeout(repo.Spec.Timeout.Duration),
-		extgetter.WithPassCredentialsAll(repo.Spec.PassCredentials),
+		helmgetter.WithPassCredentialsAll(repo.Spec.PassCredentials),
 	}
 	if secret, err := r.getHelmRepositorySecret(ctx, &repo); err != nil {
 		return sourcev1.HelmChartNotReady(c, sourcev1.AuthenticationFailedReason, err.Error()), err
@@ -423,7 +407,7 @@ func (r *HelmChartReconciler) fromTarballArtifact(ctx context.Context, source so
 		err = fmt.Errorf("artifact untar error: %w", err)
 		return sourcev1.HelmChartNotReady(c, sourcev1.StorageOperationFailedReason, err.Error()), err
 	}
-	if err =f.Close(); err != nil {
+	if err = f.Close(); err != nil {
 		err = fmt.Errorf("artifact close error: %w", err)
 		return sourcev1.HelmChartNotReady(c, sourcev1.StorageOperationFailedReason, err.Error()), err
 	}
@@ -440,20 +424,17 @@ func (r *HelmChartReconciler) fromTarballArtifact(ctx context.Context, source so
 		return sourcev1.HelmChartNotReady(c, sourcev1.StorageOperationFailedReason, err.Error()), err
 	}
 	dm := chart.NewDependencyManager(
-		chart.WithRepositoryCallback(r.getNamespacedChartRepositoryCallback(ctx, authDir, c.GetNamespace())),
+		chart.WithRepositoryCallback(r.namespacedChartRepositoryCallback(ctx, authDir, c.GetNamespace())),
 	)
 	defer dm.Clear()
 
-	// Get any cached chart
+	// Configure builder options, including any previously cached chart
-	var cachedChart string
-	if artifact := c.Status.Artifact; artifact != nil {
-		cachedChart = artifact.Path
-	}
-
 	buildsOpts := chart.BuildOptions{
-		ValueFiles:  c.GetValuesFiles(),
+		ValueFiles: c.GetValuesFiles(),
-		CachedChart: cachedChart,
+		Force:      force,
-		Force:       force,
+	}
+	if artifact := c.Status.Artifact; artifact != nil {
+		buildsOpts.CachedChart = artifact.Path
 	}
 
 	// Add revision metadata to chart build
@@ -465,7 +446,7 @@ func (r *HelmChartReconciler) fromTarballArtifact(ctx context.Context, source so
 
 	// Build chart
 	chartB := chart.NewLocalBuilder(dm)
-	build, err := chartB.Build(ctx, chart.LocalReference{BaseDir: sourceDir, Path: chartPath}, filepath.Join(workDir, "chart.tgz"), buildsOpts)
+	build, err := chartB.Build(ctx, chart.LocalReference{WorkDir: sourceDir, Path: chartPath}, filepath.Join(workDir, "chart.tgz"), buildsOpts)
 	if err != nil {
 		return sourcev1.HelmChartNotReady(c, sourcev1.ChartPackageFailedReason, err.Error()), err
 	}
@@ -475,7 +456,8 @@ func (r *HelmChartReconciler) fromTarballArtifact(ctx context.Context, source so
 
 	// If the path of the returned build equals the cache path,
 	// there are no changes to the chart
-	if build.Path == cachedChart {
+	if apimeta.IsStatusConditionTrue(c.Status.Conditions, meta.ReadyCondition) &&
+		build.Path == buildsOpts.CachedChart {
 		// Ensure hostname is updated
 		if c.GetArtifact().URL != newArtifact.URL {
 			r.Storage.SetArtifactURL(c.GetArtifact())
@@ -515,11 +497,17 @@ func (r *HelmChartReconciler) fromTarballArtifact(ctx context.Context, source so
 	return sourcev1.HelmChartReady(c, newArtifact, cUrl, sourcev1.ChartPackageSucceededReason, build.Summary()), nil
 }
 
-// TODO(hidde): factor out to helper?
+// namespacedChartRepositoryCallback returns a chart.GetChartRepositoryCallback
-func (r *HelmChartReconciler) getNamespacedChartRepositoryCallback(ctx context.Context, dir, namespace string) chart.GetChartRepositoryCallback {
+// scoped to the given namespace. Credentials for retrieved v1beta1.HelmRepository
+// objects are stored in the given directory.
+// The returned callback returns a repository.ChartRepository configured with the
+// retrieved v1beta1.HelmRepository, or a shim with defaults if no object could
+// be found.
+func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Context, dir, namespace string) chart.GetChartRepositoryCallback {
 	return func(url string) (*repository.ChartRepository, error) {
 		repo, err := r.resolveDependencyRepository(ctx, url, namespace)
 		if err != nil {
+			// Return Kubernetes client errors, but ignore others
 			if errors.ReasonForError(err) != metav1.StatusReasonUnknown {
 				return nil, err
 			}
@@ -530,10 +518,10 @@ func (r *HelmChartReconciler) getNamespacedChartRepositoryCallback(ctx context.C
 				},
 			}
 		}
-		clientOpts := []extgetter.Option{
+		clientOpts := []helmgetter.Option{
-			extgetter.WithURL(repo.Spec.URL),
+			helmgetter.WithURL(repo.Spec.URL),
-			extgetter.WithTimeout(repo.Spec.Timeout.Duration),
+			helmgetter.WithTimeout(repo.Spec.Timeout.Duration),
-			extgetter.WithPassCredentialsAll(repo.Spec.PassCredentials),
+			helmgetter.WithPassCredentialsAll(repo.Spec.PassCredentials),
 		}
 		if secret, err := r.getHelmRepositorySecret(ctx, repo); err != nil {
 			return nil, err
@@ -801,18 +789,6 @@ func (r *HelmChartReconciler) requestsForBucketChange(o client.Object) []reconci
 	return reqs
 }
 
-// validHelmChartName returns an error if the given string is not a
-// valid Helm chart name; a valid name must be lower case letters
-// and numbers, words may be separated with dashes (-).
-// Ref: https://helm.sh/docs/chart_best_practices/conventions/#chart-names
-func validHelmChartName(s string) error {
-	chartFmt := regexp.MustCompile("^([-a-z0-9]*)$")
-	if !chartFmt.MatchString(s) {
-		return fmt.Errorf("invalid chart name %q, a valid name must be lower case letters and numbers and MAY be separated with dashes (-)", s)
-	}
-	return nil
-}
-
 func (r *HelmChartReconciler) recordSuspension(ctx context.Context, chart sourcev1.HelmChart) {
 	if r.MetricsRecorder == nil {
 		return

controllers/helmchart_controller_test.go

@@ -25,7 +25,6 @@ import (
 	"path"
 	"path/filepath"
 	"strings"
-	"testing"
 	"time"
 
 	"github.com/fluxcd/pkg/apis/meta"
@@ -1327,26 +1326,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

@@ -24,7 +24,7 @@ import (
 	"time"
 
 	"github.com/go-logr/logr"
-	extgetter "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"
@@ -43,9 +43,9 @@ import (
 	"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/getter"
 	"github.com/fluxcd/source-controller/internal/helm/repository"
-	sourcev1 "github.com/fluxcd/source-controller/api/v1beta1"
 )
 
 // +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               extgetter.Providers
+	Getters               helmgetter.Providers
 	EventRecorder         kuberecorder.EventRecorder
 	ExternalEventRecorder *events.Recorder
 	MetricsRecorder       *metrics.Recorder
@@ -171,10 +171,10 @@ func (r *HelmRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 }
 
 func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, repo sourcev1.HelmRepository) (sourcev1.HelmRepository, error) {
-	clientOpts := []extgetter.Option{
+	clientOpts := []helmgetter.Option{
-		extgetter.WithURL(repo.Spec.URL),
+		helmgetter.WithURL(repo.Spec.URL),
-		extgetter.WithTimeout(repo.Spec.Timeout.Duration),
+		helmgetter.WithTimeout(repo.Spec.Timeout.Duration),
-		extgetter.WithPassCredentialsAll(repo.Spec.PassCredentials),
+		helmgetter.WithPassCredentialsAll(repo.Spec.PassCredentials),
 	}
 	if repo.Spec.SecretRef != nil {
 		name := types.NamespacedName{
@@ -189,7 +189,7 @@ func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, repo sourcev1.
 			return sourcev1.HelmRepositoryNotReady(repo, sourcev1.AuthenticationFailedReason, err.Error()), err
 		}
 
-		authDir, err := os.MkdirTemp("", "helm-repository-")
+		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
@@ -213,7 +213,7 @@ func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, repo sourcev1.
 			return sourcev1.HelmRepositoryNotReady(repo, sourcev1.IndexationFailedReason, err.Error()), err
 		}
 	}
-	revision, err := chartRepo.CacheIndex()
+	checksum, err := chartRepo.CacheIndex()
 	if err != nil {
 		err = fmt.Errorf("failed to download repository index: %w", err)
 		return sourcev1.HelmRepositoryNotReady(repo, sourcev1.IndexationFailedReason, err.Error()), err
@@ -222,12 +222,12 @@ func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, repo sourcev1.
 
 	artifact := r.Storage.NewArtifactFor(repo.Kind,
 		repo.ObjectMeta.GetObjectMeta(),
-		revision,
+		"",
-		fmt.Sprintf("index-%s.yaml", revision))
+		fmt.Sprintf("index-%s.yaml", checksum))
 
 	// Return early on unchanged index
 	if apimeta.IsStatusConditionTrue(repo.Status.Conditions, meta.ReadyCondition) &&
-		repo.GetArtifact().HasRevision(artifact.Revision) {
+		(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)
@@ -239,7 +239,9 @@ func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, repo sourcev1.
 	if err := chartRepo.LoadFromCache(); err != nil {
 		return sourcev1.HelmRepositoryNotReady(repo, sourcev1.IndexationFailedReason, err.Error()), err
 	}
-	defer chartRepo.Unload()
+	// The repository checksum is the SHA256 of the loaded bytes, after sorting
+	artifact.Revision = chartRepo.Checksum
+	chartRepo.Unload()
 
 	// Create artifact dir
 	err = r.Storage.MkdirAll(artifact)
@@ -257,17 +259,9 @@ func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, repo sourcev1.
 	defer unlock()
 
 	// Save artifact to storage
-	storageTarget := r.Storage.LocalPath(artifact)
+	if err = r.Storage.CopyFromPath(&artifact, chartRepo.CachePath); err != nil {
-	if storageTarget == "" {
-		err := fmt.Errorf("failed to calcalute local storage path to store artifact to")
 		return sourcev1.HelmRepositoryNotReady(repo, sourcev1.StorageOperationFailedReason, err.Error()), err
 	}
-	if err = chartRepo.Index.WriteFile(storageTarget, 0644); err != nil {
-		return sourcev1.HelmRepositoryNotReady(repo, sourcev1.StorageOperationFailedReason, err.Error()), err
-	}
-	// TODO(hidde): it would be better to make the Storage deal with this
-	artifact.Checksum = chartRepo.Checksum
-	artifact.LastUpdateTime = metav1.Now()
 
 	// Update index symlink
 	indexURL, err := r.Storage.Symlink(artifact, "index.yaml")