internal/helm: introduce typed BuildError

This commit introduces a typed `BuildError` to be returned by
`Builder.Build` in case of a failure.

The `Reason` field in combination with `BuildErrorReason` can be used
to signal (or determine) the reason of a returned error within the
context of the build process.

At present this is used to determine the correct Condition Reason, but
in a future iteration this can be used to determine the negative
polarity condition that should be set to indicate a precise failure to
the user.

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

Author: hiddeco

controllers/helmchart_controller.go

@@ -18,6 +18,7 @@ package controllers
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/url"
 	"os"
@@ -29,7 +30,7 @@ import (
 	"github.com/go-logr/logr"
 	helmgetter "helm.sh/helm/v3/pkg/getter"
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/errors"
+	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"
@@ -445,19 +446,19 @@ func (r *HelmChartReconciler) fromTarballArtifact(ctx context.Context, source so
 	}
 
 	// Build chart
-	chartB := chart.NewLocalBuilder(dm)
-	build, err := chartB.Build(ctx, chart.LocalReference{WorkDir: sourceDir, Path: chartPath}, filepath.Join(workDir, "chart.tgz"), buildsOpts)
+	chartBuilder := chart.NewLocalBuilder(dm)
+	result, err := chartBuilder.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
+		return sourcev1.HelmChartNotReady(c, reasonForBuildError(err), err.Error()), err
 	}
 
-	newArtifact := r.Storage.NewArtifactFor(c.Kind, c.GetObjectMeta(), build.Version,
-		fmt.Sprintf("%s-%s.tgz", build.Name, build.Version))
+	newArtifact := r.Storage.NewArtifactFor(c.Kind, c.GetObjectMeta(), result.Version,
+		fmt.Sprintf("%s-%s.tgz", result.Name, result.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) &&
-		build.Path == buildsOpts.CachedChart {
+		result.Path == buildsOpts.CachedChart {
 		// Ensure hostname is updated
 		if c.GetArtifact().URL != newArtifact.URL {
 			r.Storage.SetArtifactURL(c.GetArtifact())
@@ -482,19 +483,19 @@ func (r *HelmChartReconciler) fromTarballArtifact(ctx context.Context, source so
 	defer unlock()
 
 	// Copy the packaged chart to the artifact path
-	if err = r.Storage.CopyFromPath(&newArtifact, build.Path); err != nil {
+	if err = r.Storage.CopyFromPath(&newArtifact, result.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", build.Name))
+	cUrl, err := r.Storage.Symlink(newArtifact, fmt.Sprintf("%s-latest.tgz", result.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.ChartPackageSucceededReason, build.Summary()), nil
+	return sourcev1.HelmChartReady(c, newArtifact, cUrl, reasonForBuildSuccess(result), result.Summary()), nil
 }
 
 // namespacedChartRepositoryCallback returns a chart.GetChartRepositoryCallback
@@ -508,7 +509,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
 		repo, err := r.resolveDependencyRepository(ctx, url, namespace)
 		if err != nil {
 			// Return Kubernetes client errors, but ignore others
-			if errors.ReasonForError(err) != metav1.StatusReasonUnknown {
+			if apierrs.ReasonForError(err) != metav1.StatusReasonUnknown {
 				return nil, err
 			}
 			repo = &sourcev1.HelmRepository{
@@ -807,3 +808,23 @@ func (r *HelmChartReconciler) recordSuspension(ctx context.Context, chart source
 		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
+	}
+	switch buildErr.Reason {
+	case chart.ErrChartMetadataPatch, chart.ErrValueFilesMerge, chart.ErrDependencyBuild, chart.ErrChartPackage:
+		return sourcev1.ChartPackageFailedReason
+	default:
+		return sourcev1.ChartPullFailedReason
+	}
+}
+
+func reasonForBuildSuccess(result *chart.Build) string {
+	if result.Packaged {
+		return sourcev1.ChartPackageSucceededReason
+	}
+	return sourcev1.ChartPullSucceededReason
+}

internal/helm/chart/builder.go

@@ -144,7 +144,7 @@ func (b *Build) Summary() string {
 
 	var s strings.Builder
 
-	action := "Fetched"
+	var action = "Pulled"
 	if b.Packaged {
 		action = "Packaged"
 	}

internal/helm/chart/builder_local.go

@@ -51,14 +51,14 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string,
 	}
 
 	if err := ref.Validate(); err != nil {
-		return nil, err
+		return nil, &BuildError{Reason: ErrChartPull, Err: err}
 	}
 
 	// Load the chart metadata from the LocalReference to ensure it points
 	// to a chart
 	curMeta, err := LoadChartMetadata(localRef.Path)
 	if err != nil {
-		return nil, err
+		return nil, &BuildError{Reason: ErrChartPull, Err: err}
 	}
 
 	result := &Build{}
@@ -69,10 +69,12 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string,
 	if opts.VersionMetadata != "" {
 		ver, err := semver.NewVersion(curMeta.Version)
 		if err != nil {
-			return nil, fmt.Errorf("failed to parse chart version from metadata as SemVer: %w", err)
+			err = fmt.Errorf("failed to parse version from chart metadata as SemVer: %w", err)
+			return nil, &BuildError{Reason: ErrChartMetadataPatch, Err: err}
 		}
 		if *ver, err = ver.SetMetadata(opts.VersionMetadata); err != nil {
-			return nil, fmt.Errorf("failed to set metadata on chart version: %w", err)
+			err = fmt.Errorf("failed to set SemVer metadata on chart version: %w", err)
+			return nil, &BuildError{Reason: ErrChartMetadataPatch, Err: err}
 		}
 		result.Version = ver.String()
 	}
@@ -92,8 +94,8 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string,
 	// options are set, we can copy the chart without making modifications
 	isChartDir := pathIsDir(localRef.Path)
 	if !isChartDir && len(opts.GetValueFiles()) == 0 {
-		if err := copyFileToPath(localRef.Path, p); err != nil {
-			return nil, err
+		if err = copyFileToPath(localRef.Path, p); err != nil {
+			return nil, &BuildError{Reason: ErrChartPull, Err: err}
 		}
 		result.Path = p
 		return result, nil
@@ -103,7 +105,7 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string,
 	var mergedValues map[string]interface{}
 	if len(opts.GetValueFiles()) > 0 {
 		if mergedValues, err = mergeFileValues(localRef.WorkDir, opts.ValueFiles); err != nil {
-			return nil, fmt.Errorf("failed to merge value files: %w", err)
+			return nil, &BuildError{Reason: ErrValueFilesMerge, Err: err}
 		}
 	}
 
@@ -112,32 +114,33 @@ 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, err
+		return nil, &BuildError{Reason: ErrChartPackage, Err: err}
 	}
 	// Set earlier resolved version (with metadata)
 	chart.Metadata.Version = result.Version
 
 	// Overwrite default values with merged values, if any
 	if ok, err = OverwriteChartDefaultValues(chart, mergedValues); ok || err != nil {
 		if err != nil {
-			return nil, err
+			return nil, &BuildError{Reason: ErrValueFilesMerge, Err: err}
 		}
 		result.ValueFiles = opts.GetValueFiles()
 	}
 
 	// Ensure dependencies are fetched if building from a directory
 	if isChartDir {
 		if b.dm == nil {
-			return nil, fmt.Errorf("local chart builder requires dependency manager for unpackaged charts")
+			err = fmt.Errorf("local chart builder requires dependency manager for unpackaged charts")
+			return nil, &BuildError{Reason: ErrDependencyBuild, Err: err}
 		}
 		if result.ResolvedDependencies, err = b.dm.Build(ctx, ref, chart); err != nil {
-			return nil, err
+			return nil, &BuildError{Reason: ErrDependencyBuild, Err: err}
 		}
 	}
 
 	// Package the chart
 	if err = packageToPath(chart, p); err != nil {
-		return nil, err
+		return nil, &BuildError{Reason: ErrChartPackage, Err: err}
 	}
 	result.Path = p
 	result.Packaged = true

internal/helm/chart/builder_remote.go

@@ -54,18 +54,20 @@ func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, o
 	}
 
 	if err := ref.Validate(); err != nil {
-		return nil, err
+		return nil, &BuildError{Reason: ErrChartPull, Err: err}
 	}
 
 	if err := b.remote.LoadFromCache(); err != nil {
-		return nil, fmt.Errorf("could not load repository index for remote chart reference: %w", err)
+		err = fmt.Errorf("could not load repository index for remote chart reference: %w", err)
+		return nil, &BuildError{Reason: ErrChartPull, Err: err}
 	}
 	defer b.remote.Unload()
 
 	// Get the current version for the RemoteReference
 	cv, err := b.remote.Get(remoteRef.Name, remoteRef.Version)
 	if err != nil {
-		return nil, fmt.Errorf("failed to get chart version for remote reference: %w", err)
+		err = fmt.Errorf("failed to get chart version for remote reference: %w", err)
+		return nil, &BuildError{Reason: ErrChartPull, Err: err}
 	}
 
 	result := &Build{}
@@ -75,10 +77,12 @@ func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, o
 	if opts.VersionMetadata != "" {
 		ver, err := semver.NewVersion(result.Version)
 		if err != nil {
-			return nil, err
+			err = fmt.Errorf("failed to parse version from chart metadata as SemVer: %w", err)
+			return nil, &BuildError{Reason: ErrChartMetadataPatch, Err: err}
 		}
 		if *ver, err = ver.SetMetadata(opts.VersionMetadata); err != nil {
-			return nil, err
+			err = fmt.Errorf("failed to set SemVer metadata on chart version: %w", err)
+			return nil, &BuildError{Reason: ErrChartMetadataPatch, Err: err}
 		}
 		result.Version = ver.String()
 	}
@@ -97,14 +101,15 @@ func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, o
 	// Download the package for the resolved version
 	res, err := b.remote.DownloadChart(cv)
 	if err != nil {
-		return nil, fmt.Errorf("failed to download chart for remote reference: %w", err)
+		err = fmt.Errorf("failed to download chart for remote reference: %w", err)
+		return nil, &BuildError{Reason: ErrChartPull, Err: err}
 	}
 
 	// Use literal chart copy from remote if no custom value files options are
 	// set or build option version metadata isn't set.
 	if len(opts.GetValueFiles()) == 0 && opts.VersionMetadata == "" {
 		if err = validatePackageAndWriteToPath(res, p); err != nil {
-			return nil, err
+			return nil, &BuildError{Reason: ErrChartPull, Err: err}
 		}
 		result.Path = p
 		return result, nil
@@ -113,26 +118,27 @@ func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, o
 	// Load the chart and merge chart values
 	var chart *helmchart.Chart
 	if chart, err = loader.LoadArchive(res); err != nil {
-		return nil, fmt.Errorf("failed to load downloaded chart: %w", err)
+		err = fmt.Errorf("failed to load downloaded chart: %w", err)
+		return nil, &BuildError{Reason: ErrChartPackage, Err: err}
 	}
+	chart.Metadata.Version = result.Version
 
 	mergedValues, err := mergeChartValues(chart, opts.ValueFiles)
 	if err != nil {
-		return nil, fmt.Errorf("failed to merge chart values: %w", err)
+		err = fmt.Errorf("failed to merge chart values: %w", err)
+		return nil, &BuildError{Reason: ErrValueFilesMerge, Err: err}
 	}
 	// Overwrite default values with merged values, if any
 	if ok, err = OverwriteChartDefaultValues(chart, mergedValues); ok || err != nil {
 		if err != nil {
-			return nil, err
+			return nil, &BuildError{Reason: ErrValueFilesMerge, Err: err}
 		}
 		result.ValueFiles = opts.GetValueFiles()
 	}
 
-	chart.Metadata.Version = result.Version
-
 	// Package the chart with the custom values
 	if err = packageToPath(chart, p); err != nil {
-		return nil, err
+		return nil, &BuildError{Reason: ErrChartPackage, Err: err}
 	}
 	result.Path = p
 	result.Packaged = true

internal/helm/chart/builder_test.go

@@ -143,7 +143,7 @@ func TestChartBuildResult_Summary(t *testing.T) {
 				Name:    "chart",
 				Version: "1.2.3-rc.1+bd6bf40",
 			},
-			want: "Fetched 'chart' chart with version '1.2.3-rc.1+bd6bf40'.",
+			want: "Pulled 'chart' chart with version '1.2.3-rc.1+bd6bf40'.",
 		},
 		{
 			name: "With value files",

internal/helm/chart/errors.go

@@ -0,0 +1,65 @@
+/*
+Copyright 2021 The Flux authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package chart
+
+import (
+	"errors"
+	"fmt"
+)
+
+// BuildErrorReason is the descriptive reason for a BuildError.
+type BuildErrorReason string
+
+// Error returns the string representation of BuildErrorReason.
+func (e BuildErrorReason) Error() string {
+	return string(e)
+}
+
+// BuildError contains a wrapped Err and a Reason indicating why it occurred.
+type BuildError struct {
+	Reason error
+	Err    error
+}
+
+// Error returns Err as a string, prefixed with the Reason, if any.
+func (e *BuildError) Error() string {
+	if e.Reason == nil {
+		return e.Err.Error()
+	}
+	return fmt.Sprintf("%s: %s", e.Reason.Error(), e.Err.Error())
+}
+
+// Is returns true of the Reason or Err equals target.
+func (e *BuildError) Is(target error) bool {
+	if e.Reason != nil && e.Reason == target {
+		return true
+	}
+	return errors.Is(e.Err, target)
+}
+
+// Unwrap returns the underlying Err.
+func (e *BuildError) Unwrap() error {
+	return e.Err
+}
+
+var (
+	ErrChartPull          = BuildErrorReason("chart pull error")
+	ErrChartMetadataPatch = BuildErrorReason("chart metadata patch error")
+	ErrValueFilesMerge    = BuildErrorReason("value files merge error")
+	ErrDependencyBuild    = BuildErrorReason("dependency build error")
+	ErrChartPackage       = BuildErrorReason("chart package error")
+)