From 9a829c8d80470f3a312e3d446fd451af499c825f Mon Sep 17 00:00:00 2001 From: "Per G. da Silva" Date: Wed, 14 Oct 2026 15:33:56 +0200 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=90=9B=20Fix=20VAP=20access-control?= =?UTF-8?q?=20e2e=20flake=20under=20HA=20control=20planes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "Denied - ..." scenarios in vap-access-control.feature intermittently failed on HA (multi-replica) control planes with "expected apply to fail ..., but it succeeded". ResourceApplyAsUserFails waited for ValidatingAdmissionPolicy enforcement with a server dry-run poll and then fired a single real apply. VAP binding enforcement propagates per kube-apiserver replica and is eventually consistent, so the dry-run could be served by a replica that already enforces the binding while the real apply is load-balanced to one that has not caught up yet, admitting the resource and failing the assertion. Keep the (cheap, non-persisting) dry-run wait and replace the single real apply with a bounded retry loop: succeed as soon as the real apply is denied with the expected message; if a request slips through the propagation window, track the admitted object for cleanup, delete it, and retry until enforcement converges or the step timeout elapses. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Per G. da Silva --- test/e2e/steps/steps.go | 60 +++++++++++++++++++++++++++++++++-------- 1 file changed, 49 insertions(+), 11 deletions(-) diff --git a/test/e2e/steps/steps.go b/test/e2e/steps/steps.go index 330dffc41e..d76fbc38de 100644 --- a/test/e2e/steps/steps.go +++ b/test/e2e/steps/steps.go @@ -485,7 +485,7 @@ func ResourceIsAppliedAsUser(ctx context.Context, user, group string, yamlTempla func ResourceApplyAsUserFails(ctx context.Context, user, group, errMsg string, yamlTemplate *godog.DocString) error { sc := scenarioCtx(ctx) yamlContent := substituteScenarioVars(yamlTemplate.Content, sc) - _, err := toUnstructured(yamlContent) + res, err := toUnstructured(yamlContent) if err != nil { return fmt.Errorf("failed to parse resource yaml: %v", err) } @@ -499,18 +499,56 @@ func ResourceApplyAsUserFails(ctx context.Context, user, group, errMsg string, y } return strings.Contains(stdErr, errMsg) }) - _, stdErr, err := k8scliWithInput(ctx, yamlContent, "apply", "-f", "-", "--as="+user, "--as-group="+group) - if err == nil { - res, _ := toUnstructured(yamlContent) - if res != nil { - sc.addedResources = append(sc.addedResources, resource{name: res.GetName(), kind: strings.ToLower(res.GetKind())}) + // The dry-run above may have been served by a kube-apiserver replica that already enforces the + // binding while the real apply is load-balanced to a different replica that has not caught up yet — + // VAP binding enforcement propagates per-replica and is eventually consistent, so a single dry-run + // denial does not guarantee the next request is denied. Poll the real apply until it is denied with + // the expected message; if a request slips through the propagation window, the object is persisted, + // so delete it and retry. This converges without leaking the resource or reporting a false + // "it succeeded". + var lastErr error + // pendingCleanup is true once an apply has been admitted and the persisted object has not yet been + // confirmed removed. We must not report success while a leaked object remains — a following + // "is not found" step would time out, and ScenarioCleanup skips tracked-resource deletion once the + // scenario has failed, so the object would also leak into the next scenario. + pendingCleanup := false + deadline := time.Now().Add(timeout) + for { + _, stdErr, applyErr := k8scliWithInput(ctx, yamlContent, "apply", "-f", "-", "--as="+user, "--as-group="+group) + switch { + case applyErr == nil: + // Admitted before enforcement propagated to this replica. Track for cleanup and remove it below. + if !pendingCleanup { + sc.addedResources = append(sc.addedResources, resource{name: res.GetName(), kind: strings.ToLower(res.GetKind())}) + } + pendingCleanup = true + lastErr = fmt.Errorf("apply was admitted before enforcement propagated") + case strings.Contains(stdErr, errMsg): + // Denied with the expected message. Succeed only once any earlier-admitted object is gone. + if !pendingCleanup { + return nil + } + default: + lastErr = fmt.Errorf("expected apply error containing %q, got: %s", errMsg, stdErr) + } + if pendingCleanup { + // A blocking delete (default --wait) returns only once the object is actually removed, so a + // nil error confirms removal. Keep pendingCleanup set on failure so a later denial is not accepted. + delArgs := []string{"delete", res.GetKind(), res.GetName(), "--ignore-not-found"} + if ns := res.GetNamespace(); ns != "" { + delArgs = append(delArgs, "-n", ns) + } + if _, delErr := k8sClient(ctx, delArgs...); delErr != nil { + lastErr = fmt.Errorf("failed to remove admitted resource: %w", delErr) + } else { + pendingCleanup = false + } } - return fmt.Errorf("expected apply to fail with %q, but it succeeded", errMsg) - } - if !strings.Contains(stdErr, errMsg) { - return fmt.Errorf("expected apply error containing %q, got: %s", errMsg, stdErr) + if time.Now().After(deadline) { + return fmt.Errorf("expected apply to fail with %q within %s: %w", errMsg, timeout, lastErr) + } + time.Sleep(tick) } - return nil } // ResourceApplyEmitsWarning asserts the last kubectl apply produced stderr output containing the expected warning. From 07b748c9dd7bce781af492dd62d9adbdf00feff6 Mon Sep 17 00:00:00 2001 From: "Per G. da Silva" Date: Thu, 15 Oct 2026 09:12:00 +0200 Subject: [PATCH 2/2] =?UTF-8?q?=F0=9F=90=9B=20Bound=20VAP=20cleanup=20dele?= =?UTF-8?q?te=20by=20remaining=20step=20budget?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ResourceApplyAsUserFails cleanup loop used a blocking kubectl delete (default --wait) without a timeout. The "Denied" scenarios apply a ClusterExtension, which carries a finalizer, so a slipped-through apply could make the delete wait on its controller and push the step past its deadline (checked only after the delete returns). Pass --timeout set to the remaining budget (clamped to one tick); a timed-out delete returns non-nil, so pendingCleanup stays set and the removal-confirmation invariant is preserved. Co-Authored-By: Claude Opus 4.8 --- test/e2e/steps/steps.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/test/e2e/steps/steps.go b/test/e2e/steps/steps.go index d76fbc38de..aa63ba2afd 100644 --- a/test/e2e/steps/steps.go +++ b/test/e2e/steps/steps.go @@ -534,7 +534,13 @@ func ResourceApplyAsUserFails(ctx context.Context, user, group, errMsg string, y if pendingCleanup { // A blocking delete (default --wait) returns only once the object is actually removed, so a // nil error confirms removal. Keep pendingCleanup set on failure so a later denial is not accepted. - delArgs := []string{"delete", res.GetKind(), res.GetName(), "--ignore-not-found"} + // Bound the wait by the remaining step budget (at least one tick) so a finalizer-blocked delete — + // e.g. a ClusterExtension waiting on its controller — cannot push the step past deadline. + remaining := time.Until(deadline) + if remaining < tick { + remaining = tick + } + delArgs := []string{"delete", res.GetKind(), res.GetName(), "--ignore-not-found", "--timeout=" + remaining.String()} if ns := res.GetNamespace(); ns != "" { delArgs = append(delArgs, "-n", ns) }