From 0ce695acff711edf0481d7ea6760318b085e7b91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Wei=C3=9Fe?= Date: Thu, 7 Sep 2023 13:32:52 +0200 Subject: [PATCH] Check chart versions against target in users config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Weiße --- cli/internal/helm/BUILD.bazel | 2 + cli/internal/helm/actionfactory.go | 46 ++-- cli/internal/helm/actionfactory_test.go | 267 ++++++++++++++++++++++++ cli/internal/helm/helm.go | 4 +- cli/internal/helm/helm_test.go | 11 +- cli/internal/helm/loader.go | 10 +- 6 files changed, 316 insertions(+), 24 deletions(-) create mode 100644 cli/internal/helm/actionfactory_test.go diff --git a/cli/internal/helm/BUILD.bazel b/cli/internal/helm/BUILD.bazel index 663ca1951e..c10c5856f0 100644 --- a/cli/internal/helm/BUILD.bazel +++ b/cli/internal/helm/BUILD.bazel @@ -452,6 +452,7 @@ go_library( go_test( name = "helm_test", srcs = [ + "actionfactory_test.go", "helm_test.go", "loader_test.go", ], @@ -475,6 +476,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", ], diff --git a/cli/internal/helm/actionfactory.go b/cli/internal/helm/actionfactory.go index 54705786fe..106f9c8b0f 100644 --- a/cli/internal/helm/actionfactory.go +++ b/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,21 @@ 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) } 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) && configTargetVersion.Compare(newVersion) != 0 { + return fmt.Errorf( + "unable to install release %s at %s: this CLI only supports microservice version %s for upgrading", + release.ReleaseName, configTargetVersion, newVersion, + ) + } + a.log.Debugf("Release %s not found, adding to new releases...", release.ReleaseName) *actions = append(*actions, a.newInstall(release)) return nil @@ -88,18 +94,30 @@ 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 { - return fmt.Errorf("invalid upgrade for %s: %w", release.ReleaseName, err) + // For charts we package ourselves, the version is equal to the CLI version (charts are embedded in the binary). + // 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 configTargetVersion.Compare(newVersion) != 0 { + return fmt.Errorf( + "unable to upgrade release %s to %s: this CLI only supports microservice version %s for upgrading", + release.ReleaseName, configTargetVersion, 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 diff --git a/cli/internal/helm/actionfactory_test.go b/cli/internal/helm/actionfactory_test.go new file mode 100644 index 0000000000..af3176ae9e --- /dev/null +++ b/cli/internal/helm/actionfactory_test.go @@ -0,0 +1,267 @@ +/* +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: func(assert *assert.Assertions, err error) { + var invalidUpgrade *compatibility.InvalidUpgradeError + assert.False(errors.As(err, &invalidUpgrade)) + }, + }, + "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, + }, + "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 +} diff --git a/cli/internal/helm/helm.go b/cli/internal/helm/helm.go index 261b431e23..363efaf7c8 100644 --- a/cli/internal/helm/helm.go +++ b/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 } @@ -93,7 +93,7 @@ func (h Client) PrepareApply(conf *config.Config, validK8sversion versions.Valid 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 } diff --git a/cli/internal/helm/helm_test.go b/cli/internal/helm/helm_test.go index f813ffbacd..c39551e8ec 100644 --- a/cli/internal/helm/helm_test.go +++ b/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) } diff --git a/cli/internal/helm/loader.go b/cli/internal/helm/loader.go index 02047ad196..acdbc6c302 100644 --- a/cli/internal/helm/loader.go +++ b/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 }