Fix "consul-k8s install fails: now can't re-install or upgrade" (hash…

…icorp#1115)

* Allow tests to pass in args to Helm and CLI actions

* Add CLI scenario tests

* Add scenario test for upgrading a cluster in a namespace other than consul

* Pass and use the name of the cluster correctly

* Allow for setting the namespace the CLI uses in test

* Update changelog

* Allow CLI actions to error silently

* Merge defaults with passed-in args

* scenario -> upgrade + docstring

* Update CHANGELOG.md

Co-authored-by: Iryna Shustava <[email protected]>

* Remove 15m explicit default

* Add no error requirement back in.

* Remove acceptance tests for upgrade

* fixtures -> test_fixtures

* Use LoadChart in install cmd

* Add Helm mocks (attempt)

* Tidy InitActionConfig comment

* Add stubbed TestFetchChartValues

* Add a Helm logger "factory" function and stubbed test

* Remove changes to acceptance tests

* Finish removing acceptance test changes

* Remove test

* Remove logger fn

* Remove helm mock

* Fix Chart tests

Co-authored-by: Iryna Shustava <[email protected]>

CHANGELOG.md

@@ -1,5 +1,8 @@
 ## UNRELEASED
 
+BUG FIXES:
+  * Fix issue where clusters not in the same namespace as their deployment name could not be upgraded. [[GH-1115](https://github.com/hashicorp/consul-k8s/issue/1005)]
+
 ## 0.42.0 (April 04, 2022)
 
 BREAKING CHANGES:

cli/cmd/install/install.go

@@ -17,7 +17,6 @@ import (
 	"github.com/hashicorp/consul-k8s/cli/release"
 	"github.com/hashicorp/consul-k8s/cli/validation"
 	"helm.sh/helm/v3/pkg/action"
-	"helm.sh/helm/v3/pkg/chart/loader"
 	helmCLI "helm.sh/helm/v3/pkg/cli"
 	"helm.sh/helm/v3/pkg/cli/values"
 	"helm.sh/helm/v3/pkg/getter"
@@ -358,15 +357,8 @@ func (c *Command) Run(args []string) int {
 	install.Wait = c.flagWait
 	install.Timeout = c.timeoutDuration
 
-	// Read the embedded chart files into []*loader.BufferedFile.
-	chartFiles, err := helm.ReadChartFiles(consulChart.ConsulHelmChart, common.TopLevelChartDirName)
-	if err != nil {
-		c.UI.Output(err.Error(), terminal.WithErrorStyle())
-		return 1
-	}
-
-	// Create a *chart.Chart object from the files to run the installation from.
-	chart, err := loader.LoadFiles(chartFiles)
+	// Load the Helm chart.
+	chart, err := helm.LoadChart(consulChart.ConsulHelmChart, common.TopLevelChartDirName)
 	if err != nil {
 		c.UI.Output(err.Error(), terminal.WithErrorStyle())
 		return 1

cli/cmd/upgrade/upgrade.go

@@ -230,7 +230,7 @@ func (c *Command) Run(args []string) int {
 	}
 	c.UI.Output("Loaded charts", terminal.WithSuccessStyle())
 
-	currentChartValues, err := helm.FetchChartValues(namespace, settings, uiLogger)
+	currentChartValues, err := helm.FetchChartValues(namespace, name, settings, uiLogger)
 	if err != nil {
 		c.UI.Output(err.Error(), terminal.WithErrorStyle())
 		return 1

cli/helm/action.go

@@ -9,8 +9,9 @@ import (
 	"k8s.io/cli-runtime/pkg/genericclioptions"
 )
 
-// InitActionConfig initializes a Helm Go SDK action configuration. This function currently uses a hack to override the
-// namespace field that gets set in the K8s client set up by the SDK.
+// InitActionConfig initializes a Helm Go SDK action configuration. This
+// function currently uses a hack to override the namespace field that gets set
+// in the K8s client set up by the SDK.
 func InitActionConfig(actionConfig *action.Configuration, namespace string, settings *helmCLI.EnvSettings, logger action.DebugLog) (*action.Configuration, error) {
 	getter := settings.RESTClientGetter()
 	configFlags := getter.(*genericclioptions.ConfigFlags)

cli/helm/chart.go

@@ -17,22 +17,43 @@ const (
 	templatesDirName = "templates"
 )
 
-// LoadChart will attempt to load a chart from the embedded file system.
+// LoadChart will attempt to load a Helm chart from the embedded file system.
 func LoadChart(chart embed.FS, chartDirName string) (*chart.Chart, error) {
-	chartFiles, err := ReadChartFiles(chart, chartDirName)
+	chartFiles, err := readChartFiles(chart, chartDirName)
 	if err != nil {
 		return nil, err
 	}
 
 	return loader.LoadFiles(chartFiles)
 }
 
-// ReadChartFiles reads the chart files from the embedded file system, and loads their contents into
-// []*loader.BufferedFile. This is a format that the Helm Go SDK functions can read from to create a chart to install
-// from. The names of these files are important, as there are case statements in the Helm Go SDK looking for files named
-// "Chart.yaml" or "templates/<templatename>.yaml", which is why even though the embedded file system has them named
-// "consul/Chart.yaml" we have to strip the "consul" prefix out, which is done by the call to the helper method readFile.
-func ReadChartFiles(chart embed.FS, chartDirName string) ([]*loader.BufferedFile, error) {
+// FetchChartValues will attempt to fetch the values from the currently
+// installed Helm chart.
+func FetchChartValues(namespace, name string, settings *helmCLI.EnvSettings, uiLogger action.DebugLog) (map[string]interface{}, error) {
+	cfg := new(action.Configuration)
+	cfg, err := InitActionConfig(cfg, namespace, settings, uiLogger)
+	if err != nil {
+		return nil, err
+	}
+
+	status := action.NewStatus(cfg)
+	release, err := status.Run(name)
+	if err != nil {
+		return nil, err
+	}
+
+	return release.Config, nil
+}
+
+// readChartFiles reads the chart files from the embedded file system, and loads
+// their contents into []*loader.BufferedFile. This is a format that the Helm Go
+// SDK functions can read from to create a chart to install from. The names of
+// these files are important, as there are case statements in the Helm Go SDK
+// looking for files named "Chart.yaml" or "templates/<templatename>.yaml",
+// which is why even though the embedded file system has them named
+// "consul/Chart.yaml" we have to strip the "consul" prefix out, which is done
+// by the call to the helper method readFile.
+func readChartFiles(chart embed.FS, chartDirName string) ([]*loader.BufferedFile, error) {
 	var chartFiles []*loader.BufferedFile
 
 	// NOTE: Because we're using the embedded filesystem, we must use path.* functions,
@@ -55,6 +76,7 @@ func ReadChartFiles(chart embed.FS, chartDirName string) ([]*loader.BufferedFile
 	if err != nil {
 		return nil, err
 	}
+
 	for _, f := range dirs {
 		if f.IsDir() {
 			// We only need to include files in the templates directory.
@@ -71,23 +93,8 @@ func ReadChartFiles(chart embed.FS, chartDirName string) ([]*loader.BufferedFile
 	return chartFiles, nil
 }
 
-// FetchChartValues will attempt to fetch the values from the currently installed Helm chart.
-func FetchChartValues(namespace string, settings *helmCLI.EnvSettings, uiLogger action.DebugLog) (map[string]interface{}, error) {
-	cfg := new(action.Configuration)
-	cfg, err := InitActionConfig(cfg, namespace, settings, uiLogger)
-	if err != nil {
-		return nil, err
-	}
-
-	status := action.NewStatus(cfg)
-	release, err := status.Run(namespace)
-	if err != nil {
-		return nil, err
-	}
-
-	return release.Config, nil
-}
-
+// readFile reads the contents of the file from the embedded file system, and
+// returns a *loader.BufferedFile.
 func readFile(chart embed.FS, f string, pathPrefix string) (*loader.BufferedFile, error) {
 	bytes, err := chart.ReadFile(f)
 	if err != nil {

cli/helm/chart_test.go

@@ -8,11 +8,11 @@ import (
 )
 
 // Embed a test chart to test against.
-//go:embed fixtures/consul/* fixtures/consul/templates/_helpers.tpl
+//go:embed test_fixtures/consul/* test_fixtures/consul/templates/_helpers.tpl
 var testChartFiles embed.FS
 
 func TestLoadChart(t *testing.T) {
-	directory := "fixtures/consul"
+	directory := "test_fixtures/consul"
 
 	expectedApiVersion := "v2"
 	expectedName := "Foo"
@@ -32,15 +32,15 @@ func TestLoadChart(t *testing.T) {
 }
 
 func TestReadChartFiles(t *testing.T) {
-	directory := "fixtures/consul"
+	directory := "test_fixtures/consul"
 	expectedFiles := map[string]string{
 		"Chart.yaml":             "# This is a mock Helm Chart.yaml file used for testing.\napiVersion: v2\nname: Foo\nversion: 0.1.0\ndescription: Mock Helm Chart for testing.",
 		"values.yaml":            "# This is a mock Helm values.yaml file used for testing.\nkey: value",
 		"templates/_helpers.tpl": "helpers",
 		"templates/foo.yaml":     "foo: bar\n",
 	}
 
-	files, err := ReadChartFiles(testChartFiles, directory)
+	files, err := readChartFiles(testChartFiles, directory)
 	require.NoError(t, err)
 
 	actualFiles := make(map[string]string, len(files))