controllers: more tidying of wiring

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{
- extgetter.WithURL(repo.Spec.URL),
- extgetter.WithTimeout(repo.Spec.Timeout.Duration),
- extgetter.WithPassCredentialsAll(repo.Spec.PassCredentials),
+ clientOpts := []helmgetter.Option{
+ helmgetter.WithURL(repo.Spec.URL),
+ helmgetter.WithTimeout(repo.Spec.Timeout.Duration),
+ 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
- var cachedChart string
- if artifact := c.Status.Artifact; artifact != nil {
- cachedChart = artifact.Path
- }
-
+ // Configure builder options, including any previously cached chart
  buildsOpts := chart.BuildOptions{
- ValueFiles: c.GetValuesFiles(),
- CachedChart: cachedChart,
- Force: force,
+ ValueFiles: c.GetValuesFiles(),
+ 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?
-func (r *HelmChartReconciler) getNamespacedChartRepositoryCallback(ctx context.Context, dir, namespace string) chart.GetChartRepositoryCallback {
+// namespacedChartRepositoryCallback returns a 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{
- extgetter.WithURL(repo.Spec.URL),
- extgetter.WithTimeout(repo.Spec.Timeout.Duration),
- extgetter.WithPassCredentialsAll(repo.Spec.PassCredentials),
+ clientOpts := []helmgetter.Option{
+ helmgetter.WithURL(repo.Spec.URL),
+ helmgetter.WithTimeout(repo.Spec.Timeout.Duration),
+ 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{
- extgetter.WithURL(repo.Spec.URL),
- extgetter.WithTimeout(repo.Spec.Timeout.Duration),
- extgetter.WithPassCredentialsAll(repo.Spec.PassCredentials),
+ clientOpts := []helmgetter.Option{
+ helmgetter.WithURL(repo.Spec.URL),
+ helmgetter.WithTimeout(repo.Spec.Timeout.Duration),
+ 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,16 +259,9 @@ func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, repo sourcev1.
  defer unlock()
 
  // Save artifact to storage
- storageTarget := r.Storage.LocalPath(artifact)
- 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 {
+ if err = r.Storage.CopyFromPath(&artifact, chartRepo.CachePath); 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