cli: fix duplicate backup creation during upgrade apply (#1997)

* Use CLI to fetch measurements in e2e test

* Abort helm service upgrade early if user confirmation is missing

* Add container push to CLI build action

---------

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

.github/actions/build_cli/action.yml

@@ -31,6 +31,10 @@ inputs:
   outputPath:
     description: "Output path of the binary"
     required: false
+  push:
+    description: "Push container images"
+    required: false
+    default: false
 runs:
   using: "composite"
   steps:
@@ -66,6 +70,11 @@ runs:
         export PATH="$PATH:$(dirname "${OUTPUT_PATH}")"
         echo "::endgroup::"
 
+    - name: Upload container images
+      if: inputs.push == 'true'
+      shell: bash
+      run: bazel run //bazel/release:push
+
     # TODO(3u13r): Replace with https://github.com/sigstore/sigstore-installer/tree/initial
     # once it has the functionality
     - name: Install Cosign

.github/actions/e2e_test/action.yml

@@ -131,6 +131,7 @@ runs:
         targetArch: ${{ steps.determine-build-target.outputs.hostArch }}
         enterpriseCLI: true
         outputPath: "build/constellation"
+        push: ${{ inputs.cliVersion == '' }}
 
     - name: Download CLI
       if: inputs.cliVersion != ''
@@ -162,11 +163,6 @@ runs:
         targetOS: ${{ steps.determine-build-target.outputs.hostOS }}
         targetArch: ${{ steps.determine-build-target.outputs.hostArch }}
 
-    - name: Upload container images
-      if: inputs.cliVersion == ''
-      shell: bash
-      run: bazel run //bazel/release:push
-
     - name: Login to GCP (IAM service account)
       if: inputs.cloudProvider == 'gcp'
       uses: ./.github/actions/login_gcp

.github/workflows/e2e-upgrade.yml

@@ -156,6 +156,13 @@ jobs:
           registry: ghcr.io
           githubToken: ${{ secrets.GITHUB_TOKEN }}
 
+      - name: Build CLI
+        uses: ./.github/actions/build_cli
+        with:
+          enterpriseCLI: true
+          outputPath: "build/constellation"
+          push: true
+
       - name: Run upgrade test
         env:
           KUBECONFIG: ${{ steps.e2e_test.outputs.kubeconfig }}
@@ -176,7 +183,6 @@ jobs:
             KUBERNETES_FLAG="--target-kubernetes=$KUBERNETES"
           fi
 
-          bazel run //bazel/release:push
           bazel run //e2e/internal/upgrade:upgrade_test -- --want-worker "$WORKERNODES" --want-control "$CONTROLNODES" --target-image "$IMAGE" "$KUBERNETES_FLAG" "$MICROSERVICES_FLAG"
 
       - name: Always fetch logs

cli/internal/helm/backup.go

@@ -18,6 +18,7 @@ import (
 )
 
 func (c *Client) backupCRDs(ctx context.Context, upgradeID string) ([]apiextensionsv1.CustomResourceDefinition, error) {
+	c.log.Debugf("Starting CRD backup")
 	crds, err := c.kubectl.GetCRDs(ctx)
 	if err != nil {
 		return nil, fmt.Errorf("getting CRDs: %w", err)
@@ -30,6 +31,8 @@ func (c *Client) backupCRDs(ctx context.Context, upgradeID string) ([]apiextensi
 	for i := range crds {
 		path := filepath.Join(crdBackupFolder, crds[i].Name+".yaml")
 
+		c.log.Debugf("Creating CRD backup: %s", path)
+
 		// We have to manually set kind/apiversion because of a long-standing limitation of the API:
 		// https://github.com/kubernetes/kubernetes/issues/3030#issuecomment-67543738
 		// The comment states that kind/version are encoded in the type.
@@ -44,14 +47,15 @@ func (c *Client) backupCRDs(ctx context.Context, upgradeID string) ([]apiextensi
 		if err := c.fs.Write(path, yamlBytes); err != nil {
 			return nil, err
 		}
-
-		c.log.Debugf("Created backup crd: %s", path)
 	}
+	c.log.Debugf("CRD backup complete")
 	return crds, nil
 }
 
 func (c *Client) backupCRs(ctx context.Context, crds []apiextensionsv1.CustomResourceDefinition, upgradeID string) error {
+	c.log.Debugf("Starting CR backup")
 	for _, crd := range crds {
+		c.log.Debugf("Creating backup for resource type: %s", crd.Name)
 		for _, version := range crd.Spec.Versions {
 			gvr := schema.GroupVersionResource{Group: crd.Spec.Group, Version: version.Name, Resource: crd.Spec.Names.Plural}
 			crs, err := c.kubectl.GetCRs(ctx, gvr)
@@ -76,8 +80,9 @@ func (c *Client) backupCRs(ctx context.Context, crds []apiextensionsv1.CustomRes
 			}
 		}
 
-		c.log.Debugf("Created backups for resource type: %s", crd.Name)
+		c.log.Debugf("Backup for resource type %q complete", crd.Name)
 	}
+	c.log.Debugf("CR backup complete")
 	return nil
 }
 

cli/internal/helm/client.go

@@ -133,6 +133,14 @@ func (c *Client) Upgrade(ctx context.Context, config *config.Config, timeout tim
 			return fmt.Errorf("should upgrade %s: %w", info.releaseName, err)
 		case err == nil:
 			upgradeReleases = append(upgradeReleases, chart)
+
+			// Check if installing/upgrading the chart could be destructive
+			// If so, we don't want to perform any actions,
+			// unless the user confirms it to be OK.
+			if !allowDestructive &&
+				info.chartName == certManagerInfo.chartName {
+				return ErrConfirmationMissing
+			}
 		}
 	}
 
@@ -149,7 +157,7 @@ func (c *Client) Upgrade(ctx context.Context, config *config.Config, timeout tim
 	}
 
 	for _, chart := range upgradeReleases {
-		err = c.upgradeRelease(ctx, timeout, config, chart, allowDestructive)
+		err = c.upgradeRelease(ctx, timeout, config, chart)
 		if err != nil {
 			return fmt.Errorf("upgrading %s: %w", chart.Metadata.Name, err)
 		}
@@ -245,7 +253,7 @@ func (s ServiceVersions) ConstellationServices() string {
 }
 
 func (c *Client) upgradeRelease(
-	ctx context.Context, timeout time.Duration, conf *config.Config, chart *chart.Chart, allowDestructive bool,
+	ctx context.Context, timeout time.Duration, conf *config.Config, chart *chart.Chart,
 ) error {
 	// 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.
@@ -268,10 +276,6 @@ func (c *Client) upgradeRelease(
 	case certManagerInfo.chartName:
 		releaseName = certManagerInfo.releaseName
 		values = loader.loadCertManagerValues()
-
-		if !allowDestructive {
-			return ErrConfirmationMissing
-		}
 	case constellationOperatorsInfo.chartName:
 		releaseName = constellationOperatorsInfo.releaseName
 		values, err = loader.loadOperatorsValues()

cli/internal/helm/client_test.go

@@ -60,22 +60,11 @@ func TestShouldUpgrade(t *testing.T) {
 
 func TestUpgradeRelease(t *testing.T) {
 	testCases := map[string]struct {
-		allowDestructive   bool
+		version   string
-		version            string
+		wantError bool
-		assertCorrectError func(t *testing.T, err error) bool
-		wantError          bool
 	}{
 		"allow": {
-			allowDestructive: true,
+			version: "1.9.0",
-			version:          "1.9.0",
-		},
-		"deny": {
-			allowDestructive: false,
-			version:          "1.9.0",
-			assertCorrectError: func(t *testing.T, err error) bool {
-				return assert.ErrorIs(t, err, ErrConfirmationMissing)
-			},
-			wantError: true,
 		},
 	}
 
@@ -88,9 +77,9 @@ func TestUpgradeRelease(t *testing.T) {
 
 			chart, err := loadChartsDir(helmFS, certManagerInfo.path)
 			require.NoError(err)
-			err = client.upgradeRelease(context.Background(), 0, config.Default(), chart, tc.allowDestructive)
+			err = client.upgradeRelease(context.Background(), 0, config.Default(), chart)
 			if tc.wantError {
-				tc.assertCorrectError(t, err)
+				assert.Error(err)
 				return
 			}
 			assert.NoError(err)

e2e/internal/upgrade/BUILD.bazel

@@ -5,18 +5,12 @@ go_library(
     name = "upgrade",
     srcs = [
         "helm.go",
-        "image.go",
         "upgrade.go",
     ],
     importpath = "github.com/edgelesssys/constellation/v2/e2e/internal/upgrade",
     visibility = ["//e2e:__subpackages__"],
     deps = [
-        "//internal/api/versionsapi",
-        "//internal/attestation/measurements",
-        "//internal/attestation/variant",
-        "//internal/cloud/cloudprovider",
         "//internal/constants",
-        "//internal/imagefetcher",
         "//internal/logger",
         "@sh_helm_helm_v3//pkg/action",
         "@sh_helm_helm_v3//pkg/cli",
@@ -44,6 +38,7 @@ go_test(
         "//internal/config",
         "//internal/constants",
         "//internal/file",
+        "//internal/imagefetcher",
         "//internal/semver",
         "//internal/versions",
         "@com_github_spf13_afero//:afero",

e2e/internal/upgrade/image.go

@@ -1,64 +0,0 @@
-//go:build e2e
-
-/*
-Copyright (c) Edgeless Systems GmbH
-
-SPDX-License-Identifier: AGPL-3.0-only
-*/
-
-package upgrade
-
-import (
-	"context"
-	"net/http"
-
-	"github.com/edgelesssys/constellation/v2/internal/api/versionsapi"
-	"github.com/edgelesssys/constellation/v2/internal/attestation/measurements"
-	"github.com/edgelesssys/constellation/v2/internal/attestation/variant"
-	"github.com/edgelesssys/constellation/v2/internal/cloud/cloudprovider"
-	"github.com/edgelesssys/constellation/v2/internal/imagefetcher"
-)
-
-type upgradeInfo struct {
-	measurements measurements.M
-	shortPath    string
-	imageRef     string
-}
-
-func fetchUpgradeInfo(ctx context.Context, csp cloudprovider.Provider,
-	attestationVariant variant.Variant, toImage, region string,
-) (upgradeInfo, error) {
-	info := upgradeInfo{
-		measurements: make(measurements.M),
-		shortPath:    toImage,
-	}
-
-	ver, err := versionsapi.NewVersionFromShortPath(toImage, versionsapi.VersionKindImage)
-	if err != nil {
-		return upgradeInfo{}, err
-	}
-
-	measurementsURL, _, err := versionsapi.MeasurementURL(ver)
-	if err != nil {
-		return upgradeInfo{}, err
-	}
-
-	fetchedMeasurements := measurements.M{}
-	if err := fetchedMeasurements.FetchNoVerify(
-		ctx, http.DefaultClient,
-		measurementsURL,
-		ver, csp, attestationVariant,
-	); err != nil {
-		return upgradeInfo{}, err
-	}
-	info.measurements = fetchedMeasurements
-
-	fetcher := imagefetcher.New()
-	imageRef, err := fetcher.FetchReference(ctx, csp, attestationVariant, toImage, region)
-	if err != nil {
-		return upgradeInfo{}, err
-	}
-	info.imageRef = imageRef
-
-	return info, nil
-}

e2e/internal/upgrade/upgrade_test.go

@@ -29,6 +29,7 @@ import (
 	"github.com/edgelesssys/constellation/v2/internal/config"
 	"github.com/edgelesssys/constellation/v2/internal/constants"
 	"github.com/edgelesssys/constellation/v2/internal/file"
+	"github.com/edgelesssys/constellation/v2/internal/imagefetcher"
 	"github.com/edgelesssys/constellation/v2/internal/semver"
 	"github.com/edgelesssys/constellation/v2/internal/versions"
 	"github.com/spf13/afero"
@@ -86,6 +87,12 @@ func TestUpgrade(t *testing.T) {
 
 	targetVersions := writeUpgradeConfig(require, *targetImage, *targetKubernetes, *targetMicroservices)
 
+	log.Println("Fetching measurements for new image.")
+	cmd = exec.CommandContext(context.Background(), cli, "config", "fetch-measurements", "--insecure", "--debug")
+	stdout, stderr, err = runCommandWithSeparateOutputs(cmd)
+	require.NoError(err, "Stdout: %s\nStderr: %s", string(stdout), string(stderr))
+	log.Println(string(stdout))
+
 	data, err := os.ReadFile("./constellation-conf.yaml")
 	require.NoError(err)
 	log.Println(string(data))
@@ -254,8 +261,8 @@ func testNodesEventuallyAvailable(t *testing.T, k *kubernetes.Clientset, wantCon
 
 func writeUpgradeConfig(require *require.Assertions, image string, kubernetes string, microservices string) versionContainer {
 	fileHandler := file.NewHandler(afero.NewOsFs())
-	fetcher := attestationconfigapi.NewFetcher()
+	attestationFetcher := attestationconfigapi.NewFetcher()
-	cfg, err := config.New(fileHandler, constants.ConfigFilename, fetcher, true)
+	cfg, err := config.New(fileHandler, constants.ConfigFilename, attestationFetcher, true)
 	var cfgErr *config.ValidationError
 	var longMsg string
 	if errors.As(err, &cfgErr) {
@@ -263,7 +270,8 @@ func writeUpgradeConfig(require *require.Assertions, image string, kubernetes st
 	}
 	require.NoError(err, longMsg)
 
-	info, err := fetchUpgradeInfo(
+	imageFetcher := imagefetcher.New()
+	imageRef, err := imageFetcher.FetchReference(
 		context.Background(),
 		cfg.GetProvider(),
 		cfg.GetAttestationConfig().GetVariant(),
@@ -272,9 +280,8 @@ func writeUpgradeConfig(require *require.Assertions, image string, kubernetes st
 	)
 	require.NoError(err)
 
-	log.Printf("Setting image version: %s\n", info.shortPath)
+	log.Printf("Setting image version: %s\n", image)
-	cfg.Image = info.shortPath
+	cfg.Image = image
-	cfg.UpdateMeasurements(info.measurements)
 
 	defaultConfig := config.Default()
 	var kubernetesVersion semver.Semver
@@ -301,7 +308,7 @@ func writeUpgradeConfig(require *require.Assertions, image string, kubernetes st
 	err = fileHandler.WriteYAML(constants.ConfigFilename, cfg, file.OptOverwrite)
 	require.NoError(err)
 
-	return versionContainer{imageRef: info.imageRef, kubernetes: kubernetesVersion, microservices: microserviceVersion}
+	return versionContainer{imageRef: imageRef, kubernetes: kubernetesVersion, microservices: microserviceVersion}
 }
 
 // runUpgradeCheck executes 'upgrade check' and does basic checks on the output.