kacerr/egress-proxies-operator
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Simplify the KVM provider after the /simplify review

Browse Source 
Collapse duplicated logic the review found in PR #5 without changing
behaviour: one releaseFinalizer helper for the four finalizer exits in
reconcileDelete, classify reusing isClassified and provider.Class, the
image URL check and format/firmware values shared from the provider
package, imageSource and the metadata parser embedding their config
structs instead of copying fields, and the SSH loader as a small struct
that keeps only the SSH fields and is seeded by New instead of loading
the key twice. Tracing builds its V(2)-only lists only at V(2).

Tests: drop a duplicate New test, fold near-identical tests into tables
(connection drops, metadata parsing, cached images), add mustSSHAuth,
and use wantClass where classes were checked by hand.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
kacerr
2026-09-11 06:24:43 +00:00
co-authored by Claude Opus 5
parent 2b4b61cdc4
commit 82eeeef9da
13 changed files with 303 additions and 376 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/provider/kvm/conn_test.go
+78 -124
View File
@@ -110,41 +110,48 @@ func TestHostPool_call_redialsAfterDisconnect(t *testing.T) {
}
}
func TestHostPool_call_transportErrorDropsConnection(t *testing.T) {
// Only a transport error is evidence about the link. A daemon-reported
// libvirt error, or a verdict fn reached locally, must not cost the
// connection.
func TestHostPool_call_dropsConnectionOnlyOnTransportError(t *testing.T) {
t.Parallel()
d := &fakeDialer{}
p := newHostPool(d.dial)
err := p.call(context.Background(), discard, testHost, func(libvirtAPI) error { return io.ErrUnexpectedEOF })
if !errors.Is(err, io.ErrUnexpectedEOF) {
t.Fatalf("call() error = %v, want the transport error passed through", err)
tests := []struct {
name string
err error
wantDropped bool
}{
{"transport error", io.ErrUnexpectedEOF, true},
{"libvirt error", lvErr(libvirt.ErrNoDomain), false},
{"locally classified error", fmt.Errorf("%w: creating iso writer: read-only file system", provider.ErrPermanent), false},
}
if !d.clients[0].closed.Load() {
t.Error("connection not closed after a transport error")
}
if err := p.call(context.Background(), discard, testHost, noop); err != nil {
t.Fatalf("call() after transport error = %v", err)
}
if d.dialCount() != 2 {
t.Errorf("dials = %d, want 2", d.dialCount())
}
}
func TestHostPool_call_libvirtErrorKeepsConnection(t *testing.T) {
t.Parallel()
d := &fakeDialer{}
p := newHostPool(d.dial)
err := p.call(context.Background(), discard, testHost, func(libvirtAPI) error { return lvErr(libvirt.ErrNoDomain) })
if !hasCode(err, libvirt.ErrNoDomain) {
t.Fatalf("call() error = %v, want the libvirt error passed through", err)
}
if err := p.call(context.Background(), discard, testHost, noop); err != nil {
t.Fatalf("call() error = %v", err)
}
if d.dialCount() != 1 {
t.Errorf("dials = %d, want 1: a daemon-reported error must not cost the connection", d.dialCount())
}
if d.clients[0].closed.Load() {
t.Error("connection closed after a libvirt error")
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
d := &fakeDialer{}
p := newHostPool(d.dial)
log, snapshot := captureLogger(1)
err := p.call(context.Background(), log, testHost, func(libvirtAPI) error { return tc.err })
if !errors.Is(err, tc.err) {
t.Fatalf("call() error = %v, want %v passed through", err, tc.err)
}
if got := d.clients[0].closed.Load(); got != tc.wantDropped {
t.Errorf("connection closed = %v, want %v", got, tc.wantDropped)
}
joined := strings.Join(snapshot(), "\n")
if logged := strings.Contains(joined, `"msg"="libvirt connection dropped"`) && strings.Contains(joined, `"reason"="transport error"`); logged != tc.wantDropped {
t.Errorf("transport drop logged = %v, want %v:\n%s", logged, tc.wantDropped, joined)
}
if err := p.call(context.Background(), discard, testHost, noop); err != nil {
t.Fatalf("next call() error = %v", err)
}
wantDials := 1
if tc.wantDropped {
wantDials = 2
}
if d.dialCount() != wantDials {
t.Errorf("dials = %d, want %d", d.dialCount(), wantDials)
}
})
}
}
@@ -388,13 +395,19 @@ func newSSHFixture(t *testing.T, answerGlobalRequests bool) *sshFixture {
}
}
func TestSSHAuth_dialSSH_forwardsUnixSocket(t *testing.T) {
t.Parallel()
f := newSSHFixture(t, true)
auth, err := newSSHAuth(f.cfg)
func mustSSHAuth(t *testing.T, cfg provider.KVMConfig) *sshAuth {
t.Helper()
auth, err := newSSHAuth(cfg)
if err != nil {
t.Fatalf("newSSHAuth() error = %v", err)
}
return auth
}
func TestSSHAuth_dialSSH_forwardsUnixSocket(t *testing.T) {
t.Parallel()
f := newSSHFixture(t, true)
auth := mustSSHAuth(t, f.cfg)
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
client, err := auth.dialSSH(ctx, f.host)
@@ -428,10 +441,7 @@ func TestSSHAuth_dialSSH_rejectsUnknownHostKey(t *testing.T) {
if err := os.WriteFile(f.cfg.KnownHostsFile, []byte(line+"\n"), 0o600); err != nil {
t.Fatal(err)
}
auth, err := newSSHAuth(f.cfg)
if err != nil {
t.Fatalf("newSSHAuth() error = %v", err)
}
auth := mustSSHAuth(t, f.cfg)
client, err := auth.dialSSH(context.Background(), f.host)
if err == nil {
client.Close()
@@ -448,11 +458,7 @@ func TestSSHAuth_dialSSH_insecureIgnoreHostKey(t *testing.T) {
f := newSSHFixture(t, true)
f.cfg.KnownHostsFile = ""
f.cfg.InsecureIgnoreHostKey = true
auth, err := newSSHAuth(f.cfg)
if err != nil {
t.Fatalf("newSSHAuth() error = %v", err)
}
client, err := auth.dialSSH(context.Background(), f.host)
client, err := mustSSHAuth(t, f.cfg).dialSSH(context.Background(), f.host)
if err != nil {
t.Fatalf("dialSSH() error = %v", err)
}
@@ -466,11 +472,7 @@ func TestSSHAuth_dialSSH_rejectsWrongClientKey(t *testing.T) {
if err := os.WriteFile(f.keyFile, otherPEM, 0o600); err != nil {
t.Fatal(err)
}
auth, err := newSSHAuth(f.cfg)
if err != nil {
t.Fatalf("newSSHAuth() error = %v", err)
}
client, err := auth.dialSSH(context.Background(), f.host)
client, err := mustSSHAuth(t, f.cfg).dialSSH(context.Background(), f.host)
if err == nil {
client.Close()
t.Fatal("dialSSH() succeeded with a key the server does not know")
@@ -483,13 +485,9 @@ func TestSSHAuth_dialSSH_rejectsWrongClientKey(t *testing.T) {
func TestSSHAuth_dialSSH_cancelledContext(t *testing.T) {
t.Parallel()
f := newSSHFixture(t, true)
auth, err := newSSHAuth(f.cfg)
if err != nil {
t.Fatalf("newSSHAuth() error = %v", err)
}
ctx, cancel := context.WithCancel(context.Background())
cancel()
if client, err := auth.dialSSH(ctx, f.host); err == nil {
if client, err := mustSSHAuth(t, f.cfg).dialSSH(ctx, f.host); err == nil {
client.Close()
t.Fatal("dialSSH() succeeded with an already-cancelled context")
}
@@ -528,7 +526,7 @@ 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) {
func TestLazySSH_load_healsWhenTheKeyAppears(t *testing.T) {
t.Parallel()
f := newSSHFixture(t, true)
keyPEM, err := os.ReadFile(f.keyFile)
@@ -538,7 +536,7 @@ func TestLazyAuth_healsWhenTheKeyAppears(t *testing.T) {
if err := os.Remove(f.keyFile); err != nil {
t.Fatal(err)
}
auth := lazyAuth(f.cfg)
auth := newLazySSH(f.cfg).load
if _, err := auth(); err == nil {
t.Fatal("loading with no key file succeeded, want an error")
@@ -562,13 +560,13 @@ func TestLazyAuth_healsWhenTheKeyAppears(t *testing.T) {
}
}
func TestLazyAuth_unparseableKeyIsPermanent(t *testing.T) {
func TestLazySSH_load_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)()
_, err := newLazySSH(f.cfg).load()
if classify(err) != provider.ErrPermanent {
t.Errorf("class = %v, want ErrPermanent: retrying cannot fix a key that does not parse", classify(err))
}
@@ -578,13 +576,13 @@ func TestLazyAuth_unparseableKeyIsPermanent(t *testing.T) {
// 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) {
func TestLazySSH_dial_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)
_, err := newLazySSH(f.cfg).dial(ctx, f.host)
if err == nil {
t.Fatal("dial to an echo socket succeeded, want a libvirt handshake error")
}
@@ -619,11 +617,7 @@ func TestNew_missingSSHMaterialStillBuilds(t *testing.T) {
func TestKeepalive_closesClientWhenServerStalls(t *testing.T) {
t.Parallel()
f := newSSHFixture(t, false)
auth, err := newSSHAuth(f.cfg)
if err != nil {
t.Fatalf("newSSHAuth() error = %v", err)
}
client, err := auth.dialSSH(context.Background(), f.host)
client, err := mustSSHAuth(t, f.cfg).dialSSH(context.Background(), f.host)
if err != nil {
t.Fatalf("dialSSH() error = %v", err)
}
@@ -644,11 +638,7 @@ func TestKeepalive_closesClientWhenServerStalls(t *testing.T) {
func TestKeepalive_keepsHealthyClientOpen(t *testing.T) {
t.Parallel()
f := newSSHFixture(t, true)
auth, err := newSSHAuth(f.cfg)
if err != nil {
t.Fatalf("newSSHAuth() error = %v", err)
}
client, err := auth.dialSSH(context.Background(), f.host)
client, err := mustSSHAuth(t, f.cfg).dialSSH(context.Background(), f.host)
if err != nil {
t.Fatalf("dialSSH() error = %v", err)
}
@@ -705,64 +695,28 @@ func TestHostPool_call_noTracingBelowV1(t *testing.T) {
}
}
func TestHostPool_call_logsDropReason(t *testing.T) {
t.Parallel()
t.Run("transport error", func(t *testing.T) {
t.Parallel()
d := &fakeDialer{}
p := newHostPool(d.dial)
log, snapshot := captureLogger(1)
_ = p.call(context.Background(), log, testHost, func(libvirtAPI) error { return io.ErrUnexpectedEOF })
joined := strings.Join(snapshot(), "\n")
if !strings.Contains(joined, `"msg"="libvirt connection dropped"`) || !strings.Contains(joined, `"reason"="transport error"`) {
t.Errorf("output missing the drop line:\n%s", joined)
}
})
t.Run("context cancelled", func(t *testing.T) {
t.Parallel()
d := &fakeDialer{}
p := newHostPool(d.dial)
log, snapshot := captureLogger(1)
ctx, cancel := context.WithCancel(context.Background())
cancel()
released := make(chan struct{})
err := p.call(ctx, log, testHost, func(libvirtAPI) error {
<-d.clients[0].done
close(released)
return io.EOF
})
if !errors.Is(err, context.Canceled) {
t.Fatalf("call() error = %v, want context.Canceled", err)
}
<-released
joined := strings.Join(snapshot(), "\n")
if !strings.Contains(joined, `"reason"="context cancelled"`) {
t.Errorf("output missing the cancel drop line:\n%s", joined)
}
if strings.Contains(joined, `"reason"="transport error"`) {
t.Errorf("cancel was also reported as a transport error:\n%s", joined)
}
})
}
func TestHostPool_call_keepsConnectionOnClassifiedError(t *testing.T) {
func TestHostPool_call_logsCancelDropReason(t *testing.T) {
t.Parallel()
d := &fakeDialer{}
p := newHostPool(d.dial)
log, snapshot := captureLogger(1)
err := p.call(context.Background(), log, testHost, func(libvirtAPI) error {
return fmt.Errorf("%w: creating iso writer: read-only file system", provider.ErrPermanent)
ctx, cancel := context.WithCancel(context.Background())
cancel()
released := make(chan struct{})
err := p.call(ctx, log, testHost, func(libvirtAPI) error {
<-d.clients[0].done
close(released)
return io.EOF
})
if !errors.Is(err, provider.ErrPermanent) {
t.Fatalf("call() error = %v, want the classified error passed through", err)
if !errors.Is(err, context.Canceled) {
t.Fatalf("call() error = %v, want context.Canceled", err)
}
if d.clients[0].closed.Load() {
t.Error("connection closed after a locally classified error; only transport errors should drop it")
<-released
joined := strings.Join(snapshot(), "\n")
if !strings.Contains(joined, `"reason"="context cancelled"`) {
t.Errorf("output missing the cancel drop line:\n%s", joined)
}
if joined := strings.Join(snapshot(), "\n"); strings.Contains(joined, "connection dropped") {
t.Errorf("drop logged for a local error:\n%s", joined)
}
if err := p.call(context.Background(), log, testHost, noop); err != nil || d.dialCount() != 1 {
t.Errorf("next call redialled (dials=%d, err=%v), want the cached connection reused", d.dialCount(), err)
if strings.Contains(joined, `"reason"="transport error"`) {
t.Errorf("cancel was also reported as a transport error:\n%s", joined)
}
}