fix: avoid O(n^2) envoy validation on initial sync; cache validation; add reconcile concurrency flag

[danielwatrous-zendesk]

Problem

With 300+ ingresses, the ingress controller becomes very slow to reconcile, and pod restarts can take an hour or more to catch up.

Root cause: DataBrokerReconciler.Set() (used by the initial full sync) validates the entire merged config with the embedded envoy subprocess once per ingress, inside the merge loop. For N ingresses that is N full-config envoy validations of an ever-growing config — O(n²). Each saveConfig on the steady-state path also re-runs a full envoy validation even when the resulting config is unchanged.

Changes

1. fix(sync): avoid O(n²) envoy validation during initial sync.
Set() now builds the merged config using only cheap, in-process policy/option validation (validateCheap) and lets saveConfig run the single full envoy validation once. If that batch validation fails, it falls back to the previous per-ingress incremental full validation to isolate and skip only the offending ingress(es) — so one bad ingress still can't reject the whole batch, but the O(n) subprocess cost is only paid in that rare failure case.

2. fix(envoy): cache last successfully-validated config.
envoy.Validate caches the SHA-256 of the last successfully-validated bootstrap and skips the subprocess on an exact match. Single-slot, mutex-guarded; the check-then-store gap can only cause a redundant (correct) revalidation, never an incorrect skip.

3. feat(ingress): add --max-concurrent-reconciles flag.
Exposes controller-runtime reconcile concurrency (default 1, so behavior is unchanged unless set).

Validation

Measured against a real 352-ingress config set with the embedded envoy binary:

• Old path: ~8m18s. New path: ~4s (~124× faster).
• Old and new paths produce a byte-identical databroker config (proto.Equal), so the change is behavior-preserving.
• New unit tests added alongside existing ones (testify, same package conventions): pomerium/sync_test.go, pomerium/envoy/validate_cache_test.go, controllers/ingress/concurrency_test.go.

⚠️ Note on --max-concurrent-reconciles

In an all-in-one deployment with no leader election, every replica reconciles independently, and each concurrent reconcile clones the full config. We deployed --max-concurrent-reconciles=4 with ~420 ingresses and an 8Gi limit and pods were OOMKilled/evicted (each pod holding 4× the full config). It defaults to 1, but operators should only raise it alongside a higher memory limit and/or leader election. Happy to adjust the flag's help text or gate it differently per maintainer preference.

🤖 Generated with Claude Code

[CLAassistant]

All committers have signed the CLA.

[greptile-apps]

Greptile Summary

This PR addresses a real-world performance regression (352 ingresses → ~8 min restart) by eliminating O(n²) envoy subprocess calls during initial sync and adding a SHA-256 cache to skip redundant validations. It also adds a --max-concurrent-reconciles flag (default 1, behavior-preserving).

• Set() fast path: replaces per-ingress full envoy validation with a cheap in-process loop (buildConfigCheap), then runs one envoy validation on the merged config; falls back to per-ingress O(n) only when the batch fails.
• Envoy validation cache: single-slot mutex-guarded SHA-256 cache skips the subprocess when the serialized bootstrap is unchanged — correct and race-safe.
• Concurrency flag: wires MaxConcurrentReconciles into controller-runtime; safe at default 1, but values > 1 expose concurrent access to the non-thread-safe DataBrokerReconciler.

Confidence Score: 3/5

Safe to merge only with --max-concurrent-reconciles left at the default of 1; both defects are only triggered when that default is raised or when the databroker is transiently unavailable.

The O(n²) to O(1) envoy reduction is correct and well-tested. Two problems temper the score: the Set() fallback fires on any saveConfig error (including transient Put failures), wastefully running O(n) envoy subprocesses and logging misleading messages for non-validation failures. The --max-concurrent-reconciles flag enables concurrent access to DataBrokerReconciler, which is explicitly documented as not thread-safe, creating a TOCTOU race that silently drops ingress routes at any value above 1.

pomerium/sync_databroker.go (fallback error discrimination) and controllers/ingress/controller.go (concurrency guard on DataBrokerReconciler)

Important Files Changed

Filename	Overview
pomerium/sync_databroker.go	Core O(n²) fix. Happy path is correct, but the fallback triggers on ALL saveConfig errors, not just validation failures, causing unnecessary O(n) envoy work on transient network errors.
controllers/ingress/controller.go	Wires MaxConcurrentReconciles into controller-runtime. Safe at default 1; values > 1 create a TOCTOU race on the non-thread-safe DataBrokerReconciler that silently drops ingress routes.
pomerium/envoy/validate_cache.go	New single-slot mutex-guarded validation cache. Correct locking; check-then-store gap only produces redundant safe revalidation.
pomerium/envoy/validate_envoy.go	SHA-256 cache lookup before envoy subprocess; stores only on success. Correct integration.
pomerium/validate.go	Clean split into validateOptions (cheap) and validate (full envoy). No logic changes to the existing full-validation path.
cmd/ingress_opts.go	Adds --max-concurrent-reconciles flag. Struct validation catches CLI zero case; help text missing thread-safety and memory warnings.
pomerium/sync_test.go	New tests for buildConfigCheap covering all-valid and skip-invalid scenarios.
controllers/ingress/concurrency_test.go	Tests WithMaxConcurrentReconciles setter; does not cover the concurrent-access race on DataBrokerReconciler.
pomerium/envoy/validate_cache_test.go	Good coverage of cache hit/miss semantics and concurrent access under -race.

_{Reviews (1): Last reviewed commit: "feat(ingress): add --max-concurrent-reco..." | Re-trigger Greptile}

[greptile-apps]

Fallback triggered on non-validation errors

saveConfig can fail for reasons other than envoy bootstrap validation — removeUnusedCerts, and especially r.Put (transient databroker network error). When it does, the code still falls through to buildConfigIncremental, logging the misleading "batch config validation failed" message and spinning up O(n) envoy subprocesses before failing again on the second r.Put. During a restart where the databroker is momentarily unavailable, this can add several minutes of unnecessary envoy overhead and produce confusing logs.

Red test (fails today with a databroker-network error):

func TestSet_FallbackNotTriggeredByNetworkError(t *testing.T) {
    networkErr := status.Error(codes.Unavailable, "connection refused")
    ctrl := gomock.NewController(t)
    mock := mock_databroker.NewMockDataBrokerServiceClient(ctrl)
    mock.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).
        Return(recordFor(&pb.Config{}), nil).AnyTimes()
    mock.EXPECT().Put(gomock.Any(), gomock.Any(), gomock.Any()).
        Return(nil, networkErr).Times(1) // must not be called a second time

    r := &DataBrokerReconciler{ConfigID: IngressControllerConfigID, DataBrokerServiceClient: mock}
    ics := []*model.IngressConfig{
        validIngressConfig("a", "a.example.com"),
        validIngressConfig("b", "b.example.com"),
    }
    _, err := r.Set(context.Background(), ics)
    // Fails: Times(1) is violated because fallback calls saveConfig a second time.
    require.ErrorIs(t, err, networkErr)
}

Context Used: For every finding, write a red test with proof and... (source)

[greptile-apps]

Concurrent reconciles race on DataBrokerReconciler

DataBrokerReconciler is explicitly documented as "not thread-safe". When maxConcurrentReconciles > 1, multiple Reconcile goroutines call Upsert/Delete on the same shared instance. Both follow getConfig → mutate → saveConfig with no locking; two goroutines observing the same prev race and the last Put wins, silently dropping the other goroutine's ingress route.

Red test (demonstrates the TOCTOU lost-write):

func TestConcurrentUpsertLosesRoute(t *testing.T) {
    ctrl := gomock.NewController(t)
    var (mu sync.Mutex; stored = &pb.Config{})
    mock := mock_databroker.NewMockDataBrokerServiceClient(ctrl)
    mock.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).
        Return(recordFor(&pb.Config{}), nil).AnyTimes()
    mock.EXPECT().Put(gomock.Any(), gomock.Any(), gomock.Any()).
        DoAndReturn(func(_ context.Context, req *databroker.PutRequest, _ ...grpc.CallOption) (*databroker.PutResponse, error) {
            mu.Lock(); defer mu.Unlock()
            stored = unpackConfig(req)
            return &databroker.PutResponse{}, nil
        }).AnyTimes()
    r := &DataBrokerReconciler{ConfigID: IngressControllerConfigID, DataBrokerServiceClient: mock}
    var wg sync.WaitGroup
    for _, host := range []string{"a.example.com", "b.example.com"} {
        wg.Add(1)
        go func(h string) { defer wg.Done(); _, _ = r.Upsert(context.Background(), validIngressConfig("ing", h)) }(host)
    }
    wg.Wait()
    // FAILS: only 1 route present
    assert.Len(t, stored.GetRoutes(), 2, "both routes must be present")
}

Context Used: For every finding, write a red test with proof and... (source)

[greptile-apps]

The flag help text doesn't mention the thread-safety or memory risks described in the PR. Operators who set this above 1 will silently lose ingress routes (TOCTOU race) and risk OOMKill without leader election.

Red test:

func TestMaxConcurrentReconcilesFlagHelpMentionsRisk(t *testing.T) {
    fs := pflag.NewFlagSet("test", pflag.ContinueOnError)
    opts := &ingressControllerOpts{}
    opts.setupFlags(fs)
    f := fs.Lookup("max-concurrent-reconciles")
    require.NotNil(t, f)
    // Fails today: no thread-safety warning in usage text.
    assert.Contains(t, f.Usage, "thread-safe")
}

Suggested change

	flags.IntVar(&s.MaxConcurrentReconciles, maxConcurrentReconciles, 1,
	"maximum number of concurrent ingress reconciles")
	flags.IntVar(&s.MaxConcurrentReconciles, maxConcurrentReconciles, 1,
	"maximum number of concurrent ingress reconciles; values above 1 are experimental: "+
	"DataBrokerReconciler is not thread-safe, so concurrent reconciles can race and silently drop routes. "+
	"Raising this also multiplies per-pod memory by the concurrency factor.")

Context Used: For every finding, write a red test with proof and... (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!