diff --git a/cmd/helm/install.go b/cmd/helm/install.go index b287b1f72..d4a768e15 100644 --- a/cmd/helm/install.go +++ b/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 diff --git a/cmd/helm/upgrade.go b/cmd/helm/upgrade.go index d1ce7624e..72f29c637 100644 --- a/cmd/helm/upgrade.go +++ b/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) diff --git a/pkg/action/action_test.go b/pkg/action/action_test.go index c816c84af..f0c8a0c51 100644 --- a/pkg/action/action_test.go +++ b/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{ diff --git a/pkg/action/install.go b/pkg/action/install.go index e16e69aeb..10291d3e7 100644 --- a/pkg/action/install.go +++ b/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 { diff --git a/pkg/action/install_test.go b/pkg/action/install_test.go index 45e5a2670..1fde091a3 100644 --- a/pkg/action/install_test.go +++ b/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) + }) + } +} diff --git a/pkg/action/testdata/files/external.txt b/pkg/action/testdata/files/external.txt new file mode 100644 index 000000000..068c4db58 --- /dev/null +++ b/pkg/action/testdata/files/external.txt @@ -0,0 +1 @@ +out-of-chart-dir diff --git a/pkg/action/upgrade.go b/pkg/action/upgrade.go index 2826076d2..17defcd1e 100644 --- a/pkg/action/upgrade.go +++ b/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 diff --git a/pkg/action/upgrade_test.go b/pkg/action/upgrade_test.go index 62922b373..b64145f51 100644 --- a/pkg/action/upgrade_test.go +++ b/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) + }) + } +}