egress-proxies-operator/docs/reviews/2026-08-10-1134-proxy-operator-pr-review.md

PR review findings: feat/proxy-operator

Created: 2026-08-10 11:34 Scope: origin/main...feat/proxy-operator (merge-base 076bc66, 25 commits, ~80 files) Reviewers: go-operator-reviewer + operator-reviewer agents; findings consolidated, most severe first. Check off items as they're processed.

Both reviewers rated the core reconcile architecture sound: single status writer with one deferred patch, finalizer added before any provider call, Get-before-RemoveFinalizer on delete, CEL immutability rules correctly split to avoid the oldSelf-on-CREATE trap, leader-election gating on destructive runnables, GC tombstone rules (MinAge, UID-less instances never deleted).

Merge-blockers

• Discovery leases proxies with an empty IP — found independently by both reviewers. internal/discovery/handlers.go:151, internal/controller/proxy_controller.go:226 During instance replacement (and the Get→NotFound recovery path) the reconciler clears status.ip but only the create branch removes the Healthy condition, and the health engine prunes state for empty-host proxies so nothing refreshes it. For the whole delete→recreate window (minutes on GCP), isHealthy still returns true and handleAcquireLease grants 201 Created with "ip": "", burning a MaxLeases slot. Fix: add EffectiveHost() != "" to isHealthy (covers list + acquire), and remove/downgrade Healthy wherever status.IP is cleared.

• Orphan GC deletes other installations' fleets in a shared GCP project. internal/provider/gcp/insert.go:57, internal/gc/gc.go:93 Instances are tagged only proxy-operator-managed=true + CR UID; the sweeper deletes any tagged instance whose UID isn't in its own cluster's Proxy list. Two clusters sharing a GCP project delete each other's VMs every GC interval in a permanent loop. Fix: add an installation-identity label (cluster/deployment ID) set by both providers and filtered on in ListByTag.

• Permanent-error latch wedges proxies on failures that aren't spec-caused. internal/controller/proxy_controller.go:126 Latch keys on observedGeneration == generation, but two failure inputs live outside the spec: an unconfigured provider (config fix + restart doesn't bump generation, and spec.provider is CEL-immutable → stuck Failed short of deleting the CR) and resolved Secret content (Secret fix enqueues a reconcile that short-circuits at the latch before re-resolving cloud-init). Fix: latch should also consider current spec-hash / provider availability.

Worth fixing

• Deletion-path failures invisible in status — flagged by both reviewers. internal/controller/proxy_controller.go:319, :266 deletionFailure swallows ErrQuotaExceeded (nil error, no status write); unconfigured provider returns a bare error forever. A Proxy wedged in Deleting shows nothing in kubectl describe. Stage setProvisioned(p, False, ReasonDeleting, ...) before returning. Also: Delete is resubmitted on every DeletionPoll pass, churning GCP quota — a state check on the Get result would avoid it.

• Lost providerID on setSpecHash conflict. internal/controller/proxy_controller.go:161 On Update conflict the function returns before p.Status.ProviderID = id, so the deferred patch persists an empty providerID for a just-created instance. Self-heals via GC. Fix: set p.Status.ProviderID = id before returning the error (one line).

• Terminating pods still report StateRunning. internal/provider/kubernetes/kubernetes.go:151 A pod with a deletionTimestamp keeps phase=Running + PodIP while terminating, so drift reconcile republishes Provisioned=True and discovery keeps leasing a dying pod. Fix: map non-zero pod.DeletionTimestamp to StateTerminated in instanceFromPod.

• Stale-cache spec-hash race deletes the freshly created replacement instance. internal/controller/proxy_controller.go:176 Instance name derives from CR UID, so old and new instances share a providerID. A reconcile served a cached object from before a just-completed replacement re-enters replaceInstance and deletes the new healthy instance. Converges, but destroys a good instance. Fix: re-read uncached before the destructive branch, or compare inst.CreatedAt against the annotation-update time.

• observedGeneration written before the generation is actually processed. internal/controller/status.go:114 Set unconditionally in patchStatusIfChanged, including on the finalizer-add pass and resolveCloudInit failures — misleads kstatus-style tooling. Set it only once the state machine has genuinely evaluated the spec.

• No event filtering on the Proxy watch. internal/controller/proxy_controller.go:402 Every self-inflicted status patch triggers a follow-up reconcile with an extra cloud Get, roughly doubling provider read traffic. Caution: a plain GenerationChangedPredicate breaks the finalizer flow (relies on its own Update event to re-enter) — needs a status-only/resourceVersion-only filter or an explicit requeue in the finalizer pass.

• Unlabelled cloud-init Secrets produce a misleading NotFound with endless backoff. internal/controller/proxy_controller.go:344, cmd/main.go:210 The label-restricted cache turns "exists but missing crawl.example.com/cloud-init=true" into CloudInitError: not found. Mention the label requirement in the condition message, or read via uncached APIReader and validate the label explicitly.

• RBAC over-grant. config/rbac/role.yaml:25 create;delete on proxies is scaffold residue (controller never creates/deletes CRs); cluster-wide pods create/delete and secrets get/list/watch apply even when only the GCP provider is configured — pod rules belong in an optional kustomize component.

Simplifications

• Delete internal/provider/registry — 14 lines of logic, one caller (cmd/main.go:148); fold Build/Constructor into the composition root. Also fixes the two-sources-of-truth problem: internal/provider/config.go:84 hardcodes "kubernetes"/"gcp" while registry.Build dispatches through a caller-supplied map — validate against the constructor map instead. Net −1 package, −44 lines, −92 test lines.

• Collapse LeaseStore interface to *lease.Store. internal/discovery/server.go:28 Single implementation and not a test seam (tests wire the real lease.NewStore). Keep HealthSnapshotter and instancesAPI — those are genuine seams.

• Replace metrics nil-guards with no-op defaults. internal/health/engine.go:68, internal/discovery/server.go:38, internal/provider/metrics.go:8 Keep the interfaces (legit "no prometheus in domain packages" rationale) but default the fields to a no-op impl — provider.WithMetrics already dereferences unconditionally, so the guards are inconsistent anyway.

Nice-to-have

• Add an OwnerReference to provider pods (internal/provider/kubernetes/pod.go:24) — free cascading deletion if the finalizer is ever bypassed; CreateRequest already carries Namespace/ProxyName/UID.
• Close() the GCP *compute.InstancesClient (internal/provider/gcp/gcp.go:89) — harmless today, a leak the moment providers are rebuilt on config reload.
• Fix .golangci config: it references a missing logcheck plugin, so the linter only runs with the project config disabled.
• External-mode endpoint edits don't reset health-engine counters (internal/health/engine.go:206 keys on name+UID): flipping endpoint.host keeps the old host's Healthy=True for failureThreshold × interval. Arguably a replacement, not a flap.
• init() funcs at api/v1alpha1/proxy_types.go:353 and cmd/main.go:67 conflict with the repo's "no init()" convention; kubebuilder-idiomatic, but scheme registration could use the scaffold's SchemeBuilder.Register at package var scope.