egress-proxies-operator/docs/plans-executions/2026-09-11-1526-provider-health-checks.md

# Execution: provider health checks
Plan: [docs/plans/2026-09-11-1526-provider-health-checks.md](../plans/2026-09-11-1526-provider-health-checks.md)

- [x] Step 0 — Save the plan
- [x] Step 1 — ProviderHealth API and CRD
- [x] Step 2 — Contract plus the kubernetes and GCP checks
- [x] Step 3 — KVM check
- [x] Step 4 — Monitor
- [x] Step 5 — Metrics and wiring
- [x] Step 6 — Docs
- [ ] Step 7 — Validate, record, push, open the PR

## Step 0 — Save the plan

Branched off `main` (at the PR #5 merge, `eb8f075`). The plan was copied out of
`~/.claude/plans/` and committed on its own, before any code, as the repo's plan rules
require:

```bash
git checkout -b feat/provider-health main
cp ~/.claude/plans/now-i-would-like-flickering-wall.md docs/plans/2026-09-11-1526-provider-health-checks.md
```

The uncommitted `.claude/settings.json` change was left unstaged and stays out of this
branch.

## Step 1 — ProviderHealth API and CRD

`kubebuilder` isn't installed here (only `bin/controller-gen`, `bin/kustomize` and
`bin/setup-envtest` are), so `api/v1alpha1/providerhealth_types.go` and the `PROJECT`
entry were written by hand instead of with `kubebuilder create api`. For a cluster-scoped
resource `PROJECT` simply omits `namespaced`, which is what kubebuilder itself writes.

The new types are registered in the existing `AddKnownTypes` call in `proxy_types.go`,
not in a second `init()`.

The deepcopy code, the CRD and the RBAC come from the usual generators:

```bash
make generate manifests
go build ./... && go vet ./api/... && go test -short ./api/...
```

To confirm the new CRD and the viewer role actually reach the rendered install, I built
the kustomization and grepped for them:

```bash
bin/kustomize build config/default | grep -n providerhealth
# providerhealths.crawl.example.com (scope: Cluster, shortName ph)
# egress-proxies-operator-providerhealth-viewer-role
```

Worth noting:
- The only role added is the viewer role. The operator is the object's only writer, so
  the admin and editor roles kubebuilder scaffolds for Proxy were not added.
- The type file has no unit test, since it has no logic. The schema, including the
  required `ready`/`lastTransitionTime` and the list-map keys, is proven by the envtest
  round-trip in Step 4.
- `config/rbac/role.yaml` is unchanged so far. The manager's own permissions arrive in
  Step 4 with the Monitor's RBAC markers.

## Step 2 — Contract plus the kubernetes and GCP checks

`provider.Checker` is an optional interface in `internal/provider/check.go`, kept out of
`Provider`. Each implementation has a compile-time assertion, because the monitor's
type assertion would otherwise fail silently.

- **kubernetes:** lists at most one managed Pod cluster-wide, using the same client, verb
  and scope as `ListByTag`. The target is `apiserver`, at the `restCfg.Host` now stored
  in the new `apiHost` field.
- **GCP:** calls a new seam method, `Probe`. `realInstances.Probe` lives in `check.go`,
  next to its only caller. `ListByTag`'s `Sprintf` filter became the shared
  `managedFilter` constant, so the check and the GC sweep can't drift apart.

Before relying on "MaxResults is only a page size", I checked the SDK's REST
`AggregatedList` in the module cache:

```bash
grep -n "func (c \*instancesRESTClient) AggregatedList" -A80 \
  "$(go list -m -f '{{.Dir}}' cloud.google.com/go/compute)"/apiv1/instances_client.go
# InternalFetch: req.MaxResults = pageSize ... return elems, resp.GetNextPageToken(), nil
```

```bash
go build ./... && go vet ./internal/provider/... && go test -race -count=1 ./internal/provider/...
```

Worth noting:
- The planned `TestRealInstances_Probe_fetchesOnePage` became a three-case
  `TestRealInstances_Probe` table.
  - It keeps the first page's `nextPageToken`, and asserts exactly one request with
    `maxResults=1` for each case.
  - "empty project" covers `iterator.Done`.
  - "api error keeps its status" proves the real client returns a `*googleapi.Error`, so
    a disabled Compute API is reported as `Forbidden` rather than `Unreachable`.
- Once a context has ended, both checks report `Timeout` whatever the error says. The
  monitor's timeout is the usual cause, and the underlying error only repeats it.

## Step 3 — KVM check

`conn.go` now wraps each of the four dial stages in a `dialError`: ssh-material, tcp,
ssh-handshake and libvirt-connect. `Error()` returns the inner message unchanged and
`Unwrap()` returns the cause, so `classify`/`isClassified` and every existing
message-matching test behave as before. `TestDialError_keepsClassification` pins this
down.

The three new RPCs went into `libvirtAPI`, `tracingAPI` and `fakeAPI`. The method-count
guard in `TestTracingAPI_logsEveryMethod` fails until a traced row exists for each new
method, which is what forced the three new rows.

`Check` runs one goroutine per host, over the shared connection, through
`hostPool.call`. The pool verdict is decided only after `call` returns, because
returning it as an error from inside the callback would make `call` drop a perfectly
good connection.

Build, vet (also with the integration tag, since that file changed) and race tests:

```bash
go build ./internal/provider/... && go vet ./internal/provider/kvm/ \
  && go vet -tags=integration ./internal/provider/kvm/ \
  && go test -race -count=1 ./internal/provider/kvm/
```

Worth noting:
- **Pool state wording:** `poolState` uses virsh's words (`running`, `inactive`, …)
  rather than go-libvirt's constant names, so it reads like `virsh pool-list` output.
- **Details:** each host also reports its SSH `user`. That is the first thing to check
  when the reason is `SSHHandshakeFailed`.
- **Tests renamed or merged:**
  - The planned `TestSSHAuth_dial_stages` is `TestLazySSH_dial_tagsTheFailingStage`. It
    dials through `lazySSH`, so the ssh-material stage is covered as well as the three
    stages the in-process SSH server can trigger.
  - The pool-inactive and pool-missing cases are merged into `TestCheck_libvirtVerdicts`,
    which also covers a daemon error. Each case runs two checks and asserts a single
    dial.
- **Tests added beyond the plan:**
  - `TestCheck_timeout`: a daemon that stops answering gives `Timeout`.
  - `TestDialError_keepsClassification`.
- **Not run yet:** `TestKVMCheck_realHost` in the integration file only compiles here.
  It needs beelink, so it goes on the Step 7 checklist for the user to run.

## Step 4 — Monitor

`internal/providerhealth` has the `Monitor` runnable plus the pure `buildStatus`.

- **Scheduling:** the Monitor is leader-only. It cleans up once at start, checks
  immediately, then checks on a ticker.
- **Each round:** providers are checked concurrently under one timeout, then logged and
  published one after another. A failed write is logged, and the next round writes
  again.
- **Reads:** objects are read through the reader it is given. `main.go` passes the
  manager's API reader (Step 5).

The first `make manifests` left `config/rbac/role.yaml` unchanged, with no error.
I had put the `+kubebuilder:rbac` markers inside `Monitor`'s doc comment, and
controller-gen only reads RBAC markers from package-level comment groups. So they were
silently attached to the type and ignored. An empty `git diff config/rbac/role.yaml` was
the only sign. Moving them into their own comment block fixed it:

```bash
make manifests && git diff config/rbac/role.yaml
# + providerhealths: create, delete, get, list
# + providerhealths/status: get, patch
```

Vet, race tests and the envtest round-trip (using the `bin/k8s/1.36.2-linux-amd64`
binaries `make setup-envtest` had already fetched):

```bash
go vet ./internal/providerhealth/ && go test -race -count=1 -v ./internal/providerhealth/
# --- PASS: TestMonitor_envtestRoundTrip (6.45s) ... ok
```

Worth noting:
- **Fake client scope:** the fake client needs a RESTMapper that registers
  ProviderHealth as `RESTScopeRoot`. Its default mapper treats every kind as namespaced.
- **Why the envtest calls `publish` directly:** `checkOnce` only logs write errors, so a
  schema rejection would look like a missing object. Calling `publish` makes the API
  server's answer the test failure.
  - Its first round writes a 2000-character message; the stored message is truncated to
    1024.
  - Its second round proves that only the target that flipped gets a new
    `lastTransitionTime`.
- **No guard against a hung checker:** the Monitor trusts `Checker`'s contract to return
  once the context ends, and doesn't abandon a checker that ignores it. All three
  checkers stop when the context ends: KVM through `awaitOrCancel`, and kubernetes and
  GCP through their clients.
- **Test renamed:** the planned `TestMonitor_timeoutBoundsBlockingChecker` is
  `TestMonitor_checkOnce_timeoutBoundsEachRound`. It also asserts that a fast provider
  still publishes next to a hung one.

## Step 5 — Metrics and wiring

`metrics.RegisterProviderTargets` registers a scrape-time collector for three gauges,
each labelled `{provider,target}`: `up`, check duration and check timestamp.
`Register`'s signature is unchanged.

`cmd/main.go`:
- gains `--provider-check-interval` (1m) and `--provider-check-timeout` (15s);
- validates them before even loading the providers config;
- collects the `Checker`s from the unwrapped providers in config order, just before the
  metrics and tracing wrap loop;
- adds the Monitor with the manager's API reader;
- registers the collector through a small `providerTargetSamples` closure next to
  `proxyPhaseCounts`.

Build, vet and metrics tests:

```bash
go build ./... && go vet ./cmd/... ./internal/metrics/ && go test -race -count=1 ./internal/metrics/
```

To confirm bad flags fail fast, before anything touches the cluster, I ran the binary
with a 5s interval. zap's development mode prints a stack trace after the error, hence
the grep:

```bash
go run ./cmd --providers-config=/dev/null --provider-check-interval=5s 2>&1 | grep -m1 "Invalid provider check flags"
# ERROR setup Invalid provider check flags {"error": "--provider-check-interval must be 0 (off) or at least 10s, got 5s"}
go vet -tags=e2e ./test/e2e/
```

The e2e suite gets a new ordered spec, "should publish provider health". It waits for
`providerhealth kubernetes` to report `Ready=True`. It runs before the existing metrics
spec, which now also asserts
`proxy_operator_provider_target_up{provider="kubernetes",target="apiserver"} 1`. By then
the first round is known to be done, so the series is certain to exist.

Worth noting:
- **Main-only wiring:** `providerTargetSamples` is an 8-line loop in `package main`,
  which has no test file. The e2e metrics assertion is its only coverage.
- **The KVM entry isn't asserted in e2e:** the default providers config also ships
  `kvm-home` (beelink), which would report `SSHMaterialUnavailable` in kind. That entry
  is the user's own host, and asserting it would break the suite as soon as the default
  config changes.
- **Manifest:** `config/manager/manager.yaml` only gained a comment about the two flags.
  The defaults need no args.

## Step 6 — Docs

- **`docs/architecture.md`:**
  - component rows for the CRD, the `Checker` contract and the monitor;
  - a new §11 covering the provider table, the loop diagram, the write, read and
    decorator choices, the gauges, and sample `kubectl get ph` output;
  - cross-references from the metrics (§9) and tracing (§10) sections;
  - four Decisions entries.
- **`docs/kvm-provider.md`:**
  - a "Health check" section with the reason → meaning → where-to-look table and the
    two consequences of sharing the connection;
  - under Known gaps, the capacity bullet now points at `poolAvailable`, and a new
    bullet says a rotated `kvm-ssh` Secret needs a restart;
  - the Testing section names `TestKVMCheck_realHost`.
- **`docs/testing.md`:**
  - suite rows for the three `check_test.go` files, `internal/providerhealth` (unit and
    envtest) and the metrics gauges;
  - "17 libvirt calls" is now 20;
  - the integration and e2e sections describe the new tests.
- **`CLAUDE.md`:**
  - the Architecture TODO now points at `docs/architecture.md`;
  - the stale "only `go.mod` exists" gotcha is replaced by a note that the "What this
    is" and "Commands" sections predate the code, plus Step 4's RBAC-marker gotcha.

Worth noting:
- `docs/api.md` is untouched. It documents the discovery HTTP API, which this change
  doesn't alter.
- `CLAUDE.md`'s "What this is" and "Commands" sections were left stale on purpose.
  Rewriting them is outside this plan, so the new gotcha flags them instead of leaving
  them silently wrong.