From d96e56f1a0a822cb8fad038af0202e35b190fcfa Mon Sep 17 00:00:00 2001 From: kacerr Date: Wed, 16 Sep 2026 08:54:40 +0000 Subject: [PATCH] Write the provider blueprint docs/providers.md collects the provider contract that was spread across comments, callers and per-provider docs: MUST/SHOULD/MAY rules per method, each tied to the reconciler or GC behaviour it protects, the error taxonomy, a feature matrix for kubernetes/gcp/kvm with file:line notes, a checklist for adding a provider, and the known gaps. Linked from architecture.md and CLAUDE.md. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 7 +- docs/architecture.md | 2 +- .../2026-09-16-0841-provider-blueprint.md | 70 +++ docs/providers.md | 520 ++++++++++++++++++ 4 files changed, 595 insertions(+), 4 deletions(-) create mode 100644 docs/plans-executions/2026-09-16-0841-provider-blueprint.md create mode 100644 docs/providers.md diff --git a/CLAUDE.md b/CLAUDE.md index 08b04a9..62b750b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -27,9 +27,10 @@ entrypoint yet, so there's nothing to run locally. Documented in [docs/architecture.md](docs/architecture.md). It holds the component table, the reconcile flow, and one section per runnable: health engine, discovery and -lease API, orphan GC, metrics, tracing, and provider health checks. Provider-specific -detail is in `docs/kvm-provider.md` and the GCP docs; the test layout is in -`docs/testing.md`. +lease API, orphan GC, metrics, tracing, and provider health checks. The provider +contract (MUST/SHOULD rules, feature matrix, checklist for adding a provider, known +gaps) is in [docs/providers.md](docs/providers.md). Provider-specific detail is in +`docs/kvm-provider.md` and the GCP docs; the test layout is in `docs/testing.md`. ## Conventions diff --git a/docs/architecture.md b/docs/architecture.md index c847d55..983d69c 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -6,7 +6,7 @@ |---|---|---|---|---| | Proxy + ProviderHealth CRDs | `api/v1alpha1` | types | — | `Proxy` spec/status, CEL validation, defaulting, pure helpers; the status-only, cluster-scoped `ProviderHealth` | | Reconciler | `internal/controller` | controller | yes (with the manager) | the state machine: provision, replace, delete, represent health | -| Provider contract | `internal/provider` | library | — | `Provider` interface, optional `Checker`, error taxonomy, deterministic naming, config, metrics decorator | +| Provider contract | `internal/provider` | library | — | `Provider` interface, optional `Checker`, error taxonomy, deterministic naming, config, metrics decorator; rules and per-provider matrix in [providers.md](providers.md) | | kubernetes provider | `internal/provider/kubernetes` | library | — | real Squid pods in this cluster (local dev/CI) | | gcp provider | `internal/provider/gcp` | library | — | Compute Engine VMs, four API calls, fire-and-forget ops | | kvm provider | `internal/provider/kvm` | library | — | VMs on libvirt hosts over SSH-forwarded sockets; image cache, overlay disks, NoCloud seeds | diff --git a/docs/plans-executions/2026-09-16-0841-provider-blueprint.md b/docs/plans-executions/2026-09-16-0841-provider-blueprint.md new file mode 100644 index 0000000..b5c3589 --- /dev/null +++ b/docs/plans-executions/2026-09-16-0841-provider-blueprint.md @@ -0,0 +1,70 @@ +# Execution: provider blueprint +Plan: [docs/plans/2026-09-16-0841-provider-blueprint.md](../plans/2026-09-16-0841-provider-blueprint.md) + +- [x] Step 0 — Save the plan; wait for the merge +- [x] Step 1 — Collect evidence (read-only, in parallel) +- [x] Step 2 — Write `docs/providers.md` +- [ ] Step 3 — Verify, then open the MR + +## Step 0 — Save the plan; wait for the merge + +The plan was committed on `feat/provider-health` (`80b2e9c`) and reached `main` +through PR #6. After the user confirmed the merge, the branch was cut from the +updated `main`: + +```bash +git switch main && git pull --ff-only +git switch -c feat/provider-blueprint +``` + +Worth noting: `.claude/settings.json` has local, uncommitted permission edits +that carried over through the switch. They are deliberately left out of this +branch. + +## Step 1 — Collect evidence (read-only, in parallel) + +Three Explore agents answered the plan's 12-item questionnaire, one each for +kubernetes, gcp and kvm. Meanwhile I read the shared layer: +`internal/provider/*.go`, `registry.go`, the provider wiring in `cmd/main.go`, +`reconcileManaged`/`replaceInstance`/`reconcileDelete` in the controller, +`internal/gc/gc.go`, and how the monitor uses `Checker`. + +Before relying on the agents' claims, I spot-checked them against the code. One +was wrong and did not go into the doc: the kvm agent said `Delete` skips volume +removal when `DomainDestroy` returns `ErrNoDomain`. In fact `removeDomain` +returns nil there, and `Delete` still calls `removeVolumes` afterwards +(`kvm.go:477-482`). + +Rules that exist only in the callers, and not in any comment, were found by +reading the controller and GC. They went into the doc as MUST rules: +- A zero `CreatedAt` makes an instance immediately collectable (`gc.go:120`). +- `Get` returning `ErrPermanent` on the deletion path makes the reconciler call + `Delete` anyway. +- Replacement waits for `Get` to return NotFound before creating an instance + with the same name. +- `Stopped` and `Terminated` both lead to delete-and-recreate. + +Worth noting, found outside this plan's scope and not changed here: +- `docs/gcp-vm-validation.md` filters on `labels.proxy-operator-managed=yes`, + but the value is `true`. +- According to the gcp agent, `docs/gcp-in-specific-project.md` holds a real + project ID, IAM output and an egress IP. The user was told. + +## Step 2 — Write `docs/providers.md` + +Wrote the eight sections the plan lists. Linked the new doc from the Provider +contract row in `docs/architecture.md` and from the CLAUDE.md "Architecture" +section. + +Judgment calls: +- The file:line evidence sits in per-provider notes under the matrix, as + planned. The contract rules in §2 cite the reconciler and GC lines each rule + protects. +- The gap list includes documentation drift (kvm-provider.md's ARP order and + "from startup" dial, gcp-vm-validation.md's label value and its claim that + async errors reach status), as well as code gaps. Both would mislead the next + provider author in the same way. +- "Proposed features" stays a flat list with no design, as the user decided. + +Worth noting: the line numbers in the doc are a snapshot. The doc says so and +asks readers to look for the named function when a line has moved. diff --git a/docs/providers.md b/docs/providers.md new file mode 100644 index 0000000..49f7c68 --- /dev/null +++ b/docs/providers.md @@ -0,0 +1,520 @@ +# Providers: the blueprint + +This page is for whoever writes the next provider. It explains the contract a +provider must meet, which caller depends on each rule, how the three existing +providers (kubernetes, gcp, kvm) meet it, and what is still missing. + +Where the rest lives: + +- **Data flow:** the calls each provider makes are in + [architecture.md §5](architecture.md#5-what-provider-calls-do-in-the-outside-world). +- **Health checks:** the monitor that runs them is in + [architecture.md §11](architecture.md#11-provider-health-checks-internalproviderhealth). +- **Per-provider guides:** [kvm-provider.md](kvm-provider.md) and + [gcp-vm-validation.md](gcp-vm-validation.md). +- **Code:** the contract itself is + [`internal/provider/provider.go`](../internal/provider/provider.go), + [`errors.go`](../internal/provider/errors.go) and + [`check.go`](../internal/provider/check.go). + +Rule keywords: **MUST** means breaking the rule breaks a caller. **SHOULD** +means every current provider does it and a new one needs a reason not to. +**MAY** means it's allowed. + +Code references are `path:line` at the time of writing. When a line has moved, +look for the function named next to it. + +## 1. What a provider is + +A provider is a Go type implementing `provider.Provider`: four methods over one +VM-like instance, identified by an opaque `providerID` string. Nothing else in +the operator knows how a backend works: + +- **The reconciler** (`internal/controller`) calls `Create`, `Get` and `Delete` + and branches only on the error class and `Instance.State`. +- **Orphan GC** (`internal/gc`) calls `ListByTag` and `Delete`. +- **The provider health monitor** (`internal/providerhealth`) calls the optional + `Checker`. + +The reconciler never watches backend resources. Everything it knows comes from +polling `Get` (`ProvisioningPoll` and `DeletionPoll` 10s, `RequeueNow` 1s, +`QuotaRetry` 5m, `internal/controller/proxy_controller.go:452-467`). Up to 3 +Proxies reconcile at once (`MaxConcurrentReconciles: 3`, `:442`). + +Providers are named instances of a type. `gcp-eu` and `gcp-us` can both have +type `gcp`. The name is what `Proxy.spec.provider` refers to. + +## 2. The contract, method by method + +### Create(ctx, CreateRequest) (providerID, error) + +`CreateRequest` carries: + +- `Name`: from `NameFromUID`, 22 characters, `proxy-` plus base32. +- `UID`: the owning Proxy's UID. +- `Namespace` and `ProxyName`. +- `Placement`: `Region`, `Zone`, `MachineType`, `Image`. +- `CloudInit`: already resolved from inline text or a Secret. +- `Port`. + +Rules: + +- **MUST be idempotent by `req.Name`.** A repeat call for a name that already + exists returns that instance's providerID with a nil error. It must not return + an error and must not create a duplicate. + *Why:* the reconciler records the ID in status only after `Create` returns + (`proxy_controller.go:155-172`). A crash between the two, or a status write + that is lost, means `Create` runs again for the same Proxy. `replaceInstance` + also depends on this to recover from a crash between its two writes + (`:240-244`). +- **MUST mark the instance as managed when it is created**, with + `LabelManaged=true` and `LabelUID=` or the backend's equivalent. kvm + uses domain `` because libvirt has no labels. + *Why:* orphan GC finds instances only through that mark (`internal/gc/gc.go:105`), + and never deletes one with an empty UID (`gc.go:113-117`). An instance that + exists for a while without the mark leaks if the operator crashes. +- **MUST return a providerID that is self-contained.** `Get` and `Delete` + receive only the ID, and the config or spec may have changed since `Create`, + for example a zone edit during a replacement. So put the location in the ID: + - kubernetes: `/` + - gcp: `zones//instances/` + - kvm: `hosts//domains/` + + Validate any config values that end up in the ID so they can't contain the + separator (`config_kvm.go:108-114`). +- **MUST honour `ctx`.** The reconcile context is cancelled on shutdown and when + leadership is lost. +- **SHOULD return once the request is submitted, not once the instance is + running.** Readiness is discovered by polling `Get`. A blocking `Create` holds + one of the 3 reconcile workers for its whole duration. kvm breaks this rule on + purpose, see §8. +- **SHOULD reject a request it can never satisfy** with `ErrPermanent` before + touching the backend: a missing zone, machine type or image, or a name that is + not in its catalogue (gcp `gcp.go:137-141`, kvm `kvm.go:126-144`, `:160-164`). +- **MAY ignore `Placement` fields that don't apply** (`provider.go:56-57`). If a + field is ignored, the provider's doc page MUST say so, because the fields are + still part of the spec hash. Changing an ignored field replaces the instance + and has no other effect. +- **SHOULD deliver `CloudInit` as user-data.** A provider that can't (kubernetes) + documents that it ignores the field. +- **MUST NOT log `CloudInit` contents.** It can hold secrets. Log its byte count + instead. gcp and kvm test this with a sentinel string (`kvm_test.go:975-995`, + `gcp_test.go:330-407`). + +### Get(ctx, providerID) (*Instance, error) + +- **MUST return `ErrNotFound` when, and only when, the instance is gone.** + *Why:* `ErrNotFound` triggers three actions: + - In normal reconciling, the reconciler clears the ID and creates again + (`proxy_controller.go:197-202`). + - During replacement, it is the signal to create the new instance + (`:237-251`). + - During deletion, it releases the finalizer (`:287-291`). + + A false NotFound during an outage creates a duplicate or loses the instance. + "Unreachable", "wrong credentials" and "can't tell" are Transient or + Permanent, never NotFound. +- **MUST keep returning the instance, not NotFound, while a delete is in + progress** and the name is still taken. + *Why:* the replacement instance gets the same name. The reconciler waits for + NotFound before creating it (`:228-233` comment, `:237-251`). A NotFound too + early would make `Create` adopt the dying instance. +- **MUST report `Running` only with a non-empty `IP`.** Running without an + address yet is `Provisioning`. + *Why:* the IP from a `Running` answer goes straight into status and the health + engine (`:212-215`). All three providers do this. +- **MUST report `Stopped` or `Terminated` only when the instance will not + recover by itself.** SHOULD map states it doesn't recognise to `Stopped`. + *Why:* the reconciler deletes and recreates on either state (`:216-226`); the + instances are treated as disposable. The two states cause exactly the same + reaction. +- **MUST fill `UID` and `CreatedAt` by reading them back from the backend**, + never from memory. + *Why:* GC compares `UID` with the live Proxies. It deletes anything whose + `CreatedAt` is older than `MinAge` (10m), and **a zero `CreatedAt` counts as + very old** (`gc.go:120`). A provider that forgets `CreatedAt` makes instances + still being created collectable. An unparsable timestamp gives the zero time + in both gcp and kvm. That is accepted because it never shields an orphan + forever (`gcp.go:253-256`). +- **SHOULD return `ErrPermanent` for an ID it can never resolve**, such as a + malformed ID or a host no longer in the config. + *Why:* the deletion path then tries `Delete` anyway and releases the + finalizer if that works (`:292-302`). If the provider returned Transient, + deletion would poll forever. + +### Delete(ctx, providerID) error + +- **MUST be idempotent.** Deleting something that is already gone returns nil. + *Why:* the reconciler calls `Delete` again on every poll until `Get` returns + NotFound (`:305-311`). GC and the reconciler can also race. +- **MUST remove everything `Create` made**: disks, volumes, seeds, addresses. + Either delete them explicitly or rely on the backend doing it (gcp + `AutoDelete` on the boot disk). + *Why:* GC sees only what `ListByTag` returns, so secondary resources have no + other cleanup path. +- **MAY return before the instance is gone** (gcp, kubernetes), as long as `Get` + keeps showing it until it is (see Get). +- **SHOULD succeed for an ID whose `Get` fails permanently** when there is + nothing left it could reach. Example: kvm on a host that was removed from the + config returns nil (`kvm.go:466-476`). + *Why:* on the deletion path a permanent error isn't latched. It keeps + retrying (`deletionFailure`, `:349-359`), so a Proxy is only released through + the `crawl.example.com/skip-cleanup` annotation. + +### ListByTag(ctx) ([]Instance, error) + +- **MUST return only instances that carry the managed mark**, each with an `ID` + that `Delete` accepts, plus `UID` and `CreatedAt`. + *Why:* GC deletes every listed instance whose UID isn't live and whose age is + past `MinAge` (`gc.go:111-131`). Listing something without the mark is how an + operator deletes a VM it doesn't own. +- **MUST return an error rather than a list it can't vouch for.** An error makes + GC skip that provider for the sweep (`gc.go:105-110`). +- **MAY leave out instances it couldn't read.** Leaving something out is always + safe, because GC never deletes what isn't listed. The current providers make + different choices here: + - gcp silently leaves out unreachable zones (`ReturnPartialSuccess`, + `gcp.go:229`). + - kvm fails the whole list when one host is down but skips a single unreadable + domain (`kvm.go:540-565`, `:580-596`). + - kubernetes makes a single call. +- **SHOULD cover the whole scope the provider can create in**: every namespace, + zone or host. Anything outside that scope is never collected. +- **Scope caveat:** GC protects instances by UID, not by operator install. Two + installs, or two clusters, sharing one GCP project or hypervisor will delete + each other's instances ([gcp-vm-validation.md](gcp-vm-validation.md#gotchas)). + Two provider entries of one install that share a scope are safe, because they + share the live-UID set. + +## 3. Error taxonomy + +Every error a provider method returns MUST be built with +`provider.Wrap(class, op, providerName, id, err)`. `op` is one of `create`, +`get`, `delete` or `list`, and `providerName` is the **configured name** +(`cfg.Name`), not the type. + +`Wrap` keeps both `errors.Is(err, class)` and `errors.As` to the SDK error +working (`errors.go:53-62`). The shared metrics and tracing decorators label +results with the class (`metrics.go:61-74`, `tracing.go:67-80`). + +| Class | Use it for | During normal reconciling | While deleting | +|---|---|---|---| +| `ErrNotFound` | the instance doesn't exist | Get: recreate or advance (§2); other ops: returned as the error, so exponential backoff | Get: release the finalizer | +| `ErrQuotaExceeded` | quota or rate limit | condition `QuotaExceeded`, requeue after `QuotaRetry` (5m), not counted as an error (`:337-340`) | requeue after `QuotaRetry` | +| `ErrTransient` | network, 5xx, timeouts, cancellation, anything unknown | error returned, so workqueue exponential backoff | same | +| `ErrPermanent` | bad config, bad argument, auth or permission denied, malformed ID | **latched**: condition `PermanentError`, no more provider calls until `metadata.generation` changes (`:130-135`, `:341-343`) | error returned and retried forever; Get Permanent also triggers `Delete` (`:292-302`) | + +Pitfalls: + +- **Unclassified means Transient.** `provider.Class` returns `ErrTransient` for + any error not built with `Wrap` (`errors.go:71-91`). That is the safe default, + but a permanent failure that was never classified retries forever, and a quota + error backs off far too fast. +- **Permanent is sticky.** Use it only for errors that editing the Proxy spec or + fixing the config can resolve. A 403 that could be a quota problem is the hard + case: gcp checks the error reason (`gcp/errors.go:31-34`), while kubernetes + can't tell quota from RBAC and picks Permanent (`kubernetes.go:201-206`). +- **Don't return NotFound from `Create`.** The reconciler treats it as Transient + there, but it misleads the metrics and anyone reading logs. kvm can currently + do this (§8). +- **Classify in one `classify` function** with a table-driven test. All three + providers do. + +## 4. Identity, names and GC marks + +- **Names:** `provider.NameFromUID(uid)` gives `proxy-` plus 16 lowercase base32 + characters (`name.go:29-33`). It is valid as a GCP name, a DNS label and a + libvirt domain name. A backend with stricter rules SHOULD map it + deterministically rather than invent a new scheme, because idempotency depends + on the name being the same every time. +- **providerID:** opaque to everyone except the provider. Keep it + `/`-shaped, parse it strictly, and return `ErrPermanent` for + a malformed one (§2). +- **Marks:** `LabelManaged = "proxy-operator-managed"` with value `"true"`, and + `LabelUID = "proxy-operator-uid"` (`provider.go:28-32`). Use native labels or + tags where the backend has them. Otherwise store them in metadata that is + written in the same call that creates the instance (kvm: `metadata.go`). +- **`CreatedAt`:** read it from the backend's own creation timestamp. If the + backend has none, write it into the metadata at creation (kvm: + `kvm.go:277-298`). + +## 5. Optional interfaces: `Checker` + +`provider.Checker` is `Check(ctx) []TargetStatus` (`check.go:37-46`). The +monitor type-asserts it on the **unwrapped** provider, before `cmd/main.go` adds +the decorators (`cmd/main.go:211-222`). A provider without it shows up as +unsupported, not as failed. + +- **MUST never fail as a whole.** Every failure is a `TargetStatus` with + `Ready=false`. +- **MUST return promptly once `ctx` ends.** The monitor gives the whole round + one timeout (`monitor.go:180`). +- **MUST return one entry per target, in a stable order.** Targets are what can + fail independently: the apiserver, a project, each hypervisor. +- **MUST NOT disrupt reconciles.** A check that shares a connection with + provisioning must not tear that connection down when it times out. kvm uses + `hostPool.probe` for this, not `call` (`kvm/check.go:65`, `kvm/conn.go:213-242`). +- **SHOULD use the shared reasons** (`Reachable`, `Timeout`, `Unreachable`, + `Unauthorized`, `Forbidden`, `APIError`). Add provider-specific reasons only + for failures an admin would fix differently: kvm has `SSHHandshakeFailed`, + `PoolNotFound` and others. +- **SHOULD check with the same credentials, verb and scope that `ListByTag` and + `Create` use**, and be cheap: one page, one object. +- **MAY fill `Details`** with facts useful when triaging, such as versions or + capacity. Only kvm does today. + +## 6. Feature matrix + +Legend: **yes** / **partial** / **no**, with a short note. The evidence is in +the per-provider notes below the table. + +| Feature | kubernetes | gcp | kvm | +|---|---|---|---| +| providerID self-contained | yes: `ns/name` | yes: `zones/z/instances/n`; project from config | yes: `hosts/h/domains/n` | +| Create idempotent | yes: AlreadyExists = success | yes: 409 = success | yes: lookup by name; overlay reused, seed rebuilt | +| Create verifies adopted instance's UID | no | no | no | +| Create non-blocking | yes | yes: operation not awaited | **no**: waits for `DomainCreate`; first use of an image downloads it | +| Placement used | none | Zone, MachineType, Image | Zone (host), MachineType, Image | +| CloudInit delivered | **no**: ignored | yes: `user-data` metadata | yes: NoCloud seed ISO | +| Port used | yes: Squid config and containerPort | no: firewall is manual | no | +| GC marks on create | yes: Pod labels | yes: instance labels | yes: domain ``; volumes unmarked | +| States produced | Provisioning, Running, Terminated | all four | Provisioning, Running, Stopped | +| IP source | PodIP | external NAT IP only | ARP, then guest agent; first IPv4 | +| UID / CreatedAt readback | yes | yes | yes | +| Delete idempotent | yes | yes | yes, at each step | +| Delete waits until gone | no | no | yes | +| Delete cleans secondary resources | n/a | yes: boot disk auto-deleted | yes: overlay and seed; base image kept | +| ListByTag scope | cluster-wide | one project, all zones | configured hosts | +| ListByTag on partial failure | n/a: single call | leaves out failed zones | whole list fails if a host is down; unreadable domain skipped | +| Quota detection | no | yes: 429 and 403 quota reasons | no | +| Error.Provider = configured name | **no**: hardcoded `"kubernetes"` | yes | yes | +| Checker | yes: `apiserver` | yes: the project | yes: each host | +| Checker Details | no | no | yes | +| Credentials | in-cluster config / kubeconfig | ADC | SSH key + known_hosts files, usually a Secret | +| Credential rotation without restart | n/a | via ADC | no: cached after first load | +| Connection handling | client-go | SDK client; SDK retries on Get and List | one pooled SSH+libvirt connection per host, keepalive | +| Logging | none in the package | V(1) ops, V(2) detail, wire log | V(1) ops and RPCs, V(2) detail | +| Unit tests with a fake | yes: controller-runtime fake client | yes: `fakeAPI` + httptest | yes: `fakeAPI` + in-process SSH server | +| Real-backend test | kind e2e | manual runbook | `//go:build integration` | + +### kubernetes notes (`internal/provider/kubernetes`) + +- **ID:** `kubernetes.go:152-165`. An empty name part is not rejected. +- **Create:** `kubernetes.go:90-100` and `pod.go:23-50`. Only `Name`, + `Namespace`, `UID` and `Port` are used. Squid config is generated from the + port and passed through `SQUID_CONF` (`pod.go:38-75`). +- **State:** `stateFromPod`, `kubernetes.go:185-199`. Pending, or Running with + no PodIP, is Provisioning. Succeeded, Failed and Unknown are Terminated. A Pod + being deleted still reports its phase. +- **Delete:** `kubernetes.go:118-127`. ListByTag: `kubernetes.go:134-146`. +- **Errors:** `classify`, `kubernetes.go:201-218`. 429 and timeouts are + Transient. Forbidden, Invalid, BadRequest and Unauthorized are Permanent. + Nothing is Quota. +- **Check:** `check.go:21-53`. It lists at most one managed Pod cluster-wide, so + it proves read and list access, not create. +- **Config:** only `kubernetes.image` (`config.go:45-48`, default at + `kubernetes.go:28`). + +### gcp notes (`internal/provider/gcp`) + +- **ID:** `gcp.go:289-299`. +- **Create:** `gcp.go:135-167` and `insert.go:34-74`. `Operation.Wait` is never + called. There is one NIC with an ephemeral external IP, and the boot disk has + `AutoDelete`. +- **State:** `mapState`, `gcp.go:271-287`. RUNNING without a NAT IP is + Provisioning. Unknown statuses are Stopped. +- **Get:** `gcp.go:172-192`. Delete: `gcp.go:196-219`. ListByTag: + `gcp.go:224-244`. +- **Errors:** `errors.go:20-39`. Non-googleapi errors are Transient. 404 is + NotFound. 429 and 403 with reason `quotaExceeded` or `rateLimitExceeded` are + Quota. 400, 401 and other 403s are Permanent. +- **Check:** `check.go:30-78`. One `AggregatedList` page with `maxResults=1`. +- **Config:** `project` is required. `network`, `networkTag` and `diskSizeGb` + have defaults (`config.go:51-61`, `insert.go:12-30`). +- **Logging:** V(1) and V(2) tiers in `gcp.go`. The wire log in `wirelog.go` + runs at V(5), with payloads elided unless `--gcp-wire-log-full-payloads` is + set. + +### kvm notes (`internal/provider/kvm`) + +- **ID:** `id.go:11-21`. Host names are restricted by `config_kvm.go:108-114`. +- **Create:** `kvm.go:154-221`: + - Looks up the domain by name and starts it if needed (`ensureRunning`, + `:300-318`). + - Otherwise: base image (`image.go`), then overlay (reused if it exists, + `:225-245`), then seed (always rebuilt, `:249-275`), then define with + metadata (`:277-298`), then start. +- **Get:** `kvm.go:323-356`. States come from `mapState` (`:442-453`): nostate + is Provisioning, running or blocked without an IP is Provisioning, everything + else is Stopped. +- **Addresses:** `address` (`:397-428`) tries ARP first, then the guest agent. +- **Delete:** `kvm.go:461-538`. Destroy, undefine (with NVRAM, managed save and + snapshot metadata), then remove the overlay and seed by name. A host that is no + longer configured is a success. +- **ListByTag:** `kvm.go:545-596`. +- **Errors:** `errors.go:21-42`. No domain or no volume is NotFound. XML, + unsupported, invalid-argument, auth and access errors, and a missing pool, are + Permanent. Everything else is Transient, including SSH handshake and auth + failures. Nothing is Quota. +- **Check:** `check.go:41-139`. Runs concurrently per host through + `hostPool.probe`. Reasons are tagged by the dial stage. +- **Connections:** `conn.go`. Dials lazily, one connection per host with a 15s + keepalive. On cancel, `call` closes the shared connection; `probe` abandons + the RPC and leaves the connection open. +- **Full guide:** [kvm-provider.md](kvm-provider.md). + +## 7. Adding a provider: checklist + +1. **Package:** create `internal/provider//` with the provider type and a + compile-time `var _ provider.Provider = (*Provider)(nil)` (and + `provider.Checker`). + - Put the backend SDK behind a small interface (gcp `instancesAPI`, kvm + `libvirtAPI`) so a fake can stand in for it in tests. + - Build the logger with + `logf.FromContext(ctx).WithName("").WithValues("provider", name)`. +2. **Config:** in `internal/provider/config.go`: + - Add a `Config` block and a `ProviderConfig` field. + - Add the block to `setBlocks`. + - Add the type to the `switch p.Type` allow-list (`config.go:102-108`). + - Add required-field validation, in its own `config_.go` if it is + large. + - Add rows to `config_test.go`. + + The file is parsed with `UnmarshalStrict`, so unknown keys already fail. +3. **Constructor:** `New(ctx, provider.ProviderConfig) (provider.Provider, + error)`, registered in the `registry.Build` map in `cmd/main.go:195-205`. + - A constructor error stops the manager. + - So fail only on config errors, and **don't dial the backend here**: one + unreachable hypervisor or API must not take down the other providers (kvm + logs a warning and connects lazily, `kvm.go:89-101`). +4. **Decorators:** nothing to do. `cmd/main.go:219-222` wraps every provider in + `WithTracing(WithMetrics(...))`. Don't count requests or open per-operation + spans yourself. +5. **Contract:** implement each rule in §2, and test each one against the fake: + - repeat `Create` + - `Get` after `Delete` + - `Delete` twice + - malformed ID + - unknown state mapped to `Stopped` + - `Running` requires an IP + - `UID` and `CreatedAt` read back + - `ListByTag` excludes unmarked instances +6. **Errors:** one `classify` plus a table test covering every class, including + "not an SDK error → Transient". Use `cfg.Name` in `provider.Wrap`. +7. **Checker:** shared reasons first, a table test for the reasons, and a test + that `Check` returns promptly with an expired `ctx`. +8. **Logging:** + - V(1): one line per operation outcome, plus a `logAPIError`-style line with + the raw backend error. + - V(2): request and response detail. + - Never log CloudInit contents; add a sentinel test for it. +9. **Real-backend test:** behind `//go:build integration`, skipped when its + environment variables are unset. It runs the whole lifecycle: create, create + again, poll until Running, list, delete, delete again, and confirm it is gone + and cleaned up. See `kvm/kvm_integration_test.go` and + [testing.md](testing.md). +10. **RBAC:** if the provider talks to the cluster, add package-level + `+kubebuilder:rbac` markers, then run `make manifests` and check + `git diff config/rbac/role.yaml`. +11. **Docs:** + - A `docs/-provider.md` covering prerequisites, config reference, + placement mapping, what lands on the backend, the health check, and known + gaps. + - A row in the architecture.md components table, and a block in §5 and + §11. + - A column in the §6 matrix above. + - A sample in `config/samples/`. +12. **Optional:** a demo script under `docs/demo/`. + +## 8. Gaps and proposed features + +These are listed only. Nothing below is designed or implemented. + +### Current gaps + +**Across providers and the contract** + +- **Adoption isn't verified.** `Create` adopts any existing instance with the + right name without checking its `LabelUID`. None of the three providers check + it. +- **The Create comment doesn't match kvm.** The contract says `Create` returns + as soon as the request is submitted (`provider.go:90-94`). kvm blocks until + the domain has started, and on first use of an image, for the whole download. +- **`Stopped` and `Terminated` are indistinguishable to the reconciler.** + kubernetes never produces Stopped, and kvm never produces Terminated. +- **Different ListByTag partial-failure policies.** gcp leaves failed parts out, + kvm fails the whole list. Both are safe for GC; the contract doesn't say which + to prefer. +- **Config is read once per process.** Adding a provider or host, or rotating + credentials, needs a manager restart. +- **Stale comments.** `provider.go:3-4` and `registry/registry.go:5-12` still + describe a `mock` subpackage, which was replaced by the kubernetes provider. + +**kubernetes** + +- **Wrong provider name in errors.** `Error.Provider` is hardcoded to + `"kubernetes"` (`kubernetes.go:105-216`), so errors and logs show the wrong + name when the configured name differs. +- **Spec fields ignored.** `CloudInit` and all of `Placement` are ignored. +- **No quota class.** A quota 403 is Permanent. +- **Deleting Pods look healthy.** A Pod being deleted still reports its phase, + so it can read as `Running`. +- **Unguarded Pods.** No securityContext, no resources, and + `http_access allow all` (`pod.go`). +- **No logging** in the package, and **no Checker `Details`**. + +**gcp** + +- **Asynchronous insert failures are never read.** An insert that the API + accepts but that fails later (quota, bad image) is never seen. The next `Get` + returns NotFound and the reconciler creates it again, so the reason never + reaches status. [gcp-vm-validation.md](gcp-vm-validation.md#gotchas) says + such errors do reach status. +- **External IP only.** A VM without an external NAT IP stays `Provisioning` + forever. +- **No firewall management.** The rule for the proxy port is a manual + prerequisite, and `req.Port` is unused. +- **The SDK client is never closed**, and `Placement.Region` is unused. +- **Wrong label value in the docs.** `gcp-vm-validation.md` filters on + `labels.proxy-operator-managed=yes`; the value is `true`. + +**kvm** (see also [kvm-provider.md § Known gaps](kvm-provider.md#known-gaps)) + +- **No quota or capacity detection.** +- **Create blocks**, including the first image download on each host. +- **GC blind spots.** Volumes without a domain are invisible to GC, and so are + VMs on a host removed from the config. `Delete` treats the latter as done. +- **SSH material is cached after the first successful load**, so rotating keys + needs a restart. +- **Cancelling one operation cuts off the others on that host.** A cancelled + `call` closes the shared per-host connection, aborting other operations in + flight on that host (`conn.go:199-211`). +- **SSH auth failures retry forever.** SSH authentication and host-key failures + are Transient, while libvirt auth errors are Permanent. +- **`Create` can return `ErrNotFound`** when a volume or domain disappears + during the call. +- **No reused-overlay check.** An existing overlay is reused without checking + its backing image or size. +- **Doc drift in kvm-provider.md:** + - It says ARP is the fallback; the code tries ARP first. + - It describes a connection kept open "from startup"; the dial is lazy. + - It says `host` and `name` are on every log line; the check and list paths + lack them. + +### Proposed features (not implemented) + +- **A shared conformance suite:** for example `internal/provider/providertest`, + which runs the §2 rules against any provider plus a fake backend, so a new + provider gets the contract tests for free. +- **Verify the UID when `Create` adopts an instance.** Return `ErrPermanent` on + a mismatch instead of adopting an instance that belongs to someone else. +- **Report asynchronous create failures:** gcp keeps the operation name, and + `Get` reports the failure instead of NotFound. +- **Capacity or quota signals** for kvm, such as pool free space and host + memory, mapped to `ErrQuotaExceeded`. +- **GC for secondary resources:** mark volumes and similar resources, and let + `ListByTag` or a second listing method cover them. +- **Reload credentials and config without a restart.** +- **Checker `Details`** for kubernetes (server version) and gcp (quota + headroom). +- **Internal-IP mode** for gcp, and a firewall rule managed per provider.