From 01c459dbd30c7e0bb5251c624ba32ff2dc5b2f94 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Tue, 16 Nov 2021 09:50:07 +0100 Subject: [PATCH] controllers: more tidying of wiring --- controllers/helmchart_controller.go | 84 +++++++++--------------- controllers/helmchart_controller_test.go | 24 ------- controllers/helmrepository_controller.go | 37 +++++------ 3 files changed, 46 insertions(+), 99 deletions(-) diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index d31f6c2bb..3c1be0a7d 100644 --- a/controllers/helmchart_controller.go +++ b/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 diff --git a/controllers/helmchart_controller_test.go b/controllers/helmchart_controller_test.go index 9c325af4a..5d7136672 100644 --- a/controllers/helmchart_controller_test.go +++ b/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) - } - }) - } -} diff --git a/controllers/helmrepository_controller.go b/controllers/helmrepository_controller.go index 8ab87201d..eaee5c32e 100644 --- a/controllers/helmrepository_controller.go +++ b/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