fix(sync): bound full-state ingress reconciliation latency

[rodrigobuenoai]

Summary

This draft is a v0.32.9-based candidate for a controlled dev canary. It is related to #1477, but does not include that PR's commit history.

The current implementation repeatedly copies and validates the growing configuration for every Ingress, causing reconciliation time to grow quadratically.

This change:

• builds each Ingress route fragment once;
• combines all fragments into one complete configuration;
• validates and writes the configuration once;
• isolates only invalid Ingresses if final validation fails;
• batches related events and skips duplicate or status-only changes;
• removes stale managed statuses; and
• safely drains pods during rollouts.

Pomerium still receives one complete, validated configuration.

Local validation

• ~446 Ingresses: full-state application reduced from ~65 seconds to ~2 seconds.
• Cold start: initial sync completed in ~10 seconds.
• Unit tests, envtest, race tests and go vet passed.
• 60-minute test with three replicas:
  • 7,011 HTTP 200 responses
  • 0 HTTP 503 responses
  • 0 timeouts
  • 0 TLS/transport errors
• The test included a rollout and backend scaling from 2 → 3 → 2 endpoints.
• Pomerium Core remains at v0.32.9.

Proposed dev canary

Repeat cold starts, a three-replica rollout and backend removal in non-production GKE while monitoring:

• restart-to-ready and initial-sync duration;
• reconciliation queue and batch duration;
• HTTP 503 responses, timeouts and TLS errors;
• retries and pod restarts; and
• API server load.

Keep the current dev image available for immediate rollback.

Related issues

Related to #1477.

Checklist

• reference any related issues
• updated unit tests
• updated docs
• ready for review

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[greptile-apps]

Greptile Summary

This PR replaces the per-Ingress O(n²) reconciliation loop with a full-state batching approach: a configurable quiet-period reconcileBatchCoordinator coalesces Kubernetes change events, and a single reconcileAll + Set call writes one validated configuration to Pomerium per batch.

• New reconcileBatchCoordinator coalesces informer signals during a quiet period (trailing-edge batching) and serialises full-state writes; a maxWait ceiling bounds latency under continuous load.
• ingressConfigCache uses SHA-256 fingerprints of Kubernetes inputs (excluding status fields) to short-circuit duplicate Upsert/Set calls; validation falls back to per-ingress isolation if the combined config fails full validation.
• reconcileAll replaces per-ingress Upsert; it skips ingresses whose dependencies are temporarily unavailable rather than aborting the entire batch, and prunes stale Pomerium status entries.
• Graceful-drain lifecycle hooks (preStop sleep + extended terminationGracePeriodSeconds) are added to both deployment.yaml and config/pomerium/deployment/base.yaml.

Confidence Score: 4/5

Safe to merge as a canary behind the opt-in --reconcile-batch-window flag; the non-batch path is unchanged.

The core logic is well-structured and backed by solid unit tests covering coalescing, serialisation, retry, and cache short-circuiting. The behavioral change in reconcileAll — dropping routes for ingresses whose dependencies fail during a full-state flush — is a deliberate design trade-off but could manifest as transient downtime in production under rolling restarts or service churn. The defer/ctx.Err() issue is a real code defect (always passes nil) that happens to be harmless today because the fallback value matches the expected value in normal shutdowns. Both findings are non-blocking but should be addressed before the feature is enabled by default.

Files Needing Attention: controllers/ingress/batch_coordinator.go (defer/ctx.Err() eager evaluation), controllers/ingress/reconcile.go (routes removed for transiently-failing ingresses during batch flush), and deployment.yaml / config/pomerium/deployment/base.yaml (/busybox/sleep path assumption).

Important Files Changed

Filename	Overview
controllers/ingress/batch_coordinator.go	New trailing-edge batch coordinator. Correctly serialises flushes and coalesces signals, but defer c.finishWaiters(ctx.Err()) evaluates ctx.Err() eagerly at defer time (always nil), so shutdown waiters always receive context.Canceled instead of the actual context error.
controllers/ingress/batch_reconcile.go	New batched reconcile path; correctly delegates full-state writes via Submit and handles delete/not-found/unmanaged cases. Ingresses with missing dependencies are excluded from the batch Set, which is a behavioral change from the non-batch path where per-ingress failures leave existing Pomerium config intact.
pomerium/ingress_config_cache.go	New fingerprint cache: correctly excludes status/managed-fields from hash inputs, uses RWMutex, handles the uninitialized state, and is fully covered by tests.
pomerium/sync.go	Set gains a two-pass validation strategy (fast full-config path, per-ingress fallback) and fingerprint-based early-exit; the configValidationError sentinel type is clean. Cache is updated with fingerprints of all input ingresses including excluded ones, which is correct.
controllers/ingress/reconcile.go	reconcileAll extracts shared list+fetch+Set logic; bad ingresses are skipped rather than aborting, which is the source of the transient-route-removal concern for previously-valid ingresses.
controllers/reporter/ingress.go	PruneIngressStatuses is well-implemented; RFC 6901 escaping now correctly encodes ~ before / fixing a latent bug in the old IngressDeleted code.
controllers/ingress/batch_events.go	Clean helper predicates/mappers for wiring informer signals to the batch coordinator.
controllers/settings/controller.go	GenerationChangedPredicate correctly prevents Pomerium spec reconciliation from being re-triggered by the ingress controller's own status writes, breaking a potential tight loop.
deployment.yaml	preStop/terminationGracePeriodSeconds added; assumes /busybox/sleep is present at the exact path — needs verification against the container image.

Sequence Diagram

sequenceDiagram
    participant K8s as Kubernetes Informers
    participant Pred as batchSignalPredicate / batchSignalMap
    participant Rec as reconcileBatched
    participant Cache as ingressConfigCache
    participant Coord as BatchCoordinator (Start loop)
    participant All as reconcileAll
    participant Pom as Pomerium (databroker)

    K8s->>Pred: Ingress / Secret / Service / Endpoints event
    Pred->>Coord: Signal(source)
    Pred->>Rec: enqueue reconcile request

    Rec->>K8s: Client.Get(ingress)
    Rec->>Rec: fetchIngress (secrets, services, endpoints)
    Rec->>Cache: NeedsIngressUpdate(ic)
    alt fingerprint unchanged
        Rec->>K8s: updateIngressStatus (no-op if LB unchanged)
    else fingerprint changed
        Rec->>Coord: Submit(ctx) [waits for batch result]
        Note over Coord: quiet period expires or maxWait reached
        Coord->>All: flushReconcileBatch → reconcileAll
        All->>K8s: Client.List(ingresses)
        loop each managed ingress
            All->>K8s: fetchIngress
            alt fetch ok
                All->>All: append to ics
            else fetch fails
                All->>K8s: IngressNotReconciled (skipped from config)
            end
        end
        All->>Cache: fingerprintIngressConfigs(ics)
        alt fingerprints match cache
            Cache-->>All: matches → skip
        else
            All->>Pom: Set(ics) — fast path (no per-ingress validation)
            alt full validation fails
                Pom-->>All: configValidationError
                All->>Pom: Set(ics) — fallback (per-ingress validation)
            end
            Pom-->>All: changed, err
            All->>Cache: replace(fingerprints)
        end
        Coord-->>Rec: batch result (err or nil)
        Rec->>K8s: updateIngressStatus
        Rec->>K8s: IngressReconciled / IngressNotReconciled
    end

Loading

Comments Outside Diff (2)

1. controllers/ingress/batch_coordinator.go, line 281 (link)

   Eager evaluation of ctx.Err() in defer

   defer c.finishWaiters(ctx.Err()) evaluates ctx.Err() immediately at the point the defer statement is reached — before Start has returned — so the argument is always nil. finishWaiters handles this by substituting context.Canceled, meaning all shutdown waiters receive context.Canceled regardless of whether the coordinator's context expired with context.DeadlineExceeded. The fix is to close over ctx and call ctx.Err() lazily.

   Red test:

   func TestBatchCoordinatorFinishWaitersReceivesDeadlineExceeded(t *testing.T) {
       c := newReconcileBatchCoordinator(time.Hour, time.Hour, func(context.Context) error {
           return nil
       })
       ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(60*time.Millisecond))
       defer cancel()
   
       submitted := make(chan error, 1)
       go func() { submitted <- c.Submit(context.Background()) }()
   
       // Start blocks until the deadline expires.
       require.NoError(t, c.Start(ctx))
   
       select {
       case err := <-submitted:
           // Fails: got context.Canceled (nil fallback) instead of context.DeadlineExceeded.
           assert.ErrorIs(t, err, context.DeadlineExceeded)
       case <-time.After(time.Second):
           t.Fatal("waiter not resolved on shutdown")
       }
   }

   

2. controllers/ingress/reconcile.go, line 129-141 (link)

   Batch flush removes routes for transiently-unavailable ingresses

   When a batch flush (flushReconcileBatch → reconcileAll) runs while an ingress's dependency (Service, Secret, Endpoints) is temporarily unavailable, fetchIngress fails and the ingress is skipped from ics. The subsequent full-state Set replaces the entire Pomerium config without that ingress, actively removing routes that were previously valid. In the non-batch reconciliation path the equivalent failure only prevents an Upsert, leaving the existing Pomerium config intact. Clusters where service restarts or rolling updates coincide with batch flushes can therefore observe transient 503s until the dependency is restored and a new flush re-includes the ingress.

   Red test (add to batch_reconcile_test.go):

   func TestReconcileAllDoesNotRemoveRoutesForTransientlyMissingDependency(t *testing.T) {
       reconciler := &batchTestReconciler{tracked: map[types.NamespacedName]int64{
           {Namespace: "default", Name: "app-a"}: 2,
           {Namespace: "default", Name: "app-b"}: 2,
       }}
       r, _ := newBatchTestController(t, reconciler)
       ctx := context.Background()
   
       // Prime the cache: both app-a and app-b are applied successfully.
       _, err := r.reconcileBatched(ctx, ctrl.Request{NamespacedName: types.NamespacedName{
           Namespace: "default", Name: "app-a",
       }})
       require.NoError(t, err)
       require.True(t, reconciler.TracksIngress(types.NamespacedName{Namespace: "default", Name: "app-b"}))
   
       // Simulate a transient dependency outage: delete the shared "echo" service.
       svc := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Name: "echo", Namespace: "default"}}
       require.NoError(t, r.Client.Delete(ctx, svc))
   
       // A batch flush triggered by any unrelated event now calls reconcileAll.
       // app-b cannot be fetched (its service is gone) so it is excluded from ics.
       _, _, flushErr := r.reconcileAll(ctx)
       require.NoError(t, flushErr)
   
       // FAILS: app-b's routes were removed from Pomerium even though the outage is transient.
       assert.True(t, reconciler.TracksIngress(types.NamespacedName{Namespace: "default", Name: "app-b"}),
           "routes for an ingress whose dependency is transiently unavailable must not be removed during a batch flush")
   }

   

_{Reviews (1): Last reviewed commit: "fix(deploy): drain proxy before pod term..." | Re-trigger Greptile}

[greptile-apps]

preStop exec assumes /busybox/sleep is present

The lifecycle hook runs /busybox/sleep 30 to drain connections before SIGTERM is delivered. If the container image does not include busybox at that path the hook exits with a non-zero code; Kubernetes ignores preStop failures and proceeds with termination immediately, silently defeating the 30-second drain. The same hook appears in deployment.yaml. Consider using a shell built-in (/bin/sh -c 'sleep 30') or verifying the image ships busybox at /busybox/sleep.

Red test (shell-level proof of the silent failure):

# Run inside the pomerium/ingress-controller:v0.32.9 image.
# If /busybox/sleep is absent this exits non-zero and the drain is skipped:
docker run --rm pomerium/ingress-controller:v0.32.9 \
    /busybox/sleep 1 \
  && echo "busybox present" \
  || echo "FAIL: /busybox/sleep not found – preStop drain will be skipped silently"

[rodrigobuenoai]

Addressed the automated review findings in two separate commits:

• 9c535f1 evaluates ctx.Err() when the deferred shutdown handler runs and adds a deadline regression test.
• 5108e93 aborts a full-state batch when any dependency fetch fails, preserving the last applied routes and allowing controller-runtime to retry. The regression test proves no partial Set() occurs and that the complete state is applied after recovery.

I also verified /busybox/sleep directly in the final Linux/AMD64 image, so the 30-second preStop hook is present and executable.

Validation completed with Go 1.25.11: full unit/envtest suite, full race suite, and go vet ./.... Local note: go-m1cpu v0.1.6 segfaults during native initialization on macOS 26.5, so the race run used a temporary test-only replacement matching that package's non-Darwin behavior; the Linux/AMD64 production target excludes the failing Darwin cgo file.

The published dev-canary artifact embeds exactly Pomerium Core v0.32.9 with module hash h1:N3nbH56gGL8c1beIAC5MAidAUmxgA7RSznBVVzUVGuc= and Go 1.25.11.