فهرست منبع

test(e2e): skip the global addon uninstall on disposable clusters

Every leg ended by uninstalling the ESO release from a kind cluster that is
destroyed seconds later. It costs about 64s per leg, measured identically across
two legs, and it is the only step that can fail a leg whose specs all passed: the
release owns the CRDs, so `helm uninstall --wait` waits for CRD deletion, which
blocks on finalizers the controller being removed by the same uninstall is no
longer there to release.

E2E_SKIP_GLOBAL_TEARDOWN gates it, default off, so anything running against a
cluster it does not own behaves exactly as before. The suites share one cluster
per pod and two of them install an "eso" release with different values, so the
flag is refused whenever TEST_SUITES names more than one suite. test.managed
overrides it away, since that target runs against the current kube context.

flux's own uninstallFlux is gated with it rather than left running. It deletes the
flux-system namespace, whose termination needs helm-controller to clear the
HelmRelease finalizer that UninstallGlobalAddons used to clear first, and kubectl
delete waits on that with a 168h default.

Fixes: external-secrets/external-secrets#6777
Signed-off-by: Alexander Chernov <alexander@chernov.it>
Alexander Chernov 1 ماه پیش
والد
کامیت
3257ab3aa7

+ 7 - 0
.github/workflows/e2e-reusable.yml

@@ -231,6 +231,13 @@ jobs:
       # both into the e2e pod; entrypoint.sh runs ginkgo with them.
       TEST_SUITES: ${{ matrix.suite }}
       GINKGO_LABELS: ${{ matrix.labels }}
+      # Each leg gets a fresh kind cluster and throws it away with the runner, so
+      # uninstalling the global addons on the way out costs about a minute and
+      # buys nothing. Safe here specifically because TEST_SUITES above is a
+      # single suite: the suites share a cluster, and two of them install an
+      # "eso" release with different values, so the helper ignores this whenever
+      # more than one suite is scheduled.
+      E2E_SKIP_GLOBAL_TEARDOWN: "true"
     steps:
       - uses: step-security/harden-runner@bf7454d06d71f1098171f2acdf0cd4708d7b5920 # v2.20.0
         with:

+ 6 - 0
docs/contributing/process.md

@@ -91,6 +91,12 @@ run managed tests.
 make test.e2e GINKGO_LABELS='gcp&&!managed'
 ```
 
+Setting `E2E_SKIP_GLOBAL_TEARDOWN=true` leaves the global addons (the ESO release
+itself) installed when the suite finishes, which saves about a minute. Only do
+that against a throwaway cluster you are about to delete: it is exported like the
+other variables above, so it applies to whatever your kube context points at.
+CI sets it on the kind legs, and `make test.managed` clears it.
+
 #### Managed Kubernetes e2e tests
 
 There's another suite of e2e tests that integrate with managed Kubernetes offerings.

+ 10 - 0
e2e/Makefile

@@ -8,6 +8,10 @@ DOCKER_BUILD_ARGS     ?=
 export E2E_IMAGE_NAME ?= ghcr.io/external-secrets/external-secrets-e2e
 export GINKGO_LABELS ?= !managed
 export TEST_SUITES ?= provider generator flux argocd
+# Leave the global addons installed on the way out. Only sensible for a cluster
+# you are about to delete; ignored when TEST_SUITES names more than one suite,
+# since the suites share a cluster. Off by default, CI sets it per leg.
+export E2E_SKIP_GLOBAL_TEARDOWN ?=
 
 export OCI_IMAGE_NAME = ghcr.io/external-secrets/external-secrets
 
@@ -46,6 +50,12 @@ test.run: ## Load prebuilt image tarballs into kind and run the e2e suite
 	kind load image-archive --name="external-secrets" $(E2E_ARTIFACT_DIR)/e2e.tar
 	./run.sh
 
+# This target runs against whatever the current kube context points at, which is
+# not ours to leave dirty. Makefile exports are file scoped, so without this an
+# exported E2E_SKIP_GLOBAL_TEARDOWN would reach the recipe and leave ESO
+# installed on a real cluster. override also beats a command-line assignment,
+# which a plain target-specific value would not.
+test.managed: override E2E_SKIP_GLOBAL_TEARDOWN =
 test.managed: e2e-image ## Run e2e tests against current kube context
 	$(MAKE) -C ../ docker.build \
 		VERSION=$(VERSION) \

+ 16 - 1
e2e/README.md

@@ -21,7 +21,7 @@ scoped per provider, and how to add or enable a provider.
 | `e2e/suites/provider/cases/import.go` | Blank-imports every provider case into the single `provider.test` binary. Providers are told apart at run time by Ginkgo label. |
 | `e2e/matrix.yaml` | Source of truth for the fan-out: one `area` (leg) per provider, with its suite, label filter, secret groups, and trigger paths. |
 | `e2e/matrix.py` | Validates the matrix (`check`), emits the CI matrix JSON (`json`), and prints the per-leg credential plan (`plan`). |
-| `e2e/run.sh` | Host-side launcher. Runs `kubectl run` to start the e2e pod, forwarding `TEST_SUITES`, `GINKGO_LABELS`, and the (scoped) credentials as pod env. |
+| `e2e/run.sh` | Host-side launcher. Runs `kubectl run` to start the e2e pod, forwarding `TEST_SUITES`, `GINKGO_LABELS`, `E2E_SKIP_GLOBAL_TEARDOWN`, and the (scoped) credentials as pod env. |
 | `e2e/entrypoint.sh` | In-pod entry (image `CMD`). Loops over `TEST_SUITES` and runs `ginkgo -label-filter="$GINKGO_LABELS"` against each `<suite>.test`. |
 | `.github/workflows/e2e.yml` | Non-managed e2e. Fans out into per-provider legs. Owns the `e2e-required` gate. |
 | `.github/workflows/e2e-reusable.yml` | The reusable build + matrix-test pipeline that `e2e.yml` calls. |
@@ -157,6 +157,21 @@ make -C e2e matrix.plan
 
 # run a single provider locally (overrides the Makefile defaults)
 make -C e2e test.run TEST_SUITES=provider GINKGO_LABELS="vault && !managed"
+
+# skip uninstalling the global addons on the way out, for a cluster you are
+# about to delete anyway. Saves about a minute per run on the provider,
+# generator and argocd suites; every kind leg in e2e-reusable.yml sets it, and
+# e2e-managed.yml deliberately does not.
+#
+# Refused, with a line on stderr, when TEST_SUITES names more than one suite:
+# entrypoint.sh runs those against one cluster and two of them install an "eso"
+# release with different values. That guard only sees its own process, so two
+# separate single-suite runs against one cluster would still collide.
+#
+# Do not use it against a cluster you did not create. make test.managed clears
+# it for that reason.
+make -C e2e test.run TEST_SUITES=provider GINKGO_LABELS="vault && !managed" \
+  E2E_SKIP_GLOBAL_TEARDOWN=true
 ```
 
 ## Adding or enabling a provider

+ 66 - 0
e2e/framework/addon/addon.go

@@ -17,8 +17,11 @@ limitations under the License.
 package addon
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
+	"strconv"
+	"strings"
 
 	"github.com/onsi/ginkgo/v2"
 	"github.com/onsi/gomega"
@@ -78,6 +81,69 @@ func UninstallGlobalAddons() {
 	}
 }
 
+// skipGlobalTeardownVar opts a run out of uninstalling the global addons.
+// e2e/run.sh forwards it into the e2e pod; CI sets it per leg.
+const skipGlobalTeardownVar = "E2E_SKIP_GLOBAL_TEARDOWN"
+
+// SkipGlobalTeardown reports whether a suite should leave the global addons
+// installed instead of uninstalling them on the way out.
+//
+// CI runs every leg against a kind cluster that is discarded immediately
+// afterwards, so uninstalling the ESO release costs about a minute per leg and
+// buys nothing. It is also the only step that can fail a leg whose specs all
+// passed: the release owns the CRDs, so `helm uninstall --wait` waits for CRD
+// deletion, which blocks on finalizers that the controller being removed by the
+// same uninstall is no longer around to release.
+//
+// Off unless asked for, so a run against a cluster it does not own (notably
+// `make test.managed`) keeps cleaning up exactly as before.
+//
+// The TEST_SUITES check is not optional. entrypoint.sh runs each suite binary in
+// turn against one cluster, and the provider and generator suites both install
+// an "eso" release with different values, so a suite that left its release
+// behind would break the next suite's install. A multi-suite run therefore tears
+// down as normal and logs why the request was refused.
+func SkipGlobalTeardown() bool {
+	raw, ok := os.LookupEnv(skipGlobalTeardownVar)
+	if !ok || raw == "" {
+		return false
+	}
+	skip, err := strconv.ParseBool(raw)
+	if err != nil {
+		// Fall back to tearing down, which is the safe answer, and say so
+		// loudly. Failing here instead would unwind the whole AfterSuite, so a
+		// typo would leave the cluster with neither a teardown nor its logs.
+		teardownLogf("%s is not a boolean (%q), so the teardown will run: %v",
+			skipGlobalTeardownVar, raw, err)
+		return false
+	}
+	if !skip {
+		return false
+	}
+	// Only covers suites sharing one process's cluster, which is what
+	// entrypoint.sh does. Two separate single-suite runs pointed at the same
+	// cluster are indistinguishable from here and would still collide.
+	if suites := strings.Fields(os.Getenv("TEST_SUITES")); len(suites) > 1 {
+		teardownLogf("%s ignored: suites %q share one cluster, so the global "+
+			"addons have to come out between them", skipGlobalTeardownVar,
+			strings.Join(suites, " "))
+		return false
+	}
+	teardownLogf("%s set: leaving the global addons installed for the cluster to "+
+		"be discarded with", skipGlobalTeardownVar)
+	return true
+}
+
+// teardownLogf reports a teardown decision on stderr.
+//
+// Not log.Logf: that writes to GinkgoWriter, and ginkgo drops a passing node's
+// writer output unless the suite runs with -v, which CI does not. These lines
+// have to survive a green run, since they are the only evidence of whether the
+// skip took effect.
+func teardownLogf(format string, args ...any) {
+	fmt.Fprintf(os.Stderr, format+"\n", args...)
+}
+
 // AssetDir returns the path to the k8s asset directory
 // which holds the helm charts, vault and conjur configuration.
 // It starts at the cwd, and walks its way up to the root.

+ 1 - 0
e2e/run.sh

@@ -85,5 +85,6 @@ kubectl run --rm \
   --env="GRAFANA_TOKEN=${GRAFANA_TOKEN:-}" \
   --env="VERSION=${VERSION}" \
   --env="TEST_SUITES=${TEST_SUITES}" \
+  --env="E2E_SKIP_GLOBAL_TEARDOWN=${E2E_SKIP_GLOBAL_TEARDOWN:-}" \
   --overrides='{ "apiVersion": "v1", "spec":{"serviceAccountName": "external-secrets-e2e"}}' \
   e2e --image=${E2E_IMAGE_NAME}:${VERSION}

+ 14 - 10
e2e/suites/argocd/suite_test.go

@@ -41,18 +41,22 @@ var _ = SynchronizedBeforeSuite(func() []byte {
 var _ = SynchronizedAfterSuite(func() {
 	// noop
 }, func() {
-	_, _, cl := util.NewConfig()
-	By("Deleting any pending generator states")
-	generatorStates := &genv1alpha1.GeneratorStateList{}
-	err := cl.List(GinkgoT().Context(), generatorStates)
-	Expect(err).ToNot(HaveOccurred())
-	for _, generatorState := range generatorStates.Items {
-		err = cl.Delete(GinkgoT().Context(), &generatorState)
+	// The pre-deletion below exists only so the uninstall that follows it can
+	// complete, so it is skipped together with it.
+	if !addon.SkipGlobalTeardown() {
+		_, _, cl := util.NewConfig()
+		By("Deleting any pending generator states")
+		generatorStates := &genv1alpha1.GeneratorStateList{}
+		err := cl.List(GinkgoT().Context(), generatorStates)
 		Expect(err).ToNot(HaveOccurred())
-	}
+		for _, generatorState := range generatorStates.Items {
+			err = cl.Delete(GinkgoT().Context(), &generatorState)
+			Expect(err).ToNot(HaveOccurred())
+		}
 
-	By("Cleaning up global addons")
-	addon.UninstallGlobalAddons()
+		By("Cleaning up global addons")
+		addon.UninstallGlobalAddons()
+	}
 	if CurrentSpecReport().Failed() {
 		addon.PrintLogs()
 	}

+ 21 - 12
e2e/suites/flux/suite_test.go

@@ -41,23 +41,32 @@ var _ = SynchronizedBeforeSuite(func() []byte {
 var _ = SynchronizedAfterSuite(func() {
 	// noop
 }, func() {
-	cfg := &addon.Config{}
-	cfg.KubeConfig, cfg.KubeClientSet, cfg.CRClient = util.NewConfig()
-	By("Deleting any pending generator states")
-	generatorStates := &genv1alpha1.GeneratorStateList{}
-	err := cfg.CRClient.List(GinkgoT().Context(), generatorStates)
-	Expect(err).ToNot(HaveOccurred())
-	for _, generatorState := range generatorStates.Items {
-		err = cfg.CRClient.Delete(GinkgoT().Context(), &generatorState)
+	// uninstallFlux is gated together with the global addons, not left running.
+	// FluxHelmRelease.Uninstall deletes the HelmRelease and waits for its
+	// finalizers.fluxcd.io finalizer to clear; without that, uninstallFlux's
+	// `kubectl delete -f install.yaml` removes the flux-system namespace while
+	// helm-controller is still needed to release that finalizer, and kubectl
+	// waits on it with a 168h default. That hangs the suite until ginkgo's
+	// timeout, on a leg whose specs all passed.
+	if !addon.SkipGlobalTeardown() {
+		cfg := &addon.Config{}
+		cfg.KubeConfig, cfg.KubeClientSet, cfg.CRClient = util.NewConfig()
+		By("Deleting any pending generator states")
+		generatorStates := &genv1alpha1.GeneratorStateList{}
+		err := cfg.CRClient.List(GinkgoT().Context(), generatorStates)
 		Expect(err).ToNot(HaveOccurred())
-	}
+		for _, generatorState := range generatorStates.Items {
+			err = cfg.CRClient.Delete(GinkgoT().Context(), &generatorState)
+			Expect(err).ToNot(HaveOccurred())
+		}
 
-	By("Cleaning up global addons")
-	addon.UninstallGlobalAddons()
+		By("Cleaning up global addons")
+		addon.UninstallGlobalAddons()
+		uninstallFlux()
+	}
 	if CurrentSpecReport().Failed() {
 		addon.PrintLogs()
 	}
-	uninstallFlux()
 })
 
 func TestE2E(t *testing.T) {

+ 14 - 10
e2e/suites/generator/suite_test.go

@@ -44,18 +44,22 @@ var _ = SynchronizedBeforeSuite(func() []byte {
 var _ = SynchronizedAfterSuite(func() {
 	// noop
 }, func() {
-	cfg := &addon.Config{}
-	cfg.KubeConfig, cfg.KubeClientSet, cfg.CRClient = util.NewConfig()
-	By("Deleting any pending generator states")
-	generatorStates := &genv1alpha1.GeneratorStateList{}
-	err := cfg.CRClient.List(GinkgoT().Context(), generatorStates)
-	Expect(err).ToNot(HaveOccurred())
-	for _, generatorState := range generatorStates.Items {
-		err = cfg.CRClient.Delete(GinkgoT().Context(), &generatorState)
+	// The pre-deletion below exists only so the uninstall that follows it can
+	// complete, so it is skipped together with it.
+	if !addon.SkipGlobalTeardown() {
+		cfg := &addon.Config{}
+		cfg.KubeConfig, cfg.KubeClientSet, cfg.CRClient = util.NewConfig()
+		By("Deleting any pending generator states")
+		generatorStates := &genv1alpha1.GeneratorStateList{}
+		err := cfg.CRClient.List(GinkgoT().Context(), generatorStates)
 		Expect(err).ToNot(HaveOccurred())
+		for _, generatorState := range generatorStates.Items {
+			err = cfg.CRClient.Delete(GinkgoT().Context(), &generatorState)
+			Expect(err).ToNot(HaveOccurred())
+		}
+		By("Cleaning up global addons")
+		addon.UninstallGlobalAddons()
 	}
-	By("Cleaning up global addons")
-	addon.UninstallGlobalAddons()
 	if CurrentSpecReport().Failed() {
 		addon.PrintLogs()
 	}

+ 22 - 18
e2e/suites/provider/suite_test.go

@@ -44,29 +44,33 @@ var _ = SynchronizedBeforeSuite(func() []byte {
 var _ = SynchronizedAfterSuite(func() {
 	// noop
 }, func() {
-	cfg := &addon.Config{}
-	cfg.KubeConfig, cfg.KubeClientSet, cfg.CRClient = util.NewConfig()
+	// The pre-deletions below exist only so the uninstall that follows them can
+	// complete, so they are skipped together with it.
+	if !addon.SkipGlobalTeardown() {
+		cfg := &addon.Config{}
+		cfg.KubeConfig, cfg.KubeClientSet, cfg.CRClient = util.NewConfig()
 
-	By("Deleting any pending generator states")
-	generatorStates := &genv1alpha1.GeneratorStateList{}
-	err := cfg.CRClient.List(GinkgoT().Context(), generatorStates)
-	Expect(err).ToNot(HaveOccurred())
-	for _, generatorState := range generatorStates.Items {
-		err = cfg.CRClient.Delete(GinkgoT().Context(), &generatorState)
+		By("Deleting any pending generator states")
+		generatorStates := &genv1alpha1.GeneratorStateList{}
+		err := cfg.CRClient.List(GinkgoT().Context(), generatorStates)
 		Expect(err).ToNot(HaveOccurred())
-	}
+		for _, generatorState := range generatorStates.Items {
+			err = cfg.CRClient.Delete(GinkgoT().Context(), &generatorState)
+			Expect(err).ToNot(HaveOccurred())
+		}
 
-	By("Deleting all ClusterExternalSecrets")
-	externalSecretsList := &v1.ClusterExternalSecretList{}
-	err = cfg.CRClient.List(GinkgoT().Context(), externalSecretsList)
-	Expect(err).ToNot(HaveOccurred())
-	for _, externalSecret := range externalSecretsList.Items {
-		err = cfg.CRClient.Delete(GinkgoT().Context(), &externalSecret)
+		By("Deleting all ClusterExternalSecrets")
+		externalSecretsList := &v1.ClusterExternalSecretList{}
+		err = cfg.CRClient.List(GinkgoT().Context(), externalSecretsList)
 		Expect(err).ToNot(HaveOccurred())
-	}
+		for _, externalSecret := range externalSecretsList.Items {
+			err = cfg.CRClient.Delete(GinkgoT().Context(), &externalSecret)
+			Expect(err).ToNot(HaveOccurred())
+		}
 
-	By("Cleaning up global addons")
-	addon.UninstallGlobalAddons()
+		By("Cleaning up global addons")
+		addon.UninstallGlobalAddons()
+	}
 	if CurrentSpecReport().Failed() {
 		addon.PrintLogs()
 	}