From 1814ed1d3370de67dd367a6e5cd1464851c184b7 Mon Sep 17 00:00:00 2001 From: Neo Tlaletsi Date: Tue, 21 Jul 2026 03:40:51 +0200 Subject: [PATCH] fix: route CmdDelete through the engine and unstick finalizer teardown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitHubEvent and Deployment reconcilers computed the correct cmdType (Update vs Delete based on finalizer/deletionTimestamp) but then hardcoded Type: command.CmdUpdate when building the engine command. CmdDelete was never reached, so CleanupPrerequisites never ran on deletion — the finalizer only cleared if create-ish logic happened to succeed against a dying object, and stuck forever otherwise. Also fixes a related class of stuck finalizer: Build, Deployment, and GitRepository's CleanupPrerequisites required an Environment lookup that fails permanently once the Environment is deleted, wedging the finalizer if the Environment goes before its children. Added EnvironmentGone as a pre-check mediators can use to skip store-bound cleanup once the Environment is confirmed gone. GitHubEvent's teardown drops the Environment dependency entirely instead: GitHubEvent CRs are written by the Argo Events Sensor into the fixed argo-events namespace, never the Environment's own namespace, so the lookup was structurally unsatisfiable in production regardless of deletion order. GitHubWebhookSecretReconciler.Delete only needs the secret's name/namespace, not a store binding. Also fixes a nil-pointer panic in the build observer: applyTriggers dereferenced resolved.Spec.Policy.Triggers without checking Policy for nil, crashing on every Build CR with no policy block. Co-Authored-By: Claude Sonnet 5 --- .../controller/environments/deployment.go | 2 +- internal/controller/events/githubevent.go | 2 +- internal/controller/observers/build/build.go | 2 +- internal/domains/build/build_test.go | 13 +++-- internal/mediators/build/build.go | 15 ++++++ internal/mediators/deployment/deployment.go | 15 ++++++ internal/mediators/environment.go | 35 ++++++++++++ internal/mediators/environment_test.go | 44 +++++++++++++++ internal/mediators/githubevent/githubevent.go | 53 ++++++++++--------- .../mediators/githubevent/githubevent_test.go | 18 +++++++ .../mediators/gitrepository/gitrepository.go | 15 ++++++ 11 files changed, 181 insertions(+), 33 deletions(-) diff --git a/internal/controller/environments/deployment.go b/internal/controller/environments/deployment.go index e8b3986..3bac32a 100644 --- a/internal/controller/environments/deployment.go +++ b/internal/controller/environments/deployment.go @@ -135,7 +135,7 @@ func (r *DeploymentReconciler) Reconcile(ctx context.Context, req ctrl.Request) cmd := command.Command{ GVK: environmentsv1alpha1.GroupVersion.WithKind("Deployment"), - Type: command.CmdUpdate, + Type: cmdType, Obj: &deploymentCR, } diff --git a/internal/controller/events/githubevent.go b/internal/controller/events/githubevent.go index 5d82497..42023d1 100644 --- a/internal/controller/events/githubevent.go +++ b/internal/controller/events/githubevent.go @@ -126,7 +126,7 @@ func (r *GitHubEventReconciler) Reconcile(ctx context.Context, req ctrl.Request) // ------------------------------------------------ cmd := command.Command{ GVK: eventsv1alpha1.GroupVersion.WithKind("GitHubEvent"), - Type: command.CmdUpdate, + Type: cmdType, Obj: &gitHubEventCR, } diff --git a/internal/controller/observers/build/build.go b/internal/controller/observers/build/build.go index c565f2f..3cebdf5 100644 --- a/internal/controller/observers/build/build.go +++ b/internal/controller/observers/build/build.go @@ -113,7 +113,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu func (r *Reconciler) applyTriggers(ctx context.Context, build *buildv1.Build, resolved *buildresolution.ResolvedBuild) error { log := ctrl.LoggerFrom(ctx).WithValues("fn", "applyTriggers") - if len(resolved.Spec.Policy.Triggers) == 0 { + if resolved.Spec.Policy == nil || len(resolved.Spec.Policy.Triggers) == 0 { log.Info("trigger scan: no triggers configured on policy, skipping") return nil } diff --git a/internal/domains/build/build_test.go b/internal/domains/build/build_test.go index 93fd6ef..902b5c0 100644 --- a/internal/domains/build/build_test.go +++ b/internal/domains/build/build_test.go @@ -298,17 +298,22 @@ func TestBuildDomain_Handle_Delete_ResolutionFailure(t *testing.T) { } } +// TestBuildDomain_Handle_Delete_MissingEnvironment covers out-of-order +// deletion: the owning Environment is already gone (e.g. deleted alongside +// or ahead of its Build). Teardown must still succeed — erroring here would +// leave the Build's finalizer in place forever, since the Environment can +// never come back to satisfy a strict lookup. func TestBuildDomain_Handle_Delete_MissingEnvironment(t *testing.T) { buildCR := newBuildCR(validBuildContract()) d := newTestDomain(t, buildCR) err := d.Handle(context.Background(), command.Command{Type: command.CmdDelete, Obj: buildCR}) - if err == nil { - t.Fatal("Handle() delete with no owning Environment = nil error, want error") + if err != nil { + t.Fatalf("Handle() delete with no owning Environment = %v, want nil", err) } - if status, ok := conditionStatus(buildCR.Status.Conditions, "BuildDeleted"); !ok || status != metav1.ConditionFalse { - t.Errorf("BuildDeleted condition = (%v, found=%v), want (False, true)", status, ok) + if status, ok := conditionStatus(buildCR.Status.Conditions, "BuildDeleted"); !ok || status != metav1.ConditionTrue { + t.Errorf("BuildDeleted condition = (%v, found=%v), want (True, true)", status, ok) } } diff --git a/internal/mediators/build/build.go b/internal/mediators/build/build.go index 0b4aa3c..e88a95f 100644 --- a/internal/mediators/build/build.go +++ b/internal/mediators/build/build.go @@ -40,6 +40,8 @@ import ( utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/client-go/tools/events" "sigs.k8s.io/controller-runtime/pkg/client" + + mediatorenv "github.com/blanketops/environments-controller/internal/mediators" ) // Mediator manages the prerequisite resources a Build depends on. @@ -124,7 +126,20 @@ func (m *Mediator) CleanupPrerequisites(ctx context.Context, resolved *buildReso // Step 0: Environment lookup // Same store binding used at creation time — needed so the reconcilers // target the correct ClusterSecretStore-scoped resources on teardown. + // + // If the Environment was already deleted (e.g. out-of-order deletion + // alongside its children), query.Lookup below fails permanently and + // would otherwise leave this Build's finalizer stuck forever. Skip + // store-bound cleanup in that case and let the finalizer proceed. // ------------------------------------------------ + gone, err := mediatorenv.EnvironmentGone(ctx, m.Client, resolved.Build.Namespace, resolved.Build.Labels) + if err != nil { + return fmt.Errorf("environment existence check: %w", err) + } + if gone { + m.Log.Info("environment already deleted, skipping store-bound cleanup", "namespace", resolved.Build.Namespace) + return nil + } envCtx, err := query.Lookup(ctx, m.Client, resolved.Build.Namespace, resolved.Build.Labels) if err != nil { return fmt.Errorf("environment lookup: %w", err) diff --git a/internal/mediators/deployment/deployment.go b/internal/mediators/deployment/deployment.go index 0535aa3..57160e1 100644 --- a/internal/mediators/deployment/deployment.go +++ b/internal/mediators/deployment/deployment.go @@ -38,6 +38,8 @@ import ( utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/client-go/tools/events" "sigs.k8s.io/controller-runtime/pkg/client" + + mediatorenv "github.com/blanketops/environments-controller/internal/mediators" ) // Mediator manages the prerequisite resources a Deployment depends on. @@ -150,7 +152,20 @@ func (m *Mediator) CleanupPrerequisites(ctx context.Context, resolved *deploymen // Step 0: Environment lookup // Same store binding used at creation time — needed so the reconcilers // target the correct ClusterSecretStore-scoped resources on teardown. + // + // If the Environment was already deleted (e.g. out-of-order deletion + // alongside its children), query.Lookup below fails permanently and + // would otherwise leave this Deployment's finalizer stuck forever. Skip + // store-bound cleanup in that case and let the finalizer proceed. // ------------------------------------------------ + gone, err := mediatorenv.EnvironmentGone(ctx, m.Client, deploy.Namespace, deploy.Labels) + if err != nil { + return fmt.Errorf("environment existence check: %w", err) + } + if gone { + log.Info("environment already deleted, skipping store-bound cleanup") + return nil + } envCtx, err := query.Lookup(ctx, m.Client, deploy.Namespace, deploy.Labels) if err != nil { return fmt.Errorf("environment lookup: %w", err) diff --git a/internal/mediators/environment.go b/internal/mediators/environment.go index 263cab0..0fec875 100644 --- a/internal/mediators/environment.go +++ b/internal/mediators/environment.go @@ -27,6 +27,41 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) +// LabelEnvironmentName mirrors query.LabelEnvironmentName. Duplicated here +// rather than imported to keep this package free of the engine module's +// query package — EnvironmentGone is a pre-check callers run before invoking +// query.Lookup, not a replacement for it. +const LabelEnvironmentName = "environments.blanketops.dev/name" + +// EnvironmentGone reports whether the Environment CR named by the +// environments.blanketops.dev/name label no longer exists. Mediators call +// this at the top of CleanupPrerequisites before query.Lookup: if the +// Environment was deleted out of order (ahead of, or alongside, its +// children), query.Lookup fails permanently — "must pre-exist" — which +// would keep the child's finalizer in place forever with no way to clear +// it short of a manual patch. When the Environment is confirmed gone, the +// store binding it authorized is no longer resolvable, so store-bound +// cleanup is skipped and the caller lets the finalizer proceed. +// +// A missing name label returns false (not gone) — that's a distinct +// misconfiguration query.Lookup already reports clearly, not something +// this check should paper over. +func EnvironmentGone(ctx context.Context, c client.Client, namespace string, labels map[string]string) (bool, error) { + name := labels[LabelEnvironmentName] + if name == "" { + return false, nil + } + var env env1alpha1.Environment + err := c.Get(ctx, client.ObjectKey{Namespace: namespace, Name: name}, &env) + if apierrors.IsNotFound(err) { + return true, nil + } + if err != nil { + return false, err + } + return false, nil +} + func EnsureEnvironment( ctx context.Context, c client.Client, diff --git a/internal/mediators/environment_test.go b/internal/mediators/environment_test.go index 8bd7859..24ae979 100644 --- a/internal/mediators/environment_test.go +++ b/internal/mediators/environment_test.go @@ -49,6 +49,50 @@ func newScopedObject(envName, envType string) client.Object { } } +func TestEnvironmentGone_TrueWhenDeleted(t *testing.T) { + c := testsupport.NewFakeClient() + + gone, err := EnvironmentGone(context.Background(), c, testNamespace, map[string]string{ + LabelEnvironmentName: testAppSampleName, + }) + if err != nil { + t.Fatalf("EnvironmentGone() = %v, want nil error", err) + } + if !gone { + t.Error("EnvironmentGone() = false, want true for a nonexistent Environment") + } +} + +func TestEnvironmentGone_FalseWhenPresent(t *testing.T) { + existing := &env1alpha1.Environment{ + ObjectMeta: metav1.ObjectMeta{Name: testAppSampleName, Namespace: testNamespace}, + Spec: env1alpha1.EnvironmentSpec{Contract: testsupport.RawContract(map[string]any{keyApplicationName: testAppSampleName})}, + } + c := testsupport.NewFakeClient(existing) + + gone, err := EnvironmentGone(context.Background(), c, testNamespace, map[string]string{ + LabelEnvironmentName: testAppSampleName, + }) + if err != nil { + t.Fatalf("EnvironmentGone() = %v, want nil error", err) + } + if gone { + t.Error("EnvironmentGone() = true, want false for an existing Environment") + } +} + +func TestEnvironmentGone_FalseWhenLabelMissing(t *testing.T) { + c := testsupport.NewFakeClient() + + gone, err := EnvironmentGone(context.Background(), c, testNamespace, map[string]string{}) + if err != nil { + t.Fatalf("EnvironmentGone() = %v, want nil error", err) + } + if gone { + t.Error("EnvironmentGone() = true, want false when the name label is absent — that's query.Lookup's error to report, not this check's") + } +} + func TestEnsureEnvironment_NotEnvironmentScoped(t *testing.T) { c := testsupport.NewFakeClient() obj := &env1alpha1.Build{ObjectMeta: metav1.ObjectMeta{Name: testBuildName, Namespace: testNamespace}} diff --git a/internal/mediators/githubevent/githubevent.go b/internal/mediators/githubevent/githubevent.go index 9f99ba6..49516ed 100644 --- a/internal/mediators/githubevent/githubevent.go +++ b/internal/mediators/githubevent/githubevent.go @@ -17,10 +17,18 @@ before the application layer may act: the GitHub webhook HMAC secret used by the Argo Events sensor to verify payload signatures. It is invoked by the GitHubEvent domain during command handling — after resolution, before execution — and again during teardown. -Prerequisite provisioning is gated on the Environment: the Environment CR -must pre-exist as the root of the delivery chain, and it is the sole -authority for the ClusterSecretStore binding used by every store-dependent -secret this mediator reconciles. + +Provisioning (EnsurePrerequisites) is gated on the Environment: the +Environment CR must pre-exist, and it is the sole authority for the +ClusterSecretStore binding the webhook secret is written through. + +Teardown (CleanupPrerequisites) is deliberately NOT gated on the +Environment. GitHubEvent CRs are written by the Argo Events Sensor into the +fixed argo-events namespace, not the Environment's own namespace — +Environment is namespace-scoped and dynamic, and has no authority over +argo-events. Deleting the webhook secret only needs its name and namespace, +not a store binding, so teardown skips the lookup entirely rather than +depending on a relationship that doesn't hold for this CR. */ package githubevents @@ -33,7 +41,6 @@ import ( githubeventResolution "github.com/blanketops/environments/resolution/githubevent/resolve" "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/runtime" - utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/client-go/tools/events" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -97,35 +104,29 @@ func (m *Mediator) EnsurePrerequisites(ctx context.Context, resolved *githubeven // CleanupPrerequisites reverses EnsurePrerequisites — deletes the GitHub // webhook HMAC secret this mediator provisioned. Called from the domain's -// CmdDelete branch, gated by the finalizer at the controller level. Teardown -// runs in reverse provisioning order. All teardown steps are attempted -// regardless of individual failures, and errors are aggregated. Any returned -// error keeps the finalizer in place for retry on next reconcile. +// CmdDelete branch, gated by the finalizer at the controller level. Any +// returned error keeps the finalizer in place for retry on next reconcile. +// +// Deliberately does NOT look up the owning Environment. GitHubEvent CRs are +// written by the Argo Events Sensor into the fixed argo-events namespace, +// not the Environment's own namespace — Environment is namespace-scoped and +// dynamic, and cannot own resources living in argo-events. Requiring the +// lookup here would make it permanently unsatisfiable (or, if the labels +// happen to line up, would tie teardown to Environment lifecycle it has no +// authority over). GitHubWebhookSecretReconciler.Delete only needs the +// secret's name and namespace to remove it — the store binding was only +// ever needed to create it, not to delete it — so no store context is +// needed here either. func (m *Mediator) CleanupPrerequisites(ctx context.Context, resolved *githubeventResolution.ResolvedGitHubEvent) error { if resolved == nil || resolved.Event == nil || resolved.Spec == nil { return fmt.Errorf("nil ResolvedGitHubEvent provided to mediator") } - event := resolved.Event - // ------------------------------------------------ - // Step 0: Environment lookup - // Same store binding used at creation time — needed so the reconcilers - // target the correct ClusterSecretStore-scoped resources on teardown. - // ------------------------------------------------ - envCtx, err := query.Lookup(ctx, m.Client, event.Namespace, event.Labels) - if err != nil { - return fmt.Errorf("environment lookup: %w", err) - } - m.Log.Info("environment context resolved for teardown", "environment", envCtx.Name, "type", envCtx.EnvironmentType, "store", envCtx.StoreName) - var errs []error // ------------------------------------------------------------------------------------------------------------ // Stage 1: GitHub webhook secret // ------------------------------------------------------------------------------------------------------------ - webhookSecret := github.NewGitHubWebhookSecretReconciler(m.Client, m.Log, envCtx.StoreName, envCtx.StoreKind) + webhookSecret := github.NewGitHubWebhookSecretReconciler(m.Client, m.Log, "", "") if err := webhookSecret.Delete(ctx, resolved); err != nil { - errs = append(errs, fmt.Errorf("delete github webhook secret: %w", err)) - } - if len(errs) > 0 { - return utilerrors.NewAggregate(errs) + return fmt.Errorf("delete github webhook secret: %w", err) } return nil } diff --git a/internal/mediators/githubevent/githubevent_test.go b/internal/mediators/githubevent/githubevent_test.go index 0511915..556c46f 100644 --- a/internal/mediators/githubevent/githubevent_test.go +++ b/internal/mediators/githubevent/githubevent_test.go @@ -105,6 +105,24 @@ func TestMediator_EnsurePrerequisites_Idempotent(t *testing.T) { } } +// TestMediator_CleanupPrerequisites_NoEnvironment locks in the fact that +// teardown does not depend on the Environment lookup. In production, +// GitHubEvent CRs are written by the Argo Events Sensor into the fixed +// argo-events namespace — never the Environment's own namespace — so a +// lookup gated the same way as EnsurePrerequisites would make teardown +// permanently unsatisfiable. No Environment exists in the fake client at +// all here, which would fail EnsurePrerequisites; CleanupPrerequisites must +// still succeed. +func TestMediator_CleanupPrerequisites_NoEnvironment(t *testing.T) { + resolved := newResolvedGitHubEvent() + c := testsupport.NewFakeClient(resolved.Event) + m := New(c, testsupport.NewScheme(), logr.Discard(), testsupport.NoopRawRecorder()) + + if err := m.CleanupPrerequisites(context.Background(), resolved); err != nil { + t.Fatalf("CleanupPrerequisites() with no Environment = %v, want nil", err) + } +} + func TestMediator_CleanupPrerequisites_NilResolved(t *testing.T) { m := New(testsupport.NewFakeClient(), testsupport.NewScheme(), logr.Discard(), testsupport.NoopRawRecorder()) if err := m.CleanupPrerequisites(context.Background(), nil); err == nil { diff --git a/internal/mediators/gitrepository/gitrepository.go b/internal/mediators/gitrepository/gitrepository.go index 8b9eed0..fee954c 100644 --- a/internal/mediators/gitrepository/gitrepository.go +++ b/internal/mediators/gitrepository/gitrepository.go @@ -42,6 +42,8 @@ import ( utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/client-go/tools/events" "sigs.k8s.io/controller-runtime/pkg/client" + + mediatorenv "github.com/blanketops/environments-controller/internal/mediators" ) // Mediator manages the prerequisite resources a GitRepository depends on. @@ -140,7 +142,20 @@ func (m *Mediator) CleanupPrerequisites(ctx context.Context, resolved *gitrepoRe // Step 0: Environment lookup // Same store binding used at creation time — needed so the reconcilers // target the correct ClusterSecretStore-scoped resources on teardown. + // + // If the Environment was already deleted (e.g. out-of-order deletion + // alongside its children), query.Lookup below fails permanently and + // would otherwise leave this GitRepository's finalizer stuck forever. + // Skip store-bound cleanup in that case and let the finalizer proceed. // ------------------------------------------------ + gone, err := mediatorenv.EnvironmentGone(ctx, m.Client, repo.Namespace, repo.Labels) + if err != nil { + return fmt.Errorf("environment existence check: %w", err) + } + if gone { + m.Log.Info("environment already deleted, skipping store-bound cleanup", "namespace", repo.Namespace) + return nil + } envCtx, err := query.Lookup(ctx, m.Client, repo.Namespace, repo.Labels) if err != nil { return fmt.Errorf("environment lookup: %w", err)