cli: check chart versions against target version in users config before upgrading (#2319)

* Check chart versions against target in users config

Signed-off-by: Daniel Weiße <dw@edgeless.systems>

* Cleaner cli-config version support checking

Signed-off-by: Daniel Weiße <dw@edgeless.systems>

* Return InvalidUpgradeError

Signed-off-by: Daniel Weiße <dw@edgeless.systems>

---------

Signed-off-by: Daniel Weiße <dw@edgeless.systems>

cli/internal/helm/BUILD.bazel

@@ -454,6 +454,7 @@ go_library(
 go_test(
     name = "helm_test",
     srcs = [
+        "actionfactory_test.go",
         "helm_test.go",
         "loader_test.go",
         "retryaction_test.go",
@@ -478,6 +479,7 @@ go_test(
         "@com_github_stretchr_testify//mock",
         "@com_github_stretchr_testify//require",
         "@sh_helm_helm_v3//pkg/action",
+        "@sh_helm_helm_v3//pkg/chart",
         "@sh_helm_helm_v3//pkg/chartutil",
         "@sh_helm_helm_v3//pkg/engine",
     ],

cli/internal/helm/actionfactory.go

@@ -29,7 +29,6 @@ type actionFactory struct {
 	versionLister releaseVersionLister
 	cfg           *action.Configuration
 	kubeClient    crdClient
-	cliVersion    semver.Semver
 	log           debugLog
 }
 
@@ -38,9 +37,8 @@ type crdClient interface {
 }
 
 // newActionFactory creates a new action factory for managing helm releases.
-func newActionFactory(kubeClient crdClient, lister releaseVersionLister, actionConfig *action.Configuration, cliVersion semver.Semver, log debugLog) *actionFactory {
+func newActionFactory(kubeClient crdClient, lister releaseVersionLister, actionConfig *action.Configuration, log debugLog) *actionFactory {
 	return &actionFactory{
-		cliVersion:    cliVersion,
 		versionLister: lister,
 		cfg:           actionConfig,
 		kubeClient:    kubeClient,
@@ -49,10 +47,10 @@ func newActionFactory(kubeClient crdClient, lister releaseVersionLister, actionC
 }
 
 // GetActions returns a list of actions to apply the given releases.
-func (a actionFactory) GetActions(releases []Release, force, allowDestructive bool) (actions []applyAction, includesUpgrade bool, err error) {
+func (a actionFactory) GetActions(releases []Release, configTargetVersion semver.Semver, force, allowDestructive bool) (actions []applyAction, includesUpgrade bool, err error) {
 	upgradeErrs := []error{}
 	for _, release := range releases {
-		err := a.appendNewAction(release, force, allowDestructive, &actions)
+		err := a.appendNewAction(release, configTargetVersion, force, allowDestructive, &actions)
 		var invalidUpgrade *compatibility.InvalidUpgradeError
 		if errors.As(err, &invalidUpgrade) {
 			upgradeErrs = append(upgradeErrs, err)
@@ -71,13 +69,24 @@ func (a actionFactory) GetActions(releases []Release, force, allowDestructive bo
 	return actions, includesUpgrade, errors.Join(upgradeErrs...)
 }
 
-func (a actionFactory) appendNewAction(release Release, force, allowDestructive bool, actions *[]applyAction) error {
+func (a actionFactory) appendNewAction(release Release, configTargetVersion semver.Semver, force, allowDestructive bool, actions *[]applyAction) error {
 	newVersion, err := semver.New(release.Chart.Metadata.Version)
 	if err != nil {
 		return fmt.Errorf("parsing chart version: %w", err)
 	}
+	cliSupportsConfigVersion := configTargetVersion.Compare(newVersion) != 0
+
 	currentVersion, err := a.versionLister.currentVersion(release.ReleaseName)
 	if errors.Is(err, errReleaseNotFound) {
+		// Don't install a new release if the user's config specifies a different version than the CLI offers.
+		if !force && isCLIVersionedRelease(release.ReleaseName) && cliSupportsConfigVersion {
+			return compatibility.NewInvalidUpgradeError(
+				currentVersion.String(),
+				configTargetVersion.String(),
+				fmt.Errorf("this CLI only supports installing microservice version %s", newVersion),
+			)
+		}
+
 		a.log.Debugf("Release %s not found, adding to new releases...", release.ReleaseName)
 		*actions = append(*actions, a.newInstall(release))
 		return nil
@@ -88,18 +97,31 @@ func (a actionFactory) appendNewAction(release Release, force, allowDestructive
 	a.log.Debugf("Current %s version: %s", release.ReleaseName, currentVersion)
 	a.log.Debugf("New %s version: %s", release.ReleaseName, newVersion)
 
-	// This may break for cert-manager or cilium if we decide to upgrade more than one minor version at a time.
-	// Leaving it as is since it is not clear to me what kind of sanity check we could do.
 	if !force {
-		if err := newVersion.IsUpgradeTo(currentVersion); err != nil {
+		// For charts we package ourselves, the version is equal to the CLI version (charts are embedded in the binary).
-			return fmt.Errorf("invalid upgrade for %s: %w", release.ReleaseName, err)
+		// We need to make sure this matches with the version in a user's config, if an upgrade should be applied.
+		if isCLIVersionedRelease(release.ReleaseName) {
+			// If target version is not a valid upgrade, don't upgrade any charts.
+			if err := configTargetVersion.IsUpgradeTo(currentVersion); err != nil {
+				return fmt.Errorf("invalid upgrade for %s: %w", release.ReleaseName, err)
+			}
+			// Target version is newer than current version, so we should perform an upgrade.
+			// Now make sure the target version is equal to the the CLI version.
+			if cliSupportsConfigVersion {
+				return compatibility.NewInvalidUpgradeError(
+					currentVersion.String(),
+					configTargetVersion.String(),
+					fmt.Errorf("this CLI only supports upgrading to microservice version %s", newVersion),
+				)
+			}
+		} else {
+			// This may break for external chart dependencies if we decide to upgrade more than one minor version at a time.
+			if err := newVersion.IsUpgradeTo(currentVersion); err != nil {
+				return fmt.Errorf("invalid upgrade for %s: %w", release.ReleaseName, err)
+			}
 		}
 	}
 
-	// at this point we conclude that the release should be upgraded. check that this CLI supports the upgrade.
-	if isCLIVersionedRelease(release.ReleaseName) && a.cliVersion.Compare(newVersion) != 0 {
-		return fmt.Errorf("this CLI only supports microservice version %s for upgrading", a.cliVersion.String())
-	}
 	if !allowDestructive &&
 		release.ReleaseName == certManagerInfo.releaseName {
 		return ErrConfirmationMissing

cli/internal/helm/actionfactory_test.go

@@ -0,0 +1,265 @@
+/*
+Copyright (c) Edgeless Systems GmbH
+
+SPDX-License-Identifier: AGPL-3.0-only
+*/
+
+package helm
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/edgelesssys/constellation/v2/internal/compatibility"
+	"github.com/edgelesssys/constellation/v2/internal/logger"
+	"github.com/edgelesssys/constellation/v2/internal/semver"
+	"github.com/stretchr/testify/assert"
+	"helm.sh/helm/v3/pkg/action"
+	"helm.sh/helm/v3/pkg/chart"
+)
+
+func TestAppendNewAction(t *testing.T) {
+	assertUpgradeErr := func(assert *assert.Assertions, err error) {
+		var invalidUpgrade *compatibility.InvalidUpgradeError
+		assert.True(errors.As(err, &invalidUpgrade))
+	}
+
+	testCases := map[string]struct {
+		lister              stubLister
+		release             Release
+		configTargetVersion semver.Semver
+		force               bool
+		allowDestructive    bool
+		wantErr             bool
+		assertErr           func(*assert.Assertions, error)
+	}{
+		"upgrade release": {
+			lister: stubLister{version: semver.NewFromInt(1, 0, 0, "")},
+			release: Release{
+				ReleaseName: "test",
+				Chart: &chart.Chart{
+					Metadata: &chart.Metadata{
+						Version: "1.1.0",
+					},
+				},
+			},
+			configTargetVersion: semver.NewFromInt(1, 1, 0, ""),
+		},
+		"upgrade to same version": {
+			lister: stubLister{version: semver.NewFromInt(1, 0, 0, "")},
+			release: Release{
+				ReleaseName: "test",
+				Chart: &chart.Chart{
+					Metadata: &chart.Metadata{
+						Version: "1.0.0",
+					},
+				},
+			},
+			configTargetVersion: semver.NewFromInt(1, 1, 0, ""),
+			wantErr:             true,
+			assertErr:           assertUpgradeErr,
+		},
+		"upgrade to older version": {
+			lister: stubLister{version: semver.NewFromInt(1, 1, 0, "")},
+			release: Release{
+				ReleaseName: "test",
+				Chart: &chart.Chart{
+					Metadata: &chart.Metadata{
+						Version: "1.0.0",
+					},
+				},
+			},
+			configTargetVersion: semver.NewFromInt(1, 0, 0, ""),
+			wantErr:             true,
+			assertErr:           assertUpgradeErr,
+		},
+		"upgrade to older version can be forced": {
+			lister: stubLister{version: semver.NewFromInt(1, 1, 0, "")},
+			release: Release{
+				ReleaseName: "test",
+				Chart: &chart.Chart{
+					Metadata: &chart.Metadata{
+						Version: "1.0.0",
+					},
+				},
+			},
+			configTargetVersion: semver.NewFromInt(1, 0, 0, ""),
+			force:               true,
+		},
+		"non semver in chart metadata": {
+			lister: stubLister{version: semver.NewFromInt(1, 0, 0, "")},
+			release: Release{
+				ReleaseName: "test",
+				Chart: &chart.Chart{
+					Metadata: &chart.Metadata{
+						Version: "some-version",
+					},
+				},
+			},
+			wantErr: true,
+		},
+		"listing release fails": {
+			lister: stubLister{err: assert.AnError},
+			release: Release{
+				ReleaseName: "test",
+				Chart: &chart.Chart{
+					Metadata: &chart.Metadata{
+						Version: "1.1.0",
+					},
+				},
+			},
+			configTargetVersion: semver.NewFromInt(1, 1, 0, ""),
+			wantErr:             true,
+		},
+		"release not installed": {
+			lister: stubLister{err: errReleaseNotFound},
+			release: Release{
+				ReleaseName: "test",
+				Chart: &chart.Chart{
+					Metadata: &chart.Metadata{
+						Version: "1.1.0",
+					},
+				},
+			},
+			configTargetVersion: semver.NewFromInt(1, 1, 0, ""),
+		},
+		"destructive release upgrade requires confirmation": {
+			lister: stubLister{version: semver.NewFromInt(1, 0, 0, "")},
+			release: Release{
+				Chart: &chart.Chart{
+					Metadata: &chart.Metadata{
+						Version: "1.1.0",
+					},
+				},
+				ReleaseName: certManagerInfo.releaseName,
+			},
+			configTargetVersion: semver.NewFromInt(1, 1, 0, ""),
+			wantErr:             true,
+			assertErr: func(assert *assert.Assertions, err error) {
+				assert.ErrorIs(err, ErrConfirmationMissing)
+			},
+		},
+		"destructive release upgrade can be accepted": {
+			lister: stubLister{version: semver.NewFromInt(1, 0, 0, "")},
+			release: Release{
+				Chart: &chart.Chart{
+					Metadata: &chart.Metadata{
+						Version: "1.1.0",
+					},
+				},
+				ReleaseName: certManagerInfo.releaseName,
+			},
+			configTargetVersion: semver.NewFromInt(1, 1, 0, ""),
+			allowDestructive:    true,
+		},
+		"config version takes precedence over CLI version": {
+			lister: stubLister{version: semver.NewFromInt(1, 0, 0, "")},
+			release: Release{
+				ReleaseName: constellationServicesInfo.releaseName,
+				Chart: &chart.Chart{
+					Metadata: &chart.Metadata{
+						Version: "1.1.0",
+					},
+				},
+			},
+			configTargetVersion: semver.NewFromInt(1, 0, 0, ""),
+			wantErr:             true,
+			assertErr:           assertUpgradeErr,
+		},
+		"error if CLI version does not match config version on upgrade": {
+			lister: stubLister{version: semver.NewFromInt(1, 0, 0, "")},
+			release: Release{
+				ReleaseName: constellationServicesInfo.releaseName,
+				Chart: &chart.Chart{
+					Metadata: &chart.Metadata{
+						Version: "1.1.5",
+					},
+				},
+			},
+			configTargetVersion: semver.NewFromInt(1, 1, 0, ""),
+			wantErr:             true,
+			assertErr:           assertUpgradeErr,
+		},
+		"config version matches CLI version on upgrade": {
+			lister: stubLister{version: semver.NewFromInt(1, 0, 0, "")},
+			release: Release{
+				ReleaseName: constellationServicesInfo.releaseName,
+				Chart: &chart.Chart{
+					Metadata: &chart.Metadata{
+						Version: "1.1.5",
+					},
+				},
+			},
+			configTargetVersion: semver.NewFromInt(1, 1, 5, ""),
+		},
+		"config - CLI version mismatch can be forced through": {
+			lister: stubLister{version: semver.NewFromInt(1, 0, 0, "")},
+			release: Release{
+				ReleaseName: constellationServicesInfo.releaseName,
+				Chart: &chart.Chart{
+					Metadata: &chart.Metadata{
+						Version: "1.1.5",
+					},
+				},
+			},
+			configTargetVersion: semver.NewFromInt(1, 1, 0, ""),
+			force:               true,
+		},
+		"installing new release requires matching config and CLI version": {
+			lister: stubLister{err: errReleaseNotFound},
+			release: Release{
+				ReleaseName: constellationServicesInfo.releaseName,
+				Chart: &chart.Chart{
+					Metadata: &chart.Metadata{
+						Version: "1.1.0",
+					},
+				},
+			},
+			configTargetVersion: semver.NewFromInt(1, 0, 0, ""),
+			wantErr:             true,
+			assertErr:           assertUpgradeErr,
+		},
+		"config - CLI version mismatch for new releases can be forced through": {
+			lister: stubLister{err: errReleaseNotFound},
+			release: Release{
+				ReleaseName: constellationServicesInfo.releaseName,
+				Chart: &chart.Chart{
+					Metadata: &chart.Metadata{
+						Version: "1.1.0",
+					},
+				},
+			},
+			configTargetVersion: semver.NewFromInt(1, 0, 0, ""),
+			force:               true,
+		},
+	}
+
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			assert := assert.New(t)
+
+			actions := []applyAction{}
+			actionFactory := newActionFactory(nil, tc.lister, &action.Configuration{}, logger.NewTest(t))
+
+			err := actionFactory.appendNewAction(tc.release, tc.configTargetVersion, tc.force, tc.allowDestructive, &actions)
+			if tc.wantErr {
+				assert.Error(err)
+				if tc.assertErr != nil {
+					tc.assertErr(assert, err)
+				}
+				return
+			}
+			assert.NoError(err)
+			assert.Len(actions, 1) // no error == actions gets appended
+		})
+	}
+}
+
+type stubLister struct {
+	err     error
+	version semver.Semver
+}
+
+func (s stubLister) currentVersion(_ string) (semver.Semver, error) {
+	return s.version, s.err
+}

cli/internal/helm/helm.go

@@ -73,7 +73,7 @@ func NewClient(kubeConfigPath string, log debugLog) (*Client, error) {
 	}
 	lister := ReleaseVersionClient{actionConfig}
 	cliVersion := constants.BinaryVersion()
-	factory := newActionFactory(kubeClient, lister, actionConfig, cliVersion, log)
+	factory := newActionFactory(kubeClient, lister, actionConfig, log)
 	return &Client{factory, cliVersion, log}, nil
 }
 
@@ -96,7 +96,7 @@ func (h Client) PrepareApply(
 		return nil, false, fmt.Errorf("loading Helm releases: %w", err)
 	}
 	h.log.Debugf("Loaded Helm releases")
-	actions, includesUpgrades, err := h.factory.GetActions(releases, flags.Force, flags.AllowDestructive)
+	actions, includesUpgrades, err := h.factory.GetActions(releases, conf.MicroserviceVersion, flags.Force, flags.AllowDestructive)
 	return &ChartApplyExecutor{actions: actions, log: h.log}, includesUpgrades, err
 }
 

cli/internal/helm/helm_test.go

@@ -175,6 +175,7 @@ func TestHelmApply(t *testing.T) {
 
 	cfg := config.Default()
 	cfg.RemoveProviderAndAttestationExcept(csp)
+	cfg.MicroserviceVersion = cliVersion
 	log := logger.NewTest(t)
 	options := Options{
 		Conformance:      false,
@@ -184,9 +185,9 @@ func TestHelmApply(t *testing.T) {
 	}
 	for name, tc := range testCases {
 		t.Run(name, func(t *testing.T) {
-			lister := &ReleaseVersionStub{}
+			lister := &releaseVersionMock{}
 			sut := Client{
-				factory:    newActionFactory(nil, lister, &action.Configuration{}, cliVersion, log),
+				factory:    newActionFactory(nil, lister, &action.Configuration{}, log),
 				log:        log,
 				cliVersion: cliVersion,
 			}
@@ -244,7 +245,7 @@ func getActionReleaseNames(actions []applyAction) []string {
 	return releaseActionNames
 }
 
-func helmListVersion(l *ReleaseVersionStub, releaseName string, installedVersion string) {
+func helmListVersion(l *releaseVersionMock, releaseName string, installedVersion string) {
 	if installedVersion == "" {
 		l.On("currentVersion", releaseName).Return(semver.Semver{}, errReleaseNotFound)
 		return
@@ -253,11 +254,11 @@ func helmListVersion(l *ReleaseVersionStub, releaseName string, installedVersion
 	l.On("currentVersion", releaseName).Return(v, nil)
 }
 
-type ReleaseVersionStub struct {
+type releaseVersionMock struct {
 	mock.Mock
 }
 
-func (s *ReleaseVersionStub) currentVersion(release string) (semver.Semver, error) {
+func (s *releaseVersionMock) currentVersion(release string) (semver.Semver, error) {
 	args := s.Called(release)
 	return args.Get(0).(semver.Semver), args.Error(1)
 }

cli/internal/helm/loader.go

@@ -195,17 +195,21 @@ func (i *chartLoader) loadRelease(info chartInfo, helmWaitMode WaitMode) (Releas
 	case certManagerInfo.releaseName:
 		values = i.loadCertManagerValues()
 	case constellationOperatorsInfo.releaseName:
-		updateVersions(chart, i.cliVersion)
 		values = i.loadOperatorsValues()
 	case constellationServicesInfo.releaseName:
-		updateVersions(chart, i.cliVersion)
 		values = i.loadConstellationServicesValues()
 	case awsLBControllerInfo.releaseName:
 		values = i.loadAWSLBControllerValues()
 	case csiInfo.releaseName:
-		updateVersions(chart, i.cliVersion)
 		values = i.loadCSIValues()
 	}
+
+	// Charts we package ourselves have version 0.0.0.
+	// Before use, we need to update the version of the chart to the current CLI version.
+	if isCLIVersionedRelease(info.releaseName) {
+		updateVersions(chart, i.cliVersion)
+	}
+
 	return Release{Chart: chart, Values: values, ReleaseName: info.releaseName, WaitMode: helmWaitMode}, nil
 }