cli: upgrade errors for microservice (#1259)

Handle invalid upgrade errors similarly as for images and k8s.

cli/internal/cloudcmd/upgrade.go

@@ -37,23 +37,6 @@ import (
 // ErrInProgress signals that an upgrade is in progress inside the cluster.
 var ErrInProgress = errors.New("upgrade in progress")
 
-// InvalidUpgradeError present an invalid upgrade. It wraps the source and destination version for improved debuggability.
-type InvalidUpgradeError struct {
-	from     string
-	to       string
-	innerErr error
-}
-
-// Unwrap returns the inner error, which is nil in this case.
-func (e *InvalidUpgradeError) Unwrap() error {
-	return e.innerErr
-}
-
-// Error returns the String representation of this error.
-func (e *InvalidUpgradeError) Error() string {
-	return fmt.Sprintf("upgrading from %s to %s is not a valid upgrade: %s", e.from, e.to, e.innerErr)
-}
-
 // Upgrader handles upgrading the cluster's components using the CLI.
 type Upgrader struct {
 	stableInterface  stableInterface
@@ -105,7 +88,7 @@ func (u *Upgrader) UpgradeImage(ctx context.Context, newImageReference, newImage
 	currentImageVersion := nodeVersion.Spec.ImageVersion
 
 	if err := compatibility.IsValidUpgrade(currentImageVersion, newImageVersion); err != nil {
-		return &InvalidUpgradeError{from: currentImageVersion, to: newImageVersion, innerErr: err}
+		return err
 	}
 
 	if imageUpgradeInProgress(nodeVersion) {
@@ -135,7 +118,7 @@ func (u *Upgrader) UpgradeK8s(ctx context.Context, newClusterVersion string, com
 	}
 
 	if err := compatibility.IsValidUpgrade(nodeVersion.Spec.KubernetesClusterVersion, newClusterVersion); err != nil {
-		return &InvalidUpgradeError{from: nodeVersion.Spec.KubernetesClusterVersion, to: newClusterVersion, innerErr: err}
+		return err
 	}
 
 	if k8sUpgradeInProgress(nodeVersion) {

cli/internal/cloudcmd/upgrade_test.go

@@ -14,6 +14,7 @@ import (
 	"testing"
 
 	"github.com/edgelesssys/constellation/v2/internal/attestation/measurements"
+	"github.com/edgelesssys/constellation/v2/internal/compatibility"
 	"github.com/edgelesssys/constellation/v2/internal/constants"
 	"github.com/edgelesssys/constellation/v2/internal/logger"
 	"github.com/edgelesssys/constellation/v2/internal/versions/components"
@@ -48,7 +49,7 @@ func TestUpgradeK8s(t *testing.T) {
 			newClusterVersion:     "v1.2.3",
 			wantErr:               true,
 			assertCorrectError: func(t *testing.T, err error) bool {
-				target := &InvalidUpgradeError{}
+				target := &compatibility.InvalidUpgradeError{}
 				return assert.ErrorAs(t, err, &target)
 			},
 		},
@@ -57,7 +58,7 @@ func TestUpgradeK8s(t *testing.T) {
 			newClusterVersion:     "v1.2.2",
 			wantErr:               true,
 			assertCorrectError: func(t *testing.T, err error) bool {
-				target := &InvalidUpgradeError{}
+				target := &compatibility.InvalidUpgradeError{}
 				return assert.ErrorAs(t, err, &target)
 			},
 		},
@@ -156,7 +157,7 @@ func TestUpgradeImage(t *testing.T) {
 			newImageVersion:     "v1.2.2",
 			wantErr:             true,
 			assertCorrectError: func(t *testing.T, err error) bool {
-				target := &InvalidUpgradeError{}
+				target := &compatibility.InvalidUpgradeError{}
 				return assert.ErrorAs(t, err, &target)
 			},
 		},
@@ -165,7 +166,7 @@ func TestUpgradeImage(t *testing.T) {
 			newImageVersion:     "v1.2.1",
 			wantErr:             true,
 			assertCorrectError: func(t *testing.T, err error) bool {
-				target := &InvalidUpgradeError{}
+				target := &compatibility.InvalidUpgradeError{}
 				return assert.ErrorAs(t, err, &target)
 			},
 		},

cli/internal/cmd/upgradeapply.go

@@ -16,6 +16,7 @@ import (
 	"github.com/edgelesssys/constellation/v2/cli/internal/helm"
 	"github.com/edgelesssys/constellation/v2/cli/internal/image"
 	"github.com/edgelesssys/constellation/v2/internal/attestation/measurements"
+	"github.com/edgelesssys/constellation/v2/internal/compatibility"
 	"github.com/edgelesssys/constellation/v2/internal/config"
 	"github.com/edgelesssys/constellation/v2/internal/file"
 	"github.com/edgelesssys/constellation/v2/internal/versions"
@@ -82,11 +83,15 @@ func (u *upgradeApplyCmd) upgradeApply(cmd *cobra.Command, imageFetcher imageFet
 		return err
 	}
 
-	if err := u.handleServiceUpgrade(cmd, conf, flags); err != nil {
+	invalidUpgradeErr := &compatibility.InvalidUpgradeError{}
+	err = u.handleServiceUpgrade(cmd, conf, flags)
+	switch {
+	case errors.As(err, &invalidUpgradeErr):
+		cmd.PrintErrf("Skipping microservice upgrades: %s\n", err)
+	case err != nil:
 		return fmt.Errorf("service upgrade: %w", err)
 	}
 
-	invalidUpgradeErr := &cloudcmd.InvalidUpgradeError{}
 	err = u.handleK8sUpgrade(cmd.Context(), conf)
 	skipCtr := 0
 	switch {

cli/internal/helm/client.go

@@ -20,7 +20,6 @@ import (
 	"github.com/edgelesssys/constellation/v2/internal/versions"
 	"github.com/pkg/errors"
 	"github.com/spf13/afero"
-	"golang.org/x/mod/semver"
 	"helm.sh/helm/v3/pkg/action"
 	"helm.sh/helm/v3/pkg/chart"
 	"helm.sh/helm/v3/pkg/cli"
@@ -170,12 +169,10 @@ func (c *Client) upgradeRelease(
 	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) {
+	// This may break for cert-manager or cilium if we decide to upgrade more than one minor version at a time.
-		c.log.Debugf(
+	// Leaving it as is since it is not clear to me what kind of sanity check we could do.
-			"Skipping upgrade of %s: new version (%s) is not an upgrade for current version (%s)",
+	if err := compatibility.IsValidUpgrade(currentVersion, chart.Metadata.Version); err != nil {
-			releaseName, chart.Metadata.Version, currentVersion,
+		return err
-		)
-		return nil
 	}
 
 	if releaseName == certManagerReleaseName && !allowDestructive {
@@ -249,29 +246,6 @@ func (c *Client) updateCRDs(ctx context.Context, chart *chart.Chart) error {
 	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

@@ -11,6 +11,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/edgelesssys/constellation/v2/internal/compatibility"
 	"github.com/edgelesssys/constellation/v2/internal/config"
 	"github.com/edgelesssys/constellation/v2/internal/logger"
 	"github.com/stretchr/testify/assert"
@@ -18,82 +19,32 @@ import (
 	"helm.sh/helm/v3/pkg/release"
 )
 
-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)
-		})
-	}
-}
-
 func TestUpgradeRelease(t *testing.T) {
 	testCases := map[string]struct {
 		allowDestructive   bool
+		version            string
+		assertCorrectError func(t *testing.T, err error) bool
 		wantError          bool
 	}{
 		"allow": {
 			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",
+			assertCorrectError: func(t *testing.T, err error) bool {
+				return assert.ErrorIs(t, err, ErrConfirmationMissing)
+			},
 			wantError: true,
 		},
 	}
@@ -102,10 +53,10 @@ func TestUpgradeRelease(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			assert := assert.New(t)
 
-			client := Client{kubectl: nil, actions: &stubActionWrapper{}, log: logger.NewTest(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)
 			if tc.wantError {
-				assert.ErrorIs(err, ErrConfirmationMissing)
+				tc.assertCorrectError(t, err)
 				return
 			}
 			assert.NoError(err)
@@ -113,11 +64,13 @@ func TestUpgradeRelease(t *testing.T) {
 	}
 }
 
-type stubActionWrapper struct{}
+type stubActionWrapper struct {
+	version string
+}
 
 // listAction returns a list of len 1 with a release that has only it's version set.
 func (a *stubActionWrapper) listAction(_ string) ([]*release.Release, error) {
-	return []*release.Release{{Chart: &chart.Chart{Metadata: &chart.Metadata{Version: "1.0.0"}}}}, nil
+	return []*release.Release{{Chart: &chart.Chart{Metadata: &chart.Metadata{Version: a.version}}}}, nil
 }
 
 func (a *stubActionWrapper) getValues(release string) (map[string]any, error) {

internal/compatibility/compatibility.go

@@ -29,6 +29,28 @@ var (
 	ErrOutdatedCLI = errors.New("target version newer than cli version")
 )
 
+// InvalidUpgradeError present an invalid upgrade. It wraps the source and destination version for improved debuggability.
+type InvalidUpgradeError struct {
+	from     string
+	to       string
+	innerErr error
+}
+
+// NewInvalidUpgradeError returns a new InvalidUpgradeError.
+func NewInvalidUpgradeError(from string, to string, innerErr error) *InvalidUpgradeError {
+	return &InvalidUpgradeError{from: from, to: to, innerErr: innerErr}
+}
+
+// Unwrap returns the inner error, which is nil in this case.
+func (e *InvalidUpgradeError) Unwrap() error {
+	return e.innerErr
+}
+
+// Error returns the String representation of this error.
+func (e *InvalidUpgradeError) Error() string {
+	return fmt.Sprintf("upgrading from %s to %s is not a valid upgrade: %s", e.from, e.to, e.innerErr)
+}
+
 // EnsurePrefixV returns the input string prefixed with the letter "v", if the string doesn't already start with that letter.
 func EnsurePrefixV(str string) string {
 	if strings.HasPrefix(str, "v") {
@@ -43,28 +65,28 @@ func IsValidUpgrade(a, b string) error {
 	b = EnsurePrefixV(b)
 
 	if !semver.IsValid(a) || !semver.IsValid(b) {
-		return ErrSemVer
+		return NewInvalidUpgradeError(a, b, ErrSemVer)
 	}
 
 	if semver.Compare(a, b) >= 0 {
-		return errors.New("current version newer than or equal to new version")
+		return NewInvalidUpgradeError(a, b, errors.New("current version newer than or equal to new version"))
 	}
 
 	aMajor, aMinor, err := parseCanonicalSemver(a)
 	if err != nil {
-		return err
+		return NewInvalidUpgradeError(a, b, err)
 	}
 	bMajor, bMinor, err := parseCanonicalSemver(b)
 	if err != nil {
-		return err
+		return NewInvalidUpgradeError(a, b, err)
 	}
 
 	if aMajor != bMajor {
-		return ErrMajorMismatch
+		return NewInvalidUpgradeError(a, b, ErrMajorMismatch)
 	}
 
 	if bMinor-aMinor > 1 {
-		return ErrMinorDrift
+		return NewInvalidUpgradeError(a, b, ErrMinorDrift)
 	}
 
 	return nil