From 8e107ea60e95d525ee4a0ef598c9127a0321c3e7 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Wed, 8 Dec 2021 22:15:27 +0100 Subject: [PATCH] HelmChartReconciler refactor Signed-off-by: Hidde Beydals --- api/v1beta2/condition_types.go | 4 + api/v1beta2/helmchart_types.go | 52 +- .../source.toolkit.fluxcd.io_helmcharts.yaml | 8 + controllers/helmchart_controller.go | 1090 ++++++++++------- controllers/helmchart_controller_test.go | 28 +- controllers/legacy_suite_test.go | 6 +- internal/helm/chart/builder.go | 50 +- internal/helm/chart/builder_local.go | 26 +- internal/helm/chart/builder_remote.go | 16 +- internal/helm/chart/builder_test.go | 41 +- internal/helm/chart/errors.go | 39 +- internal/helm/chart/errors_test.go | 4 +- main.go | 11 +- 13 files changed, 783 insertions(+), 592 deletions(-) diff --git a/api/v1beta2/condition_types.go b/api/v1beta2/condition_types.go index 13c14498..787703b3 100644 --- a/api/v1beta2/condition_types.go +++ b/api/v1beta2/condition_types.go @@ -32,6 +32,10 @@ const ( // Source may be outdated. // This is a "negative polarity" or "abnormal-true" type, and is only present on the resource if it is True. FetchFailedCondition string = "FetchFailed" + + // BuildFailedCondition indicates a transient or persistent build failure of a Source's Artifact. + // If True, the Source can be in an ArtifactOutdatedCondition + BuildFailedCondition string = "BuildFailed" ) const ( diff --git a/api/v1beta2/helmchart_types.go b/api/v1beta2/helmchart_types.go index 8b237da0..4852e0a7 100644 --- a/api/v1beta2/helmchart_types.go +++ b/api/v1beta2/helmchart_types.go @@ -19,12 +19,10 @@ package v1beta2 import ( "time" - apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/fluxcd/pkg/apis/acl" "github.com/fluxcd/pkg/apis/meta" - "github.com/fluxcd/pkg/runtime/conditions" ) // HelmChartKind is the string representation of a HelmChart. @@ -115,6 +113,16 @@ type HelmChartStatus struct { // +optional ObservedGeneration int64 `json:"observedGeneration,omitempty"` + // ObservedSourceArtifactRevision is the last observed Artifact.Revision + // of the Source reference. + // +optional + ObservedSourceArtifactRevision string `json:"observedSourceArtifactRevision,omitempty"` + + // ObservedChartName is the last observed chart name as defined by the + // resolved chart reference. + // +optional + ObservedChartName string `json:"observedChartName,omitempty"` + // Conditions holds the conditions for the HelmChart. // +optional Conditions []metav1.Condition `json:"conditions,omitempty"` @@ -148,46 +156,6 @@ const ( ChartPackageSucceededReason string = "ChartPackageSucceeded" ) -// HelmChartProgressing resets the conditions of the HelmChart to meta.Condition -// of type meta.ReadyCondition with status 'Unknown' and meta.ProgressingReason -// reason and message. It returns the modified HelmChart. -func HelmChartProgressing(chart HelmChart) HelmChart { - chart.Status.ObservedGeneration = chart.Generation - chart.Status.URL = "" - chart.Status.Conditions = []metav1.Condition{} - conditions.MarkUnknown(&chart, meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress") - return chart -} - -// HelmChartReady sets the given Artifact and URL on the HelmChart and sets the -// meta.ReadyCondition to 'True', with the given reason and message. It returns -// the modified HelmChart. -func HelmChartReady(chart HelmChart, artifact Artifact, url, reason, message string) HelmChart { - chart.Status.Artifact = &artifact - chart.Status.URL = url - conditions.MarkTrue(&chart, meta.ReadyCondition, reason, message) - return chart -} - -// HelmChartNotReady sets the meta.ReadyCondition on the given HelmChart to -// 'False', with the given reason and message. It returns the modified -// HelmChart. -func HelmChartNotReady(chart HelmChart, reason, message string) HelmChart { - conditions.MarkFalse(&chart, meta.ReadyCondition, reason, message) - return chart -} - -// HelmChartReadyMessage returns the message of the meta.ReadyCondition with -// status 'True', or an empty string. -func HelmChartReadyMessage(chart HelmChart) string { - if c := apimeta.FindStatusCondition(chart.Status.Conditions, meta.ReadyCondition); c != nil { - if c.Status == metav1.ConditionTrue { - return c.Message - } - } - return "" -} - // GetConditions returns the status conditions of the object. func (in HelmChart) GetConditions() []metav1.Condition { return in.Status.Conditions diff --git a/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml b/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml index 06d6773a..dbf29410 100644 --- a/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml +++ b/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml @@ -517,10 +517,18 @@ spec: reconcile request value, so a change of the annotation value can be detected. type: string + observedChartName: + description: ObservedChartName is the last observed chart name as + defined by the resolved chart reference. + type: string observedGeneration: description: ObservedGeneration is the last observed generation. format: int64 type: integer + observedSourceArtifactRevision: + description: ObservedSourceArtifactRevision is the last observed Artifact.Revision + of the Source reference. + type: string url: description: URL is the download link for the last chart pulled. type: string diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index d9153af1..38a95983 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -31,12 +31,11 @@ import ( helmgetter "helm.sh/helm/v3/pkg/getter" corev1 "k8s.io/api/core/v1" apierrs "k8s.io/apimachinery/pkg/api/errors" - apimeta "k8s.io/apimachinery/pkg/api/meta" 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" kuberecorder "k8s.io/client-go/tools/record" - "k8s.io/client-go/tools/reference" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" @@ -48,17 +47,51 @@ import ( "sigs.k8s.io/controller-runtime/pkg/source" "github.com/fluxcd/pkg/apis/meta" + "github.com/fluxcd/pkg/runtime/conditions" + helper "github.com/fluxcd/pkg/runtime/controller" "github.com/fluxcd/pkg/runtime/events" - "github.com/fluxcd/pkg/runtime/metrics" + "github.com/fluxcd/pkg/runtime/patch" "github.com/fluxcd/pkg/runtime/predicates" "github.com/fluxcd/pkg/untar" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" + serror "github.com/fluxcd/source-controller/internal/error" "github.com/fluxcd/source-controller/internal/helm/chart" "github.com/fluxcd/source-controller/internal/helm/getter" "github.com/fluxcd/source-controller/internal/helm/repository" + sreconcile "github.com/fluxcd/source-controller/internal/reconcile" + "github.com/fluxcd/source-controller/internal/util" ) +// Status conditions owned by the HelmChart reconciler. +var helmChartOwnedConditions = []string{ + sourcev1.BuildFailedCondition, + sourcev1.FetchFailedCondition, + sourcev1.ArtifactOutdatedCondition, + meta.ReadyCondition, + meta.ReconcilingCondition, + meta.StalledCondition, +} + +// Conditions that Ready condition is influenced by in descending order of their +// priority. +var helmChartReadyDeps = []string{ + sourcev1.BuildFailedCondition, + sourcev1.FetchFailedCondition, + sourcev1.ArtifactOutdatedCondition, + meta.StalledCondition, + meta.ReconcilingCondition, +} + +// Negative conditions that Ready condition is influenced by. +var helmChartReadyDepsNegative = []string{ + sourcev1.BuildFailedCondition, + sourcev1.FetchFailedCondition, + sourcev1.ArtifactOutdatedCondition, + meta.StalledCondition, + meta.ReconcilingCondition, +} + // +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=helmcharts,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=helmcharts/status,verbs=get;update;patch // +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=helmcharts/finalizers,verbs=get;create;update;patch;delete @@ -67,18 +100,23 @@ import ( // HelmChartReconciler reconciles a HelmChart object type HelmChartReconciler struct { client.Client - Scheme *runtime.Scheme - Storage *Storage - Getters helmgetter.Providers - EventRecorder kuberecorder.EventRecorder - ExternalEventRecorder *events.Recorder - MetricsRecorder *metrics.Recorder + kuberecorder.EventRecorder + helper.Metrics + + Storage *Storage + Getters helmgetter.Providers } func (r *HelmChartReconciler) SetupWithManager(mgr ctrl.Manager) error { return r.SetupWithManagerAndOptions(mgr, HelmChartReconcilerOptions{}) } +type HelmChartReconcilerOptions struct { + MaxConcurrentReconciles int +} + +type helmChartReconcilerFunc func(ctx context.Context, obj *sourcev1.HelmChart, build *chart.Build) (sreconcile.Result, error) + func (r *HelmChartReconciler) SetupWithManagerAndOptions(mgr ctrl.Manager, opts HelmChartReconcilerOptions) error { if err := mgr.GetCache().IndexField(context.TODO(), &sourcev1.HelmRepository{}, sourcev1.HelmRepositoryURLIndexKey, r.indexHelmRepositoryByURL); err != nil { @@ -112,211 +150,264 @@ func (r *HelmChartReconciler) SetupWithManagerAndOptions(mgr ctrl.Manager, opts Complete(r) } -func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ctrl.Result, retErr error) { start := time.Now() log := ctrl.LoggerFrom(ctx) - var chart sourcev1.HelmChart - if err := r.Get(ctx, req.NamespacedName, &chart); err != nil { - return ctrl.Result{Requeue: true}, client.IgnoreNotFound(err) + // Fetch the HelmChart + obj := &sourcev1.HelmChart{} + if err := r.Get(ctx, req.NamespacedName, obj); err != nil { + return ctrl.Result{}, client.IgnoreNotFound(err) } // Record suspended status metric - defer r.recordSuspension(ctx, chart) + r.RecordSuspend(ctx, obj, obj.Spec.Suspend) - // Add our finalizer if it does not exist - if !controllerutil.ContainsFinalizer(&chart, sourcev1.SourceFinalizer) { - patch := client.MergeFrom(chart.DeepCopy()) - controllerutil.AddFinalizer(&chart, sourcev1.SourceFinalizer) - if err := r.Patch(ctx, &chart, patch); err != nil { - log.Error(err, "unable to register finalizer") - return ctrl.Result{}, err - } - } - - // Examine if the object is under deletion - if !chart.ObjectMeta.DeletionTimestamp.IsZero() { - return r.reconcileDelete(ctx, chart) - } - - // Return early if the object is suspended. - if chart.Spec.Suspend { + // Return early if the object is suspended + if obj.Spec.Suspend { log.Info("Reconciliation is suspended for this object") return ctrl.Result{}, nil } - // Record reconciliation duration - if r.MetricsRecorder != nil { - objRef, err := reference.GetReference(r.Scheme, &chart) - if err != nil { - return ctrl.Result{}, err - } - defer r.MetricsRecorder.RecordDuration(*objRef, start) - } - - // Conditionally set progressing condition in status - resetChart, changed := r.resetStatus(chart) - if changed { - chart = resetChart - if err := r.updateStatus(ctx, req, chart.Status); err != nil { - log.Error(err, "unable to update status") - return ctrl.Result{Requeue: true}, err - } - r.recordReadiness(ctx, chart) - } - - // Record the value of the reconciliation request, if any - // TODO(hidde): would be better to defer this in combination with - // always patching the status sub-resource after a reconciliation. - if v, ok := meta.ReconcileAnnotationValue(chart.GetAnnotations()); ok { - chart.Status.SetLastHandledReconcileRequest(v) - } - - // Purge all but current artifact from storage - if err := r.gc(chart); err != nil { - log.Error(err, "unable to purge old artifacts") - } - - // Retrieve the source - source, err := r.getSource(ctx, chart) + // Initialize the patch helper + patchHelper, err := patch.NewHelper(obj, r.Client) if err != nil { - chart = sourcev1.HelmChartNotReady(*chart.DeepCopy(), sourcev1.ChartPullFailedReason, err.Error()) - if err := r.updateStatus(ctx, req, chart.Status); err != nil { - log.Error(err, "unable to update status") - } - return ctrl.Result{Requeue: true}, err + return ctrl.Result{}, err } - // Assert source is ready - if source.GetArtifact() == nil { - err = fmt.Errorf("no artifact found for source `%s` kind '%s'", - chart.Spec.SourceRef.Name, chart.Spec.SourceRef.Kind) - chart = sourcev1.HelmChartNotReady(*chart.DeepCopy(), sourcev1.ChartPullFailedReason, err.Error()) - if err := r.updateStatus(ctx, req, chart.Status); err != nil { - log.Error(err, "unable to update status") - } - r.recordReadiness(ctx, chart) - return ctrl.Result{Requeue: true}, err - } + // Result of the sub-reconciliation + var recResult sreconcile.Result - // Create working directory - 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()) - if err := r.updateStatus(ctx, req, chart.Status); err != nil { - log.Error(err, "unable to update status") - } - r.recordReadiness(ctx, chart) - return ctrl.Result{Requeue: true}, err - } + // Always attempt to patch the object after each reconciliation. + // NOTE: This deferred block only modifies the named return error. The + // result from the reconciliation remains the same. Any requeue attributes + // set in the result will continue to be effective. defer func() { - if err := os.RemoveAll(workDir); err != nil { - log.Error(err, "failed to remove working directory", "path", workDir) - } + retErr = r.summarizeAndPatch(ctx, obj, patchHelper, recResult, retErr) + + // Always record readiness and duration metrics + r.Metrics.RecordReadiness(ctx, obj) + r.Metrics.RecordDuration(ctx, obj, start) }() + // Add finalizer first if not exist to avoid the race condition + // between init and delete + if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) { + controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer) + recResult = sreconcile.ResultRequeue + return ctrl.Result{Requeue: true}, nil + } + + // Examine if the object is under deletion + if !obj.ObjectMeta.DeletionTimestamp.IsZero() { + res, err := r.reconcileDelete(ctx, obj) + return sreconcile.BuildRuntimeResult(ctx, r.EventRecorder, obj, res, err) + } + + // Reconcile actual object + reconcilers := []helmChartReconcilerFunc{ + r.reconcileStorage, + r.reconcileSource, + r.reconcileArtifact, + } + recResult, err = r.reconcile(ctx, obj, reconcilers) + return sreconcile.BuildRuntimeResult(ctx, r.EventRecorder, obj, recResult, err) +} + +// summarizeAndPatch analyzes the object conditions to create a summary of the +// status conditions and patches the object with the calculated summary. The +// reconciler error type is also used to determine the conditions and the +// returned error. +func (r *HelmChartReconciler) summarizeAndPatch(ctx context.Context, obj *sourcev1.HelmChart, patchHelper *patch.Helper, res sreconcile.Result, recErr error) error { + // Record the value of the reconciliation request, if any + if v, ok := meta.ReconcileAnnotationValue(obj.GetAnnotations()); ok { + obj.Status.SetLastHandledReconcileRequest(v) + } + + // Compute the reconcile results, obtain patch options and reconcile error. + var patchOpts []patch.Option + patchOpts, recErr = sreconcile.ComputeReconcileResult(obj, res, recErr, helmChartOwnedConditions) + + // Summarize Ready condition + conditions.SetSummary(obj, + meta.ReadyCondition, + conditions.WithConditions( + helmChartReadyDeps..., + ), + conditions.WithNegativePolarityConditions( + helmChartReadyDepsNegative..., + ), + ) + + // Finally, patch the resource + if err := patchHelper.Patch(ctx, obj, patchOpts...); err != nil { + // Ignore patch error "not found" when the object is being deleted. + if !obj.ObjectMeta.DeletionTimestamp.IsZero() { + err = kerrors.FilterOut(err, func(e error) bool { return apierrs.IsNotFound(err) }) + } + recErr = kerrors.NewAggregate([]error{recErr, err}) + } + return recErr +} + +// reconcile steps through the actual reconciliation tasks for the object, it returns early on the first step that +// produces an error. +func (r *HelmChartReconciler) reconcile(ctx context.Context, obj *sourcev1.HelmChart, reconcilers []helmChartReconcilerFunc) (sreconcile.Result, error) { + if obj.Generation != obj.Status.ObservedGeneration { + conditions.MarkReconciling(obj, "NewGeneration", "reconciling new generation %d", obj.Generation) + } + + // Run the sub-reconcilers and build the result of reconciliation. + var ( + build chart.Build + res sreconcile.Result + resErr error + ) + for _, rec := range reconcilers { + recResult, err := rec(ctx, obj, &build) + // Exit immediately on ResultRequeue. + if recResult == sreconcile.ResultRequeue { + return sreconcile.ResultRequeue, nil + } + // If an error is received, prioritize the returned results because an + // error also means immediate requeue. + if err != nil { + resErr = err + res = recResult + break + } + // Prioritize requeue request in the result. + res = sreconcile.LowestRequeuingResult(res, recResult) + } + return res, resErr +} + +// reconcileStorage ensures the current state of the storage matches the desired and previously observed state. +// +// All artifacts for the resource except for the current one are garbage collected from the storage. +// If the artifact in the Status object of the resource disappeared from storage, it is removed from the object. +// If the object does not have an artifact in its Status object, a v1beta1.ArtifactUnavailableCondition is set. +// If the hostname of the URLs on the object do not match the current storage server hostname, they are updated. +// +// The caller should assume a failure if an error is returned, or the BuildResult is zero. +func (r *HelmChartReconciler) reconcileStorage(ctx context.Context, obj *sourcev1.HelmChart, build *chart.Build) (sreconcile.Result, error) { + // Garbage collect previous advertised artifact(s) from storage + _ = r.garbageCollect(ctx, obj) + + // Determine if the advertised artifact is still in storage + if artifact := obj.GetArtifact(); artifact != nil && !r.Storage.ArtifactExist(*artifact) { + obj.Status.Artifact = nil + obj.Status.URL = "" + } + + // Record that we do not have an artifact + if obj.GetArtifact() == nil { + conditions.MarkReconciling(obj, "NoArtifact", "no artifact for resource in storage") + return sreconcile.ResultSuccess, nil + } + + // Always update URLs to ensure hostname is up-to-date + // TODO(hidde): we may want to send out an event only if we notice the URL has changed + r.Storage.SetArtifactURL(obj.GetArtifact()) + obj.Status.URL = r.Storage.SetHostname(obj.Status.URL) + + return sreconcile.ResultSuccess, nil +} + +// reconcileSource reconciles the upstream bucket with the client for the given object's Provider, and returns the +// result. +// If a SecretRef is defined, it attempts to fetch the Secret before calling the provider. If the fetch of the Secret +// fails, it records v1beta1.FetchFailedCondition=True and returns early. +// +// The caller should assume a failure if an error is returned, or the BuildResult is zero. +func (r *HelmChartReconciler) reconcileSource(ctx context.Context, obj *sourcev1.HelmChart, build *chart.Build) (sreconcile.Result, error) { + // Retrieve the source + s, err := r.getSource(ctx, obj) + if err != nil { + e := &serror.Event{ + Err: fmt.Errorf("failed to get source: %w", err), + Reason: "SourceUnavailable", + } + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, "SourceUnavailable", e.Err.Error()) + + // Return Kubernetes client errors, but ignore others which can only be + // solved by a change in generation + if apierrs.ReasonForError(err) != metav1.StatusReasonUnknown { + return sreconcile.ResultEmpty, &serror.Stalling{ + Err: fmt.Errorf("failed to get source: %w", err), + Reason: "UnsupportedSourceKind", + } + } + return sreconcile.ResultEmpty, e + } + + // Assert source has an artifact + if s.GetArtifact() == nil || !r.Storage.ArtifactExist(*s.GetArtifact()) { + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, "NoSourceArtifact", + "no artifact available for %s source '%s'", obj.Spec.SourceRef.Kind, obj.Spec.SourceRef.Name) + r.eventLogf(ctx, obj, events.EventTypeTrace, "NoSourceArtifact", + "no artifact available for %s source '%s'", obj.Spec.SourceRef.Kind, obj.Spec.SourceRef.Name) + return sreconcile.ResultRequeue, nil + } + + // Record current artifact revision as last observed + obj.Status.ObservedSourceArtifactRevision = s.GetArtifact().Revision + // Perform the reconciliation for the chart source type - var reconciledChart sourcev1.HelmChart - var reconcileErr error - switch typedSource := source.(type) { + switch typedSource := s.(type) { case *sourcev1.HelmRepository: - reconciledChart, reconcileErr = r.fromHelmRepository(ctx, *typedSource, *chart.DeepCopy(), workDir, changed) + return r.reconcileFromHelmRepository(ctx, obj, typedSource, build) case *sourcev1.GitRepository, *sourcev1.Bucket: - reconciledChart, reconcileErr = r.fromTarballArtifact(ctx, *typedSource.GetArtifact(), *chart.DeepCopy(), - workDir, changed) + return r.reconcileFromTarballArtifact(ctx, obj, *typedSource.GetArtifact(), build) default: - err := fmt.Errorf("unable to reconcile unsupported source reference kind '%s'", chart.Spec.SourceRef.Kind) - return ctrl.Result{Requeue: false}, err + // Ending up here should generally not be possible + // as getSource already validates + return sreconcile.ResultEmpty, nil } - - // Update status with the reconciliation result - if err := r.updateStatus(ctx, req, reconciledChart.Status); err != nil { - log.Error(err, "unable to update status") - return ctrl.Result{Requeue: true}, err - } - - // If reconciliation failed, record the failure and requeue immediately - if reconcileErr != nil { - r.event(ctx, reconciledChart, events.EventSeverityError, reconcileErr.Error()) - r.recordReadiness(ctx, reconciledChart) - return ctrl.Result{Requeue: true}, reconcileErr - } - - // Emit an event if we did not have an artifact before, or the revision has changed - if (chart.GetArtifact() == nil && reconciledChart.GetArtifact() != nil) || - (chart.GetArtifact() != nil && reconciledChart.GetArtifact() != nil && reconciledChart.GetArtifact().Revision != chart.GetArtifact().Revision) { - r.event(ctx, reconciledChart, events.EventSeverityInfo, sourcev1.HelmChartReadyMessage(reconciledChart)) - } - r.recordReadiness(ctx, reconciledChart) - - log.Info(fmt.Sprintf("Reconciliation finished in %s, next run in %s", - time.Since(start).String(), - chart.GetRequeueAfter().String(), - )) - return ctrl.Result{RequeueAfter: chart.GetRequeueAfter()}, nil } -type HelmChartReconcilerOptions struct { - MaxConcurrentReconciles int -} +func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context, obj *sourcev1.HelmChart, + repo *sourcev1.HelmRepository, b *chart.Build) (sreconcile.Result, error) { -func (r *HelmChartReconciler) getSource(ctx context.Context, chart sourcev1.HelmChart) (sourcev1.Source, error) { - var source sourcev1.Source - namespacedName := types.NamespacedName{ - Namespace: chart.GetNamespace(), - Name: chart.Spec.SourceRef.Name, - } - switch chart.Spec.SourceRef.Kind { - case sourcev1.HelmRepositoryKind: - var repository sourcev1.HelmRepository - err := r.Client.Get(ctx, namespacedName, &repository) - if err != nil { - return source, fmt.Errorf("failed to retrieve source: %w", err) - } - source = &repository - case sourcev1.GitRepositoryKind: - var repository sourcev1.GitRepository - err := r.Client.Get(ctx, namespacedName, &repository) - if err != nil { - return source, fmt.Errorf("failed to retrieve source: %w", err) - } - source = &repository - case sourcev1.BucketKind: - var bucket sourcev1.Bucket - err := r.Client.Get(ctx, namespacedName, &bucket) - if err != nil { - return source, fmt.Errorf("failed to retrieve source: %w", err) - } - source = &bucket - default: - return source, fmt.Errorf("source `%s` kind '%s' not supported", - chart.Spec.SourceRef.Name, chart.Spec.SourceRef.Kind) - } - return source, nil -} - -func (r *HelmChartReconciler) fromHelmRepository(ctx context.Context, repo sourcev1.HelmRepository, c sourcev1.HelmChart, - workDir string, force bool) (sourcev1.HelmChart, error) { - // Configure Index getter options + // Construct the Getter options from the HelmRepository data 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 - } else if secret != nil { - // Create temporary working directory for credentials - authDir := filepath.Join(workDir, "creds") - if err := os.Mkdir(authDir, 0700); err != nil { - err = fmt.Errorf("failed to create temporary directory for repository credentials: %w", err) - return sourcev1.HelmChartNotReady(c, sourcev1.StorageOperationFailedReason, err.Error()), err + if secret, err := r.getHelmRepositorySecret(ctx, repo); secret != nil || err != nil { + if err != nil { + e := &serror.Event{ + Err: fmt.Errorf("failed to get secret '%s': %w", repo.Spec.SecretRef.Name, err), + Reason: sourcev1.AuthenticationFailedReason, + } + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, e.Err.Error()) + // Return error as the world as observed may change + return sreconcile.ResultEmpty, e } + + // Create temporary working directory for credentials + authDir, err := util.TempDirForObj("", obj) + if err != nil { + e := &serror.Event{ + Err: fmt.Errorf("failed to create temporary working directory: %w", err), + Reason: sourcev1.StorageOperationFailedReason, + } + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.StorageOperationFailedReason, e.Err.Error()) + return sreconcile.ResultEmpty, e + } + defer os.RemoveAll(authDir) + + // Build client options from secret opts, err := getter.ClientOptionsFromSecret(authDir, *secret) if err != nil { - err = fmt.Errorf("failed to create client options for HelmRepository '%s': %w", repo.Name, err) - return sourcev1.HelmChartNotReady(c, sourcev1.AuthenticationFailedReason, err.Error()), err + e := &serror.Event{ + Err: fmt.Errorf("failed to configure Helm client with secret data: %w", err), + Reason: sourcev1.AuthenticationFailedReason, + } + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, e.Err.Error()) + // Requeue as content of secret might change + return sreconcile.ResultEmpty, e } clientOpts = append(clientOpts, opts...) } @@ -324,139 +415,170 @@ func (r *HelmChartReconciler) fromHelmRepository(ctx context.Context, repo sourc // Initialize the chart repository chartRepo, err := repository.NewChartRepository(repo.Spec.URL, r.Storage.LocalPath(*repo.GetArtifact()), r.Getters, clientOpts) if err != nil { + // Any error requires a change in generation, + // which we should be informed about by the watcher switch err.(type) { case *url.Error: - return sourcev1.HelmChartNotReady(c, sourcev1.URLInvalidReason, err.Error()), err + e := &serror.Stalling{ + Err: fmt.Errorf("invalid Helm repository URL: %w", err), + Reason: sourcev1.URLInvalidReason, + } + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.URLInvalidReason, e.Err.Error()) + return sreconcile.ResultEmpty, e default: - return sourcev1.HelmChartNotReady(c, sourcev1.ChartPullFailedReason, err.Error()), err + e := &serror.Stalling{ + Err: fmt.Errorf("failed to construct Helm client: %w", err), + Reason: meta.FailedReason, + } + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, meta.FailedReason, e.Err.Error()) + return sreconcile.ResultEmpty, e } } - // Build the chart + // Construct the chart builder with scoped configuration cb := chart.NewRemoteBuilder(chartRepo) - ref := chart.RemoteReference{Name: c.Spec.Chart, Version: c.Spec.Version} opts := chart.BuildOptions{ - ValuesFiles: c.GetValuesFiles(), - Force: force, + ValuesFiles: obj.GetValuesFiles(), + Force: obj.Generation != obj.Status.ObservedGeneration, } - if artifact := c.GetArtifact(); artifact != nil { + if artifact := obj.GetArtifact(); artifact != nil { opts.CachedChart = r.Storage.LocalPath(*artifact) } // Set the VersionMetadata to the object's Generation if ValuesFiles is defined // This ensures changes can be noticed by the Artifact consumer if len(opts.GetValuesFiles()) > 0 { - opts.VersionMetadata = strconv.FormatInt(c.Generation, 10) + opts.VersionMetadata = strconv.FormatInt(obj.Generation, 10) } - b, err := cb.Build(ctx, ref, filepath.Join(workDir, "chart.tgz"), opts) + + // Build the chart + ref := chart.RemoteReference{Name: obj.Spec.Chart, Version: obj.Spec.Version} + build, err := cb.Build(ctx, ref, util.TempPathForObj("", ".tgz", obj), opts) + + // Record both success _and_ error observations on the object + processChartBuild(obj, build, err) + + // Handle any build error if err != nil { - return sourcev1.HelmChartNotReady(c, sourcev1.ChartPullFailedReason, err.Error()), err - } - - newArtifact := r.Storage.NewArtifactFor(c.Kind, c.GetObjectMeta(), b.Version, - fmt.Sprintf("%s-%s.tgz", b.Name, b.Version)) - - // If the path of the returned build equals the cache path, - // there are no changes to the chart - if b.Path == opts.CachedChart { - // Ensure hostname is updated - if c.GetArtifact().URL != newArtifact.URL { - r.Storage.SetArtifactURL(c.GetArtifact()) - c.Status.URL = r.Storage.SetHostname(c.Status.URL) + e := fmt.Errorf("failed to build chart from remote source: %w", err) + reason := meta.FailedReason + if buildErr := new(chart.BuildError); errors.As(err, &buildErr) { + reason = buildErr.Reason.Reason + if chart.IsPersistentBuildErrorReason(buildErr.Reason) { + return sreconcile.ResultEmpty, &serror.Stalling{ + Err: e, + Reason: reason, + } + } + } + return sreconcile.ResultEmpty, &serror.Event{ + Err: e, + Reason: reason, } - return c, nil } - // Ensure artifact directory exists - err = r.Storage.MkdirAll(newArtifact) - if err != nil { - err = fmt.Errorf("unable to create chart directory: %w", err) - return sourcev1.HelmChartNotReady(c, sourcev1.StorageOperationFailedReason, err.Error()), err - } - - // Acquire a lock for the artifact - unlock, err := r.Storage.Lock(newArtifact) - if err != nil { - err = fmt.Errorf("unable to acquire lock: %w", err) - return sourcev1.HelmChartNotReady(c, sourcev1.StorageOperationFailedReason, err.Error()), err - } - defer unlock() - - // Copy the packaged chart to the artifact path - if err = r.Storage.CopyFromPath(&newArtifact, b.Path); err != nil { - err = fmt.Errorf("failed to write chart package to storage: %w", err) - return sourcev1.HelmChartNotReady(c, sourcev1.StorageOperationFailedReason, err.Error()), err - } - - // Update symlink - cUrl, err := r.Storage.Symlink(newArtifact, fmt.Sprintf("%s-latest.tgz", b.Name)) - if err != nil { - err = fmt.Errorf("storage error: %w", err) - return sourcev1.HelmChartNotReady(c, sourcev1.StorageOperationFailedReason, err.Error()), err - } - return sourcev1.HelmChartReady(c, newArtifact, cUrl, sourcev1.ChartPullSucceededReason, b.Summary()), nil + *b = *build + return sreconcile.ResultSuccess, nil } -func (r *HelmChartReconciler) fromTarballArtifact(ctx context.Context, source sourcev1.Artifact, c sourcev1.HelmChart, - workDir string, force bool) (sourcev1.HelmChart, error) { - // Create temporary working directory to untar into - sourceDir := filepath.Join(workDir, "source") +func (r *HelmChartReconciler) reconcileFromTarballArtifact(ctx context.Context, obj *sourcev1.HelmChart, source sourcev1.Artifact, b *chart.Build) (sreconcile.Result, error) { + // Create temporary working directory + tmpDir, err := util.TempDirForObj("", obj) + if err != nil { + e := &serror.Event{ + Err: fmt.Errorf("failed to create temporary working directory: %w", err), + Reason: sourcev1.StorageOperationFailedReason, + } + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.StorageOperationFailedReason, e.Err.Error()) + return sreconcile.ResultEmpty, e + } + defer os.RemoveAll(tmpDir) + + // Create directory to untar source into + sourceDir := filepath.Join(tmpDir, "source") if err := os.Mkdir(sourceDir, 0700); err != nil { - err = fmt.Errorf("failed to create temporary directory to untar source into: %w", err) - return sourcev1.HelmChartNotReady(c, sourcev1.StorageOperationFailedReason, err.Error()), err + e := &serror.Event{ + Err: fmt.Errorf("failed to create directory to untar source into: %w", err), + Reason: sourcev1.StorageOperationFailedReason, + } + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.StorageOperationFailedReason, e.Err.Error()) + return sreconcile.ResultEmpty, e } // Open the tarball artifact file and untar files into working directory f, err := os.Open(r.Storage.LocalPath(source)) if err != nil { - err = fmt.Errorf("artifact open error: %w", err) - return sourcev1.HelmChartNotReady(c, sourcev1.StorageOperationFailedReason, err.Error()), err + e := &serror.Event{ + Err: fmt.Errorf("failed to open source artifact: %w", err), + Reason: sourcev1.StorageOperationFailedReason, + } + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.StorageOperationFailedReason, e.Err.Error()) + return sreconcile.ResultEmpty, e } if _, err = untar.Untar(f, sourceDir); err != nil { _ = f.Close() - err = fmt.Errorf("artifact untar error: %w", err) - return sourcev1.HelmChartNotReady(c, sourcev1.StorageOperationFailedReason, err.Error()), err + return sreconcile.ResultEmpty, &serror.Event{ + Err: fmt.Errorf("artifact untar error: %w", err), + Reason: meta.FailedReason, + } } if err = f.Close(); err != nil { - err = fmt.Errorf("artifact close error: %w", err) - return sourcev1.HelmChartNotReady(c, sourcev1.StorageOperationFailedReason, err.Error()), err + return sreconcile.ResultEmpty, &serror.Event{ + Err: fmt.Errorf("artifact close error: %w", err), + Reason: meta.FailedReason, + } } - chartPath, err := securejoin.SecureJoin(sourceDir, c.Spec.Chart) + // Calculate (secure) absolute chart path + chartPath, err := securejoin.SecureJoin(sourceDir, obj.Spec.Chart) if err != nil { - return sourcev1.HelmChartNotReady(c, sourcev1.StorageOperationFailedReason, err.Error()), err + e := &serror.Stalling{ + Err: fmt.Errorf("Path calculation for chart '%s' failed: %w", obj.Spec.Chart, err), + Reason: "IllegalPath", + } + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, "IllegalPath", e.Err.Error()) + // We are unable to recover from this change without a change in generation + return sreconcile.ResultEmpty, e } // Setup dependency manager - authDir := filepath.Join(workDir, "creds") + authDir := filepath.Join(tmpDir, "creds") if err = os.Mkdir(authDir, 0700); err != nil { - err = fmt.Errorf("failed to create temporaRy directory for dependency credentials: %w", err) - return sourcev1.HelmChartNotReady(c, sourcev1.StorageOperationFailedReason, err.Error()), err + return sreconcile.ResultEmpty, &serror.Event{ + Err: fmt.Errorf("failed to create temporary directory for dependency credentials: %w", err), + Reason: meta.FailedReason, + } } dm := chart.NewDependencyManager( - chart.WithRepositoryCallback(r.namespacedChartRepositoryCallback(ctx, authDir, c.GetNamespace())), + chart.WithRepositoryCallback(r.namespacedChartRepositoryCallback(ctx, authDir, obj.GetNamespace())), ) defer dm.Clear() // Configure builder options, including any previously cached chart opts := chart.BuildOptions{ - ValuesFiles: c.GetValuesFiles(), - Force: force, + ValuesFiles: obj.GetValuesFiles(), + Force: obj.Generation != obj.Status.ObservedGeneration, } - if artifact := c.Status.Artifact; artifact != nil { - opts.CachedChart = artifact.Path + if artifact := obj.Status.Artifact; artifact != nil { + opts.CachedChart = r.Storage.LocalPath(*artifact) } + // Add revision metadata to chart build + if obj.Spec.ReconcileStrategy == sourcev1.ReconcileStrategyRevision { + // Isolate the commit SHA from GitRepository type artifacts by removing the branch/ prefix. + splitRev := strings.Split(source.Revision, "/") + opts.VersionMetadata = splitRev[len(splitRev)-1] + } // Configure revision metadata for chart build if we should react to revision changes - if c.Spec.ReconcileStrategy == sourcev1.ReconcileStrategyRevision { + if obj.Spec.ReconcileStrategy == sourcev1.ReconcileStrategyRevision { rev := source.Revision - if c.Spec.SourceRef.Kind == sourcev1.GitRepositoryKind { + if obj.Spec.SourceRef.Kind == sourcev1.GitRepositoryKind { // Split the reference by the `/` delimiter which may be present, // and take the last entry which contains the SHA. split := strings.Split(source.Revision, "/") rev = split[len(split)-1] } - if kind := c.Spec.SourceRef.Kind; kind == sourcev1.GitRepositoryKind || kind == sourcev1.BucketKind { + if kind := obj.Spec.SourceRef.Kind; kind == sourcev1.GitRepositoryKind || kind == sourcev1.BucketKind { // The SemVer from the metadata is at times used in e.g. the label metadata for a resource // in a chart, which has a limited length of 63 characters. // To not fill most of this space with a full length SHA hex (40 characters for SHA-1, and @@ -471,74 +593,207 @@ func (r *HelmChartReconciler) fromTarballArtifact(ctx context.Context, source so } opts.VersionMetadata = rev } - // Set the VersionMetadata to the object's Generation if ValuesFiles is defined, - // this ensures changes can be noticed by the Artifact consumer - if len(opts.GetValuesFiles()) > 0 { - if opts.VersionMetadata != "" { - opts.VersionMetadata += "." - } - opts.VersionMetadata += strconv.FormatInt(c.Generation, 10) - } // Build chart cb := chart.NewLocalBuilder(dm) - b, err := cb.Build(ctx, chart.LocalReference{WorkDir: sourceDir, Path: chartPath}, filepath.Join(workDir, "chart.tgz"), opts) + build, err := cb.Build(ctx, chart.LocalReference{ + WorkDir: sourceDir, + Path: chartPath, + }, util.TempPathForObj("", ".tgz", obj), opts) + + // Record both success _and_ error observations on the object + processChartBuild(obj, build, err) + + // Handle any build error if err != nil { - return sourcev1.HelmChartNotReady(c, reasonForBuildError(err), err.Error()), err - } - - newArtifact := r.Storage.NewArtifactFor(c.Kind, c.GetObjectMeta(), b.Version, - fmt.Sprintf("%s-%s.tgz", b.Name, b.Version)) - - // If the path of the returned build equals the cache path, - // there are no changes to the chart - if apimeta.IsStatusConditionTrue(c.Status.Conditions, meta.ReadyCondition) && - b.Path == opts.CachedChart { - // Ensure hostname is updated - if c.GetArtifact().URL != newArtifact.URL { - r.Storage.SetArtifactURL(c.GetArtifact()) - c.Status.URL = r.Storage.SetHostname(c.Status.URL) + e := fmt.Errorf("failed to build chart from source artifact: %w", err) + reason := meta.FailedReason + if buildErr := new(chart.BuildError); errors.As(err, &buildErr) { + reason = buildErr.Reason.Reason + if chart.IsPersistentBuildErrorReason(buildErr.Reason) { + return sreconcile.ResultEmpty, &serror.Stalling{ + Err: e, + Reason: reason, + } + } + } + return sreconcile.ResultEmpty, &serror.Event{ + Err: e, + Reason: reason, } - return c, nil } - // Ensure artifact directory exists - err = r.Storage.MkdirAll(newArtifact) - if err != nil { - err = fmt.Errorf("unable to create chart directory: %w", err) - return sourcev1.HelmChartNotReady(c, sourcev1.StorageOperationFailedReason, err.Error()), err + // If we actually build a chart, take a historical note of any dependencies we resolved. + // The reason this is a done conditionally, is because if we have a cached one in storage, + // we can not recover this information (and put it in a condition). Which would result in + // a sudden (partial) disappearance of observed state. + // TODO(hidde): include specific name/version information? + if depNum := build.ResolvedDependencies; depNum > 0 { + r.eventLogf(ctx, obj, corev1.EventTypeNormal, "ResolvedDependencies", "resolved %d chart dependencies", depNum) } - // Acquire a lock for the artifact - unlock, err := r.Storage.Lock(newArtifact) + *b = *build + return sreconcile.ResultSuccess, nil +} + +// reconcileArtifact reconciles the given chart.Build to an v1beta1.Artifact in the Storage, and records it +// on the object. +func (r *HelmChartReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1.HelmChart, b *chart.Build) (sreconcile.Result, error) { + // Without a complete chart build, there is little to reconcile + if !b.Complete() { + return sreconcile.ResultRequeue, nil + } + + // Always restore the conditions in case they got overwritten by transient errors + defer func() { + if obj.Status.ObservedChartName == b.Name && obj.GetArtifact().HasRevision(b.Version) { + conditions.Delete(obj, sourcev1.ArtifactOutdatedCondition) + conditions.MarkTrue(obj, meta.ReadyCondition, reasonForBuild(b), b.Summary()) + } + }() + + // Create artifact from build data + artifact := r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), b.Version, fmt.Sprintf("%s-%s.tgz", b.Name, b.Version)) + + // Return early if the build path equals the current artifact path + if curArtifact := obj.GetArtifact(); curArtifact != nil && r.Storage.LocalPath(*curArtifact) == b.Path { + r.eventLogf(ctx, obj, corev1.EventTypeNormal, meta.SucceededReason, "already up to date, current revision '%s'", curArtifact.Revision) + return sreconcile.ResultSuccess, nil + } + + // Garbage collect chart build once persisted to storage + defer os.Remove(b.Path) + + // Ensure artifact directory exists and acquire lock + if err := r.Storage.MkdirAll(artifact); err != nil { + return sreconcile.ResultEmpty, &serror.Event{ + Err: fmt.Errorf("failed to create artifact directory: %w", err), + Reason: sourcev1.StorageOperationFailedReason, + } + } + unlock, err := r.Storage.Lock(artifact) if err != nil { - err = fmt.Errorf("unable to acquire lock: %w", err) - return sourcev1.HelmChartNotReady(c, sourcev1.StorageOperationFailedReason, err.Error()), err + return sreconcile.ResultEmpty, &serror.Event{ + Err: fmt.Errorf("failed to acquire lock for artifact: %w", err), + Reason: sourcev1.StorageOperationFailedReason, + } } defer unlock() // Copy the packaged chart to the artifact path - if err = r.Storage.CopyFromPath(&newArtifact, b.Path); err != nil { - err = fmt.Errorf("failed to write chart package to storage: %w", err) - return sourcev1.HelmChartNotReady(c, sourcev1.StorageOperationFailedReason, err.Error()), err + if err = r.Storage.CopyFromPath(&artifact, b.Path); err != nil { + return sreconcile.ResultEmpty, &serror.Event{ + Err: fmt.Errorf("unable to copy Helm chart to storage: %w", err), + Reason: sourcev1.StorageOperationFailedReason, + } } - // Update symlink - cUrl, err := r.Storage.Symlink(newArtifact, fmt.Sprintf("%s-latest.tgz", b.Name)) + // Record it on the object + obj.Status.Artifact = artifact.DeepCopy() + obj.Status.ObservedChartName = b.Name + + // Publish an event + r.AnnotatedEventf(obj, map[string]string{ + "revision": artifact.Revision, + "checksum": artifact.Checksum, + }, corev1.EventTypeNormal, reasonForBuild(b), b.Summary()) + + // Update symlink on a "best effort" basis + symURL, err := r.Storage.Symlink(artifact, "latest.tar.gz") if err != nil { - err = fmt.Errorf("storage error: %w", err) - return sourcev1.HelmChartNotReady(c, sourcev1.StorageOperationFailedReason, err.Error()), err + r.eventLogf(ctx, obj, corev1.EventTypeWarning, sourcev1.StorageOperationFailedReason, + "failed to update status URL symlink: %s", err) } - - return sourcev1.HelmChartReady(c, newArtifact, cUrl, reasonForBuildSuccess(b), b.Summary()), nil + if symURL != "" { + obj.Status.URL = symURL + } + return sreconcile.ResultSuccess, nil } -// 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. +// getSource returns the v1beta1.Source for the given object, or an error describing why the source could not be +// returned. +func (r *HelmChartReconciler) getSource(ctx context.Context, obj *sourcev1.HelmChart) (sourcev1.Source, error) { + namespacedName := types.NamespacedName{ + Namespace: obj.GetNamespace(), + Name: obj.Spec.SourceRef.Name, + } + var s sourcev1.Source + switch obj.Spec.SourceRef.Kind { + case sourcev1.HelmRepositoryKind: + var repo sourcev1.HelmRepository + if err := r.Client.Get(ctx, namespacedName, &repo); err != nil { + return nil, err + } + s = &repo + case sourcev1.GitRepositoryKind: + var repo sourcev1.GitRepository + if err := r.Client.Get(ctx, namespacedName, &repo); err != nil { + return nil, err + } + s = &repo + case sourcev1.BucketKind: + var bucket sourcev1.Bucket + if err := r.Client.Get(ctx, namespacedName, &bucket); err != nil { + return nil, err + } + s = &bucket + default: + return nil, fmt.Errorf("unsupported source kind '%s', must be one of: %v", obj.Spec.SourceRef.Kind, []string{ + sourcev1.HelmRepositoryKind, sourcev1.GitRepositoryKind, sourcev1.BucketKind}) + } + return s, nil +} + +// reconcileDelete handles the delete of an object. It first garbage collects all artifacts for the object from the +// artifact storage, if successful, the finalizer is removed from the object. +func (r *HelmChartReconciler) reconcileDelete(ctx context.Context, obj *sourcev1.HelmChart) (sreconcile.Result, error) { + // Garbage collect the resource's artifacts + if err := r.garbageCollect(ctx, obj); err != nil { + // Return the error so we retry the failed garbage collection + return sreconcile.ResultEmpty, err + } + + // Remove our finalizer from the list + controllerutil.RemoveFinalizer(obj, sourcev1.SourceFinalizer) + + // Stop reconciliation as the object is being deleted + return sreconcile.ResultEmpty, nil +} + +// garbageCollect performs a garbage collection for the given v1beta1.HelmChart. It removes all but the current +// artifact except for when the deletion timestamp is set, which will result in the removal of all artifacts for the +// resource. +func (r *HelmChartReconciler) garbageCollect(ctx context.Context, obj *sourcev1.HelmChart) error { + if !obj.DeletionTimestamp.IsZero() { + if err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil { + return &serror.Event{ + Err: fmt.Errorf("garbage collection for deleted resource failed: %w", err), + Reason: "GarbageCollectionFailed", + } + } + obj.Status.Artifact = nil + // TODO(hidde): we should only push this event if we actually garbage collected something + r.eventLogf(ctx, obj, corev1.EventTypeNormal, "GarbageCollectionSucceeded", + "garbage collected artifacts for deleted resource") + return nil + } + if obj.GetArtifact() != nil { + if err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil { + return &serror.Event{ + Err: fmt.Errorf("garbage collection of old artifacts failed: %w", err), + Reason: "GarbageCollectionFailed", + } + } + // TODO(hidde): we should only push this event if we actually garbage collected something + r.eventLogf(ctx, obj, corev1.EventTypeNormal, "GarbageCollectionSucceeded", "garbage collected old artifacts") + } + return nil +} + +// 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) @@ -559,9 +814,10 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont helmgetter.WithTimeout(repo.Spec.Timeout.Duration), helmgetter.WithPassCredentialsAll(repo.Spec.PassCredentials), } - if secret, err := r.getHelmRepositorySecret(ctx, repo); err != nil { - return nil, err - } else if secret != nil { + if secret, err := r.getHelmRepositorySecret(ctx, repo); secret != nil || err != nil { + if err != nil { + return nil, err + } opts, err := getter.ClientOptionsFromSecret(dir, *secret) if err != nil { return nil, err @@ -579,99 +835,36 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont } } -func (r *HelmChartReconciler) reconcileDelete(ctx context.Context, chart sourcev1.HelmChart) (ctrl.Result, error) { - // Our finalizer is still present, so lets handle garbage collection - if err := r.gc(chart); err != nil { - r.event(ctx, chart, events.EventSeverityError, - fmt.Sprintf("garbage collection for deleted resource failed: %s", err.Error())) - // Return the error so we retry the failed garbage collection - return ctrl.Result{}, err +func (r *HelmChartReconciler) resolveDependencyRepository(ctx context.Context, url string, namespace string) (*sourcev1.HelmRepository, error) { + listOpts := []client.ListOption{ + client.InNamespace(namespace), + client.MatchingFields{sourcev1.HelmRepositoryURLIndexKey: url}, } - - // Record deleted status - r.recordReadiness(ctx, chart) - - // Remove our finalizer from the list and update it - controllerutil.RemoveFinalizer(&chart, sourcev1.SourceFinalizer) - if err := r.Update(ctx, &chart); err != nil { - return ctrl.Result{}, err - } - - // Stop reconciliation as the object is being deleted - return ctrl.Result{}, nil -} - -// resetStatus returns a modified v1beta1.HelmChart and a boolean indicating -// if the status field has been reset. -func (r *HelmChartReconciler) resetStatus(chart sourcev1.HelmChart) (sourcev1.HelmChart, bool) { - // We do not have an artifact, or it does no longer exist - if chart.GetArtifact() == nil || !r.Storage.ArtifactExist(*chart.GetArtifact()) { - chart = sourcev1.HelmChartProgressing(chart) - chart.Status.Artifact = nil - return chart, true - } - // The chart specification has changed - if chart.Generation != chart.Status.ObservedGeneration { - return sourcev1.HelmChartProgressing(chart), true - } - return chart, false -} - -// gc performs a garbage collection for the given v1beta1.HelmChart. -// It removes all but the current artifact except for when the -// deletion timestamp is set, which will result in the removal of -// all artifacts for the resource. -func (r *HelmChartReconciler) gc(chart sourcev1.HelmChart) error { - if !chart.DeletionTimestamp.IsZero() { - return r.Storage.RemoveAll(r.Storage.NewArtifactFor(chart.Kind, chart.GetObjectMeta(), "", "*")) - } - if chart.GetArtifact() != nil { - return r.Storage.RemoveAllButCurrent(*chart.GetArtifact()) - } - return nil -} - -// event emits a Kubernetes event and forwards the event to notification -// controller if configured. -func (r *HelmChartReconciler) event(ctx context.Context, chart sourcev1.HelmChart, severity, msg string) { - if r.EventRecorder != nil { - r.EventRecorder.Eventf(&chart, corev1.EventTypeNormal, severity, msg) - } - if r.ExternalEventRecorder != nil { - r.ExternalEventRecorder.Eventf(&chart, corev1.EventTypeNormal, severity, msg) - } -} - -func (r *HelmChartReconciler) recordReadiness(ctx context.Context, chart sourcev1.HelmChart) { - log := ctrl.LoggerFrom(ctx) - if r.MetricsRecorder == nil { - return - } - objRef, err := reference.GetReference(r.Scheme, &chart) + var list sourcev1.HelmRepositoryList + err := r.Client.List(ctx, &list, listOpts...) if err != nil { - log.Error(err, "unable to record readiness metric") - return + return nil, fmt.Errorf("unable to retrieve HelmRepositoryList: %w", err) } - if rc := apimeta.FindStatusCondition(chart.Status.Conditions, meta.ReadyCondition); rc != nil { - r.MetricsRecorder.RecordCondition(*objRef, *rc, !chart.DeletionTimestamp.IsZero()) - } else { - r.MetricsRecorder.RecordCondition(*objRef, metav1.Condition{ - Type: meta.ReadyCondition, - Status: metav1.ConditionUnknown, - }, !chart.DeletionTimestamp.IsZero()) + if len(list.Items) > 0 { + return &list.Items[0], nil } + return nil, fmt.Errorf("no HelmRepository found for '%s' in '%s' namespace", url, namespace) } -func (r *HelmChartReconciler) updateStatus(ctx context.Context, req ctrl.Request, newStatus sourcev1.HelmChartStatus) error { - var chart sourcev1.HelmChart - if err := r.Get(ctx, req.NamespacedName, &chart); err != nil { - return err +func (r *HelmChartReconciler) getHelmRepositorySecret(ctx context.Context, repository *sourcev1.HelmRepository) (*corev1.Secret, error) { + if repository.Spec.SecretRef == nil { + return nil, nil } - - patch := client.MergeFrom(chart.DeepCopy()) - chart.Status = newStatus - - return r.Status().Patch(ctx, &chart, patch) + name := types.NamespacedName{ + Namespace: repository.GetNamespace(), + Name: repository.Spec.SecretRef.Name, + } + var secret corev1.Secret + err := r.Client.Get(ctx, name, &secret) + if err != nil { + return nil, err + } + return &secret, nil } func (r *HelmChartReconciler) indexHelmRepositoryByURL(o client.Object) []string { @@ -694,40 +887,6 @@ func (r *HelmChartReconciler) indexHelmChartBySource(o client.Object) []string { return []string{fmt.Sprintf("%s/%s", hc.Spec.SourceRef.Kind, hc.Spec.SourceRef.Name)} } -func (r *HelmChartReconciler) resolveDependencyRepository(ctx context.Context, url string, namespace string) (*sourcev1.HelmRepository, error) { - listOpts := []client.ListOption{ - client.InNamespace(namespace), - client.MatchingFields{sourcev1.HelmRepositoryURLIndexKey: url}, - } - var list sourcev1.HelmRepositoryList - err := r.Client.List(ctx, &list, listOpts...) - if err != nil { - return nil, fmt.Errorf("unable to retrieve HelmRepositoryList: %w", err) - } - if len(list.Items) > 0 { - return &list.Items[0], nil - } - return nil, fmt.Errorf("no HelmRepository found for '%s' in '%s' namespace", url, namespace) -} - -func (r *HelmChartReconciler) getHelmRepositorySecret(ctx context.Context, repository *sourcev1.HelmRepository) (*corev1.Secret, error) { - if repository.Spec.SecretRef != nil { - name := types.NamespacedName{ - Namespace: repository.GetNamespace(), - Name: repository.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 nil, err - } - return &secret, nil - } - return nil, nil -} - func (r *HelmChartReconciler) requestsForHelmRepositoryChange(o client.Object) []reconcile.Request { repo, ok := o.(*sourcev1.HelmRepository) if !ok { @@ -746,13 +905,11 @@ func (r *HelmChartReconciler) requestsForHelmRepositoryChange(o client.Object) [ return nil } - // TODO(hidde): unlike other places (e.g. the helm-controller), - // we have no reference here to determine if the request is coming - // from the _old_ or _new_ update event, and resources are thus - // enqueued twice. var reqs []reconcile.Request for _, i := range list.Items { - reqs = append(reqs, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&i)}) + if i.Status.ObservedSourceArtifactRevision != repo.GetArtifact().Revision { + reqs = append(reqs, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&i)}) + } } return reqs } @@ -775,13 +932,11 @@ func (r *HelmChartReconciler) requestsForGitRepositoryChange(o client.Object) [] return nil } - // TODO(hidde): unlike other places (e.g. the helm-controller), - // we have no reference here to determine if the request is coming - // from the _old_ or _new_ update event, and resources are thus - // enqueued twice. var reqs []reconcile.Request for _, i := range list.Items { - reqs = append(reqs, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&i)}) + if i.Status.ObservedSourceArtifactRevision != repo.GetArtifact().Revision { + reqs = append(reqs, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&i)}) + } } return reqs } @@ -804,52 +959,63 @@ func (r *HelmChartReconciler) requestsForBucketChange(o client.Object) []reconci return nil } - // TODO(hidde): unlike other places (e.g. the helm-controller), - // we have no reference here to determine if the request is coming - // from the _old_ or _new_ update event, and resources are thus - // enqueued twice. var reqs []reconcile.Request for _, i := range list.Items { - reqs = append(reqs, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&i)}) + if i.Status.ObservedSourceArtifactRevision != bucket.GetArtifact().Revision { + reqs = append(reqs, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&i)}) + } } return reqs } -func (r *HelmChartReconciler) recordSuspension(ctx context.Context, chart sourcev1.HelmChart) { - if r.MetricsRecorder == nil { - return +func processChartBuild(obj *sourcev1.HelmChart, build *chart.Build, err error) { + if build.HasMetadata() { + if build.Name != obj.Status.ObservedChartName || !obj.GetArtifact().HasRevision(build.Version) { + conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewChart", build.Summary()) + } } - log := ctrl.LoggerFrom(ctx) - objRef, err := reference.GetReference(r.Scheme, &chart) - if err != nil { - log.Error(err, "unable to record suspended metric") + if err == nil { + conditions.Delete(obj, sourcev1.FetchFailedCondition) + conditions.Delete(obj, sourcev1.BuildFailedCondition) return } - if !chart.DeletionTimestamp.IsZero() { - r.MetricsRecorder.RecordSuspend(*objRef, false) - } else { - r.MetricsRecorder.RecordSuspend(*objRef, chart.Spec.Suspend) - } -} - -func reasonForBuildError(err error) string { var buildErr *chart.BuildError if ok := errors.As(err, &buildErr); !ok { - return sourcev1.ChartPullFailedReason + buildErr = &chart.BuildError{ + Reason: chart.ErrUnknown, + Err: err, + } } + switch buildErr.Reason { case chart.ErrChartMetadataPatch, chart.ErrValuesFilesMerge, chart.ErrDependencyBuild, chart.ErrChartPackage: - return sourcev1.ChartPackageFailedReason + conditions.Delete(obj, sourcev1.FetchFailedCondition) + conditions.MarkTrue(obj, sourcev1.BuildFailedCondition, buildErr.Reason.Reason, buildErr.Error()) default: - return sourcev1.ChartPullFailedReason + conditions.Delete(obj, sourcev1.BuildFailedCondition) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, buildErr.Reason.Reason, buildErr.Error()) } } -func reasonForBuildSuccess(result *chart.Build) string { - if result.Packaged { +func reasonForBuild(build *chart.Build) string { + if build.Packaged { return sourcev1.ChartPackageSucceededReason } return sourcev1.ChartPullSucceededReason } + +// eventLog records event and logs at the same time. This log is different from +// the debug log in the event recorder in the sense that this is a simple log, +// the event recorder debug log contains complete details about the event. +func (r *HelmChartReconciler) eventLogf(ctx context.Context, obj runtime.Object, eventType string, reason string, messageFmt string, args ...interface{}) { + msg := fmt.Sprintf(messageFmt, args...) + // Log and emit event. + if eventType == corev1.EventTypeWarning { + ctrl.LoggerFrom(ctx).Error(errors.New(reason), msg) + } else { + ctrl.LoggerFrom(ctx).Info(msg) + } + r.Eventf(obj, eventType, reason, msg) +} diff --git a/controllers/helmchart_controller_test.go b/controllers/helmchart_controller_test.go index d53afff0..c333478d 100644 --- a/controllers/helmchart_controller_test.go +++ b/controllers/helmchart_controller_test.go @@ -30,6 +30,7 @@ import ( "github.com/fluxcd/pkg/apis/meta" "github.com/fluxcd/pkg/gittestserver" "github.com/fluxcd/pkg/helmtestserver" + "github.com/fluxcd/pkg/runtime/conditions" "github.com/go-git/go-billy/v5/memfs" "github.com/go-git/go-git/v5" "github.com/go-git/go-git/v5/config" @@ -49,7 +50,7 @@ import ( sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" ) -var _ = Describe("HelmChartReconciler", func() { +var _ = FDescribe("HelmChartReconciler", func() { const ( timeout = time.Second * 30 @@ -270,7 +271,7 @@ var _ = Describe("HelmChartReconciler", func() { got := &sourcev1.HelmChart{} Eventually(func() bool { _ = k8sClient.Get(context.Background(), key, got) - return got.Status.ObservedGeneration > updated.Status.ObservedGeneration && + return got.Status.ObservedGeneration > updated.Status.ObservedGeneration && got.GetArtifact() != nil && ginkgoTestStorage.ArtifactExist(*got.Status.Artifact) }, timeout, interval).Should(BeTrue()) f, err := os.Stat(ginkgoTestStorage.LocalPath(*got.Status.Artifact)) @@ -292,6 +293,9 @@ var _ = Describe("HelmChartReconciler", func() { Eventually(func() bool { _ = k8sClient.Get(context.Background(), key, updated) for _, c := range updated.Status.Conditions { + fmt.Fprintf(GinkgoWriter, "condition type: %s\n", c.Type) + fmt.Fprintf(GinkgoWriter, "condition reason: %s\n", c.Reason) + fmt.Fprintf(GinkgoWriter, "condition message: %s\n", c.Message) if c.Reason == sourcev1.ChartPullFailedReason && strings.Contains(c.Message, "failed to retrieve source") { return true @@ -394,12 +398,8 @@ var _ = Describe("HelmChartReconciler", func() { Expect(k8sClient.Update(context.Background(), chart)).Should(Succeed()) Eventually(func() bool { _ = k8sClient.Get(context.Background(), key, chart) - for _, c := range chart.Status.Conditions { - if c.Reason == sourcev1.ChartPullFailedReason { - return true - } - } - return false + return conditions.GetReason(chart, sourcev1.FetchFailedCondition) == "InvalidChartReference" && + conditions.IsStalled(chart) }, timeout, interval).Should(BeTrue()) Expect(chart.GetArtifact()).NotTo(BeNil()) Expect(chart.Status.Artifact.Revision).Should(Equal("0.1.1")) @@ -495,13 +495,7 @@ var _ = Describe("HelmChartReconciler", func() { got := &sourcev1.HelmChart{} Eventually(func() bool { _ = k8sClient.Get(context.Background(), key, got) - for _, c := range got.Status.Conditions { - if c.Reason == sourcev1.AuthenticationFailedReason && - strings.Contains(c.Message, "auth secret error") { - return true - } - } - return false + return conditions.GetReason(got, sourcev1.FetchFailedCondition) == sourcev1.AuthenticationFailedReason }, timeout, interval).Should(BeTrue()) By("Applying secret with missing keys") @@ -515,7 +509,7 @@ var _ = Describe("HelmChartReconciler", func() { got := &sourcev1.HelmChart{} _ = k8sClient.Get(context.Background(), key, got) for _, c := range got.Status.Conditions { - if c.Reason == sourcev1.ChartPullFailedReason && + if c.Reason == "ChartPullError" && strings.Contains(c.Message, "401 Unauthorized") { return true } @@ -833,7 +827,7 @@ var _ = Describe("HelmChartReconciler", func() { // 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]") && + if strings.Contains(condn.Message, "merged values files [./testdata/charts/helmchart/override.yaml]") && ginkgoTestStorage.ArtifactExist(*got.Status.Artifact) { return true } diff --git a/controllers/legacy_suite_test.go b/controllers/legacy_suite_test.go index 9edfdd79..46237896 100644 --- a/controllers/legacy_suite_test.go +++ b/controllers/legacy_suite_test.go @@ -137,9 +137,9 @@ var _ = BeforeSuite(func() { Expect(err).ToNot(HaveOccurred(), "failed to setup HelmRepositoryReconciler") err = (&HelmChartReconciler{ - Client: k8sManager.GetClient(), - Scheme: scheme.Scheme, - Storage: ginkgoTestStorage, + Client: k8sManager.GetClient(), + Storage: ginkgoTestStorage, + EventRecorder: record.NewFakeRecorder(32), Getters: getter.Providers{getter.Provider{ Schemes: []string{"http", "https"}, New: getter.NewHTTPGetter, diff --git a/internal/helm/chart/builder.go b/internal/helm/chart/builder.go index 9aa2a17e..c44720c1 100644 --- a/internal/helm/chart/builder.go +++ b/internal/helm/chart/builder.go @@ -115,15 +115,16 @@ func (o BuildOptions) GetValuesFiles() []string { return o.ValuesFiles } -// Build contains the Builder.Build result, including specific +// Build contains the (partial) Builder.Build result, including specific // information about the built chart like ResolvedDependencies. type Build struct { - // Path is the absolute path to the packaged chart. - Path string - // Name of the packaged chart. + // Name of the chart. Name string - // Version of the packaged chart. + // Version of the chart. Version string + // Path is the absolute path to the packaged chart. + // Can be empty, in which case a failure should be assumed. + Path string // ValuesFiles is the list of files used to compose the chart's // default "values.yaml". ValuesFiles []string @@ -138,30 +139,45 @@ type Build struct { // Summary returns a human-readable summary of the Build. func (b *Build) Summary() string { - if b == nil || b.Name == "" || b.Version == "" { - return "No chart build." + if !b.HasMetadata() { + return "No chart build" } var s strings.Builder - var action = "Pulled" - if b.Packaged { - action = "Packaged" + var action = "New" + if b.Path != "" { + action = "Pulled" + if b.Packaged { + action = "Packaged" + } } s.WriteString(fmt.Sprintf("%s '%s' chart with version '%s'", action, b.Name, b.Version)) - if b.Packaged && len(b.ValuesFiles) > 0 { - s.WriteString(fmt.Sprintf(", with merged values files %v", b.ValuesFiles)) + if len(b.ValuesFiles) > 0 { + s.WriteString(fmt.Sprintf(" and merged values files %v", b.ValuesFiles)) } - if b.Packaged && b.ResolvedDependencies > 0 { - s.WriteString(fmt.Sprintf(", resolving %d dependencies before packaging", b.ResolvedDependencies)) - } - - s.WriteString(".") return s.String() } +// HasMetadata returns if the Build contains chart metadata. +// +// NOTE: This may return True while the build did not Complete successfully. +// Which means it was able to successfully collect the metadata from the chart, +// but failed further into the process. +func (b *Build) HasMetadata() bool { + if b == nil { + return false + } + return b.Name != "" && b.Version != "" +} + +// Complete returns if the Build completed successfully. +func (b *Build) Complete() bool { + return b.HasMetadata() && b.Path != "" +} + // String returns the Path of the Build. func (b *Build) String() string { if b == nil { diff --git a/internal/helm/chart/builder_local.go b/internal/helm/chart/builder_local.go index 721238fe..923008dc 100644 --- a/internal/helm/chart/builder_local.go +++ b/internal/helm/chart/builder_local.go @@ -101,6 +101,9 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string, result.Version = ver.String() } + isChartDir := pathIsDir(localRef.Path) + requiresPackaging := isChartDir || opts.VersionMetadata != "" || len(opts.GetValuesFiles()) != 0 + // If all the following is true, we do not need to package the chart: // - Chart name from cached chart matches resolved name // - Chart version from cached chart matches calculated version @@ -112,7 +115,9 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string, if err = curMeta.Validate(); err == nil { if result.Name == curMeta.Name && result.Version == curMeta.Version { result.Path = opts.CachedChart - result.ValuesFiles = opts.ValuesFiles + result.ValuesFiles = opts.GetValuesFiles() + result.Packaged = requiresPackaging + return result, nil } } @@ -121,10 +126,9 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string, // If the chart at the path is already packaged and no custom values files // options are set, we can copy the chart without making modifications - isChartDir := pathIsDir(localRef.Path) - if !isChartDir && len(opts.GetValuesFiles()) == 0 { + if !requiresPackaging { if err = copyFileToPath(localRef.Path, p); err != nil { - return nil, &BuildError{Reason: ErrChartPull, Err: err} + return result, &BuildError{Reason: ErrChartPull, Err: err} } result.Path = p return result, nil @@ -134,7 +138,7 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string, var mergedValues map[string]interface{} if len(opts.GetValuesFiles()) > 0 { if mergedValues, err = mergeFileValues(localRef.WorkDir, opts.ValuesFiles); err != nil { - return nil, &BuildError{Reason: ErrValuesFilesMerge, Err: err} + return result, &BuildError{Reason: ErrValuesFilesMerge, Err: err} } } @@ -143,7 +147,7 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string, // or because we have merged values and need to repackage chart, err := loader.Load(localRef.Path) if err != nil { - return nil, &BuildError{Reason: ErrChartPackage, Err: err} + return result, &BuildError{Reason: ErrChartPackage, Err: err} } // Set earlier resolved version (with metadata) chart.Metadata.Version = result.Version @@ -151,7 +155,7 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string, // Overwrite default values with merged values, if any if ok, err = OverwriteChartDefaultValues(chart, mergedValues); ok || err != nil { if err != nil { - return nil, &BuildError{Reason: ErrValuesFilesMerge, Err: err} + return result, &BuildError{Reason: ErrValuesFilesMerge, Err: err} } result.ValuesFiles = opts.GetValuesFiles() } @@ -160,19 +164,19 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string, if isChartDir { if b.dm == nil { err = fmt.Errorf("local chart builder requires dependency manager for unpackaged charts") - return nil, &BuildError{Reason: ErrDependencyBuild, Err: err} + return result, &BuildError{Reason: ErrDependencyBuild, Err: err} } if result.ResolvedDependencies, err = b.dm.Build(ctx, ref, chart); err != nil { - return nil, &BuildError{Reason: ErrDependencyBuild, Err: err} + return result, &BuildError{Reason: ErrDependencyBuild, Err: err} } } // Package the chart if err = packageToPath(chart, p); err != nil { - return nil, &BuildError{Reason: ErrChartPackage, Err: err} + return result, &BuildError{Reason: ErrChartPackage, Err: err} } result.Path = p - result.Packaged = true + result.Packaged = requiresPackaging return result, nil } diff --git a/internal/helm/chart/builder_remote.go b/internal/helm/chart/builder_remote.go index 3252ff22..778efd25 100644 --- a/internal/helm/chart/builder_remote.go +++ b/internal/helm/chart/builder_remote.go @@ -82,12 +82,13 @@ func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, o cv, err := b.remote.Get(remoteRef.Name, remoteRef.Version) if err != nil { err = fmt.Errorf("failed to get chart version for remote reference: %w", err) - return nil, &BuildError{Reason: ErrChartPull, Err: err} + return nil, &BuildError{Reason: ErrChartReference, Err: err} } result := &Build{} result.Name = cv.Name result.Version = cv.Version + // Set build specific metadata if instructed if opts.VersionMetadata != "" { ver, err := semver.NewVersion(result.Version) @@ -102,6 +103,8 @@ func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, o result.Version = ver.String() } + requiresPackaging := len(opts.GetValuesFiles()) != 0 || opts.VersionMetadata != "" + // If all the following is true, we do not need to download and/or build the chart: // - Chart name from cached chart matches resolved name // - Chart version from cached chart matches calculated version @@ -114,6 +117,7 @@ func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, o if result.Name == curMeta.Name && result.Version == curMeta.Version { result.Path = opts.CachedChart result.ValuesFiles = opts.GetValuesFiles() + result.Packaged = requiresPackaging return result, nil } } @@ -124,12 +128,12 @@ func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, o res, err := b.remote.DownloadChart(cv) if err != nil { err = fmt.Errorf("failed to download chart for remote reference: %w", err) - return nil, &BuildError{Reason: ErrChartPull, Err: err} + return result, &BuildError{Reason: ErrChartPull, Err: err} } // Use literal chart copy from remote if no custom values files options are - // set or build option version metadata isn't set. - if len(opts.GetValuesFiles()) == 0 && opts.VersionMetadata == "" { + // set or version metadata isn't set. + if !requiresPackaging { if err = validatePackageAndWriteToPath(res, p); err != nil { return nil, &BuildError{Reason: ErrChartPull, Err: err} } @@ -141,14 +145,14 @@ func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, o var chart *helmchart.Chart if chart, err = loader.LoadArchive(res); err != nil { err = fmt.Errorf("failed to load downloaded chart: %w", err) - return nil, &BuildError{Reason: ErrChartPackage, Err: err} + return result, &BuildError{Reason: ErrChartPackage, Err: err} } chart.Metadata.Version = result.Version mergedValues, err := mergeChartValues(chart, opts.ValuesFiles) if err != nil { err = fmt.Errorf("failed to merge chart values: %w", err) - return nil, &BuildError{Reason: ErrValuesFilesMerge, Err: err} + return result, &BuildError{Reason: ErrValuesFilesMerge, Err: err} } // Overwrite default values with merged values, if any if ok, err = OverwriteChartDefaultValues(chart, mergedValues); ok || err != nil { diff --git a/internal/helm/chart/builder_test.go b/internal/helm/chart/builder_test.go index d797a209..23c3952b 100644 --- a/internal/helm/chart/builder_test.go +++ b/internal/helm/chart/builder_test.go @@ -138,12 +138,32 @@ func TestChartBuildResult_Summary(t *testing.T) { want string }{ { - name: "Simple", + name: "Build with metadata", build: &Build{ Name: "chart", Version: "1.2.3-rc.1+bd6bf40", }, - want: "Pulled 'chart' chart with version '1.2.3-rc.1+bd6bf40'.", + want: "New 'chart' chart with version '1.2.3-rc.1+bd6bf40'", + }, + { + name: "Pulled chart", + build: &Build{ + Name: "chart", + Version: "1.2.3-rc.1+bd6bf40", + Path: "chart.tgz", + }, + want: "Pulled 'chart' chart with version '1.2.3-rc.1+bd6bf40'", + }, + { + name: "Packaged chart", + build: &Build{ + Name: "chart", + Version: "arbitrary-version", + Packaged: true, + ValuesFiles: []string{"a.yaml", "b.yaml"}, + Path: "chart.tgz", + }, + want: "Packaged 'chart' chart with version 'arbitrary-version' and merged values files [a.yaml b.yaml]", }, { name: "With values files", @@ -152,28 +172,19 @@ func TestChartBuildResult_Summary(t *testing.T) { Version: "arbitrary-version", Packaged: true, ValuesFiles: []string{"a.yaml", "b.yaml"}, + Path: "chart.tgz", }, - want: "Packaged 'chart' chart with version 'arbitrary-version', with merged values files [a.yaml b.yaml].", - }, - { - name: "With dependencies", - build: &Build{ - Name: "chart", - Version: "arbitrary-version", - Packaged: true, - ResolvedDependencies: 5, - }, - want: "Packaged 'chart' chart with version 'arbitrary-version', resolving 5 dependencies before packaging.", + want: "Packaged 'chart' chart with version 'arbitrary-version' and merged values files [a.yaml b.yaml]", }, { name: "Empty build", build: &Build{}, - want: "No chart build.", + want: "No chart build", }, { name: "Nil build", build: nil, - want: "No chart build.", + want: "No chart build", }, } for _, tt := range tests { diff --git a/internal/helm/chart/errors.go b/internal/helm/chart/errors.go index dddd2e29..5b3a5bec 100644 --- a/internal/helm/chart/errors.go +++ b/internal/helm/chart/errors.go @@ -22,22 +22,29 @@ import ( ) // BuildErrorReason is the descriptive reason for a BuildError. -type BuildErrorReason string +type BuildErrorReason struct { + // Reason is the programmatic build error reason in CamelCase. + Reason string + + // Summary is the human build error reason, used to provide + // the Error string, and further context to the BuildError. + Summary string +} // Error returns the string representation of BuildErrorReason. func (e BuildErrorReason) Error() string { - return string(e) + return e.Summary } // BuildError contains a wrapped Err and a Reason indicating why it occurred. type BuildError struct { - Reason error + Reason BuildErrorReason Err error } // Error returns Err as a string, prefixed with the Reason to provide context. func (e *BuildError) Error() string { - if e.Reason == nil { + if e.Reason.Error() == "" { return e.Err.Error() } return fmt.Sprintf("%s: %s", e.Reason.Error(), e.Err.Error()) @@ -49,7 +56,7 @@ func (e *BuildError) Error() string { // err := &BuildError{Reason: ErrChartPull, Err: errors.New("arbitrary transport error")} // errors.Is(err, ErrChartPull) func (e *BuildError) Is(target error) bool { - if e.Reason != nil && e.Reason == target { + if e.Reason == target { return true } return errors.Is(e.Err, target) @@ -60,11 +67,21 @@ func (e *BuildError) Unwrap() error { return e.Err } +func IsPersistentBuildErrorReason(err error) bool { + switch err { + case ErrChartReference, ErrChartMetadataPatch, ErrValuesFilesMerge: + return true + default: + return false + } +} + var ( - ErrChartReference = BuildErrorReason("chart reference error") - ErrChartPull = BuildErrorReason("chart pull error") - ErrChartMetadataPatch = BuildErrorReason("chart metadata patch error") - ErrValuesFilesMerge = BuildErrorReason("values files merge error") - ErrDependencyBuild = BuildErrorReason("dependency build error") - ErrChartPackage = BuildErrorReason("chart package error") + ErrChartReference = BuildErrorReason{Reason: "InvalidChartReference", Summary: "invalid chart reference"} + ErrChartPull = BuildErrorReason{Reason: "ChartPullError", Summary: "chart pull error"} + ErrChartMetadataPatch = BuildErrorReason{Reason: "MetadataPatchError", Summary: "chart metadata patch error"} + ErrValuesFilesMerge = BuildErrorReason{Reason: "ValuesFilesError", Summary: "values files merge error"} + ErrDependencyBuild = BuildErrorReason{Reason: "DependencyBuildError", Summary: "dependency build error"} + ErrChartPackage = BuildErrorReason{Reason: "ChartPackageError", Summary: "chart package error"} + ErrUnknown = BuildErrorReason{Reason: "Unknown", Summary: "unknown build error"} ) diff --git a/internal/helm/chart/errors_test.go b/internal/helm/chart/errors_test.go index f006f336..13428e6c 100644 --- a/internal/helm/chart/errors_test.go +++ b/internal/helm/chart/errors_test.go @@ -26,7 +26,7 @@ import ( func TestBuildErrorReason_Error(t *testing.T) { g := NewWithT(t) - err := BuildErrorReason("reason") + err := BuildErrorReason{"Reason", "reason"} g.Expect(err.Error()).To(Equal("reason")) } @@ -39,7 +39,7 @@ func TestBuildError_Error(t *testing.T) { { name: "with reason", err: &BuildError{ - Reason: BuildErrorReason("reason"), + Reason: BuildErrorReason{"Reason", "reason"}, Err: errors.New("error"), }, want: "reason: error", diff --git a/main.go b/main.go index 31bd1cb9..27054803 100644 --- a/main.go +++ b/main.go @@ -189,12 +189,11 @@ func main() { os.Exit(1) } if err = (&controllers.HelmChartReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - Storage: storage, - Getters: getters, - EventRecorder: eventRecorder, - MetricsRecorder: metricsH.MetricsRecorder, + Client: mgr.GetClient(), + Storage: storage, + Getters: getters, + EventRecorder: eventRecorder, + Metrics: metricsH, }).SetupWithManagerAndOptions(mgr, controllers.HelmChartReconcilerOptions{ MaxConcurrentReconciles: concurrent, }); err != nil {