cli: only create resource backups if upgrade is executed (#1437)

Previously backups were created even if no service upgrades were
executed. To allow this some things are restructured:
* new chartInfo type that holds release name, path and chart name
* upgrade execution and version validity are checked separately

cli/internal/helm/client.go

@@ -69,10 +69,57 @@ func NewClient(client crdClient, kubeConfigPath, helmNamespace string, log debug
 	return &Client{kubectl: client, fs: fileHandler, actions: actions{config: actionConfig}, log: log}, nil
 }
 
+func (c *Client) shouldUpgrade(releaseName string, localChart *chart.Chart) error {
+	currentVersion, err := c.currentVersion(releaseName)
+	if err != nil {
+		return fmt.Errorf("getting current version: %w", err)
+	}
+	c.log.Debugf("Current %s version: %s", releaseName, currentVersion)
+	c.log.Debugf("New %s version: %s", releaseName, localChart.Metadata.Version)
+
+	// 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 err := compatibility.IsValidUpgrade(currentVersion, localChart.Metadata.Version); err != nil {
+		return err
+	}
+	c.log.Debugf("Upgrading %s from %s to %s", releaseName, currentVersion, localChart.Metadata.Version)
+
+	return nil
+}
+
 // Upgrade runs a helm-upgrade on all deployments that are managed via Helm.
 // If the CLI receives an interrupt signal it will cancel the context.
 // Canceling the context will prompt helm to abort and roll back the ongoing upgrade.
 func (c *Client) Upgrade(ctx context.Context, config *config.Config, timeout time.Duration, allowDestructive bool) error {
+	upgradeErrs := []error{}
+	upgradeReleases := []*chart.Chart{}
+	invalidUpgrade := &compatibility.InvalidUpgradeError{}
+
+	for _, info := range []chartInfo{ciliumInfo, certManagerInfo, constellationOperatorsInfo, constellationServicesInfo} {
+		chart, err := loadChartsDir(helmFS, info.path)
+		if err != nil {
+			return fmt.Errorf("loading chart: %w", err)
+		}
+		if info == constellationOperatorsInfo || info == constellationServicesInfo {
+			// ensure that the services chart has the same version as the CLI
+			updateVersions(chart, compatibility.EnsurePrefixV(constants.VersionInfo()))
+		}
+
+		err = c.shouldUpgrade(info.releaseName, chart)
+		switch {
+		case errors.As(err, &invalidUpgrade):
+			upgradeErrs = append(upgradeErrs, fmt.Errorf("skipping %s upgrade: %w", info.releaseName, err))
+		case err != nil:
+			return fmt.Errorf("upgrading %s: %w", info.releaseName, err)
+		case err == nil:
+			upgradeReleases = append(upgradeReleases, chart)
+		}
+	}
+
+	if len(upgradeReleases) == 0 {
+		return errors.Join(upgradeErrs...)
+	}
+
 	crds, err := c.backupCRDs(ctx)
 	if err != nil {
 		return fmt.Errorf("creating CRD backup: %w", err)
@@ -81,38 +128,11 @@ func (c *Client) Upgrade(ctx context.Context, config *config.Config, timeout tim
 		return fmt.Errorf("creating CR backup: %w", err)
 	}
 
-	upgradeErrs := []error{}
+	for _, chart := range upgradeReleases {
-	invalidUpgrade := &compatibility.InvalidUpgradeError{}
+		err = c.upgradeRelease(ctx, timeout, config, chart, allowDestructive)
-	err = c.upgradeRelease(ctx, timeout, config, ciliumPath, ciliumReleaseName, false, allowDestructive)
+		if err != nil {
-	switch {
+			return fmt.Errorf("upgrading %s: %w", chart.Metadata.Name, err)
-	case errors.As(err, &invalidUpgrade):
-		upgradeErrs = append(upgradeErrs, fmt.Errorf("skipping Cilium upgrade: %w", err))
-	case err != nil:
-		return fmt.Errorf("upgrading cilium: %s", err)
 		}
-
-	err = c.upgradeRelease(ctx, timeout, config, certManagerPath, certManagerReleaseName, false, allowDestructive)
-	switch {
-	case errors.As(err, &invalidUpgrade):
-		upgradeErrs = append(upgradeErrs, fmt.Errorf("skipping cert-manager upgrade: %w", err))
-	case err != nil:
-		return fmt.Errorf("upgrading cert-manager: %w", err)
-	}
-
-	err = c.upgradeRelease(ctx, timeout, config, conOperatorsPath, conOperatorsReleaseName, true, allowDestructive)
-	switch {
-	case errors.As(err, &invalidUpgrade):
-		upgradeErrs = append(upgradeErrs, fmt.Errorf("skipping constellation operators upgrade: %w", err))
-	case err != nil:
-		return fmt.Errorf("upgrading constellation operators: %w", err)
-	}
-
-	err = c.upgradeRelease(ctx, timeout, config, conServicesPath, conServicesReleaseName, false, allowDestructive)
-	switch {
-	case errors.As(err, &invalidUpgrade):
-		upgradeErrs = append(upgradeErrs, fmt.Errorf("skipping constellation-services upgrade: %w", err))
-	case err != nil:
-		return fmt.Errorf("upgrading constellation-services: %w", err)
 	}
 
 	return errors.Join(upgradeErrs...)
@@ -120,7 +140,7 @@ func (c *Client) Upgrade(ctx context.Context, config *config.Config, timeout tim
 
 // Versions queries the cluster for running versions and returns a map of releaseName -> version.
 func (c *Client) Versions() (string, error) {
-	serviceVersion, err := c.currentVersion(conServicesReleaseName)
+	serviceVersion, err := c.currentVersion(constellationServicesInfo.releaseName)
 	if err != nil {
 		return "", fmt.Errorf("getting constellation-services version: %w", err)
 	}
@@ -153,13 +173,8 @@ func (c *Client) currentVersion(release string) (string, error) {
 var ErrConfirmationMissing = errors.New("action requires user confirmation")
 
 func (c *Client) upgradeRelease(
-	ctx context.Context, timeout time.Duration, conf *config.Config, chartPath string, releaseName string, hasCRDs bool, allowDestructive bool,
+	ctx context.Context, timeout time.Duration, conf *config.Config, chart *chart.Chart, allowDestructive bool,
 ) error {
-	chart, err := loadChartsDir(helmFS, chartPath)
-	if err != nil {
-		return fmt.Errorf("loading chart: %w", err)
-	}
-
 	// We need to load all values that can be statically loaded before merging them with the cluster
 	// values. Otherwise the templates are not rendered correctly.
 	k8sVersion, err := versions.NewValidK8sVersion(conf.KubernetesVersion)
@@ -167,55 +182,44 @@ func (c *Client) upgradeRelease(
 		return fmt.Errorf("invalid k8s version: %w", err)
 	}
 	loader := NewLoader(conf.GetProvider(), k8sVersion)
+
 	var values map[string]any
-	switch releaseName {
+	var releaseName string
-	case ciliumReleaseName:
+
+	switch chart.Metadata.Name {
+	case ciliumInfo.chartName:
+		releaseName = ciliumInfo.releaseName
 		values, err = loader.loadCiliumValues()
-	case certManagerReleaseName:
+	case certManagerInfo.chartName:
+		releaseName = certManagerInfo.releaseName
 		values = loader.loadCertManagerValues()
-	case conOperatorsReleaseName:
+
-		// ensure that the operator chart has the same version as the CLI
+		if !allowDestructive {
-		updateVersions(chart, compatibility.EnsurePrefixV(constants.VersionInfo()))
+			return ErrConfirmationMissing
+		}
+	case constellationOperatorsInfo.chartName:
+		releaseName = constellationOperatorsInfo.releaseName
 		values, err = loader.loadOperatorsValues()
-	case conServicesReleaseName:
+
-		// ensure that the services chart has the same version as the CLI
+		if err := c.updateCRDs(ctx, chart); err != nil {
-		updateVersions(chart, compatibility.EnsurePrefixV(constants.VersionInfo()))
+			return fmt.Errorf("updating CRDs: %w", err)
+		}
+	case constellationServicesInfo.chartName:
+		releaseName = constellationServicesInfo.releaseName
 		values, err = loader.loadConstellationServicesValues()
 	default:
-		return fmt.Errorf("invalid release name: %s", releaseName)
+		return fmt.Errorf("unknown chart name: %s", chart.Metadata.Name)
 	}
+
 	if err != nil {
 		return fmt.Errorf("loading values: %w", err)
 	}
 
-	currentVersion, err := c.currentVersion(releaseName)
-	if err != nil {
-		return fmt.Errorf("getting current version: %w", err)
-	}
-	c.log.Debugf("Current %s version: %s", releaseName, currentVersion)
-	c.log.Debugf("New %s version: %s", releaseName, chart.Metadata.Version)
-
-	// 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 err := compatibility.IsValidUpgrade(currentVersion, chart.Metadata.Version); err != nil {
-		return err
-	}
-
-	if releaseName == certManagerReleaseName && !allowDestructive {
-		return ErrConfirmationMissing
-	}
-
-	if hasCRDs {
-		if err := c.updateCRDs(ctx, chart); err != nil {
-			return fmt.Errorf("updating CRDs: %w", err)
-		}
-	}
 	values, err = c.prepareValues(values, releaseName)
 	if err != nil {
 		return fmt.Errorf("preparing values: %w", err)
 	}
 
-	c.log.Debugf("Upgrading %s from %s to %s", releaseName, currentVersion, chart.Metadata.Version)
 	err = c.actions.upgradeAction(ctx, releaseName, chart, values, timeout)
 	if err != nil {
 		return err
@@ -231,7 +235,7 @@ func (c *Client) upgradeRelease(
 // reuse-values does not ensure this.
 func (c *Client) prepareValues(localValues map[string]any, releaseName string) (map[string]any, error) {
 	// Ensure installCRDs is set for cert-manager chart.
-	if releaseName == certManagerReleaseName {
+	if releaseName == certManagerInfo.releaseName {
 		localValues["installCRDs"] = true
 	}
 	clusterValues, err := c.actions.getValues(releaseName)

cli/internal/helm/client_test.go

@@ -15,10 +15,49 @@ import (
 	"github.com/edgelesssys/constellation/v2/internal/config"
 	"github.com/edgelesssys/constellation/v2/internal/logger"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"helm.sh/helm/v3/pkg/chart"
 	"helm.sh/helm/v3/pkg/release"
 )
 
+func TestShouldUpgrade(t *testing.T) {
+	testCases := map[string]struct {
+		version            string
+		assertCorrectError func(t *testing.T, err error) bool
+		wantError          bool
+	}{
+		"valid upgrade": {
+			version: "1.9.0",
+		},
+		"not a valid upgrade": {
+			version: "1.0.0",
+			assertCorrectError: func(t *testing.T, err error) bool {
+				target := &compatibility.InvalidUpgradeError{}
+				return assert.ErrorAs(t, err, &target)
+			},
+			wantError: true,
+		},
+	}
+
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			assert := assert.New(t)
+			require := require.New(t)
+
+			client := Client{kubectl: nil, actions: &stubActionWrapper{version: tc.version}, log: logger.NewTest(t)}
+
+			chart, err := loadChartsDir(helmFS, certManagerInfo.path)
+			require.NoError(err)
+			err = client.shouldUpgrade(certManagerInfo.releaseName, chart)
+			if tc.wantError {
+				tc.assertCorrectError(t, err)
+				return
+			}
+			assert.NoError(err)
+		})
+	}
+}
+
 func TestUpgradeRelease(t *testing.T) {
 	testCases := map[string]struct {
 		allowDestructive   bool
@@ -30,15 +69,6 @@ func TestUpgradeRelease(t *testing.T) {
 			allowDestructive: true,
 			version:          "1.9.0",
 		},
-		"not a valid upgrade": {
-			allowDestructive: true,
-			version:          "1.0.0",
-			assertCorrectError: func(t *testing.T, err error) bool {
-				target := &compatibility.InvalidUpgradeError{}
-				return assert.ErrorAs(t, err, &target)
-			},
-			wantError: true,
-		},
 		"deny": {
 			allowDestructive: false,
 			version:          "1.9.0",
@@ -52,9 +82,13 @@ func TestUpgradeRelease(t *testing.T) {
 	for name, tc := range testCases {
 		t.Run(name, func(t *testing.T) {
 			assert := assert.New(t)
+			require := require.New(t)
 
 			client := Client{kubectl: nil, actions: &stubActionWrapper{version: tc.version}, log: logger.NewTest(t)}
-			err := client.upgradeRelease(context.Background(), 0, config.Default(), certManagerPath, certManagerReleaseName, false, tc.allowDestructive)
+
+			chart, err := loadChartsDir(helmFS, certManagerInfo.path)
+			require.NoError(err)
+			err = client.upgradeRelease(context.Background(), 0, config.Default(), chart, tc.allowDestructive)
 			if tc.wantError {
 				tc.assertCorrectError(t, err)
 				return

cli/internal/helm/loader.go

@@ -38,16 +38,17 @@ import (
 //go:embed all:charts/*
 var helmFS embed.FS
 
-const (
+type chartInfo struct {
-	ciliumReleaseName       = "cilium"
+	releaseName string
-	conServicesReleaseName  = "constellation-services"
+	chartName   string
-	conOperatorsReleaseName = "constellation-operators"
+	path        string
-	certManagerReleaseName  = "cert-manager"
+}
 
-	conServicesPath  = "charts/edgeless/constellation-services"
+var (
-	conOperatorsPath = "charts/edgeless/operators"
+	ciliumInfo                 = chartInfo{releaseName: "cilium", chartName: "cilium", path: "charts/cilium"}
-	certManagerPath  = "charts/cert-manager"
+	certManagerInfo            = chartInfo{releaseName: "cert-manager", chartName: "cert-manager", path: "charts/cert-manager"}
-	ciliumPath       = "charts/cilium"
+	constellationOperatorsInfo = chartInfo{releaseName: "constellation-operators", chartName: "constellation-operators", path: "charts/edgeless/operators"}
+	constellationServicesInfo  = chartInfo{releaseName: "constellation-services", chartName: "constellation-services", path: "charts/edgeless/constellation-services"}
 )
 
 // ChartLoader loads embedded helm charts.
@@ -95,7 +96,7 @@ func NewLoader(csp cloudprovider.Provider, k8sVersion versions.ValidK8sVersion)
 
 // AvailableServiceVersions returns the chart version number of the bundled service versions.
 func AvailableServiceVersions() (string, error) {
-	servicesChart, err := loadChartsDir(helmFS, conServicesPath)
+	servicesChart, err := loadChartsDir(helmFS, constellationServicesInfo.path)
 	if err != nil {
 		return "", fmt.Errorf("loading constellation-services chart: %w", err)
 	}
@@ -140,7 +141,7 @@ func (i *ChartLoader) Load(config *config.Config, conformanceMode bool, masterSe
 
 // loadCilium prepares a helm release for use in a helm install action.
 func (i *ChartLoader) loadCilium() (helm.Release, error) {
-	chart, err := loadChartsDir(helmFS, ciliumPath)
+	chart, err := loadChartsDir(helmFS, ciliumInfo.path)
 	if err != nil {
 		return helm.Release{}, fmt.Errorf("loading cilium chart: %w", err)
 	}
@@ -154,7 +155,7 @@ func (i *ChartLoader) loadCilium() (helm.Release, error) {
 		return helm.Release{}, fmt.Errorf("packaging cilium chart: %w", err)
 	}
 
-	return helm.Release{Chart: chartRaw, Values: values, ReleaseName: ciliumReleaseName, Wait: false}, nil
+	return helm.Release{Chart: chartRaw, Values: values, ReleaseName: ciliumInfo.releaseName, Wait: false}, nil
 }
 
 // loadCiliumValues is used to separate the marshalling step from the loading step.
@@ -194,7 +195,7 @@ func extendCiliumValues(in map[string]any, conformanceMode bool) {
 }
 
 func (i *ChartLoader) loadCertManager() (helm.Release, error) {
-	chart, err := loadChartsDir(helmFS, certManagerPath)
+	chart, err := loadChartsDir(helmFS, certManagerInfo.path)
 	if err != nil {
 		return helm.Release{}, fmt.Errorf("loading cert-manager chart: %w", err)
 	}
@@ -205,7 +206,7 @@ func (i *ChartLoader) loadCertManager() (helm.Release, error) {
 		return helm.Release{}, fmt.Errorf("packaging cert-manager chart: %w", err)
 	}
 
-	return helm.Release{Chart: chartRaw, Values: values, ReleaseName: certManagerReleaseName, Wait: false}, nil
+	return helm.Release{Chart: chartRaw, Values: values, ReleaseName: certManagerInfo.releaseName, Wait: false}, nil
 }
 
 // loadCertManagerHelper is used to separate the marshalling step from the loading step.
@@ -278,7 +279,7 @@ func (i *ChartLoader) loadCertManagerValues() map[string]any {
 }
 
 func (i *ChartLoader) loadOperators() (helm.Release, error) {
-	chart, err := loadChartsDir(helmFS, conOperatorsPath)
+	chart, err := loadChartsDir(helmFS, constellationOperatorsInfo.path)
 	if err != nil {
 		return helm.Release{}, fmt.Errorf("loading operators chart: %w", err)
 	}
@@ -295,7 +296,7 @@ func (i *ChartLoader) loadOperators() (helm.Release, error) {
 		return helm.Release{}, fmt.Errorf("packaging operators chart: %w", err)
 	}
 
-	return helm.Release{Chart: chartRaw, Values: values, ReleaseName: conOperatorsReleaseName, Wait: false}, nil
+	return helm.Release{Chart: chartRaw, Values: values, ReleaseName: constellationOperatorsInfo.releaseName, Wait: false}, nil
 }
 
 // loadOperatorsHelper is used to separate the marshalling step from the loading step.
@@ -365,9 +366,9 @@ func (i *ChartLoader) loadOperatorsValues() (map[string]any, error) {
 
 // loadConstellationServices prepares a helm release for use in a helm install action.
 func (i *ChartLoader) loadConstellationServices() (helm.Release, error) {
-	chart, err := loadChartsDir(helmFS, conServicesPath)
+	chart, err := loadChartsDir(helmFS, constellationServicesInfo.path)
 	if err != nil {
-		return helm.Release{}, fmt.Errorf("loading constellation-services chart: %w", err)
+		return helm.Release{}, fmt.Errorf("loadingg constellation-services chart: %w", err)
 	}
 
 	updateVersions(chart, compatibility.EnsurePrefixV(constants.VersionInfo()))
@@ -382,7 +383,7 @@ func (i *ChartLoader) loadConstellationServices() (helm.Release, error) {
 		return helm.Release{}, fmt.Errorf("packaging constellation-services chart: %w", err)
 	}
 
-	return helm.Release{Chart: chartRaw, Values: values, ReleaseName: conServicesReleaseName, Wait: false}, nil
+	return helm.Release{Chart: chartRaw, Values: values, ReleaseName: constellationServicesInfo.releaseName, Wait: false}, nil
 }
 
 // loadConstellationServicesHelper is used to separate the marshalling step from the loading step.

cli/internal/helm/loader_test.go

@@ -109,7 +109,7 @@ func TestConstellationServices(t *testing.T) {
 				konnectivityImage:        "konnectivityImage",
 				gcpGuestAgentImage:       "gcpGuestAgentImage",
 			}
-			chart, err := loadChartsDir(helmFS, conServicesPath)
+			chart, err := loadChartsDir(helmFS, constellationServicesInfo.path)
 			require.NoError(err)
 			values, err := chartLoader.loadConstellationServicesValues()
 			require.NoError(err)
@@ -180,7 +180,7 @@ func TestOperators(t *testing.T) {
 				constellationOperatorImage:   "constellationOperatorImage",
 				nodeMaintenanceOperatorImage: "nodeMaintenanceOperatorImage",
 			}
-			chart, err := loadChartsDir(helmFS, conOperatorsPath)
+			chart, err := loadChartsDir(helmFS, constellationOperatorsInfo.path)
 			require.NoError(err)
 			vals, err := chartLoader.loadOperatorsValues()
 			require.NoError(err)