From de6b10b8b6b26058c15576c11b931a7fb1c7c92c Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Thu, 7 Apr 2022 13:14:23 -0400 Subject: [PATCH] Fix "consul-k8s install fails: now can't re-install or upgrade" (#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 * 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 --- CHANGELOG.md | 3 + cli/cmd/install/install.go | 12 +--- cli/cmd/upgrade/upgrade.go | 2 +- cli/helm/action.go | 5 +- cli/helm/chart.go | 57 +++++++++++-------- cli/helm/chart_test.go | 8 +-- .../consul/Chart.yaml | 0 .../consul/templates/_helpers.tpl | 0 .../consul/templates/foo.yaml | 0 .../consul/values.yaml | 0 10 files changed, 45 insertions(+), 42 deletions(-) rename cli/helm/{fixtures => test_fixtures}/consul/Chart.yaml (100%) rename cli/helm/{fixtures => test_fixtures}/consul/templates/_helpers.tpl (100%) rename cli/helm/{fixtures => test_fixtures}/consul/templates/foo.yaml (100%) rename cli/helm/{fixtures => test_fixtures}/consul/values.yaml (100%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d66cabc8d..9fa0521c6b 100644 --- a/CHANGELOG.md +++ b/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: diff --git a/cli/cmd/install/install.go b/cli/cmd/install/install.go index 9aa21737fc..c994febc98 100644 --- a/cli/cmd/install/install.go +++ b/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 diff --git a/cli/cmd/upgrade/upgrade.go b/cli/cmd/upgrade/upgrade.go index a0923e3089..4ef26661e8 100644 --- a/cli/cmd/upgrade/upgrade.go +++ b/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 diff --git a/cli/helm/action.go b/cli/helm/action.go index c4d9de9650..df8ba5fb07 100644 --- a/cli/helm/action.go +++ b/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) diff --git a/cli/helm/chart.go b/cli/helm/chart.go index a4a13dff8f..1a91ee19d5 100644 --- a/cli/helm/chart.go +++ b/cli/helm/chart.go @@ -17,9 +17,9 @@ 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 } @@ -27,12 +27,33 @@ func LoadChart(chart embed.FS, chartDirName string) (*chart.Chart, error) { 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/.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/.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 { diff --git a/cli/helm/chart_test.go b/cli/helm/chart_test.go index 1bed8bbdbe..71fa6355be 100644 --- a/cli/helm/chart_test.go +++ b/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,7 +32,7 @@ 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", @@ -40,7 +40,7 @@ func TestReadChartFiles(t *testing.T) { "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)) diff --git a/cli/helm/fixtures/consul/Chart.yaml b/cli/helm/test_fixtures/consul/Chart.yaml similarity index 100% rename from cli/helm/fixtures/consul/Chart.yaml rename to cli/helm/test_fixtures/consul/Chart.yaml diff --git a/cli/helm/fixtures/consul/templates/_helpers.tpl b/cli/helm/test_fixtures/consul/templates/_helpers.tpl similarity index 100% rename from cli/helm/fixtures/consul/templates/_helpers.tpl rename to cli/helm/test_fixtures/consul/templates/_helpers.tpl diff --git a/cli/helm/fixtures/consul/templates/foo.yaml b/cli/helm/test_fixtures/consul/templates/foo.yaml similarity index 100% rename from cli/helm/fixtures/consul/templates/foo.yaml rename to cli/helm/test_fixtures/consul/templates/foo.yaml diff --git a/cli/helm/fixtures/consul/values.yaml b/cli/helm/test_fixtures/consul/values.yaml similarity index 100% rename from cli/helm/fixtures/consul/values.yaml rename to cli/helm/test_fixtures/consul/values.yaml