Only upgrade helm releases if versions changed (#818)

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

cli/internal/cloudcmd/upgrade.go

@@ -118,11 +118,6 @@ func (u *Upgrader) UpgradeHelmServices(ctx context.Context, config *config.Confi
 	return u.helmClient.Upgrade(ctx, config, timeout)
 }
 
-// CurrentHelmVersion returns the version of the currently installed helm release.
-func (u *Upgrader) CurrentHelmVersion(release string) (string, error) {
-	return u.helmClient.CurrentVersion(release)
-}
-
 // KubernetesVersion returns the version of Kubernetes the Constellation is currently running on.
 func (u *Upgrader) KubernetesVersion() (string, error) {
 	return u.stableInterface.kubernetesVersion()
@@ -244,7 +239,6 @@ func (u *stableClient) kubernetesVersion() (string, error) {
 }
 
 type helmInterface interface {
-	CurrentVersion(release string) (string, error)
 	Upgrade(ctx context.Context, config *config.Config, timeout time.Duration) error
 }
 

cli/internal/helm/client.go

@@ -16,6 +16,7 @@ import (
 	"github.com/edgelesssys/constellation/v2/internal/constants"
 	"github.com/edgelesssys/constellation/v2/internal/deploy/helm"
 	"github.com/pkg/errors"
+	"golang.org/x/mod/semver"
 	"helm.sh/helm/v3/pkg/action"
 	"helm.sh/helm/v3/pkg/chart"
 	"helm.sh/helm/v3/pkg/cli"
@@ -47,8 +48,31 @@ func NewClient(kubeConfigPath, helmNamespace string, client *apiextensionsclient
 	return &Client{config: actionConfig, crdClient: client, log: log}, nil
 }
 
-// CurrentVersion returns the version of the currently installed helm release.
+// Upgrade runs a helm-upgrade on all deployments that are managed via Helm.
-func (c *Client) CurrentVersion(release string) (string, error) {
+// 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) error {
+	if err := c.upgradeRelease(ctx, timeout, ciliumPath, ciliumReleaseName, false); err != nil {
+		return fmt.Errorf("upgrading cilium: %w", err)
+	}
+
+	if err := c.upgradeRelease(ctx, timeout, certManagerPath, certManagerReleaseName, false); err != nil {
+		return fmt.Errorf("upgrading cert-manager: %w", err)
+	}
+
+	if err := c.upgradeRelease(ctx, timeout, conOperatorsPath, conOperatorsReleaseName, true); err != nil {
+		return fmt.Errorf("upgrading constellation operators: %w", err)
+	}
+
+	if err := c.upgradeRelease(ctx, timeout, conServicesPath, conServicesReleaseName, false); err != nil {
+		return fmt.Errorf("upgrading constellation-services: %w", err)
+	}
+
+	return nil
+}
+
+// currentVersion returns the version of the currently installed helm release.
+func (c *Client) currentVersion(release string) (string, error) {
 	client := action.NewList(c.config)
 	client.Filter = release
 	rel, err := client.Run()
@@ -70,67 +94,46 @@ func (c *Client) CurrentVersion(release string) (string, error) {
 	return rel[0].Chart.Metadata.Version, nil
 }
 
-// Upgrade runs a helm-upgrade on all deployments that are managed via Helm.
+func (c *Client) upgradeRelease(
-// If the CLI receives an interrupt signal it will cancel the context.
+	ctx context.Context, timeout time.Duration, chartPath, releaseName string, hasCRDs bool,
-// Canceling the context will prompt helm to abort and roll back the ongoing upgrade.
+) error {
-func (c *Client) Upgrade(ctx context.Context, config *config.Config, timeout time.Duration) error {
+	chart, err := loadChartsDir(helmFS, chartPath)
+	if err != nil {
+		return fmt.Errorf("loading chart: %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)
+
+	if !isUpgrade(currentVersion, chart.Metadata.Version) {
+		c.log.Debugf(
+			"Skipping upgrade of %s: new version (%s) is not an upgrade for current version (%s)",
+			releaseName, chart.Metadata.Version, currentVersion,
+		)
+		return nil
+	}
+
+	if hasCRDs {
+		if err := c.updateCRDs(ctx, chart); err != nil {
+			return fmt.Errorf("updating CRDs: %w", err)
+		}
+	}
+	values, err := c.prepareValues(chart, 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)
 	action := action.NewUpgrade(c.config)
 	action.Atomic = true
 	action.Namespace = constants.HelmNamespace
 	action.ReuseValues = false
 	action.Timeout = timeout
-
+	if _, err := action.RunWithContext(ctx, releaseName, chart, values); err != nil {
-	ciliumChart, err := loadChartsDir(helmFS, ciliumPath)
+		return fmt.Errorf("upgrading %s: %w", releaseName, err)
-	if err != nil {
-		return fmt.Errorf("loading cilium: %w", err)
-	}
-	certManagerChart, err := loadChartsDir(helmFS, certManagerPath)
-	if err != nil {
-		return fmt.Errorf("loading cert-manager: %w", err)
-	}
-	conOperatorChart, err := loadChartsDir(helmFS, conOperatorsPath)
-	if err != nil {
-		return fmt.Errorf("loading operators: %w", err)
-	}
-	conServicesChart, err := loadChartsDir(helmFS, conServicesPath)
-	if err != nil {
-		return fmt.Errorf("loading constellation-services chart: %w", err)
-	}
-
-	values, err := c.prepareValues(ciliumChart, ciliumReleaseName)
-	if err != nil {
-		return err
-	}
-	if _, err := action.RunWithContext(ctx, ciliumReleaseName, ciliumChart, values); err != nil {
-		return fmt.Errorf("upgrading cilium: %w", err)
-	}
-
-	values, err = c.prepareValues(certManagerChart, certManagerReleaseName)
-	if err != nil {
-		return err
-	}
-	if _, err := action.RunWithContext(ctx, certManagerReleaseName, certManagerChart, values); err != nil {
-		return fmt.Errorf("upgrading cert-manager: %w", err)
-	}
-
-	err = c.updateOperatorCRDs(ctx, conOperatorChart)
-	if err != nil {
-		return fmt.Errorf("updating operator CRDs: %w", err)
-	}
-	values, err = c.prepareValues(conOperatorChart, conOperatorsReleaseName)
-	if err != nil {
-		return err
-	}
-	if _, err := action.RunWithContext(ctx, conOperatorsReleaseName, conOperatorChart, values); err != nil {
-		return fmt.Errorf("upgrading services: %w", err)
-	}
-
-	values, err = c.prepareValues(conServicesChart, conServicesReleaseName)
-	if err != nil {
-		return err
-	}
-	if _, err := action.RunWithContext(ctx, conServicesReleaseName, conServicesChart, values); err != nil {
-		return fmt.Errorf("upgrading operators: %w", err)
 	}
 
 	return nil
@@ -197,14 +200,14 @@ func parseCRD(crdString []byte) (*v1.CustomResourceDefinition, error) {
 	return nil, errors.New("parsed []byte, but did not find a CRD")
 }
 
-// updateOperatorCRDs walks through the dependencies of the given chart and applies
+// updateCRDs walks through the dependencies of the given chart and applies
 // the files in the dependencie's 'crds' folder.
 // This function is NOT recursive!
-func (c *Client) updateOperatorCRDs(ctx context.Context, chart *chart.Chart) error {
+func (c *Client) updateCRDs(ctx context.Context, chart *chart.Chart) error {
 	for _, dep := range chart.Dependencies() {
 		for _, crdFile := range dep.Files {
 			if strings.HasPrefix(crdFile.Name, "crds/") {
-				c.log.Debugf("updating crd: %s", crdFile.Name)
+				c.log.Debugf("Updating crd: %s", crdFile.Name)
 				err := c.ApplyCRD(ctx, crdFile.Data)
 				if err != nil {
 					return err
@@ -215,6 +218,29 @@ func (c *Client) updateOperatorCRDs(ctx context.Context, chart *chart.Chart) err
 	return nil
 }
 
+// isUpgrade returns true if the new version is greater than the current version.
+// Versions should adhere to the semver spec, but this function will prefix the versions with 'v' if they don't.
+func isUpgrade(currentVersion, newVersion string) bool {
+	if !strings.HasPrefix(currentVersion, "v") {
+		currentVersion = "v" + currentVersion
+	}
+	if !strings.HasPrefix(newVersion, "v") {
+		newVersion = "v" + newVersion
+	}
+
+	// If the current version is not a valid semver,
+	// we cant compare it to the new version.
+	// -> We can't upgrade.
+	if !semver.IsValid(currentVersion) {
+		return false
+	}
+
+	if semver.Compare(currentVersion, newVersion) < 0 {
+		return true
+	}
+	return false
+}
+
 type debugLog interface {
 	Debugf(format string, args ...any)
 	Sync()

cli/internal/helm/client_test.go

@@ -44,3 +44,69 @@ func TestParseCRDs(t *testing.T) {
 		})
 	}
 }
+
+func TestIsUpgrade(t *testing.T) {
+	testCases := map[string]struct {
+		currentVersion string
+		newVersion     string
+		wantUpgrade    bool
+	}{
+		"upgrade": {
+			currentVersion: "0.1.0",
+			newVersion:     "0.2.0",
+			wantUpgrade:    true,
+		},
+		"downgrade": {
+			currentVersion: "0.2.0",
+			newVersion:     "0.1.0",
+			wantUpgrade:    false,
+		},
+		"equal": {
+			currentVersion: "0.1.0",
+			newVersion:     "0.1.0",
+			wantUpgrade:    false,
+		},
+		"invalid current version": {
+			currentVersion: "asdf",
+			newVersion:     "0.1.0",
+			wantUpgrade:    false,
+		},
+		"invalid new version": {
+			currentVersion: "0.1.0",
+			newVersion:     "asdf",
+			wantUpgrade:    false,
+		},
+		"patch version": {
+			currentVersion: "0.1.0",
+			newVersion:     "0.1.1",
+			wantUpgrade:    true,
+		},
+		"pre-release version": {
+			currentVersion: "0.1.0",
+			newVersion:     "0.1.1-rc1",
+			wantUpgrade:    true,
+		},
+		"pre-release version downgrade": {
+			currentVersion: "0.1.1-rc1",
+			newVersion:     "0.1.0",
+			wantUpgrade:    false,
+		},
+		"pre-release of same version": {
+			currentVersion: "0.1.0",
+			newVersion:     "0.1.0-rc1",
+			wantUpgrade:    false,
+		},
+	}
+
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			assert := assert.New(t)
+
+			upgrade := isUpgrade(tc.currentVersion, tc.newVersion)
+			assert.Equal(tc.wantUpgrade, upgrade)
+
+			upgrade = isUpgrade("v"+tc.currentVersion, "v"+tc.newVersion)
+			assert.Equal(tc.wantUpgrade, upgrade)
+		})
+	}
+}