Refactor and document DependencyManager

hiddeco · hiddeco · commit 29a051c5f426 · 2020-12-15T15:37:42.000+01:00
Mostly to re-use the fields of the structure instead of copying things
around.

Signed-off-by: Hidde Beydals &lt;hello@hidde.co&gt;
diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go
@@ -515,7 +515,7 @@ func (r *HelmChartReconciler) reconcileFromTarballArtifact(ctx context.Context,
 			if strings.HasPrefix(dep.Repository, "file://") {
 				dwr = append(dwr, &helm.DependencyWithRepository{
 					Dependency: dep,
-					Repo:       nil,
+					Repository: nil,
 				})
 				continue
 			}
@@ -577,15 +577,15 @@ func (r *HelmChartReconciler) reconcileFromTarballArtifact(ctx context.Context,
 
 			dwr = append(dwr, &helm.DependencyWithRepository{
 				Dependency: dep,
-				Repo:       chartRepo,
+				Repository: chartRepo,
 			})
 		}
 
 		// Construct dependencies for chart if any
 		if len(dwr) > 0 {
 			dm := &helm.DependencyManager{
-				BaseDir:      tmpDir,
-				ChartPath:    chartPath,
+				WorkingDir:   tmpDir,
+				ChartPath:    chart.Spec.Chart,
 				Chart:        helmChart,
 				Dependencies: dwr,
 			}
diff --git a/internal/helm/dependency_manager.go b/internal/helm/dependency_manager.go
@@ -19,6 +19,7 @@ package helm
 import (
 	"context"
 	"fmt"
+	"net/url"
 	"os"
 	"path/filepath"
 	"strings"
@@ -30,22 +31,35 @@ import (
 	"helm.sh/helm/v3/pkg/chart/loader"
 )
 
-// DependencyWithRepository is a container for a dependency and its respective
-// repository
+// DependencyWithRepository is a container for a Helm chart dependency
+// and its respective repository.
 type DependencyWithRepository struct {
+	// Dependency holds the reference to a chart.Chart dependency.
 	Dependency *helmchart.Dependency
-	Repo       *ChartRepository
+	// Repository is the ChartRepository the dependency should be
+	// available at and can be downloaded from. If there is none,
+	// a local ('file://') dependency is assumed.
+	Repository *ChartRepository
 }
 
-// DependencyManager manages dependencies for helm charts
+// DependencyManager manages dependencies for a Helm chart.
 type DependencyManager struct {
-	BaseDir      string
-	ChartPath    string
-	Chart        *helmchart.Chart
+	// WorkingDir is the chroot path for dependency manager operations,
+	// Dependencies that hold a local (relative) path reference are not
+	// allowed to traverse outside this directory.
+	WorkingDir string
+	// ChartPath is the path of the Chart relative to the WorkingDir,
+	// the combination of the WorkingDir and ChartPath is used to
+	// determine the absolute path of a local dependency.
+	ChartPath string
+	// Chart holds the loaded chart.Chart from the ChartPath.
+	Chart *helmchart.Chart
+	// Dependencies contains a list of dependencies, and the respective
+	// repository the dependency can be found at.
 	Dependencies []*DependencyWithRepository
 }
 
-// Build compiles and builds the chart dependencies
+// Build compiles and builds the dependencies of the Chart.
 func (dm *DependencyManager) Build(ctx context.Context) error {
 	if len(dm.Dependencies) == 0 {
 		return nil
@@ -60,85 +74,90 @@ func (dm *DependencyManager) Build(ctx context.Context) error {
 			default:
 			}
 
-			var (
-				ch  *helmchart.Chart
-				err error
-			)
-			if strings.HasPrefix(item.Dependency.Repository, "file://") {
-				ch, err = chartForLocalDependency(item.Dependency, dm.BaseDir, dm.ChartPath)
-			} else {
-				ch, err = chartForRemoteDependency(item.Dependency, item.Repo)
-			}
-			if err != nil {
-				return err
+			var err error
+			switch item.Repository {
+			case nil:
+				err = dm.addLocalDependency(item)
+			default:
+				err = dm.addRemoteDependency(item)
 			}
-			dm.Chart.AddDependency(ch)
-			return nil
+			return err
 		})
 	}
 
 	return errs.Wait()
 }
 
-func chartForLocalDependency(dep *helmchart.Dependency, baseDir, chartPath string) (*helmchart.Chart, error) {
-	origPath, err := securejoin.SecureJoin(baseDir,
-		filepath.Join(strings.TrimPrefix(chartPath, baseDir), strings.TrimPrefix(dep.Repository, "file://")))
+func (dm *DependencyManager) addLocalDependency(dpr *DependencyWithRepository) error {
+	sLocalChartPath, err := dm.secureLocalChartPath(dpr)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	if _, err := os.Stat(origPath); os.IsNotExist(err) {
-		err := fmt.Errorf("chart path %s not found: %w", origPath, err)
-		return nil, err
-	} else if err != nil {
-		return nil, err
+	if _, err := os.Stat(sLocalChartPath); err != nil {
+		if os.IsNotExist(err) {
+			return fmt.Errorf("no chart found at '%s' (reference '%s') for dependency '%s'",
+				strings.TrimPrefix(sLocalChartPath, dm.WorkingDir), dpr.Dependency.Repository, dpr.Dependency.Name)
+		}
+		return err
 	}
 
-	ch, err := loader.Load(origPath)
+	ch, err := loader.Load(sLocalChartPath)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	constraint, err := semver.NewConstraint(dep.Version)
+	constraint, err := semver.NewConstraint(dpr.Dependency.Version)
 	if err != nil {
-		err := fmt.Errorf("dependency %s has an invalid version/constraint format: %w", dep.Name, err)
-		return nil, err
+		err := fmt.Errorf("dependency '%s' has an invalid version/constraint format: %w", dpr.Dependency.Name, err)
+		return err
 	}
 
 	v, err := semver.NewVersion(ch.Metadata.Version)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	if !constraint.Check(v) {
-		err = fmt.Errorf("can't get a valid version for dependency %s", dep.Name)
-		return nil, err
+		err = fmt.Errorf("can't get a valid version for dependency '%s'", dpr.Dependency.Name)
+		return err
 	}
 
-	return ch, nil
+	dm.Chart.AddDependency(ch)
+	return nil
 }
 
-func chartForRemoteDependency(dep *helmchart.Dependency, chartRepo *ChartRepository) (*helmchart.Chart, error) {
-	if chartRepo == nil {
-		return nil, fmt.Errorf("chartrepo should not be nil")
+func (dm *DependencyManager) addRemoteDependency(dpr *DependencyWithRepository) error {
+	if dpr.Repository == nil {
+		return fmt.Errorf("no ChartRepository given for '%s' dependency", dpr.Dependency.Name)
 	}
 
-	// Lookup the chart version in the chart repository index
-	chartVer, err := chartRepo.Get(dep.Name, dep.Version)
+	chartVer, err := dpr.Repository.Get(dpr.Dependency.Name, dpr.Dependency.Version)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	// Download chart
-	res, err := chartRepo.DownloadChart(chartVer)
+	res, err := dpr.Repository.DownloadChart(chartVer)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	ch, err := loader.LoadArchive(res)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	return ch, nil
+	dm.Chart.AddDependency(ch)
+	return nil
+}
+
+func (dm *DependencyManager) secureLocalChartPath(dep *DependencyWithRepository) (string, error) {
+	localUrl, err := url.Parse(dep.Dependency.Repository)
+	if err != nil {
+		return "", fmt.Errorf("failed to parse alleged local chart reference: %w", err)
+	}
+	if localUrl.Scheme != "file" {
+		return "", fmt.Errorf("'%s' is not a local chart reference", dep.Dependency.Repository)
+	}
+	return securejoin.SecureJoin(dm.WorkingDir, filepath.Join(dm.ChartPath, localUrl.Host, localUrl.Path))
 }
diff --git a/internal/helm/dependency_manager_test.go b/internal/helm/dependency_manager_test.go
@@ -73,6 +73,15 @@ func TestBuild_WithLocalChart(t *testing.T) {
 				Repository: chartLocalRepository,
 			},
 		},
+		{
+			name: "allowed traversing path",
+			dep: helmchart.Dependency{
+				Name:       chartName,
+				Alias:      "aliased",
+				Version:    chartVersion,
+				Repository: "file://../../../testdata/charts/helmchartwithdeps/../helmchart",
+			},
+		},
 		{
 			name: "invalid path",
 			dep: helmchart.Dependency{
@@ -81,7 +90,17 @@ func TestBuild_WithLocalChart(t *testing.T) {
 				Repository: "file://../invalid",
 			},
 			wantErr: true,
-			errMsg:  "no such file or directory",
+			errMsg:  "no chart found at",
+		},
+		{
+			name: "illegal traversing path",
+			dep: helmchart.Dependency{
+				Name:       chartName,
+				Version:    chartVersion,
+				Repository: "file://../../../../../controllers/testdata/charts/helmchart",
+			},
+			wantErr: true,
+			errMsg:  "no chart found at",
 		},
 		{
 			name: "invalid version constraint format",
@@ -109,13 +128,13 @@ func TestBuild_WithLocalChart(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			c := chartFixture
 			dm := DependencyManager{
-				BaseDir:   "./",
-				ChartPath: "testdata/charts/helmchart",
-				Chart:     &c,
+				WorkingDir: "./",
+				ChartPath:  "testdata/charts/helmchart",
+				Chart:      &c,
 				Dependencies: []*DependencyWithRepository{
 					{
 						Dependency: &tt.dep,
-						Repo:       nil,
+						Repository: nil,
 					},
 				},
 			}
@@ -132,7 +151,7 @@ func TestBuild_WithLocalChart(t *testing.T) {
 				}
 				return
 			} else if err != nil {
-				t.Errorf("Build() expected to return error")
+				t.Errorf("Build() not expected to return an error: %s", err)
 				return
 			}
 
@@ -168,7 +187,7 @@ func TestBuild_WithRemoteChart(t *testing.T) {
 		Dependencies: []*DependencyWithRepository{
 			{
 				Dependency: &remoteDepFixture,
-				Repo:       cr,
+				Repository: cr,
 			},
 		},
 	}
@@ -189,10 +208,10 @@ func TestBuild_WithRemoteChart(t *testing.T) {
 	}
 
 	// When repo is not set
-	dm.Dependencies[0].Repo = nil
+	dm.Dependencies[0].Repository = nil
 	if err := dm.Build(context.TODO()); err == nil {
 		t.Errorf("Build() expected to return error")
-	} else if !strings.Contains(err.Error(), "chartrepo should not be nil") {
+	} else if !strings.Contains(err.Error(), "is not a local chart reference") {
 		t.Errorf("Build() expected to return different error, got: %s", err)
 	}
 }
