Repository: openservicemesh/osm (go)

Issue:
Upgrade should correctly handle fields removed from charts in the new version
<!--

This issue tracker is a best-effort forum for users and customers to report bugs.

Be sure to not include any sensitive information. Sensitive information should __NOT__ be included in this issue.

-->

**Bug description**:
Currently, an upgrade from version X -> Y breaks if the Helm chart in version Y removed a field that was previously referenced in X. A field used X may be removed after being marked as deprecated, and when the field is no longer required in version Y. Due to how we perform upgrades, this breaks because the removed field is no longer supported in Y, and the schema validation fails in Y.

**Affected area (please mark with X where applicable)**:
- Upgrade [ ]

**Expected behavior**:
Upgrade should work when a field is removed from the new version's Helm chart.


Pull request:
fix(upgrade): handle removed values

<!--

Please describe the motivation for this PR and provide enough
information so that others can review it.

-->
**Description**:
Currently, upgrading OSM with `osm mesh upgrade` fails when the newer
chart version removes or renames values from the installed chart because
values.schema.json defines `"additionalProperties": false` on each
object and `osm mesh upgrade` uses all the values from the installed
chart.

Now, the `osm mesh upgrade` command will not use any values from the old
release. Users can specify them with the new `--set` flag.

Fixes #4330

<!--

Please describe how this change was tested. You could include supporting information
such as logs, snippets, and screenshots.

-->
**Testing done**: Manually ran affected tests and other upgrade scenarios based on the existing e2e test.

<!--

Please mark with X for applicable areas.

-->
**Affected area**:
| Functional Area            |     |
| -------------------------- | --- |
| New Functionality          | [ ] |
| CI System                  | [X] |
| CLI Tool                   | [X] |
| Certificate Management     | [ ] |
| Control Plane              | [ ] |
| Demo                       | [ ] |
| Documentation              | [ ] |
| Egress                     | [ ] |
| Ingress                    | [ ] |
| Install                    | [ ] |
| Networking                 | [ ] |
| Observability              | [ ] |
| Performance                | [ ] |
| SMI Policy                 | [ ] |
| Security                   | [ ] |
| Sidecar Injection          | [ ] |
| Tests                      | [ ] |
| Upgrade                    | [X] |
| Other                      | [ ] |


Please answer the following questions with yes/no.

1. Does this change contain code from or inspired by another project? No
    -   Did you notify the maintainers and provide attribution?

2. Is this a breaking change? Yes, release notes have been updated.


Patch:
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 28e9d7ad..79ff7df1 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -248,7 +248,7 @@ jobs:
         if: ${{ github.event_name == 'pull_request' }}
         env:
           K8S_NAMESPACE: "osm-system"
-        run: go test ./tests/e2e -test.v -ginkgo.v -ginkgo.progress -installType=KindCluster -test.timeout 0 -test.failfast -ginkgo.failFast -ginkgo.focus='\[Bucket ${{ matrix.bucket }}\]' -ginkgo.skip='Upgrade'
+        run: go test ./tests/e2e -test.v -ginkgo.v -ginkgo.progress -installType=KindCluster -test.timeout 0 -test.failfast -ginkgo.failFast -ginkgo.focus='\[Bucket ${{ matrix.bucket }}\]'
         continue-on-error: true
       - name: Upload PR test logs
         if: ${{ steps.pr_test.conclusion != 'skipped' }}
@@ -269,7 +269,7 @@ jobs:
         if: ${{ github.event_name == 'push' }}
         env:
           K8S_NAMESPACE: "osm-system"
-        run: go test ./tests/e2e -test.v -ginkgo.v -ginkgo.progress -installType=KindCluster -test.timeout 0 -test.failfast -ginkgo.failFast -ginkgo.focus='\[Bucket ${{ matrix.bucket }}\]' -ginkgo.skip='Upgrade'
+        run: go test ./tests/e2e -test.v -ginkgo.v -ginkgo.progress -installType=KindCluster -test.timeout 0 -test.failfast -ginkgo.failFast -ginkgo.focus='\[Bucket ${{ matrix.bucket }}\]'
         continue-on-error: true
       - name: Upload Push test logs
         if: ${{ steps.push_test.conclusion != 'skipped' }}
@@ -284,25 +284,6 @@ jobs:
         if: ${{ steps.push_test.conclusion != 'skipped' }}
         run: rm -rf /tmp/test*
 
-      # Upgrade tests
-      - name: Run Upgrade tests
-        id: upgrade_tests
-        env:
-          K8S_NAMESPACE: "osm-system"
-        run: go test ./tests/e2e -test.v -ginkgo.v -ginkgo.progress -installType=KindCluster -test.timeout 0 -test.failfast -ginkgo.failFast -ginkgo.focus='\[Bucket ${{ matrix.bucket }}\].*Upgrade'
-        continue-on-error: true
-      - name: Upload Upgrade test logs
-        uses: actions/upload-artifact@v2
-        with:
-          name: upgrade_test_logs_bucket_${{ matrix.bucket }}
-          path: /tmp/test**/*
-      - name: Check continue Upgrade tests
-        # Only fail CI when upgrade fails if we're running on a release branch. The main branch may introduce breaking changes at any time.
-        if: ${{ steps.upgrade_tests.outcome == 'failure' && !(!(github.event_name == 'push' && startsWith(github.ref, 'refs/heads/release-') || github.event_name == 'pull_request' && startsWith(github.base_ref, 'refs/heads/release-'))) }}
-        run: exit 1
-      - name: Clean Upgrade test logs
-        run: rm -rf /tmp/test*
-
   integration-tresor:
     name: Integration Test with Tresor, SMI traffic policies, and egress disabled
     runs-on: ubuntu-latest
diff --git a/cmd/cli/mesh_upgrade.go b/cmd/cli/mesh_upgrade.go
index 386bf230..a69f744a 100644
--- a/cmd/cli/mesh_upgrade.go
+++ b/cmd/cli/mesh_upgrade.go
@@ -6,16 +6,12 @@ import (
 	"io"
 	"time"
 
+	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
 	helm "helm.sh/helm/v3/pkg/action"
 	"helm.sh/helm/v3/pkg/chart"
 	"helm.sh/helm/v3/pkg/chart/loader"
-	"helm.sh/helm/v3/pkg/chartutil"
-)
-
-const (
-	defaultContainerRegistry = "openservicemesh"
-	defaultOsmImageTag       = "latest-main"
+	"helm.sh/helm/v3/pkg/strvals"
 )
 
 const upgradeDesc = `
@@ -26,10 +22,9 @@ The mesh to upgrade is identified by its mesh name and namespace. If either were
 overridden from the default for the "osm install" command, the --mesh-name and
 --osm-namespace flags need to be specified.
 
-Values from the current Helm release will be carried over to the new release
-with the exception of osm.image.registry (--container-registry) and
-osm.image.tag (--osm-image-tag), which will be overridden from the
-old release by default.
+Values from the current Helm release will NOT be carried over to the new
+release. Use --set to pass any overridden values from the old release to the new
+release.
 
 Note: edits to resources NOT made by Helm or the OSM CLI may not persist after
 "osm mesh upgrade" is run.
@@ -58,8 +53,7 @@ type meshUpgradeCmd struct {
 	meshName string
 	chart    *chart.Chart
 
-	containerRegistry string
-	osmImageTag       string
+	setOptions []string
 }
 
 func newMeshUpgradeCmd(config *helm.Configuration, out io.Writer) *cobra.Command {
@@ -73,7 +67,7 @@ func newMeshUpgradeCmd(config *helm.Configuration, out io.Writer) *cobra.Command
 		Short:   "upgrade osm control plane",
 		Long:    upgradeDesc,
 		Example: meshUpgradeExample,
-		RunE: func(cmd *cobra.Command, _ []string) error {
+		RunE: func(_ *cobra.Command, _ []string) error {
 			if chartPath != "" {
 				var err error
 				upg.chart, err = loader.Load(chartPath)
@@ -90,8 +84,7 @@ func newMeshUpgradeCmd(config *helm.Configuration, out io.Writer) *cobra.Command
 
 	f.StringVar(&upg.meshName, "mesh-name", defaultMeshName, "Name of the mesh to upgrade")
 	f.StringVar(&chartPath, "osm-chart-path", "", "path to osm chart to override default chart")
-	f.StringVar(&upg.containerRegistry, "container-registry", defaultContainerRegistry, "container registry that hosts control plane component images")
-	f.StringVar(&upg.osmImageTag, "osm-image-tag", defaultOsmImageTag, "osm image tag")
+	f.StringArrayVar(&upg.setOptions, "set", nil, "Set arbitrary chart values (can specify multiple or separate values with commas: key1=val1,key2=val2)")
 
 	return cmd
 }
@@ -106,7 +99,7 @@ func (u *meshUpgradeCmd) run(config *helm.Configuration) error {
 	}
 
 	// Add the overlay values to be updated to the current release's values map
-	values, err := u.resolveValues(config)
+	values, err := u.resolveValues()
 	if err != nil {
 		return err
 	}
@@ -123,27 +116,12 @@ func (u *meshUpgradeCmd) run(config *helm.Configuration) error {
 	return nil
 }
 
-func (u *meshUpgradeCmd) resolveValues(config *helm.Configuration) (map[string]interface{}, error) {
-	vals := map[string]interface{}{
-		"image": map[string]interface{}{
-			"tag":      u.osmImageTag,
-			"registry": u.containerRegistry,
-		},
-	}
-
-	vals = map[string]interface{}{
-		"osm": vals,
-	}
-
-	oldRelease, err := config.Releases.Deployed(u.meshName)
-	if err != nil {
-		return nil, err
+func (u *meshUpgradeCmd) resolveValues() (map[string]interface{}, error) {
+	vals := make(map[string]interface{})
+	for _, val := range u.setOptions {
+		if err := strvals.ParseInto(val, vals); err != nil {
+			return nil, errors.Wrap(err, "invalid format for --set")
+		}
 	}
-
-	// The final merged values from the previous release
-	oldVals := chartutil.CoalesceTables(oldRelease.Config, oldRelease.Chart.Values)
-
-	vals = chartutil.CoalesceTables(vals, oldVals)
-
 	return vals, nil
 }
diff --git a/docs/release_guide.md b/docs/release_guide.md
index 71305638..4816681a 100644
--- a/docs/release_guide.md
+++ b/docs/release_guide.md
@@ -85,6 +85,7 @@ Create a new commit on the patch branch to update the hardcoded version informat
 * The chart and app version in [charts/osm/Chart.yaml](/charts/osm/Chart.yaml) to the release version.
 * The Helm chart [README.md](/charts/osm/README.md)
   - Necessary changes should be made automatically by running `make chart-readme`
+* If this the first release on a new release branch, update the [upgrade e2e test](/tests/e2e/e2e_upgrade_test.go) install version from `i.Version = ">0.0.0-0"` to the previous minor release. e.g. When cutting the release-v1.1 branch, this should be updated to `"1.0"`.
 
 Once patches and version information have been updated on the patch branch off of the release branch, create a pull request from the patch branch to the release branch. When creating your pull request, generate the release checklist for the description by adding the following to the PR URL: `?expand=1&template=release_pull_request_template.md`. Alternatively, copy the raw template from [release_pull_request_template.md](/.github/PULL_REQUEST_TEMPLATE/release_pull_request_template.md).
 
diff --git a/docs/release_notes.md b/docs/release_notes.md
index d7775224..42af6610 100644
--- a/docs/release_notes.md
+++ b/docs/release_notes.md
@@ -15,6 +15,7 @@
 The following changes are not backward compatible with the previous release.
 
 - Top level Helm chart keys are renamed from `OpenServiceMesh` to `osm`
+- `osm mesh upgrade` no longer carries over values from previous releases. Use the `--set` flag on `osm mesh upgrade` to pass values as needed. The `--container-registry` and `--osm-image-tag` flags have also been removed in favor of `--set`.
 
 ### Deprecation notes
 


Test patch:
diff --git a/cmd/cli/mesh_upgrade_test.go b/cmd/cli/mesh_upgrade_test.go
index 30e27468..0cba13a8 100644
--- a/cmd/cli/mesh_upgrade_test.go
+++ b/cmd/cli/mesh_upgrade_test.go
@@ -36,11 +36,9 @@ func defaultMeshUpgradeCmd() *meshUpgradeCmd {
 	}
 
 	return &meshUpgradeCmd{
-		out:               ioutil.Discard,
-		meshName:          defaultMeshName,
-		chart:             chart,
-		containerRegistry: defaultContainerRegistry,
-		osmImageTag:       defaultOsmImageTag,
+		out:      ioutil.Discard,
+		meshName: defaultMeshName,
+		chart:    chart,
 	}
 }
 
@@ -56,10 +54,12 @@ func TestMeshUpgradeDefault(t *testing.T) {
 
 	u := defaultMeshUpgradeCmd()
 
-	upgraded, err := action.NewGet(config).Run(u.meshName)
+	getVals := action.NewGetValues(config)
+	getVals.AllValues = true
+	upgraded, err := getVals.Run(u.meshName)
 	a.Nil(err)
 
-	meshName, err := chartutil.Values(upgraded.Config).PathValue("osm.meshName")
+	meshName, err := chartutil.Values(upgraded).PathValue("osm.meshName")
 	a.Nil(err)
 	a.Equal(defaultMeshName, meshName)
 	err = u.run(config)
@@ -78,32 +78,38 @@ func TestMeshUpgradeOverridesInstallDefaults(t *testing.T) {
 	a.Nil(err)
 
 	u := defaultMeshUpgradeCmd()
-	u.osmImageTag = "upgraded"
+	defaultImageRegVal, err := chartutil.Values(u.chart.Values).PathValue("osm.image.registry")
+	a.NoError(err)
+	defaultImageReg := defaultImageRegVal.(string)
+	upgradedImageReg := "upgraded-" + defaultImageReg
+	u.setOptions = []string{"osm.image.registry=" + upgradedImageReg}
 
 	err = u.run(config)
 	a.Nil(err)
 
-	upgraded, err := action.NewGet(config).Run(u.meshName)
+	getVals := action.NewGetValues(config)
+	getVals.AllValues = true
+	upgraded, err := getVals.Run(u.meshName)
 	a.Nil(err)
 
-	osmImageTag, err := chartutil.Values(upgraded.Config).PathValue("osm.image.tag")
+	osmImageReg, err := chartutil.Values(upgraded).PathValue("osm.image.registry")
 	a.Nil(err)
-	a.Equal("upgraded", osmImageTag)
+	a.Equal(upgradedImageReg, osmImageReg)
 
 	// Successive upgrades overriddes image-tag values from the previous upgrade
 	u = defaultMeshUpgradeCmd()
 	err = u.run(config)
 	a.Nil(err)
 
-	upgraded, err = action.NewGet(config).Run(u.meshName)
+	upgraded, err = getVals.Run(u.meshName)
 	a.Nil(err)
 
-	osmImageTag, err = chartutil.Values(upgraded.Config).PathValue("osm.image.tag")
+	osmImageReg, err = chartutil.Values(upgraded).PathValue("osm.image.registry")
 	a.Nil(err)
-	a.Equal(defaultOsmImageTag, osmImageTag)
+	a.Equal(defaultImageReg, osmImageReg)
 }
 
-func TestMeshUpgradeKeepsInstallOverrides(t *testing.T) {
+func TestMeshUpgradeDropsInstallOverrides(t *testing.T) {
 	a := assert.New(t)
 
 	config := meshUpgradeConfig()
@@ -112,7 +118,7 @@ func TestMeshUpgradeKeepsInstallOverrides(t *testing.T) {
 	i.chartPath = testChartPath
 	i.setOptions = []string{
 		"osm.enableEgress=true",
-		"osm.osmImageTag=installed",
+		"osm.image.registry=installed",
 		"osm.envoyLogLevel=trace",
 	}
 
@@ -124,23 +130,19 @@ func TestMeshUpgradeKeepsInstallOverrides(t *testing.T) {
 	err = u.run(config)
 	a.Nil(err)
 
-	upgraded, err := action.NewGet(config).Run(u.meshName)
+	getVals := action.NewGetValues(config)
+	getVals.AllValues = true
+	upgraded, err := getVals.Run(u.meshName)
 	a.Nil(err)
 
-	// enableEgress should be unchanged by default
-	egressEnabled, err := chartutil.Values(upgraded.Config).PathValue("osm.enableEgress")
-	a.Nil(err)
-	a.Equal(true, egressEnabled)
-
-	// envoyLogLevel should be unchanged by default
-	envoyLogLevel, err := chartutil.Values(upgraded.Config).PathValue("osm.envoyLogLevel")
-	a.Nil(err)
-	a.Equal("trace", envoyLogLevel)
-
-	// image tag should be updated by default
-	tag, err := chartutil.Values(upgraded.Config).PathValue("osm.image.tag")
-	a.Nil(err)
-	a.Equal(defaultOsmImageTag, tag)
+	// Values overridden at install should be the same as their defaults in the
+	// chart after an upgrade that sets no values
+	for _, valKey := range []string{"osm.enableEgress", "osm.image.registry", "osm.envoyLogLevel"} {
+		def, defErr := chartutil.Values(u.chart.Values).PathValue(valKey)
+		upgradedVal, upgErr := chartutil.Values(upgraded).PathValue(valKey)
+		a.Equal(def, upgradedVal)
+		a.Equal(defErr, upgErr)
+	}
 }
 
 func TestMeshUpgradeToModifiedChart(t *testing.T) {
@@ -172,11 +174,36 @@ func TestMeshUpgradeToModifiedChart(t *testing.T) {
 	err = u.run(config)
 	a.Nil(err)
 
-	upgraded, err := action.NewGet(config).Run(u.meshName)
+	getVals := action.NewGetValues(config)
+	getVals.AllValues = true
+	upgraded, err := getVals.Run(u.meshName)
 	a.Nil(err)
 
-	// When a default is changed in values.yaml, keep the old one
-	namespace, err := chartutil.Values(upgraded.Config).PathValue("osm.namespace")
+	// When a default is changed in values.yaml, use the new one
+	namespace, err := chartutil.Values(upgraded).PathValue("osm.namespace")
 	a.Nil(err)
-	a.Equal(oldNamespace, namespace)
+	a.Equal(newNamespace, namespace)
+}
+
+func TestMeshUpgradeRemovedValue(t *testing.T) {
+	a := assert.New(t)
+
+	config := meshUpgradeConfig()
+
+	i := getDefaultInstallCmd(ioutil.Discard)
+	i.chartPath = testChartPath
+	err := i.run(config)
+	a.NoError(err)
+
+	u := defaultMeshUpgradeCmd()
+
+	// Upgrade to a "new version" of the chart with a deleted value.
+	_, err = chartutil.Values(u.chart.Values).PathValue("osm.namespace")
+	a.NoError(err)
+	delete(u.chart.Values["osm"].(map[string]interface{}), "namespace")
+	// Schema only accepting the remaining values
+	u.chart.Schema = []byte(`{"properties": {"osm": {"properties": {"image": {}, "imagePullSecrets": {}}, "additionalProperties": false}}}`)
+
+	err = u.run(config)
+	a.NoError(err)
 }
diff --git a/tests/e2e/e2e_upgrade_test.go b/tests/e2e/e2e_upgrade_test.go
index edb09e19..6085f05e 100644
--- a/tests/e2e/e2e_upgrade_test.go
+++ b/tests/e2e/e2e_upgrade_test.go
@@ -1,10 +1,10 @@
 package e2e
 
 import (
-	"context"
 	"fmt"
 	"os/exec"
 	"path/filepath"
+	"strconv"
 	"time"
 
 	. "github.com/onsi/ginkgo"
@@ -12,8 +12,6 @@ import (
 	"helm.sh/helm/v3/pkg/action"
 	"helm.sh/helm/v3/pkg/chart/loader"
 	"helm.sh/helm/v3/pkg/cli"
-	corev1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	. "github.com/openservicemesh/osm/tests/framework"
 )
@@ -45,21 +43,19 @@ var _ = OSMDescribe("Upgrade from latest",
 			const releaseName = "osm"
 			i := action.NewInstall(helmCfg)
 
-			// Latest version excluding pre-releases used by default. Using the
-			// latest assumes we aren't maintaining multiple release branches
-			// at once. e.g. if a patch is cut for both v0.5.0 and v0.6.0, we
-			// wouldn't want to test "upgrading" backwards from v0.6.0 to
-			// v0.5.1.
 			i.ChartPathOptions.RepoURL = "https://openservicemesh.github.io/osm"
-			i.Version = ">0.0.0-0" // Include pre-releases
+			// On the main branch, this should refer to the latest release. On
+			// release branches, it should specify the most recent patch of the
+			// previous minor release. e.g. on the release-v1.0 branch, this
+			// should be "0.11".
+			i.Version = ">0.0.0-0"
 			i.Namespace = Td.OsmNamespace
 			i.Wait = true
 			i.ReleaseName = releaseName
 			i.Timeout = 120 * time.Second
 			vals := map[string]interface{}{
-				"osm": map[string]interface{}{
+				"OpenServiceMesh": map[string]interface{}{
 					"deployPrometheus": true,
-					"deployJaeger":     false,
 					// Init container must be privileged if an OpenShift cluster is being used
 					"enablePrivilegedInitContainer": Td.DeployOnOpenShift,
 
@@ -208,7 +204,8 @@ var _ = OSMDescribe("Upgrade from latest",
 				Expect(Td.LoadOSMImagesIntoKind()).To(Succeed())
 			}
 
-			stdout, stderr, err := Td.RunLocal(filepath.FromSlash("../../bin/osm"), "mesh", "upgrade", "--osm-namespace="+Td.OsmNamespace, "--container-registry="+Td.CtrRegistryServer, "--osm-image-tag="+Td.OsmImageTag)
+			setArgs := "--set=osm.image.tag=" + Td.OsmImageTag + ",osm.image.registry=" + Td.CtrRegistryServer + ",osm.deployPrometheus=true,osm.enablePrivilegedInitContainer=" + strconv.FormatBool(Td.DeployOnOpenShift) + ",osm.osmController.resource.requests.cpu=0.3,osm.injector.resource.requests.cpu=0.1,osm.prometheus.resources.requests.cpu=0.1,osm.prometheus.resources.requests.memory=256M"
+			stdout, stderr, err := Td.RunLocal(filepath.FromSlash("../../bin/osm"), "mesh", "upgrade", "--osm-namespace="+Td.OsmNamespace, setArgs)
 			Td.T.Log(stdout.String())
 			if err != nil {
 				Td.T.Log("stderr:\n" + stderr.String())
@@ -225,15 +222,6 @@ var _ = OSMDescribe("Upgrade from latest",
 			}
 			Expect(err).NotTo(HaveOccurred())
 
-			By("Recreating client and server pods")
-			for _, pod := range []corev1.Pod{srcPodDef, serverPodDef} {
-				err = Td.Client.CoreV1().Pods(ns).Delete(context.Background(), pod.Name, metav1.DeleteOptions{})
-				Expect(err).NotTo(HaveOccurred())
-				_, err = Td.CreatePod(ns, pod)
-				Expect(err).NotTo(HaveOccurred())
-			}
-			Expect(Td.WaitForPodsRunningReady(ns, 90*time.Second, 2, nil)).To(Succeed())
-
 			checkClientToServerOK()
 			checkProxiesConnected()
 		})