From 90dbeae16bf5c82463bd1719ed29d8969f69e1bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Wei=C3=9Fe?= <66256922+daniel-weisse@users.noreply.github.com> Date: Mon, 3 Jul 2023 15:13:36 +0200 Subject: [PATCH] cli: fix duplicate backup creation during `upgrade apply` (#1997) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- .github/actions/build_cli/action.yml | 9 ++++ .github/actions/e2e_test/action.yml | 6 +-- .github/workflows/e2e-upgrade.yml | 8 +++- cli/internal/helm/backup.go | 11 +++-- cli/internal/helm/client.go | 16 ++++--- cli/internal/helm/client_test.go | 21 +++------ e2e/internal/upgrade/BUILD.bazel | 7 +-- e2e/internal/upgrade/image.go | 64 ---------------------------- e2e/internal/upgrade/upgrade_test.go | 21 ++++++--- 9 files changed, 55 insertions(+), 108 deletions(-) delete mode 100644 e2e/internal/upgrade/image.go diff --git a/.github/actions/build_cli/action.yml b/.github/actions/build_cli/action.yml index 073fb29ba..bef9f9e6a 100644 --- a/.github/actions/build_cli/action.yml +++ b/.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 diff --git a/.github/actions/e2e_test/action.yml b/.github/actions/e2e_test/action.yml index 29c807079..9c792af9c 100644 --- a/.github/actions/e2e_test/action.yml +++ b/.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 diff --git a/.github/workflows/e2e-upgrade.yml b/.github/workflows/e2e-upgrade.yml index 0ba83bb9c..de29f2816 100644 --- a/.github/workflows/e2e-upgrade.yml +++ b/.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 diff --git a/cli/internal/helm/backup.go b/cli/internal/helm/backup.go index d7ec77a4a..35deea614 100644 --- a/cli/internal/helm/backup.go +++ b/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 } diff --git a/cli/internal/helm/client.go b/cli/internal/helm/client.go index 84dff51dc..e1da410d0 100644 --- a/cli/internal/helm/client.go +++ b/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() diff --git a/cli/internal/helm/client_test.go b/cli/internal/helm/client_test.go index f04808347..efd16f226 100644 --- a/cli/internal/helm/client_test.go +++ b/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 - assertCorrectError func(t *testing.T, err error) bool - wantError bool + version string + wantError bool }{ "allow": { - allowDestructive: true, - 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, + version: "1.9.0", }, } @@ -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) diff --git a/e2e/internal/upgrade/BUILD.bazel b/e2e/internal/upgrade/BUILD.bazel index a86eab72f..3549227d6 100644 --- a/e2e/internal/upgrade/BUILD.bazel +++ b/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", diff --git a/e2e/internal/upgrade/image.go b/e2e/internal/upgrade/image.go deleted file mode 100644 index b6330eb13..000000000 --- a/e2e/internal/upgrade/image.go +++ /dev/null @@ -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 -} diff --git a/e2e/internal/upgrade/upgrade_test.go b/e2e/internal/upgrade/upgrade_test.go index 383151e3d..fbaacaee8 100644 --- a/e2e/internal/upgrade/upgrade_test.go +++ b/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() - cfg, err := config.New(fileHandler, constants.ConfigFilename, fetcher, true) + attestationFetcher := attestationconfigapi.NewFetcher() + 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) - cfg.Image = info.shortPath - cfg.UpdateMeasurements(info.measurements) + log.Printf("Setting image version: %s\n", image) + cfg.Image = image 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.