upgrade: fix 2.6 -> 2.7 migration for 2.7.1 patch (#1649)

* upgrade: fix 2.6 -> 2.7 migration for 2.7.1 patch

Also correctly set microservice version from config.
Previously the key was ignored and microservices were always
tried for an upgrade.

* ci: add simulatedTargetVersion to e2e-upgrade

This allows us to build a CLI that reports the given version during
an upgrade test. With this we can test patch upgrades.

Signed-off-by: Otto Bittner <cobittner@posteo.net>

* ci: e2e-upgrade, case-insensitive string compare

* hack: fix v-prefixing in pseudo-version tool

pre-release versions and release versions behaved differently.
This lead to a duplicate v prefix in the cli's version.

Signed-off-by: Otto Bittner <cobittner@posteo.net>

---------

Signed-off-by: Otto Bittner <cobittner@posteo.net>
Co-authored-by: Otto Bittner <cobittner@posteo.net>

.github/workflows/e2e-upgrade.yml

@@ -37,6 +37,10 @@ on:
         description: Microservice version to target for the upgrade, empty for target's default version.
         type: string
         required: false
+      simulatedTargetVersion:
+        description: Enter a version to build the CLI with. This can be used to simulate a patch-upgrade.
+        type: string
+        required: false
   workflow_call:
     inputs:
       cloudProvider:
@@ -72,13 +76,10 @@ on:
         description: Kubernetes version to target for the upgrade, empty for target's default version.
         type: string
         required: false
-
+      simulatedTargetVersion:
-
+        description: Enter a version to build the CLI with. This can be used to simulate a patch-upgrade.
-env:
+        type: string
-  ARM_CLIENT_ID: ${{ secrets.AZURE_E2E_CLIENT_ID }}
+        required: false
-  ARM_CLIENT_SECRET: ${{ secrets.AZURE_E2E_CLIENT_SECRET }}
-  ARM_SUBSCRIPTION_ID: ${{ secrets.AZURE_E2E_SUBSCRIPTION_ID }}
-  ARM_TENANT_ID: ${{ secrets.AZURE_E2E_TENANT_ID }}
 
 jobs:
   e2e-upgrade:
@@ -137,6 +138,11 @@ jobs:
           ref: main
           stream: nightly
 
+      - name: Simulate patch upgrade
+        if: inputs.simulatedTargetVersion != ''
+        run: |
+          echo ${{ inputs.simulatedTargetVersion }} > version.txt
+
       - name: Create cluster with 'fromVersion' CLI.
         id: e2e_test
         uses: ./.github/actions/e2e_test

cli/internal/helm/BUILD.bazel

@@ -326,6 +326,7 @@ go_library(
         "//internal/constants",
         "//internal/deploy/helm",
         "//internal/file",
+        "//internal/semver",
         "//internal/versions",
         "@com_github_pkg_errors//:errors",
         "@com_github_spf13_afero//:afero",

cli/internal/helm/client.go

@@ -21,6 +21,7 @@ import (
 	"github.com/edgelesssys/constellation/v2/internal/constants"
 	"github.com/edgelesssys/constellation/v2/internal/deploy/helm"
 	"github.com/edgelesssys/constellation/v2/internal/file"
+	"github.com/edgelesssys/constellation/v2/internal/semver"
 	"github.com/edgelesssys/constellation/v2/internal/versions"
 	"github.com/spf13/afero"
 	"helm.sh/helm/v3/pkg/action"
@@ -75,20 +76,26 @@ 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 {
+func (c *Client) shouldUpgrade(releaseName, newVersion string) error {
 	currentVersion, err := c.currentVersion(releaseName)
 	if err != nil {
-		return fmt.Errorf("getting current version: %w", err)
+		return fmt.Errorf("getting version for %s: %w", releaseName, err)
 	}
 	c.log.Debugf("Current %s version: %s", releaseName, currentVersion)
-	c.log.Debugf("New %s version: %s", releaseName, localChart.Metadata.Version)
+	c.log.Debugf("New %s version: %s", 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 err := compatibility.IsValidUpgrade(currentVersion, localChart.Metadata.Version); err != nil {
+	if err := compatibility.IsValidUpgrade(currentVersion, newVersion); err != nil {
 		return err
 	}
-	c.log.Debugf("Upgrading %s from %s to %s", releaseName, currentVersion, localChart.Metadata.Version)
+	// at this point we conclude that the release should be upgraded. check that this CLI supports the upgrade.
+	if releaseName == constellationOperatorsInfo.releaseName || releaseName == constellationServicesInfo.releaseName {
+		if compatibility.EnsurePrefixV(constants.VersionInfo()) != compatibility.EnsurePrefixV(newVersion) {
+			return fmt.Errorf("this CLI only supports microservice version %s for upgrading", constants.VersionInfo())
+		}
+	}
+	c.log.Debugf("Upgrading %s from %s to %s", releaseName, currentVersion, newVersion)
 
 	return nil
 }
@@ -106,12 +113,18 @@ func (c *Client) Upgrade(ctx context.Context, config *config.Config, timeout tim
 		if err != nil {
 			return fmt.Errorf("loading chart: %w", err)
 		}
+
+		// define target version the chart is upgraded to
+		var upgradeVersion string
 		if info == constellationOperatorsInfo || info == constellationServicesInfo {
 			// ensure that the services chart has the same version as the CLI
 			updateVersions(chart, compatibility.EnsurePrefixV(constants.VersionInfo()))
+			upgradeVersion = config.MicroserviceVersion
+		} else {
+			upgradeVersion = chart.Metadata.Version
 		}
 
-		err = c.shouldUpgrade(info.releaseName, chart)
+		err = c.shouldUpgrade(info.releaseName, upgradeVersion)
 		switch {
 		case errors.As(err, &invalidUpgrade):
 			upgradeErrs = append(upgradeErrs, fmt.Errorf("skipping %s upgrade: %w", info.releaseName, err))
@@ -134,7 +147,6 @@ func (c *Client) Upgrade(ctx context.Context, config *config.Config, timeout tim
 		return fmt.Errorf("creating CR backup: %w", err)
 	}
 
-	// TODO: v2.8: remove fileHanlder.
 	fileHandler := file.NewHandler(afero.NewOsFs())
 	for _, chart := range upgradeReleases {
 		err = c.upgradeRelease(ctx, timeout, config, chart, allowDestructive, fileHandler)
@@ -232,7 +244,6 @@ func (s ServiceVersions) ConstellationServices() string {
 	return s.constellationServices
 }
 
-// TODO: v2.8: remove fileHandler argument.
 func (c *Client) upgradeRelease(
 	ctx context.Context, timeout time.Duration, conf *config.Config, chart *chart.Chart, allowDestructive bool, fileHandler file.Handler,
 ) error {
@@ -251,6 +262,9 @@ func (c *Client) upgradeRelease(
 	case ciliumInfo.chartName:
 		releaseName = ciliumInfo.releaseName
 		values, err = loader.loadCiliumValues()
+		if err != nil {
+			return fmt.Errorf("loading values: %w", err)
+		}
 	case certManagerInfo.chartName:
 		releaseName = certManagerInfo.releaseName
 		values = loader.loadCertManagerValues()
@@ -261,6 +275,9 @@ func (c *Client) upgradeRelease(
 	case constellationOperatorsInfo.chartName:
 		releaseName = constellationOperatorsInfo.releaseName
 		values, err = loader.loadOperatorsValues()
+		if err != nil {
+			return fmt.Errorf("loading values: %w", err)
+		}
 
 		if err := c.updateCRDs(ctx, chart); err != nil {
 			return fmt.Errorf("updating CRDs: %w", err)
@@ -268,15 +285,61 @@ func (c *Client) upgradeRelease(
 	case constellationServicesInfo.chartName:
 		releaseName = constellationServicesInfo.releaseName
 		values, err = loader.loadConstellationServicesValues()
+		if err != nil {
+			return fmt.Errorf("loading values: %w", err)
+		}
 
-		// TODO: v2.8: remove this call.
+		if err := c.applyMigrations(releaseName, values, conf, fileHandler); err != nil {
+			return fmt.Errorf("applying migrations: %w", err)
+		}
+	default:
+		return fmt.Errorf("unknown chart name: %s", chart.Metadata.Name)
+	}
+
+	values, err = c.prepareValues(values, releaseName)
+	if err != nil {
+		return fmt.Errorf("preparing values: %w", err)
+	}
+
+	err = c.actions.upgradeAction(ctx, releaseName, chart, values, timeout)
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
+// applyMigrations checks the from version and applies the necessary migrations.
+// The function assumes the caller has verified that our version drift restriction is not violated,
+// Currently, this is done during config validation.
+func (c *Client) applyMigrations(releaseName string, values map[string]any, conf *config.Config, fileHandler file.Handler) error {
+	current, err := c.currentVersion(releaseName)
+	if err != nil {
+		return fmt.Errorf("getting %s version: %w", releaseName, err)
+	}
+	currentV, err := semver.New(current)
+	if err != nil {
+		return fmt.Errorf("parsing current version: %w", err)
+	}
+
+	if currentV.Major == 2 && currentV.Minor == 6 {
+		return migrateFrom2_6(values, conf, fileHandler)
+	}
+
+	return nil
+}
+
+// migrateFrom2_6 applies the necessary migrations for upgrading from v2.6.x to v2.7.x.
+// migrateFrom2_6 should be applied for v2.6.x --> v2.7.x.
+// migrateFrom2_6 should NOT be applied for v2.7.0 --> v2.7.x.
+// This function can be removed once we are sure that we will no longer provide backports for v2.6.
+func migrateFrom2_6(values map[string]any, conf *config.Config, fileHandler file.Handler) error {
 	// Manually setting attestationVariant is required here since upgrade normally isn't allowed to change this value.
 	// However, to introduce the value into a 2.6 cluster for the first time we have to set it nevertheless.
 	if err := setAttestationVariant(values, conf.AttestationVariant); err != nil {
 		return fmt.Errorf("setting attestationVariant: %w", err)
 	}
 
-		// TODO: v2.8: remove from here...
 	// Manually setting idKeyConfig is required here since upgrade normally isn't allowed to change this value.
 	// However, to introduce the value into a 2.6 cluster for the first time we have to set it nevertheless.
 	var idFile clusterid.File
@@ -290,25 +353,6 @@ func (c *Client) upgradeRelease(
 	if err := setIdkeyConfig(values, conf, idFile.AttestationURL); err != nil {
 		return fmt.Errorf("setting id key config: %w", err)
 	}
-		// TODO: v2.8: to here.
-	default:
-		return fmt.Errorf("unknown chart name: %s", chart.Metadata.Name)
-	}
-
-	if err != nil {
-		return fmt.Errorf("loading values: %w", err)
-	}
-
-	values, err = c.prepareValues(values, releaseName)
-	if err != nil {
-		return fmt.Errorf("preparing values: %w", err)
-	}
-
-	err = c.actions.upgradeAction(ctx, releaseName, chart, values, timeout)
-	if err != nil {
-		return err
-	}
-
 	return nil
 }
 
@@ -360,7 +404,6 @@ func (c *Client) updateCRDs(ctx context.Context, chart *chart.Chart) error {
 	return nil
 }
 
-// TODO: v2.8: remove. This function is only temporarily needed as a migration from 2.6 to 2.7.
 // setAttestationVariant sets the attesationVariant value on verification-service and join-service value maps.
 func setAttestationVariant(values map[string]any, variant string) error {
 	joinServiceVals, ok := values["join-service"].(map[string]any)
@@ -378,7 +421,6 @@ func setAttestationVariant(values map[string]any, variant string) error {
 	return nil
 }
 
-// TODO: v2.8: remove. This function is only temporarily needed as a migration from 2.6 to 2.7.
 // setIdkeyConfig sets the idkeyconfig value on the join-service value maps.
 func setIdkeyConfig(values map[string]any, config *config.Config, maaURL string) error {
 	joinServiceVals, ok := values["join-service"].(map[string]any)

cli/internal/helm/client_test.go

@@ -50,7 +50,7 @@ func TestShouldUpgrade(t *testing.T) {
 
 			chart, err := loadChartsDir(helmFS, certManagerInfo.path)
 			require.NoError(err)
-			err = client.shouldUpgrade(certManagerInfo.releaseName, chart)
+			err = client.shouldUpgrade(certManagerInfo.releaseName, chart.Metadata.Version)
 			if tc.wantError {
 				tc.assertCorrectError(t, err)
 				return

e2e/internal/upgrade/image.go

@@ -28,7 +28,7 @@ import (
 type upgradeInfo struct {
 	measurements measurements.M
 	shortPath    string
-	wantImage    string
+	imageRef     string
 }
 
 func fetchUpgradeInfo(ctx context.Context, csp cloudprovider.Provider, toImage string) (upgradeInfo, error) {
@@ -61,7 +61,7 @@ func fetchUpgradeInfo(ctx context.Context, csp cloudprovider.Provider, toImage s
 	}
 	info.measurements = fetchedMeasurements
 
-	wantImage, err := fetchWantImage(ctx, versionsClient, csp, versionsapi.ImageInfo{
+	imageRef, err := fetchImageRef(ctx, versionsClient, csp, versionsapi.ImageInfo{
 		Ref:     ver.Ref,
 		Stream:  ver.Stream,
 		Version: ver.Version,
@@ -69,7 +69,7 @@ func fetchUpgradeInfo(ctx context.Context, csp cloudprovider.Provider, toImage s
 	if err != nil {
 		return upgradeInfo{}, err
 	}
-	info.wantImage = wantImage
+	info.imageRef = imageRef
 
 	return info, nil
 }
@@ -124,7 +124,7 @@ func getFromURL(ctx context.Context, client *http.Client, sourceURL *url.URL) ([
 	return content, nil
 }
 
-func fetchWantImage(ctx context.Context, client *fetcher.Fetcher, csp cloudprovider.Provider, imageInfo versionsapi.ImageInfo) (string, error) {
+func fetchImageRef(ctx context.Context, client *fetcher.Fetcher, csp cloudprovider.Provider, imageInfo versionsapi.ImageInfo) (string, error) {
 	imageInfo, err := client.FetchImageInfo(ctx, imageInfo)
 	if err != nil {
 		return "", err

e2e/internal/upgrade/upgrade_test.go

@@ -101,7 +101,7 @@ func TestUpgrade(t *testing.T) {
 	log.Println(string(data))
 
 	log.Println("Triggering upgrade.")
-	cmd := exec.CommandContext(context.Background(), cli, "upgrade", "apply", "--force", "--debug")
+	cmd := exec.CommandContext(context.Background(), cli, "upgrade", "apply", "--force", "--debug", "-y")
 	msg, err := cmd.CombinedOutput()
 	require.NoErrorf(err, "%s", string(msg))
 	require.NoError(containsUnexepectedMsg(string(msg)))
@@ -192,7 +192,7 @@ func writeUpgradeConfig(require *require.Assertions, image string, kubernetes st
 	err = fileHandler.WriteYAML(constants.ConfigFilename, cfg, file.OptOverwrite)
 	require.NoError(err)
 
-	return versionContainer{image: info.wantImage, kubernetes: kubernetesVersion, microservices: microserviceVersion}
+	return versionContainer{imageRef: info.imageRef, kubernetes: kubernetesVersion, microservices: microserviceVersion}
 }
 
 func testMicroservicesEventuallyHaveVersion(t *testing.T, wantMicroserviceVersion string, timeout time.Duration) {
@@ -226,8 +226,8 @@ func testNodesEventuallyHaveVersion(t *testing.T, k *kubernetes.Clientset, targe
 		for _, node := range nodes.Items {
 			for key, value := range node.Annotations {
 				if key == "constellation.edgeless.systems/node-image" {
-					log.Printf("\t%s: Image %s\n", node.Name, value)
+					if !strings.EqualFold(value, targetVersions.imageRef) {
-					if value != targetVersions.image {
+						log.Printf("\t%s: Image %s, want %s\n", node.Name, value, targetVersions.imageRef)
 						allUpdated = false
 					}
 				}
@@ -329,7 +329,7 @@ func testNodesEventuallyAvailable(t *testing.T, k *kubernetes.Clientset, wantCon
 }
 
 type versionContainer struct {
-	image         string
+	imageRef      string
 	kubernetes    semver.Semver
 	microservices string
 }

hack/pseudo-version/pseudo-version.go

@@ -95,9 +95,15 @@ func main() {
 	default:
 		if !strings.Contains(*base, "pre") {
 			// "v2.7.0" inside the version.txt will lead to "v2.7.0" as version
-			fmt.Println(*base)
+			// "2.7.0" inside the version.txt will lead to "2.7.0" as version
+			if *skipV {
+				fmt.Println(strings.TrimPrefix(*base, "v"))
 			} else {
-			// "2.7.0-pre" inside the version.txt will lead to "v2.7.0-pre.0.20230313121936-bab76e8a9acf" as version
+				fmt.Println(*base)
+			}
+		} else {
+			// "v2.7.0-pre" inside the version.txt will lead to "v2.7.0-pre.0.20230313121936-bab76e8a9acf" as version
+			// "2.7.0-pre" inside the version.txt will lead to "v0.0.0-20230313121936-bab76e8a9acf" as version
 			fmt.Println(version)
 		}
 	}