Add GCP deployment docs, PR review notes, and Claude tooling updates
docs/gcp-in-specific-project.md: SA + firewall setup for the egress-proxy project, in-kube secret, and apply-ready ConfigMap/Deployment/Proxy manifests (Ubuntu image — debian-cloud lacks cloud-init). docs/gcp-vm-validation.md: end-to-end GCP VM validation walkthrough. docs/reviews/: proxy-operator PR review notes from 2026-08-10. .claude/: operator-reviewer agent, accumulated permission allowlist. .gitignore: never commit sa_key.json (live SA key stays untracked). Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
131
docs/reviews/2026-08-10-1134-proxy-operator-pr-review.md
Normal file
131
docs/reviews/2026-08-10-1134-proxy-operator-pr-review.md
Normal file
@@ -0,0 +1,131 @@
|
||||
# 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.
|
||||
Reference in New Issue
Block a user