From 10ca71331ec30fbbe6ef3fa96342c707d0a9dbc0 Mon Sep 17 00:00:00 2001 From: kacerr Date: Thu, 10 Sep 2026 22:34:28 +0000 Subject: [PATCH] Load the KVM ssh material on first use instead of at startup The kvm-ssh Secret is mounted optional, but kvm.New read the key and known_hosts eagerly and registry.Build aborts the whole provider set on a constructor error, so a missing or misnamed Secret exited the manager before any controller started and took the kubernetes and gcp providers down with it. lazyAuth loads the material on the first connection and caches it once it works, so a Secret created after the manager starts heals the affected proxies on the next reconcile with no restart. A missing file classifies transient for that reason; a key that does not parse is permanent. New still checks both files and logs a WARNING: line when they are unreadable, keeping the startup diagnostic the eager load was there for. Co-Authored-By: Claude Opus 5 (1M context) --- docs/kvm-provider.md | 17 ++-- .../2026-09-10-2222-kvm-review-fixes.md | 35 ++++++- internal/provider/kvm/conn.go | 53 ++++++++++- internal/provider/kvm/conn_test.go | 91 +++++++++++++++++++ internal/provider/kvm/kvm.go | 18 ++-- internal/provider/kvm/kvm_test.go | 14 ++- 6 files changed, 206 insertions(+), 22 deletions(-) diff --git a/docs/kvm-provider.md b/docs/kvm-provider.md index 96b8ef3..8e1f9a9 100644 --- a/docs/kvm-provider.md +++ b/docs/kvm-provider.md @@ -78,9 +78,10 @@ ssh virt@nas1.home.lan ip link show br0 `/etc/proxy-operator/kvm` as two files, `id_ed25519` and `known_hosts`, which is what `sshKeyFile` and `knownHostsFile` in the provider config point at. The deployment starts fine without the - Secret as long as no `kvm` provider is configured. Rotating the key: - re-run the command (or the make target) and restart the manager; it - reads both files once at startup. + Secret, even with a `kvm` provider configured: only that provider's + proxies fail, and they recover once the Secret exists. Rotating the + key: re-run the command (or the make target) and restart the manager; + both files are read once, on the first connection, and cached. 2. Add a provider entry to the providers config (`config/manager/providers_config.yaml` in-cluster; full sample in @@ -104,9 +105,13 @@ ssh virt@nas1.home.lan ip link show br0 bridge: br0 ``` - The operator reads the key and known_hosts at startup and refuses to - start if either is missing — a bad path fails fast, not on the first - Proxy. + The operator checks both files at startup and logs a `WARNING:` line + when either is unreadable, but it does not refuse to start: the + provider set is built all-or-nothing, so exiting here would crash-loop + the manager and take the `kubernetes` and `gcp` providers with it. + The files are read for real on the first connection. A missing file is + a transient failure, so proxies heal by themselves once the Secret + lands; a key that does not parse is permanent and needs a fix. 3. Apply a Proxy (`config/samples/proxy_kvm.yaml`) and watch it: 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 8dfc80d..00a5aa4 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 @@ -4,7 +4,7 @@ Plan: [docs/plans/2026-09-10-2222-kvm-review-fixes.md](../plans/2026-09-10-2222- - [x] Step 0 — Save plan into the repo - [x] Step 1 — Unwedge deletion (finding 1) - [x] Step 2 — Validate names that become identifiers (finding 2) -- [ ] Step 3 — Read the SSH material lazily (finding 3) +- [x] Step 3 — Read the SSH material lazily (finding 3) - [ ] Step 4 — Guest agent via vendor-data script (findings 4 and 5) - [ ] Step 5 — Validate, record, push @@ -65,6 +65,39 @@ boundary. It is still cheaper to reject at load than to explain a stuck Proxy later. Both new table cases assert the message text, so the wording that names the consequence is now pinned by tests. +## Step 3 — Read the SSH material lazily + +`kvm.New` no longer loads the key; `lazyAuth` does, on the first connection, +caching the result once it works. `registry.Build` is all-or-nothing by +design, so the old constructor error did not merely fail the KVM provider, +it exited the manager before any controller started. + +The plan said "lazy dialer", and the first attempt was exactly that — one +closure wrapping both the loading and the dial. It could not be tested: the +fixture's SSH server is real, but the socket behind it is an echo server, so +every successful dial dies in the libvirt handshake and the assertion could +not tell "the key worked" from "the key did not". Splitting the seam in two, +`lazyAuth` for the material and `lazyDialer` for the wiring, made both +testable: the healing path against `lazyAuth`, and a separate test that the +dialer really authenticates, asserted by the error being past SSH and by the +server having seen the forward request. + +Error classes carry the intent. A missing file stays unclassified, so +`provider.Class` defaults it to transient and a Proxy heals on its own when +the Secret appears — the common case, since the Secret is usually created +after the manager. A key that does not parse wraps `ErrPermanent`, because +retrying cannot fix it. `newSSHAuth` distinguishes the two with +`errors.Is(err, fs.ErrNotExist)`. + +Worth noting: the startup check is kept, just non-fatally, logged with the +`WARNING:` prefix the repo already uses where logr has no warn level. That +preserves the diagnostic the eager version was written for. The caching test +deliberately deletes the key file after a successful load and asserts the +cached value is reused, which pins the rotation behaviour the docs promise: +a rotated Secret needs a manager restart. `TestNew_badKeyPathFailsFast` was +inverted rather than deleted, so the old contract stays visible in the diff +next to the new one. + 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 diff --git a/internal/provider/kvm/conn.go b/internal/provider/kvm/conn.go index cf90e53..8cb419f 100644 --- a/internal/provider/kvm/conn.go +++ b/internal/provider/kvm/conn.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "io" + "io/fs" "net" "os" "sync" @@ -180,14 +181,17 @@ func awaitOrCancel(ctx context.Context, fn func() error, abort func()) error { } // sshAuth holds the pieces of the SSH client config shared by every host: -// the private key and the host-key policy, both loaded once at startup so a -// missing or unreadable file fails the operator fast rather than on the -// first Create. +// the private key and the host-key policy. Both are loaded on the first +// connection rather than at startup — see lazyDialer. type sshAuth struct { signer ssh.Signer hostKey ssh.HostKeyCallback } +// newSSHAuth loads the key material. A file that is not there yet stays +// unclassified, so the provider treats it as transient and heals when the +// Secret appears; content that will never parse is permanent, since +// retrying cannot fix it. func newSSHAuth(cfg provider.KVMConfig) (*sshAuth, error) { key, err := os.ReadFile(cfg.SSHKeyFile) if err != nil { @@ -195,18 +199,57 @@ func newSSHAuth(cfg provider.KVMConfig) (*sshAuth, error) { } signer, err := ssh.ParsePrivateKey(key) if err != nil { - return nil, fmt.Errorf("parsing kvm.sshKeyFile: %w", err) + return nil, fmt.Errorf("%w: parsing kvm.sshKeyFile: %v", provider.ErrPermanent, err) } hostKey := ssh.InsecureIgnoreHostKey() //nolint:gosec // opt-in via config, documented as insecure if !cfg.InsecureIgnoreHostKey { hostKey, err = knownhosts.New(cfg.KnownHostsFile) - if err != nil { + if err != nil && errors.Is(err, fs.ErrNotExist) { return nil, fmt.Errorf("loading kvm.knownHostsFile: %w", err) } + if err != nil { + return nil, fmt.Errorf("%w: loading kvm.knownHostsFile: %v", provider.ErrPermanent, err) + } } return &sshAuth{signer: signer, hostKey: hostKey}, nil } +// lazyAuth defers loading the SSH material to the first connection and +// caches it once it works. The kvm-ssh Secret is an optional mount: the +// manager has to start without it and keep every other provider running, +// and a Secret created afterwards has to heal the KVM proxies on the next +// reconcile rather than needing a restart. +func lazyAuth(cfg provider.KVMConfig) func() (*sshAuth, error) { + var ( + mu sync.Mutex + cached *sshAuth + ) + return func() (*sshAuth, error) { + mu.Lock() + defer mu.Unlock() + if cached != nil { + return cached, nil + } + auth, err := newSSHAuth(cfg) + if err != nil { + return nil, err + } + cached = auth + return auth, nil + } +} + +func lazyDialer(cfg provider.KVMConfig) hostDialer { + auth := lazyAuth(cfg) + return func(ctx context.Context, host provider.KVMHost) (*hostClient, error) { + a, err := auth() + if err != nil { + return nil, err + } + return a.dialer()(ctx, host) + } +} + // dialer returns the production hostDialer: SSH to the host, forward its // libvirt unix socket through the connection, and open a libvirt session // over that. diff --git a/internal/provider/kvm/conn_test.go b/internal/provider/kvm/conn_test.go index 2896546..5893d9f 100644 --- a/internal/provider/kvm/conn_test.go +++ b/internal/provider/kvm/conn_test.go @@ -525,6 +525,97 @@ func TestNewSSHAuth_missingFiles(t *testing.T) { } } +// The kvm-ssh Secret is an optional mount, so the provider has to be +// constructible without it and has to start working once it appears — the +// alternative was a constructor error that crash-looped the whole manager. +func TestLazyAuth_healsWhenTheKeyAppears(t *testing.T) { + t.Parallel() + f := newSSHFixture(t, true) + keyPEM, err := os.ReadFile(f.keyFile) + if err != nil { + t.Fatal(err) + } + if err := os.Remove(f.keyFile); err != nil { + t.Fatal(err) + } + auth := lazyAuth(f.cfg) + + if _, err := auth(); err == nil { + t.Fatal("loading with no key file succeeded, want an error") + } else if class := classify(err); class != provider.ErrTransient { + t.Errorf("class = %v, want ErrTransient so the proxy retries once the Secret lands", class) + } + if err := os.WriteFile(f.keyFile, keyPEM, 0o600); err != nil { + t.Fatal(err) + } + loaded, err := auth() + if err != nil { + t.Fatalf("loading after the key appeared: %v", err) + } + // Cached from here on: the file may vanish again (a Secret rotated out) + // without breaking connections that already work. + if err := os.Remove(f.keyFile); err != nil { + t.Fatal(err) + } + if again, err := auth(); err != nil || again != loaded { + t.Errorf("auth() = %v, %v; want the cached value reused", again, err) + } +} + +func TestLazyAuth_unparseableKeyIsPermanent(t *testing.T) { + t.Parallel() + f := newSSHFixture(t, true) + if err := os.WriteFile(f.keyFile, []byte("not a key"), 0o600); err != nil { + t.Fatal(err) + } + _, err := lazyAuth(f.cfg)() + if classify(err) != provider.ErrPermanent { + t.Errorf("class = %v, want ErrPermanent: retrying cannot fix a key that does not parse", classify(err)) + } +} + +// The dialer must authenticate with the material the lazy loader produced, +// not a stale or empty one. The echo socket is not a libvirt server, so the +// call gets as far as the libvirt handshake and no further; what matters is +// that it is past SSH by then. +func TestLazyDialer_authenticatesOverSSH(t *testing.T) { + t.Parallel() + f := newSSHFixture(t, true) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + _, err := lazyDialer(f.cfg)(ctx, f.host) + if err == nil { + t.Fatal("dial to an echo socket succeeded, want a libvirt handshake error") + } + if strings.Contains(err.Error(), "sshKeyFile") || strings.Contains(err.Error(), "ssh handshake") { + t.Errorf("dial failed before libvirt: %v", err) + } + if paths := f.server.requestedPaths(); len(paths) != 1 || paths[0] != f.host.Socket { + t.Errorf("server saw forward requests %v, want [%s]", paths, f.host.Socket) + } +} + +// New must not fail on missing key material, or one misconfigured provider +// aborts registry.Build and the manager exits before any controller starts. +func TestNew_missingSSHMaterialStillBuilds(t *testing.T) { + t.Parallel() + p, err := New(context.Background(), provider.ProviderConfig{ + Name: "kvm-home", Type: "kvm", + KVM: &provider.KVMConfig{ + SSHKeyFile: filepath.Join(t.TempDir(), "absent"), + KnownHostsFile: filepath.Join(t.TempDir(), "absent"), + Hosts: []provider.KVMHost{{Name: "nas1", Address: "nas1:22", User: "virt", Bridge: "br0"}}, + }, + }) + if err != nil || p == nil { + t.Fatalf("New() = %v, %v; want a usable provider", p, err) + } + if _, err := p.Get(context.Background(), "hosts/nas1/domains/proxy-x"); provider.Class(err) != provider.ErrTransient { + t.Errorf("Get class = %v, want ErrTransient", provider.Class(err)) + } +} + func TestKeepalive_closesClientWhenServerStalls(t *testing.T) { t.Parallel() f := newSSHFixture(t, false) diff --git a/internal/provider/kvm/kvm.go b/internal/provider/kvm/kvm.go index a7fc2c4..e0d51b4 100644 --- a/internal/provider/kvm/kvm.go +++ b/internal/provider/kvm/kvm.go @@ -77,18 +77,22 @@ type Provider struct { } // New builds a Provider that reaches its hosts over SSH. The key and -// known_hosts files are read here so a bad path fails at startup rather -// than on the first Create. Satisfies registry.Constructor. -func New(_ context.Context, pc provider.ProviderConfig) (provider.Provider, error) { +// known_hosts files are read on the first connection, not here: they come +// from an optional Secret mount, and a constructor that failed without it +// would abort the whole provider set and crash-loop the manager, taking +// the kubernetes and gcp providers down with it. Unreadable files are +// still reported at startup, as a warning. Satisfies registry.Constructor. +func New(ctx context.Context, pc provider.ProviderConfig) (provider.Provider, error) { cfg := provider.KVMConfig{} if pc.KVM != nil { cfg = *pc.KVM } - auth, err := newSSHAuth(cfg) - if err != nil { - return nil, fmt.Errorf("kvm provider %q: %w", pc.Name, err) + if _, err := newSSHAuth(cfg); err != nil { + logf.FromContext(ctx).WithName("kvm").WithValues("provider", pc.Name).Info( + "WARNING: kvm ssh material is not usable; proxies on this provider will fail until it is", + "error", err.Error()) } - return newWithDialer(pc, auth.dialer(), nil), nil + return newWithDialer(pc, lazyDialer(cfg), nil), nil } // newWithDialer is the test seam: the dialer decides what libvirtAPI each diff --git a/internal/provider/kvm/kvm_test.go b/internal/provider/kvm/kvm_test.go index 4993126..f8c6cf3 100644 --- a/internal/provider/kvm/kvm_test.go +++ b/internal/provider/kvm/kvm_test.go @@ -906,12 +906,20 @@ func TestMapState(t *testing.T) { } } -func TestNew_badKeyPathFailsFast(t *testing.T) { +// A bad key path no longer fails the constructor: registry.Build aborts the +// whole provider set on a constructor error, so that took the manager down +// with every other provider. It degrades per-proxy instead — see +// TestNew_missingSSHMaterialStillBuilds and TestLazyAuth_healsWhenTheKeyAppears. +func TestNew_badKeyPathDefersToFirstUse(t *testing.T) { t.Parallel() cfg := testConfig("nas1") cfg.KVM.SSHKeyFile = t.TempDir() + "/missing" - if _, err := New(context.Background(), cfg); err == nil { - t.Fatal("New succeeded with a missing key file") + p, err := New(context.Background(), cfg) + if err != nil { + t.Fatalf("New() error = %v, want a provider that fails only when used", err) + } + if err := p.Delete(context.Background(), "hosts/nas1/domains/proxy-x"); provider.Class(err) != provider.ErrTransient { + t.Errorf("Delete class = %v, want ErrTransient", provider.Class(err)) } }