Show diff when upgrading (#934)

* Made an initial try at the consul-k8s upgrade command. Running into issues with the connect-injector webhook not starting on an install?

* notes from sync with Saad on what's left

* Made an initial try at the consul-k8s upgrade command. Running into issues with the connect-injector webhook not starting on an install?

* First pass at upgrade was successful.

* notes from sync with Saad on what's left

* Some basic cleanup

* Add the namespace and install flags

* Add flag test and remove install option

* Move presets into a config package

* Add Changelog

* Made an initial try at the consul-k8s upgrade command. Running into issues with the connect-injector webhook not starting on an install?

* Upgrade commit

* notes from sync with Saad on what's left

* This commit contains the MapDiff function to compare the YAML of the previous Release and the new upgrade.

* Remove double comment on name setting

* Clean up some merge issues from rebase

* Move MergeMaps to util

* Reduce indent on check for installation

* Clean up Help fn

* Use Synopsis in Help

* Remove duplicated MapMerge from git rebase

* Add IsValidLabel to utils

* Move chart loading code to chart file

* Use the IsValidLabel in common

* Use LoadChart

* Move create UI logger closer to it's first call

* Rename vals to chartValues

* Add godebug dep

* Add WithDiff{Added,Removed}Style

* Run go mod tidy

* Remove double k8serrors from git

* Run go mod tidy

* Remove errant changelog entry for upgrade

* Fix some issues from the Git rebase

* Show a diff using a simple YAML differ

* Fix a val to chartValues misnaming from Git

* Add the diffing dependency

* Clean up comments and move some non-validating behavior out of the validation method

* Start writing my own diff

* `go mod tidy` 2k22

* Add several diff testing scenarios

* Handle diffs using YAML parsing

* Get diff working with more complex maps

* Move FetchChartValues so that public functions are at the top of the chart file

* Handle error on invalid timeout flag

* Undo that weird indent change.

* Catch the diff error

* Modify chart fixtures to be loadable

* Test loading charts

* Check the error returned from diffRecursively.

* Update changelog

* Update cli/cmd/common/chart.go

Co-authored-by: Nitya Dhanushkodi <[email protected]>

* TestReadChartFiles checks contents of files

* Test uppercase and underscores

* Test removal scenarios

* Fix formatting

* Update cli/cmd/common/diff.go

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

* Change aMapSlice to aMapWithKey

* `go mod tidy`

* Replace "reflection" with better code

* Remove double printing config

* Return string, err from diffRecursively

* Move common package up to cli root

* Move chart to helm package

* Fix error string on install

* Delete the binary I added accidentally

* Move diff to helm pkg

* Match the update to /x/sys

* Move diffing code back to common

* Fix indentation on the diff and add a header explaining

* Remove extra name/namespace print

* Remove extra dry-run printing

* Fix comment on WithDiffUnchangedStyle

* Remove unused constants

* Add a test case for valid label with leading numbers

* Add test for MergeMaps

* Reassure users with a dry run header

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

CHANGELOG.md

@@ -11,6 +11,8 @@ IMPROVEMENTS:
   * Support `ui.dashboardURLTemplates.service` value for setting [dashboard URL templates](https://www.consul.io/docs/agent/options#ui_config_dashboard_url_templates_service). [[GH-937](https://github.com/hashicorp/consul-k8s/pull/937)]
   * Allow using dash-separated names for config entries when using `kubectl`. [[GH-965](https://github.com/hashicorp/consul-k8s/pull/965)]
   * Support Pod Security Policies with Vault integration. [[GH-985](https://github.com/hashicorp/consul-k8s/pull/985)]
+* CLI
+  * Show a diff when upgrading a Consul installation on Kubernetes [[GH-934](https://github.com/hashicorp/consul-k8s/pull/934)]
 * Control Plane
   * Support the value `$POD_NAME` for the annotation `consul.hashicorp.com/service-meta-*` that will now be interpolated and set to the pod's name in the service's metadata. [[GH-982](https://github.com/hashicorp/consul-k8s/pull/982)]
   * Allow managing Consul sidecar resources via annotations. [[GH-956](https://github.com/hashicorp/consul-k8s/pull/956)]

cli/cmd/install/install.go

@@ -9,10 +9,11 @@ import (
 	"time"
 
 	consulChart "github.com/hashicorp/consul-k8s/charts"
-	"github.com/hashicorp/consul-k8s/cli/cmd/common"
-	"github.com/hashicorp/consul-k8s/cli/cmd/common/flag"
-	"github.com/hashicorp/consul-k8s/cli/cmd/common/terminal"
+	"github.com/hashicorp/consul-k8s/cli/common"
+	"github.com/hashicorp/consul-k8s/cli/common/flag"
+	"github.com/hashicorp/consul-k8s/cli/common/terminal"
 	"github.com/hashicorp/consul-k8s/cli/config"
+	"github.com/hashicorp/consul-k8s/cli/helm"
 	"helm.sh/helm/v3/pkg/action"
 	"helm.sh/helm/v3/pkg/chart/loader"
 	helmCLI "helm.sh/helm/v3/pkg/cli"
@@ -313,7 +314,7 @@ func (c *Command) Run(args []string) int {
 	// Without informing the user, default global.name to consul if it hasn't been set already. We don't allow setting
 	// the release name, and since that is hardcoded to "consul", setting global.name to "consul" makes it so resources
 	// aren't double prefixed with "consul-consul-...".
-	vals = MergeMaps(config.Convert(config.GlobalNameConsul), vals)
+	vals = common.MergeMaps(config.Convert(config.GlobalNameConsul), vals)
 
 	// Dry Run should exit here, no need to actual locate/download the charts.
 	if c.flagDryRun {
@@ -342,7 +343,7 @@ func (c *Command) Run(args []string) int {
 
 	// Setup action configuration for Helm Go SDK function calls.
 	actionConfig := new(action.Configuration)
-	actionConfig, err = common.InitActionConfig(actionConfig, c.flagNamespace, settings, uiLogger)
+	actionConfig, err = helm.InitActionConfig(actionConfig, c.flagNamespace, settings, uiLogger)
 	if err != nil {
 		c.UI.Output(err.Error(), terminal.WithErrorStyle())
 		return 1
@@ -357,7 +358,7 @@ func (c *Command) Run(args []string) int {
 	install.Timeout = c.timeoutDuration
 
 	// Read the embedded chart files into []*loader.BufferedFile.
-	chartFiles, err := common.ReadChartFiles(consulChart.ConsulHelmChart, common.TopLevelChartDirName)
+	chartFiles, err := helm.ReadChartFiles(consulChart.ConsulHelmChart, common.TopLevelChartDirName)
 	if err != nil {
 		c.UI.Output(err.Error(), terminal.WithErrorStyle())
 		return 1
@@ -455,32 +456,11 @@ func (c *Command) mergeValuesFlagsWithPrecedence(settings *helmCLI.EnvSettings)
 	if c.flagPreset != defaultPreset {
 		// Note the ordering of the function call, presets have lower precedence than set vals.
 		presetMap := config.Presets[c.flagPreset].(map[string]interface{})
-		vals = MergeMaps(presetMap, vals)
+		vals = common.MergeMaps(presetMap, vals)
 	}
 	return vals, err
 }
 
-// MergeMaps is a helper function used in Run. Merges two maps giving b precedent.
-// @source: https://github.com/helm/helm/blob/main/pkg/cli/values/options.go
-func MergeMaps(a, b map[string]interface{}) map[string]interface{} {
-	out := make(map[string]interface{}, len(a))
-	for k, v := range a {
-		out[k] = v
-	}
-	for k, v := range b {
-		if v, ok := v.(map[string]interface{}); ok {
-			if bv, ok := out[k]; ok {
-				if bv, ok := bv.(map[string]interface{}); ok {
-					out[k] = MergeMaps(bv, v)
-					continue
-				}
-			}
-		}
-		out[k] = v
-	}
-	return out
-}
-
 // validateFlags is a helper function that performs sanity checks on the user's provided flags.
 func (c *Command) validateFlags(args []string) error {
 	if err := c.set.Parse(args); err != nil {
@@ -490,14 +470,14 @@ func (c *Command) validateFlags(args []string) error {
 		return errors.New("should have no non-flag arguments")
 	}
 	if len(c.flagValueFiles) != 0 && c.flagPreset != defaultPreset {
-		return fmt.Errorf("Cannot set both -%s and -%s", flagNameConfigFile, flagNamePreset)
+		return fmt.Errorf("cannot set both -%s and -%s", flagNameConfigFile, flagNamePreset)
 	}
 	if _, ok := config.Presets[c.flagPreset]; c.flagPreset != defaultPreset && !ok {
 		return fmt.Errorf("'%s' is not a valid preset", c.flagPreset)
 	}
-	if !validLabel(c.flagNamespace) {
+	if !common.IsValidLabel(c.flagNamespace) {
 		return fmt.Errorf("'%s' is an invalid namespace. Namespaces follow the RFC 1123 label convention and must "+
-			"consist of a lower case alphanumeric character or '-' and must start/end with an alphanumeric", c.flagNamespace)
+			"consist of a lower case alphanumeric character or '-' and must start/end with an alphanumeric character", c.flagNamespace)
 	}
 	duration, err := time.ParseDuration(c.flagTimeout)
 	if err != nil {
@@ -507,7 +487,7 @@ func (c *Command) validateFlags(args []string) error {
 	if len(c.flagValueFiles) != 0 {
 		for _, filename := range c.flagValueFiles {
 			if _, err := os.Stat(filename); err != nil && os.IsNotExist(err) {
-				return fmt.Errorf("File '%s' does not exist.", filename)
+				return fmt.Errorf("file '%s' does not exist", filename)
 			}
 		}
 	}
@@ -518,21 +498,6 @@ func (c *Command) validateFlags(args []string) error {
 	return nil
 }
 
-// validLabel is a helper function that checks if a string follows RFC 1123 labels.
-func validLabel(s string) bool {
-	for i, c := range s {
-		alphanum := ('a' <= c && c <= 'z') || ('0' <= c && c <= '9')
-		// If the character is not the last or first, it can be a dash.
-		if i != 0 && i != (len(s)-1) {
-			alphanum = alphanum || (c == '-')
-		}
-		if !alphanum {
-			return false
-		}
-	}
-	return true
-}
-
 // checkValidEnterprise checks and validates an enterprise installation.
 // When an enterprise license secret is provided, check that the secret exists
 // in the "consul" namespace.

cli/cmd/install/install_test.go

@@ -5,7 +5,7 @@ import (
 	"os"
 	"testing"
 
-	"github.com/hashicorp/consul-k8s/cli/cmd/common"
+	"github.com/hashicorp/consul-k8s/cli/common"
 	"github.com/hashicorp/go-hclog"
 	"github.com/stretchr/testify/require"
 	v1 "k8s.io/api/core/v1"
@@ -121,54 +121,6 @@ func TestValidateFlags(t *testing.T) {
 	}
 }
 
-// TestValidLabel calls validLabel() which checks strings match RFC 1123 label convention.
-func TestValidLabel(t *testing.T) {
-	testCases := []struct {
-		description string
-		input       string
-		expected    bool
-	}{
-		{
-			"Standard name with leading numbers works.",
-			"1234-abc",
-			true,
-		},
-		{
-			"All lower case letters works.",
-			"peppertrout",
-			true,
-		},
-		{
-			"Test that dashes in the middle are allowed.",
-			"pepper-trout",
-			true,
-		},
-		{
-			"Capitals violate RFC 1123 lower case label.",
-			"Peppertrout",
-			false,
-		},
-		{
-			"Underscores are not permitted anywhere.",
-			"ab_cd",
-			false,
-		},
-		{
-			"The dash must be in the middle of the word, not on the start/end character.",
-			"peppertrout-",
-			false,
-		},
-	}
-
-	for _, testCase := range testCases {
-		t.Run(testCase.description, func(t *testing.T) {
-			if result := validLabel(testCase.input); result != testCase.expected {
-				t.Errorf("Incorrect output, got %v and expected %v", result, testCase.expected)
-			}
-		})
-	}
-}
-
 // getInitializedCommand sets up a command struct for tests.
 func getInitializedCommand(t *testing.T) *Command {
 	t.Helper()