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)) } }