2026-09-16 10:46:47 +02:00
1 changed files with 354 additions and 0 deletions
@@ -0,0 +1,354 @@
+# Plan: Provider health checks — see whether the cluster can reach each provider
+**Created:** 2026-09-11 15:26 UTC
+
+## Context
+
+Today the operator can't say whether a provider's backend is reachable. The `Provider`
+interface ([provider.go:89-109](internal/provider/provider.go#L89-L109)) only has
+Create/Get/Delete/ListByTag, and `/healthz` and `/readyz` are bare `healthz.Ping`. A
+broken KVM path only shows up later: as a Proxy stuck in Provisioning, or as an orphan-GC
+sweep logging an error. Several failures stay invisible until a Proxy needs that host:
+
+- a missing `kvm-ssh` Secret;
+- a rotated host key;
+- a Tailscale route that is down;
+- a storage pool that is inactive or full.
+
+The goal is a periodic sanity check of every configured provider from inside the cluster,
+with results you can see through `kubectl` and Prometheus.
+
+Decisions made by the user during planning:
+
+1. **Surface:** a new operator-owned, read-only, cluster-scoped **`ProviderHealth`** CRD,
+   with one object per configured provider, a `Ready` condition and per-target detail.
+   Also Prometheus gauges, and a log line whenever a target goes up or down. No
+   EventRecorder, matching the earlier decision.
+2. **Scope:** real checks for all three providers.
+   - **kvm:** checked per host.
+   - **kubernetes:** confirms the API server and RBAC work.
+   - **gcp:** confirms credentials and the API work.
+3. **KVM depth:** reachability plus the storage pool.
+   - Over the existing pooled SSH+libvirt connection, call the libvirt version and
+     hostname RPCs.
+   - Confirm the configured pool is running and report its free space.
+   - No bridge check.
+
+Not affected: the Proxy reconcile behaviour, and the manager's `/healthz` and `/readyz`.
+A dead hypervisor must never make the manager unready.
+
+## Design
+
+### Contract: `internal/provider/check.go`
+
+```go
+type TargetStatus struct {
+    Name, Address   string
+    Ready           bool
+    Reason, Message string
+    Latency         time.Duration
+    Details         map[string]string
+}
+// Check never errors; each target carries its own verdict. Must return promptly once ctx ends.
+type Checker interface{ Check(ctx context.Context) []TargetStatus }
+```
+
+- **Shared reasons:** `Reachable`, `Timeout`, `Unreachable`, `Unauthorized`, `Forbidden`,
+  `APIError`.
+- **Compile-time checks:** each provider's `check.go` has `var _ provider.Checker = (*Provider)(nil)`.
+- **Bypass the wrappers.** `cmd/main.go:197-201` wraps every provider in
+  `WithTracing(WithMetrics(...))`, so the Checkers are collected from the **raw** providers
+  map, before that loop. As a result, checks don't inflate `provider_requests_total`.
+
+### The checks
+
+**kvm** (`internal/provider/kvm/check.go`)
+- **Concurrency:** one goroutine per `cfg.Hosts` entry (`wg.Go`), writing to `out[i]` so
+  results stay in config order.
+- **Inside `p.hosts.call`:** `ConnectGetLibVersion`, `ConnectGetHostname`,
+  `StoragePoolLookupByName(host.Pool)`, then `StoragePoolGetInfo`.
+- **Rules for `fn`:**
+  - It returns only raw RPC errors. `call` drops the shared connection on any other error
+    ([conn.go:160](internal/provider/kvm/conn.go#L160)), so "pool not running" is decided
+    after `call` returns, never returned from `fn`.
+  - It fills a captured result struct only after all four calls succeed. That struct is
+    read only when `call` returned nil, which avoids a race with an abandoned goroutine.
+- **Details:** `hostname`, `libvirtVersion` (decoded as major.minor.patch), `pool`,
+  `poolState`, `poolCapacity` and `poolAvailable` (in GiB).
+- **New RPCs:** add the three to `libvirtAPI` ([conn.go:29-47](internal/provider/kvm/conn.go#L29-L47)),
+  `tracingAPI` (`trace.go`) and `fakeAPI` (`fake_test.go`). `fakeAPI` gets a hostname, a
+  fixed version, and a per-pool state map that defaults to Running.
+- **Telling failure stages apart without matching strings:** add
+  `type dialError struct{ stage dialStage; err error }` in `conn.go`, with `Error()`
+  returning the inner message unchanged and `Unwrap()`. It wraps at four places:
+  - `lazySSH.dial`, when loading the key material fails (`ssh-material`)
+  - `dialSSH`, at the dial step (`tcp`)
+  - `dialSSH`, at the handshake (`ssh-handshake`)
+  - `connectLibvirt` (`libvirt-connect`)
+
+  Because `Unwrap` is kept, `classify` and `isClassified` still see `ErrPermanent` and
+  `fs.ErrNotExist`, so reconcile behaviour doesn't change.
+- **`checkReason(err)`, tested in order:**
+
+  | Condition | Reason |
+  |---|---|
+  | stage ssh-material | `SSHMaterialUnavailable` |
+  | stage tcp | `Unreachable` |
+  | stage ssh-handshake | `SSHHandshakeFailed` |
+  | stage libvirt-connect | `LibvirtConnectFailed` |
+  | ctx deadline or cancel | `Timeout` |
+  | `ErrNoStoragePool` | `PoolNotFound` |
+  | any other libvirt error | `LibvirtError` |
+  | anything else | `Unreachable` |
+  | success, pool not running | `PoolNotActive` |
+  | success | `Reachable` |
+- **Accepted behaviour:**
+  - A check that hits its timeout drops the shared connection. That already happens for
+    any cancelled reconcile context, and it only fires when the host isn't answering.
+  - The leader now keeps one SSH connection per host open from startup, with keepalives
+    every 15s. Before, the connection only opened with the first KVM reconcile.
+  - No forced periodic redial. `lazySSH` caches the key material forever, so a redial
+    would not pick up a rotated Secret anyway; that gap goes into the docs under
+    "Known gaps".
+
+**kubernetes** (`internal/provider/kubernetes/check.go`)
+- **Call:** `List` pods with the managed-label selector and `client.Limit(1)`,
+  cluster-wide. It uses the same uncached client, verb and RBAC as `ListByTag` and GC.
+  There is no namespace setting in the kubernetes config.
+- **Target:** one target named `apiserver`, with the address from a new `apiHost` field
+  set in `NewWithTransportWrapper`.
+- **Reasons:**
+  - `Timeout`: the context ended, or the error is `IsTimeout`/`IsServerTimeout`.
+  - `Unauthorized`, `Forbidden`.
+  - `APIError`: any other API status.
+  - `Unreachable`: anything else.
+
+**gcp** (`internal/provider/gcp/check.go`, plus a seam change in `gcp.go`)
+- **Why a new seam method:** the existing `AggregatedList` seam reads every page
+  ([gcp.go:63-78](internal/provider/gcp/gcp.go#L63-L78)), and MaxResults is only the page
+  size. So add `Probe(ctx, *computepb.AggregatedListInstancesRequest) error`, which makes
+  one `it.Next()` call and treats `iterator.Done` as nil.
+- **Request:** `Project`, `MaxResults: 1`, `ReturnPartialSuccess: true`, and the same
+  managed-label filter `ListByTag` uses.
+- **Target:** one per project, `Name: cfg.Project`, `Address: compute.googleapis.com`.
+  GCP config has no zones, and reading them from Proxy placements would tie the check to
+  the Proxy cache.
+- **Reasons:**
+  - `Timeout`: the context ended.
+  - For `*googleapi.Error`: 401 is `Unauthorized`, 403 is `Forbidden` (this also covers a
+    disabled API), anything else is `APIError`.
+  - `Unreachable`: anything else, including a failure to get a token.
+
+### The `ProviderHealth` CRD: `api/v1alpha1/providerhealth_types.go`
+
+- **Written by hand**, since kubebuilder isn't installed; `PROJECT` is edited by hand too.
+- **Scope and markers:** cluster-scoped, `shortName=ph`, status subresource, no spec.
+- **Scheme registration:** add the type to the existing `AddKnownTypes` call in
+  [proxy_types.go:362](api/v1alpha1/proxy_types.go#L362), not a second `init()`.
+- **Status:**
+  - `type`
+  - `conditions` (`Ready`; `ConditionReady` constant)
+  - `targets[]`, as a list-map keyed by `name`, each with: `name`, `address`, `ready`,
+    `reason`, `message` (at most 1024 characters), `latencyMillis`,
+    `lastTransitionTime`, `details`
+  - `targetsReady` (for example `"1/2"`, used by a printer column)
+  - `lastCheckTime`
+- **Printer columns:** Type, Ready, Targets, Last Check, Age.
+- **Viewer role:** `config/rbac/providerhealth_viewer_role.yaml`. No editor role, since the
+  operator owns the object.
+
+### Monitor: `internal/providerhealth`
+
+- **Fields:** `Monitor{Client, Reader, Targets []Target{Name, Type, Checker}, Interval, Timeout}`.
+- **Scheduling:**
+  - `NeedLeaderElection() true`.
+  - Checks once immediately, then on a ticker. Copy the ticker loop from
+    [gc.go:54-80](internal/gc/gc.go#L54-L80).
+- **Each cycle:**
+  - Runs every provider concurrently under `context.WithTimeout(Timeout)`, inside one
+    `tracing.Start` span.
+  - Stores the snapshot under a mutex; `Snapshot()` returns a deep copy.
+  - Logs changes. Going down logs `WARNING: provider target not ready` with provider,
+    target, address, reason and message. Coming back logs `Provider target ready`. A
+    target that is ready on first sight logs at V(1) only.
+- **Writes:**
+  - Get through `mgr.GetAPIReader()`, not the cache. That avoids a cluster-wide informer
+    and the watch permission it would need, whatever `--proxy-namespace` is set to.
+  - Create the object if it is missing, labelled
+    `app.kubernetes.io/managed-by=egress-proxies-operator`.
+  - Then `Status().Patch(MergeFrom(base))` when the status changed, following
+    [status.go:114-121](internal/controller/status.go#L114-L121).
+  - This means one write per provider per interval, because `lastCheckTime` moves each
+    time. That's acceptable given the 10s minimum interval.
+- **`buildStatus(prev, result, now)` is a pure function:**
+  - A target keeps its `lastTransitionTime` unless `ready` flipped.
+  - The `Ready` condition is set through `apimeta.SetStatusCondition`:
+    - `True/AllTargetsReady` when every target is ready
+    - `False/TargetsNotReady` otherwise, with the failing targets in the message
+    - `Unknown/ChecksNotImplemented` when the provider has no Checker
+    - `Unknown/NoTargets` when there are zero targets
+- **Cleanup, once at `Start`:** delete labelled objects whose name isn't in the config.
+  The config is loaded once per process, and unlabelled objects are never touched.
+- **Interval 0 (disabled):** run the cleanup with nothing kept, then return nil, so no
+  object is left showing a stale `Ready=True`.
+- **Invalid provider names:** provider names aren't validated as DNS-1123 names. For an
+  invalid name, warn once and skip the object writes; metrics are still emitted.
+- **`ValidateSchedule(interval, timeout)`** is called in `main.go` before the manager
+  starts. Interval must be 0 or at least 10s; timeout must be in (0, interval].
+- **RBAC markers on `Monitor`:**
+  - `providerhealths`: get, list, create, delete
+  - `providerhealths/status`: get, patch
+  - `make manifests` regenerates `config/rbac/role.yaml`.
+
+### Metrics and wiring
+
+- **New function:** `metrics.RegisterProviderTargets(reg, read func() []ProviderTarget) error`.
+  `Register`'s signature is left alone.
+- **Gauges:** a scrape-time collector, in the style of `constCollector`, reads the Monitor
+  snapshot and emits three gauges labelled `{provider,target}`:
+  - `proxy_operator_provider_target_up`
+  - `proxy_operator_provider_check_duration_seconds`
+  - `proxy_operator_provider_check_timestamp_seconds`
+- **Conversion closure:** the one that turns the Monitor snapshot into gauges lives in
+  `main.go`, next to `proxyPhaseCounts`. So `providerhealth` doesn't import `metrics`.
+- **No pool-bytes gauge:** that value is in status, and node-exporter is the right source
+  for disk alerts.
+- **`cmd/main.go`:**
+  - New flags `--provider-check-interval` (1m) and `--provider-check-timeout` (15s).
+  - Build the `Targets` from `cfg.Providers` in config order, with
+    `c, _ := raw[pc.Name].(provider.Checker)`.
+  - `mgr.Add(&providerhealth.Monitor{Client: mgr.GetClient(), Reader: mgr.GetAPIReader(), ...})`,
+    then register the collector.
+
+## Steps (one commit each, execution-log entry per step)
+
+**Step 0 — Save the plan.**
+- Create branch `feat/provider-health` off `main`.
+- Copy this plan to `docs/plans/2026-09-11-1526-provider-health-checks.md` and commit it
+  alone.
+- The uncommitted `.claude/settings.json` change stays out of every commit.
+
+**Step 1 — ProviderHealth API and CRD.**
+- `api/v1alpha1/providerhealth_types.go`
+- `zz_generated.deepcopy.go`, from `make generate`
+- `config/crd/bases/crawl.example.com_providerhealths.yaml`, from `make manifests`
+- the CRD and RBAC `kustomization.yaml` files, plus the viewer role
+- `PROJECT`
+
+**Step 2 — Contract plus the kubernetes and GCP checks.**
+- `internal/provider/check.go`
+- `internal/provider/kubernetes/{kubernetes.go,check.go,check_test.go}`
+- `internal/provider/gcp/{gcp.go,check.go,check_test.go,gcp_test.go}`
+
+**Step 3 — KVM check.**
+- `internal/provider/kvm/{conn.go,trace.go,check.go,check_test.go,fake_test.go,conn_test.go,trace_test.go,kvm_integration_test.go}`
+
+**Step 4 — Monitor.**
+- `internal/providerhealth/{monitor.go,status.go,monitor_test.go,status_test.go,envtest_test.go}`
+- `config/rbac/role.yaml`, from `make manifests`
+
+**Step 5 — Metrics and wiring.**
+- `internal/metrics/{metrics.go,metrics_test.go}`
+- `cmd/main.go`
+- `config/manager/manager.yaml`: a comment for the new flags
+- `test/e2e/e2e_test.go`
+
+**Step 6 — Docs.**
+- `docs/architecture.md`: a component row plus a provider-health section
+- `docs/kvm-provider.md`: the reason table, the shared-connection behaviour, and the
+  Secret-rotation gap under "Known gaps"
+- `docs/testing.md`: the new tests and the commands
+- `CLAUDE.md`: its Architecture and Gotchas sections still say "empty scaffold"; point
+  them at `docs/architecture.md`
+
+**Step 7 — Validate, record, push, open the PR.**
+- Run `go vet ./...`, `go test -short ./...`, `make test` and `make test-e2e`.
+- Hand the user the KVM commands below, and record their results in the execution log.
+- Add a CHANGELOG entry once the user confirms.
+- Push, using the token-file credential helper from memory.
+- Open the PR against `main` through the Gitea API (`tea` isn't installed).
+- Don't merge.
+
+## Tests
+
+**Provider checks** (table-driven, `Test<Func>_<scenario>`):
+- kubernetes:
+  - reachable
+  - list options: `Limit==1`, the label selector, no namespace
+  - failure reasons: forbidden, unauthorized, timeout, cancelled, internal, and a
+    network `*url.Error`
+- gcp:
+  - reachable, asserting the request fields
+  - failure reasons: 401, 403, 500, deadline, network
+  - `TestRealInstances_Probe_fetchesOnePage` against `httptest`: exactly one request with
+    `maxResults=1`, even though the page has a `nextPageToken`
+- kvm (using `hostFakes`/`newTestProvider` from `kvm_test.go`):
+  - reachable, with details filled in
+  - per-host verdicts, order preserved
+  - pool inactive, pool missing
+  - hosts checked concurrently (a sequential check would deadlock)
+  - a pool verdict keeps the connection (the dial count stays 1)
+  - a `checkReason` table
+  - `TestSSHAuth_dial_stages`, using the in-process SSH server in `conn_test.go`
+  - three new rows in the tracing table
+
+**Monitor:**
+- Setup: the fake client is built with a status subresource and a RESTMapper that
+  registers ProviderHealth as cluster-scoped.
+- Tests:
+  - `buildStatus` table
+  - objects are created with the label, and existing ones are patched
+  - the timeout bounds a checker that blocks
+  - a nil checker gives Unknown
+  - only transitions are logged (`funcr` logger)
+  - cleanup deletes only labelled objects that aren't configured
+  - the disabled path cleans up and returns
+  - an invalid name skips the writes
+  - `ValidateSchedule` table
+- An envtest round-trip, skipped under `-short`, which proves the generated schema is
+  accepted by a real apiserver.
+
+**Metrics:** `TestRegisterProviderTargets_scrape`, using a fresh registry and
+`GatherAndCompare`; an empty snapshot produces no series.
+
+**e2e (kind):** `providerhealth kubernetes` reaches `Ready=True`, which proves the
+generated RBAC works end to end.
+
+## Verification
+
+**Automated (Claude runs):** `go vet ./...`, `go test -short ./...`, `make test` (envtest
+with `-race`) and `make test-e2e`.
+
+**Against the real hypervisor (handed to the user, not run by Claude)**, with beelink as
+the host:
+
+```bash
+KVM_TEST_HOST=beelink.tail8c69c.ts.net:22 KVM_TEST_USER=novakj KVM_TEST_SSH_KEY=~/.ssh/id_ed25519 \
+KVM_TEST_KNOWN_HOSTS=~/.ssh/known_hosts KVM_TEST_BRIDGE=<bridge> KVM_TEST_IMAGE_URL=<image-url> \
+  go test -tags=integration -count=1 -v -run TestKVMCheck ./internal/provider/kvm/
+
+make install && go run ./cmd/main.go --providers-config <scratch copy of kvm-home config, local key paths> \
+  --metrics-bind-address :8080 --metrics-secure=false --provider-check-interval=15s
+kubectl get ph -w
+kubectl get ph kvm-home -o yaml
+curl -s localhost:8080/metrics | grep -E 'proxy_operator_provider_(target_up|check_)'
+```
+
+Each negative case is a one-line edit to the scratch config; nothing changes on the host.
+
+| Edit | Expected reason |
+|---|---|
+| `pool: nope` | `PoolNotFound` |
+| `address: …:2` | `Unreachable` |
+| `knownHostsFile: /dev/null` or `user: nobody` | `SSHHandshakeFailed` |
+| `sshKeyFile: /nonexistent` | `SSHMaterialUnavailable` |
+| `socket: /nonexistent` | `LibvirtConnectFailed` |
+
+## Out of scope
+
+- Gating Proxy creates on a host that is known to be down.
+- Tying `/readyz` to provider health.
+- Bridge checks.
+- SelfSubjectAccessReview checks of create/delete rights.
+- Hot-reloading the providers config.
+- Picking up a rotated `kvm-ssh` Secret without a restart. This is an existing gap; it
+  gets documented.