Move loadExternalPaths to pkg/action and implement tests for install and upgrade

Signed-off-by: itaispiegel <itai.spiegel@gmail.com>

cmd/helm/install.go

@@ -20,11 +20,9 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"io/ioutil"
 	"log"
 	"os"
 	"os/signal"
-	"strings"
 	"syscall"
 	"time"
 
@@ -261,11 +259,6 @@ func runInstall(args []string, client *action.Install, valueOpts *values.Options
 		}
 	}
 
-	err = loadExternalPaths(chartRequested, client.ExternalPaths)
-	if err != nil {
-		return nil, err
-	}
-
 	client.Namespace = settings.Namespace()
 
 	// Create context and prepare the handle of SIGTERM
@@ -297,33 +290,6 @@ func checkIfInstallable(ch *chart.Chart) error {
 	return errors.Errorf("%s charts are not installable", ch.Metadata.Type)
 }
 
-func loadExternalPaths(ch *chart.Chart, externalPaths []string) error {
-	var errs []string
-
-	for _, path := range externalPaths {
-		allPaths, err := loader.ExpandFilePath(path)
-		debug(fmt.Sprintf("%s expanded to: %v", path, allPaths))
-		if err != nil {
-			debug(fmt.Sprintf("error loading external path %s: %v", path, err))
-			errs = append(errs, fmt.Sprintf("%s (path not accessible)", path))
-		}
-
-		for _, name := range allPaths {
-			fileContentBytes, err := ioutil.ReadFile(name)
-			if err != nil {
-				errs = append(errs, fmt.Sprintf("%s (not readable)", name))
-			} else {
-				ch.Files = append(ch.Files, &chart.File{Name: name, Data: fileContentBytes})
-			}
-		}
-	}
-
-	if len(errs) > 0 {
-		return errors.New(fmt.Sprint("Failed to load external paths: ", strings.Join(errs, "; ")))
-	}
-	return nil
-}
-
 // Provide dynamic auto-completion for the install and template commands
 func compInstall(args []string, toComplete string, client *action.Install) ([]string, cobra.ShellCompDirective) {
 	requiredArgs := 1

cmd/helm/upgrade.go

@@ -182,11 +182,6 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
 				warning("This chart is deprecated")
 			}
 
-			err = loadExternalPaths(ch, client.ExternalPaths)
-			if err != nil {
-				return err
-			}
-
 			// Create context and prepare the handle of SIGTERM
 			ctx := context.Background()
 			ctx, cancel := context.WithCancel(ctx)

pkg/action/action_test.go

@@ -17,6 +17,7 @@ package action
 
 import (
 	"flag"
+	"fmt"
 	"io/ioutil"
 	"testing"
 
@@ -32,6 +33,8 @@ import (
 	"helm.sh/helm/v3/pkg/time"
 )
 
+const withExternalPathsTemplatePath = "templates/with-external-paths"
+
 var verbose = flag.Bool("test.log", false, "enable test logging")
 
 func actionConfigFixture(t *testing.T) *Configuration {
@@ -209,6 +212,15 @@ func withSampleIncludingIncorrectTemplates() chartOption {
 	}
 }
 
+func withExternalFileTemplate(externalPath string) chartOption {
+	return func(opts *chartOptions) {
+		externalFilesTemplates := []*chart.File{
+			{Name: withExternalPathsTemplatePath, Data: []byte(fmt.Sprintf(`data: {{ .Files.Get "%s" }}`, externalPath))},
+		}
+		opts.Templates = append(opts.Templates, externalFilesTemplates...)
+	}
+}
+
 func withMultipleManifestTemplate() chartOption {
 	return func(opts *chartOptions) {
 		sampleTemplates := []*chart.File{

pkg/action/install.go

@@ -177,6 +177,38 @@ func (i *Install) installCRDs(crds []chart.CRD) error {
 	return nil
 }
 
+func loadExternalPaths(ch *chart.Chart, externalPaths []string) error {
+	var errs []string
+
+	for _, p := range externalPaths {
+		allPaths, err := expandFilePath(p)
+		if err != nil {
+			errs = append(errs, fmt.Sprintf("%s (path not accessible)", p))
+		}
+
+		for _, currentPath := range allPaths {
+			fileContentBytes, err := os.ReadFile(currentPath)
+			if err != nil {
+				errs = append(errs, fmt.Sprintf("%s (not readable)", currentPath))
+				continue
+			}
+
+			newFile := chart.File{Name: currentPath, Data: fileContentBytes}
+			for _, file := range ch.Files {
+				if file.Name == newFile.Name && bytes.Equal(file.Data, newFile.Data) {
+					continue
+				}
+			}
+			ch.Files = append(ch.Files, &newFile)
+		}
+	}
+
+	if len(errs) > 0 {
+		return errors.New(fmt.Sprint("Failed to load external paths: ", strings.Join(errs, "; ")))
+	}
+	return nil
+}
+
 // Run executes the installation
 //
 // If DryRun is set to true, this will prepare the release, but not install it
@@ -188,6 +220,10 @@ func (i *Install) Run(chrt *chart.Chart, vals map[string]interface{}) (*release.
 
 // Run executes the installation with Context
 func (i *Install) RunWithContext(ctx context.Context, chrt *chart.Chart, vals map[string]interface{}) (*release.Release, error) {
+	if err := loadExternalPaths(chrt, i.ExternalPaths); err != nil {
+		return nil, err
+	}
+
 	// Check reachability of cluster unless in client-only mode (e.g. `helm template` without `--validate`)
 	if !i.ClientOnly {
 		if err := i.cfg.KubeClient.IsReachable(); err != nil {

pkg/action/install_test.go

@@ -39,12 +39,20 @@ import (
 	helmtime "helm.sh/helm/v3/pkg/time"
 )
 
+const ExternalFileRelPath = "testdata/files/external.txt"
+
 type nameTemplateTestCase struct {
 	tpl              string
 	expected         string
 	expectedErrorStr string
 }
 
+type includeExternalPathTestCase struct {
+	Name             string
+	IncludedFilePath string
+	ExternalPath     string
+}
+
 func installAction(t *testing.T) *Install {
 	config := actionConfigFixture(t)
 	instAction := NewInstall(config)
@@ -54,6 +62,11 @@ func installAction(t *testing.T) *Install {
 	return instAction
 }
 
+func getAbsPath(path string) string {
+	absPath, _ := filepath.Abs(path)
+	return absPath
+}
+
 func TestInstallRelease(t *testing.T) {
 	is := assert.New(t)
 	req := require.New(t)
@@ -717,3 +730,86 @@ func TestNameAndChartGenerateName(t *testing.T) {
 		})
 	}
 }
+
+func TestInstallFailsWhenWrongPathsIncluded(t *testing.T) {
+	is := assert.New(t)
+	vals := map[string]interface{}{}
+
+	tests := []includeExternalPathTestCase{
+		{
+			Name:             "included paths not passed",
+			IncludedFilePath: "",
+			ExternalPath:     ExternalFileRelPath,
+		},
+		{
+			Name:             "absolute path of file is included and external file is relative",
+			IncludedFilePath: getAbsPath(ExternalFileRelPath),
+			ExternalPath:     ExternalFileRelPath,
+		},
+		{
+			Name:             "relative path of file is included and external file is absolute",
+			IncludedFilePath: ExternalFileRelPath,
+			ExternalPath:     getAbsPath(ExternalFileRelPath),
+		},
+		{
+			Name:             "absolute path of directory is included and external file is relative",
+			IncludedFilePath: getAbsPath("testdata/files"),
+			ExternalPath:     ExternalFileRelPath,
+		},
+		{
+			Name:             "relative path of directory is included and external file is absolute",
+			IncludedFilePath: "testdata/files",
+			ExternalPath:     getAbsPath(ExternalFileRelPath),
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.Name, func(t *testing.T) {
+			instAction := installAction(t)
+			instAction.ExternalPaths = append(instAction.ExternalPaths, tc.IncludedFilePath)
+
+			_, err := instAction.Run(buildChart(withExternalFileTemplate(tc.ExternalPath)), vals)
+			expectedErr := fmt.Sprintf("<.Files.Get>: error calling Get: file %s not included", tc.ExternalPath)
+			is.Error(err, expectedErr)
+		})
+	}
+}
+
+func TestInstallWhenIncludePathsPassed(t *testing.T) {
+	is := assert.New(t)
+	vals := map[string]interface{}{}
+
+	tests := []includeExternalPathTestCase{
+		{
+			Name:             "relative path of file is included and external file is relative",
+			IncludedFilePath: ExternalFileRelPath,
+			ExternalPath:     ExternalFileRelPath,
+		},
+		{
+			Name:             "absolute path of file is included and external file is absolute",
+			IncludedFilePath: getAbsPath(ExternalFileRelPath),
+			ExternalPath:     getAbsPath(ExternalFileRelPath),
+		},
+		{
+			Name:             "relative path of directory is included and external file is relative",
+			IncludedFilePath: "testdata/files",
+			ExternalPath:     ExternalFileRelPath,
+		},
+		{
+			Name:             "absolute path of directory is included and external file is absolute",
+			IncludedFilePath: getAbsPath("testdata/files"),
+			ExternalPath:     getAbsPath(ExternalFileRelPath),
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.Name, func(t *testing.T) {
+			instAction := installAction(t)
+			instAction.ExternalPaths = append(instAction.ExternalPaths, tc.IncludedFilePath)
+
+			installRelease, err := instAction.Run(buildChart(withExternalFileTemplate(tc.ExternalPath)), vals)
+			is.Contains(installRelease.Manifest, "out-of-chart-dir")
+			is.NoError(err)
+		})
+	}
+}

pkg/action/testdata/files/external.txt

@@ -0,0 +1 @@
+out-of-chart-dir

pkg/action/upgrade.go

@@ -180,6 +180,10 @@ func (u *Upgrade) prepareUpgrade(name string, chart *chart.Chart, vals map[strin
 		return nil, nil, err
 	}
 
+	if err := loadExternalPaths(chart, u.ExternalPaths); err != nil {
+		return nil, nil, err
+	}
+
 	// Concurrent `helm upgrade`s will either fail here with `errPending` or when creating the release with "already exists". This should act as a pessimistic lock.
 	if lastRelease.Info.Status.IsPending() {
 		return nil, nil, errPending

pkg/action/upgrade_test.go

@@ -388,3 +388,96 @@ func TestUpgradeRelease_Interrupted_Atomic(t *testing.T) {
 	is.Equal(updatedRes.Info.Status, release.StatusDeployed)
 
 }
+
+func TestUpgradeFailsWhenWrongPathsIncluded(t *testing.T) {
+	is := assert.New(t)
+	vals := map[string]interface{}{}
+
+	tests := []includeExternalPathTestCase{
+		{
+			Name:             "included paths not passed",
+			IncludedFilePath: "",
+			ExternalPath:     "testdata/files/external.txt",
+		},
+		{
+			Name:             "absolute path of file is included and external file is relative",
+			IncludedFilePath: getAbsPath("testdata/files/external.txt"),
+			ExternalPath:     "testdata/files/external.txt",
+		},
+		{
+			Name:             "relative path of file is included and external file is absolute",
+			IncludedFilePath: "testdata/files/external.txt",
+			ExternalPath:     getAbsPath("testdata/files/external.txt"),
+		},
+		{
+			Name:             "absolute path of directory is included and external file is relative",
+			IncludedFilePath: getAbsPath("testdata/files"),
+			ExternalPath:     "testdata/files/external.txt",
+		},
+		{
+			Name:             "relative path of directory is included and external file is absolute",
+			IncludedFilePath: "testdata/files",
+			ExternalPath:     getAbsPath("testdata/files/external.txt"),
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.Name, func(t *testing.T) {
+			upAction := upgradeAction(t)
+			upAction.ExternalPaths = append(upAction.ExternalPaths, tc.IncludedFilePath)
+
+			rel := releaseStub()
+			rel.Name = "test"
+			rel.Info.Status = release.StatusDeployed
+			upAction.cfg.Releases.Create(rel)
+
+			_, err := upAction.Run(rel.Name, buildChart(withExternalFileTemplate(tc.ExternalPath)), vals)
+			expectedErr := fmt.Sprintf("<.Files.Get>: error calling Get: file %s not included", tc.ExternalPath)
+			is.Error(err, expectedErr)
+		})
+	}
+}
+
+func TestUpgradeWhenIncludePathsPassed(t *testing.T) {
+	is := assert.New(t)
+	vals := map[string]interface{}{}
+
+	tests := []includeExternalPathTestCase{
+		{
+			Name:             "relative path of file is included and external file is relative",
+			IncludedFilePath: "testdata/files/external.txt",
+			ExternalPath:     "testdata/files/external.txt",
+		},
+		{
+			Name:             "relative path of file is included and external file is absolute",
+			IncludedFilePath: getAbsPath("testdata/files/external.txt"),
+			ExternalPath:     getAbsPath("testdata/files/external.txt"),
+		},
+		{
+			Name:             "relative path of directory is included and external file is relative",
+			IncludedFilePath: "testdata/files",
+			ExternalPath:     "testdata/files/external.txt",
+		},
+		{
+			Name:             "absolute path of directory is included and external file is absolute",
+			IncludedFilePath: getAbsPath("testdata/files"),
+			ExternalPath:     getAbsPath("testdata/files/external.txt"),
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.Name, func(t *testing.T) {
+			upAction := upgradeAction(t)
+			upAction.ExternalPaths = append(upAction.ExternalPaths, tc.IncludedFilePath)
+
+			rel := releaseStub()
+			rel.Name = "test"
+			rel.Info.Status = release.StatusDeployed
+			upAction.cfg.Releases.Create(rel)
+
+			upgradeRelease, err := upAction.Run(rel.Name, buildChart(withExternalFileTemplate(tc.ExternalPath)), vals)
+			is.Contains(upgradeRelease.Manifest, "out-of-chart-dir")
+			is.NoError(err)
+		})
+	}
+}