diff --git a/CHANGELOG.md b/CHANGELOG.md index 20989d8..a8248f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,37 @@ # Changelog +## 2026-09-10 22:58 UTC — Code-review fixes on the KVM branch: deletion, validation, lazy SSH, guest agent + +Five findings from a review of `feat/kvm-provider`, all fixed on the branch. + +- **A Proxy can always finish deleting.** The delete path used to drop the finalizer + only when `Get` answered not-found, so a permanent error there retried forever — + a KVM host removed from the config was enough. It now calls `Delete` anyway, since + that is idempotent and can finish where `Get` cannot, and + `crawl.example.com/skip-cleanup: "true"` abandons the instance for whatever stays + stuck. In the KVM provider, deleting over an unconfigured host is a no-op success; + `Get` still fails permanently there so a typo cannot look like a missing VM. +- **The guest agent ships as a vendor-data script**, superseding the cloud-config + merge in the entry below. Merging could never be made safe: cloud-init drops + vendor-data keys the user-data defines, and the strategy cannot be overridden from + either document. The Proxy's cloud-init now reaches the seed byte for byte in any + format — Jinja, MIME, gzip, scripts — where the round-trip previously re-typed + unquoted scalars (`version: 1.10` arrived as `1.1`), dropped comments and reordered + keys. New `installGuestAgent` setting, default on. +- **A missing `kvm-ssh` Secret no longer crash-loops the manager.** The SSH material + is read on first use and cached, so only KVM Proxies fail and they recover when the + Secret appears. A missing file is transient, an unparseable key permanent, and + startup still logs a warning. +- **Host names and image keys are validated.** Either can end up in a providerID or a + volume file name, where a slash made the VM unaddressable for good. +- Verified end to end: unit suite, the integration test against a real hypervisor + (22 s, agent reporting the IP), both in-cluster Proxies Ready in 2 min with 204 + through squid, the guest showing `qemu-guest-agent` active with its cloud-config + comments intact, and the manager staying up with the Secret pointed away. +- Key files: `internal/provider/kvm/{seed.go,conn.go,kvm.go}`, + `internal/controller/proxy_controller.go`, `internal/provider/config_kvm.go`, + `api/v1alpha1/proxy_types.go`. + ## 2026-09-10 21:04 UTC — KVM provider passes in-cluster; guest agent merged into cloud-config - Two KVM Proxies with real cloud-configs reach `Ready` from inside the kind cluster diff --git a/docs/plans-executions/2026-09-04-1404-kvm-provider.md b/docs/plans-executions/2026-09-04-1404-kvm-provider.md index 5ef322b..3f961cf 100644 --- a/docs/plans-executions/2026-09-04-1404-kvm-provider.md +++ b/docs/plans-executions/2026-09-04-1404-kvm-provider.md @@ -631,6 +631,26 @@ and the `beelink` host entries in `config/manager/providers_config.yaml` and `config/samples/providers-config.yaml`, which are this machine's dev setup rather than the repo's defaults. +### Run 8: review fixes + +A `/code-review high` pass over the branch found five defects, fixed in +[plan 2026-09-10-2222](../plans/2026-09-10-2222-kvm-review-fixes.md) with +its own execution log. Two of them land on Run 7's conclusions above and +supersede them: + +- The guest-agent **merge described in Run 7 is gone**. It could not be made + safe for user-data that is not a plain cloud-config, and its YAML + round-trip re-typed unquoted scalars. The bootstrap now ships as a + vendor-data shell script, which cloud-init never merges, and the Proxy's + cloud-init reaches the seed byte for byte. +- The read-only-filesystem fix from Run 7 still stands, but the eager SSH + key load it sat next to does not: a missing `kvm-ssh` Secret aborted the + whole provider set and crash-looped the manager. The material is read on + first use now. + +Re-validated the same way: integration test against beelink, both Proxies +recreated in kind, 204 through squid. + ### MR opened [PR #5](https://gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/pulls/5), diff --git a/docs/plans-executions/2026-09-10-2222-kvm-review-fixes.md b/docs/plans-executions/2026-09-10-2222-kvm-review-fixes.md index 30143f5..1973849 100644 --- a/docs/plans-executions/2026-09-10-2222-kvm-review-fixes.md +++ b/docs/plans-executions/2026-09-10-2222-kvm-review-fixes.md @@ -6,7 +6,7 @@ Plan: [docs/plans/2026-09-10-2222-kvm-review-fixes.md](../plans/2026-09-10-2222- - [x] Step 2 — Validate names that become identifiers (finding 2) - [x] Step 3 — Read the SSH material lazily (finding 3) - [x] Step 4 — Guest agent via vendor-data script (findings 4 and 5) -- [ ] Step 5 — Validate, record, push +- [x] Step 5 — Validate, record, push ## Step 0 — plan committed @@ -162,6 +162,61 @@ across seven formats. One consequence worth remembering: a user-data that sets `vendor_data: {enabled: false}` disables the agent, and cloud-init documents that as intended behaviour. +## Step 5 — Validation + +Unit suite, `go vet` and `gofmt` clean across the repo. Then the two real +runs. + +**Integration against beelink**, unchanged environment from the KVM plan's +log, passed in 22 s on a warm cache, with +`virDomainInterfaceAddresses source=DomainInterfaceAddressesSrcAgent +interfaces=2` — the agent installed by the new vendor script answering, on +a stock Debian 13 image. + +**In-cluster**, both Proxies deleted and recreated against the rebuilt +image: Ready with an address 2 min 3 s after `kubectl apply`, 204 through +squid from the dev box for both. Inside the debug VM: + +```sh +ssh novakj@192.168.0.94 'systemctl is-active qemu-guest-agent; cloud-init status; \ + sudo head -4 /var/lib/cloud/instance/user-data.txt; sudo ls /var/lib/cloud/instance/scripts/vendor/' +# active +# status: done +# #cloud-config +# # Login for debugging: user "novakj", password "kvm-dev", passwordless sudo. +# ... +# part-001 +``` + +Two things worth having seen with your own eyes there: the user's **comment +lines survived** into the guest, which the old merge stripped, and the +script really did land in the vendor directory rather than being merged +into the user's config. + +**Negative check for the Secret**, done by pointing the deployment's volume +at a name that does not exist rather than deleting the real Secret: + +```sh +kubectl -n egress-proxies-operator-system patch deploy egress-proxies-operator-controller-manager \ + --type=json -p '[{"op":"replace","path":"/spec/template/spec/volumes/1/secret/secretName","value":"kvm-ssh-absent"}]' +``` + +The manager came up and stayed up, 0 restarts, logging +`WARNING: kvm ssh material is not usable ... open /etc/proxy-operator/kvm/id_ed25519: +no such file or directory`. The Kubernetes Proxies kept reconciling; the +KVM ones failed per reconcile and retried. Before this branch the same +config exited the process before any controller started. Patched back +afterwards, everything Ready. + +Worth noting: the KVM Proxies kept reporting `Ready` during that window +rather than degrading visibly. That is existing behaviour, not a +regression — a transient provider error rides the workqueue and does not +clear a healthy status — but it does mean the only signal of a missing +Secret is the log and the reconcile errors. Also, the ListByTag sweep on +beelink still reports `managed=2` where one domain is an orphan from an +older failed run; unrelated to this work, and the same leftover noted in +the KVM plan's Run 6. + Worth noting: the review filed this as a KVM finding, but the fix is mostly in the controller, because KVM is the only provider that resolves a providerID segment against operator config at all. GCP and Kubernetes hand