From 0ed32415dcffff9a8fab93907668c639da684ebc Mon Sep 17 00:00:00 2001 From: Shreyansh Sancheti Date: Mon, 27 Apr 2026 14:54:45 +0530 Subject: [PATCH 1/2] controller/network: add unit tests for network controller Adds gomock mocks (in mocks/) and unit tests for the narrow vmNetworkManager, capabilitiesProvider, and platform-specific guestNetwork interfaces. Mocks are committed without a //go:generate directive to match existing repo convention; standardised mock generation and CI drift validation will be addressed in a separate PR. Tests cover the production failure paths: Shared (network_test.go): - New(): namespace-add capability is queried once and cached; nil capabilities do not panic the constructor. - Setup(): rejects calls in any non-NotConfigured state without mutating state; an empty namespace ID transitions the controller to StateInvalid via the deferred error handler. - Teardown(): no-ops from StateNotConfigured and StateTornDown so containerd's StopPodSandbox retries are safe. LCOW (network_lcow_test.go): - addEndpointToGuestNamespace: host-then-guest ordering, host-only when caps say no, host-failure leaves NIC untracked, guest-failure-after-host keeps the NIC tracked so Teardown can unwind. - removeEndpointFromGuestNamespace: guest-then-host ordering; guest failure short-circuits host removal; host-only path when guest lacks namespace support. - Teardown: full happy path, partial-failure cleanup chain (failed NIC stays tracked, others are removed, state moves to Invalid), host-side RemoveNIC failure path. - Half-setup recovery: host AddNIC succeeds + guest Add fails + Teardown unwinds the host-side device end-to-end; first Teardown fails on guest removal -> StateInvalid -> retry from StateInvalid drains the endpoint and reaches StateTornDown. WCOW (network_wcow_test.go): - addEndpointToGuestNamespace: full PreAdd -> AddNIC -> Add sequence, pre-add failure short-circuits, host failure leaves NIC untracked, final-add failure leaves NIC tracked. - removeEndpointFromGuestNamespace: guest-then-host ordering; guest failure short-circuits host removal. - Teardown: happy path (no-namespace variant) and partial-failure cleanup chain. - Half-setup recovery: finalize-Add failure + Teardown unwinds host; retry-from-StateInvalid succeeds. Signed-off-by: Shreyansh Sancheti --- .../network/mocks/mock_lcow_network.go | 164 +++++++ .../network/mocks/mock_wcow_network.go | 193 ++++++++ .../controller/network/network_lcow_test.go | 435 ++++++++++++++++++ internal/controller/network/network_test.go | 195 ++++++++ .../controller/network/network_wcow_test.go | 410 +++++++++++++++++ 5 files changed, 1397 insertions(+) create mode 100644 internal/controller/network/mocks/mock_lcow_network.go create mode 100644 internal/controller/network/mocks/mock_wcow_network.go create mode 100644 internal/controller/network/network_lcow_test.go create mode 100644 internal/controller/network/network_test.go create mode 100644 internal/controller/network/network_wcow_test.go diff --git a/internal/controller/network/mocks/mock_lcow_network.go b/internal/controller/network/mocks/mock_lcow_network.go new file mode 100644 index 0000000000..34c2c9e559 --- /dev/null +++ b/internal/controller/network/mocks/mock_lcow_network.go @@ -0,0 +1,164 @@ +//go:build windows && lcow + +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/Microsoft/hcsshim/internal/controller/network (interfaces: vmNetworkManager,capabilitiesProvider,guestNetwork) +// +// Generated by this command: +// +// mockgen -build_flags=-tags=windows,lcow -build_constraint=windows&&lcow -package mocks -destination mocks/mock_lcow_network.go github.com/Microsoft/hcsshim/internal/controller/network vmNetworkManager,capabilitiesProvider,guestNetwork +// + +// Package mocks is a generated GoMock package. +package mocks + +import ( + context "context" + reflect "reflect" + + gcs "github.com/Microsoft/hcsshim/internal/gcs" + hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2" + guestresource "github.com/Microsoft/hcsshim/internal/protocol/guestresource" + gomock "go.uber.org/mock/gomock" +) + +// MockvmNetworkManager is a mock of vmNetworkManager interface. +type MockvmNetworkManager struct { + ctrl *gomock.Controller + recorder *MockvmNetworkManagerMockRecorder + isgomock struct{} +} + +// MockvmNetworkManagerMockRecorder is the mock recorder for MockvmNetworkManager. +type MockvmNetworkManagerMockRecorder struct { + mock *MockvmNetworkManager +} + +// NewMockvmNetworkManager creates a new mock instance. +func NewMockvmNetworkManager(ctrl *gomock.Controller) *MockvmNetworkManager { + mock := &MockvmNetworkManager{ctrl: ctrl} + mock.recorder = &MockvmNetworkManagerMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockvmNetworkManager) EXPECT() *MockvmNetworkManagerMockRecorder { + return m.recorder +} + +// AddNIC mocks base method. +func (m *MockvmNetworkManager) AddNIC(ctx context.Context, nicID string, settings *hcsschema.NetworkAdapter) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "AddNIC", ctx, nicID, settings) + ret0, _ := ret[0].(error) + return ret0 +} + +// AddNIC indicates an expected call of AddNIC. +func (mr *MockvmNetworkManagerMockRecorder) AddNIC(ctx, nicID, settings any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddNIC", reflect.TypeOf((*MockvmNetworkManager)(nil).AddNIC), ctx, nicID, settings) +} + +// RemoveNIC mocks base method. +func (m *MockvmNetworkManager) RemoveNIC(ctx context.Context, nicID string, settings *hcsschema.NetworkAdapter) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RemoveNIC", ctx, nicID, settings) + ret0, _ := ret[0].(error) + return ret0 +} + +// RemoveNIC indicates an expected call of RemoveNIC. +func (mr *MockvmNetworkManagerMockRecorder) RemoveNIC(ctx, nicID, settings any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RemoveNIC", reflect.TypeOf((*MockvmNetworkManager)(nil).RemoveNIC), ctx, nicID, settings) +} + +// MockcapabilitiesProvider is a mock of capabilitiesProvider interface. +type MockcapabilitiesProvider struct { + ctrl *gomock.Controller + recorder *MockcapabilitiesProviderMockRecorder + isgomock struct{} +} + +// MockcapabilitiesProviderMockRecorder is the mock recorder for MockcapabilitiesProvider. +type MockcapabilitiesProviderMockRecorder struct { + mock *MockcapabilitiesProvider +} + +// NewMockcapabilitiesProvider creates a new mock instance. +func NewMockcapabilitiesProvider(ctrl *gomock.Controller) *MockcapabilitiesProvider { + mock := &MockcapabilitiesProvider{ctrl: ctrl} + mock.recorder = &MockcapabilitiesProviderMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockcapabilitiesProvider) EXPECT() *MockcapabilitiesProviderMockRecorder { + return m.recorder +} + +// Capabilities mocks base method. +func (m *MockcapabilitiesProvider) Capabilities() gcs.GuestDefinedCapabilities { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Capabilities") + ret0, _ := ret[0].(gcs.GuestDefinedCapabilities) + return ret0 +} + +// Capabilities indicates an expected call of Capabilities. +func (mr *MockcapabilitiesProviderMockRecorder) Capabilities() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Capabilities", reflect.TypeOf((*MockcapabilitiesProvider)(nil).Capabilities)) +} + +// MockguestNetwork is a mock of guestNetwork interface. +type MockguestNetwork struct { + ctrl *gomock.Controller + recorder *MockguestNetworkMockRecorder + isgomock struct{} +} + +// MockguestNetworkMockRecorder is the mock recorder for MockguestNetwork. +type MockguestNetworkMockRecorder struct { + mock *MockguestNetwork +} + +// NewMockguestNetwork creates a new mock instance. +func NewMockguestNetwork(ctrl *gomock.Controller) *MockguestNetwork { + mock := &MockguestNetwork{ctrl: ctrl} + mock.recorder = &MockguestNetworkMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockguestNetwork) EXPECT() *MockguestNetworkMockRecorder { + return m.recorder +} + +// AddNetworkInterface mocks base method. +func (m *MockguestNetwork) AddNetworkInterface(ctx context.Context, settings *guestresource.LCOWNetworkAdapter) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "AddNetworkInterface", ctx, settings) + ret0, _ := ret[0].(error) + return ret0 +} + +// AddNetworkInterface indicates an expected call of AddNetworkInterface. +func (mr *MockguestNetworkMockRecorder) AddNetworkInterface(ctx, settings any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddNetworkInterface", reflect.TypeOf((*MockguestNetwork)(nil).AddNetworkInterface), ctx, settings) +} + +// RemoveNetworkInterface mocks base method. +func (m *MockguestNetwork) RemoveNetworkInterface(ctx context.Context, settings *guestresource.LCOWNetworkAdapter) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RemoveNetworkInterface", ctx, settings) + ret0, _ := ret[0].(error) + return ret0 +} + +// RemoveNetworkInterface indicates an expected call of RemoveNetworkInterface. +func (mr *MockguestNetworkMockRecorder) RemoveNetworkInterface(ctx, settings any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RemoveNetworkInterface", reflect.TypeOf((*MockguestNetwork)(nil).RemoveNetworkInterface), ctx, settings) +} diff --git a/internal/controller/network/mocks/mock_wcow_network.go b/internal/controller/network/mocks/mock_wcow_network.go new file mode 100644 index 0000000000..8feadbb70f --- /dev/null +++ b/internal/controller/network/mocks/mock_wcow_network.go @@ -0,0 +1,193 @@ +//go:build windows && wcow + +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/Microsoft/hcsshim/internal/controller/network (interfaces: vmNetworkManager,capabilitiesProvider,guestNetwork) +// +// Generated by this command: +// +// mockgen -build_flags=-tags=windows,wcow -build_constraint=windows&&wcow -package mocks -destination mocks/mock_wcow_network.go github.com/Microsoft/hcsshim/internal/controller/network vmNetworkManager,capabilitiesProvider,guestNetwork +// + +// Package mocks is a generated GoMock package. +package mocks + +import ( + context "context" + reflect "reflect" + + hcn "github.com/Microsoft/hcsshim/hcn" + gcs "github.com/Microsoft/hcsshim/internal/gcs" + hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2" + guestrequest "github.com/Microsoft/hcsshim/internal/protocol/guestrequest" + gomock "go.uber.org/mock/gomock" +) + +// MockvmNetworkManager is a mock of vmNetworkManager interface. +type MockvmNetworkManager struct { + ctrl *gomock.Controller + recorder *MockvmNetworkManagerMockRecorder + isgomock struct{} +} + +// MockvmNetworkManagerMockRecorder is the mock recorder for MockvmNetworkManager. +type MockvmNetworkManagerMockRecorder struct { + mock *MockvmNetworkManager +} + +// NewMockvmNetworkManager creates a new mock instance. +func NewMockvmNetworkManager(ctrl *gomock.Controller) *MockvmNetworkManager { + mock := &MockvmNetworkManager{ctrl: ctrl} + mock.recorder = &MockvmNetworkManagerMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockvmNetworkManager) EXPECT() *MockvmNetworkManagerMockRecorder { + return m.recorder +} + +// AddNIC mocks base method. +func (m *MockvmNetworkManager) AddNIC(ctx context.Context, nicID string, settings *hcsschema.NetworkAdapter) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "AddNIC", ctx, nicID, settings) + ret0, _ := ret[0].(error) + return ret0 +} + +// AddNIC indicates an expected call of AddNIC. +func (mr *MockvmNetworkManagerMockRecorder) AddNIC(ctx, nicID, settings any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddNIC", reflect.TypeOf((*MockvmNetworkManager)(nil).AddNIC), ctx, nicID, settings) +} + +// RemoveNIC mocks base method. +func (m *MockvmNetworkManager) RemoveNIC(ctx context.Context, nicID string, settings *hcsschema.NetworkAdapter) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RemoveNIC", ctx, nicID, settings) + ret0, _ := ret[0].(error) + return ret0 +} + +// RemoveNIC indicates an expected call of RemoveNIC. +func (mr *MockvmNetworkManagerMockRecorder) RemoveNIC(ctx, nicID, settings any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RemoveNIC", reflect.TypeOf((*MockvmNetworkManager)(nil).RemoveNIC), ctx, nicID, settings) +} + +// MockcapabilitiesProvider is a mock of capabilitiesProvider interface. +type MockcapabilitiesProvider struct { + ctrl *gomock.Controller + recorder *MockcapabilitiesProviderMockRecorder + isgomock struct{} +} + +// MockcapabilitiesProviderMockRecorder is the mock recorder for MockcapabilitiesProvider. +type MockcapabilitiesProviderMockRecorder struct { + mock *MockcapabilitiesProvider +} + +// NewMockcapabilitiesProvider creates a new mock instance. +func NewMockcapabilitiesProvider(ctrl *gomock.Controller) *MockcapabilitiesProvider { + mock := &MockcapabilitiesProvider{ctrl: ctrl} + mock.recorder = &MockcapabilitiesProviderMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockcapabilitiesProvider) EXPECT() *MockcapabilitiesProviderMockRecorder { + return m.recorder +} + +// Capabilities mocks base method. +func (m *MockcapabilitiesProvider) Capabilities() gcs.GuestDefinedCapabilities { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Capabilities") + ret0, _ := ret[0].(gcs.GuestDefinedCapabilities) + return ret0 +} + +// Capabilities indicates an expected call of Capabilities. +func (mr *MockcapabilitiesProviderMockRecorder) Capabilities() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Capabilities", reflect.TypeOf((*MockcapabilitiesProvider)(nil).Capabilities)) +} + +// MockguestNetwork is a mock of guestNetwork interface. +type MockguestNetwork struct { + ctrl *gomock.Controller + recorder *MockguestNetworkMockRecorder + isgomock struct{} +} + +// MockguestNetworkMockRecorder is the mock recorder for MockguestNetwork. +type MockguestNetworkMockRecorder struct { + mock *MockguestNetwork +} + +// NewMockguestNetwork creates a new mock instance. +func NewMockguestNetwork(ctrl *gomock.Controller) *MockguestNetwork { + mock := &MockguestNetwork{ctrl: ctrl} + mock.recorder = &MockguestNetworkMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockguestNetwork) EXPECT() *MockguestNetworkMockRecorder { + return m.recorder +} + +// AddNetworkInterface mocks base method. +func (m *MockguestNetwork) AddNetworkInterface(ctx context.Context, adapterID string, requestType guestrequest.RequestType, settings *hcn.HostComputeEndpoint) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "AddNetworkInterface", ctx, adapterID, requestType, settings) + ret0, _ := ret[0].(error) + return ret0 +} + +// AddNetworkInterface indicates an expected call of AddNetworkInterface. +func (mr *MockguestNetworkMockRecorder) AddNetworkInterface(ctx, adapterID, requestType, settings any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddNetworkInterface", reflect.TypeOf((*MockguestNetwork)(nil).AddNetworkInterface), ctx, adapterID, requestType, settings) +} + +// AddNetworkNamespace mocks base method. +func (m *MockguestNetwork) AddNetworkNamespace(ctx context.Context, settings *hcn.HostComputeNamespace) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "AddNetworkNamespace", ctx, settings) + ret0, _ := ret[0].(error) + return ret0 +} + +// AddNetworkNamespace indicates an expected call of AddNetworkNamespace. +func (mr *MockguestNetworkMockRecorder) AddNetworkNamespace(ctx, settings any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddNetworkNamespace", reflect.TypeOf((*MockguestNetwork)(nil).AddNetworkNamespace), ctx, settings) +} + +// RemoveNetworkInterface mocks base method. +func (m *MockguestNetwork) RemoveNetworkInterface(ctx context.Context, adapterID string, requestType guestrequest.RequestType, settings *hcn.HostComputeEndpoint) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RemoveNetworkInterface", ctx, adapterID, requestType, settings) + ret0, _ := ret[0].(error) + return ret0 +} + +// RemoveNetworkInterface indicates an expected call of RemoveNetworkInterface. +func (mr *MockguestNetworkMockRecorder) RemoveNetworkInterface(ctx, adapterID, requestType, settings any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RemoveNetworkInterface", reflect.TypeOf((*MockguestNetwork)(nil).RemoveNetworkInterface), ctx, adapterID, requestType, settings) +} + +// RemoveNetworkNamespace mocks base method. +func (m *MockguestNetwork) RemoveNetworkNamespace(ctx context.Context, settings *hcn.HostComputeNamespace) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RemoveNetworkNamespace", ctx, settings) + ret0, _ := ret[0].(error) + return ret0 +} + +// RemoveNetworkNamespace indicates an expected call of RemoveNetworkNamespace. +func (mr *MockguestNetworkMockRecorder) RemoveNetworkNamespace(ctx, settings any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RemoveNetworkNamespace", reflect.TypeOf((*MockguestNetwork)(nil).RemoveNetworkNamespace), ctx, settings) +} diff --git a/internal/controller/network/network_lcow_test.go b/internal/controller/network/network_lcow_test.go new file mode 100644 index 0000000000..2dc3c9c33d --- /dev/null +++ b/internal/controller/network/network_lcow_test.go @@ -0,0 +1,435 @@ +//go:build windows && lcow + +package network + +import ( + "context" + "errors" + "testing" + + "go.uber.org/mock/gomock" + + "github.com/Microsoft/hcsshim/hcn" + "github.com/Microsoft/hcsshim/internal/controller/network/mocks" + "github.com/Microsoft/hcsshim/internal/gcs" + "github.com/Microsoft/hcsshim/internal/guest/prot" + hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2" + "github.com/Microsoft/hcsshim/internal/protocol/guestresource" +) + +var ( + errLCOWHostAdd = errors.New("host AddNIC failed") + errLCOWHostRemove = errors.New("host RemoveNIC failed") + errLCOWGuestAdd = errors.New("guest AddNetworkInterface failed") + errLCOWGuestRemove = errors.New("guest RemoveNetworkInterface failed") +) + +// newLCOWController constructs a Controller wired with the supplied mocks +// and pre-set namespace-add capability flag. The state is left at +// StateNotConfigured; callers needing a different state must override +// netState before exercising guarded entry points. +func newLCOWController( + t *testing.T, + ctrl *gomock.Controller, + namespaceAddSupported bool, +) (*Controller, *mocks.MockvmNetworkManager, *mocks.MockguestNetwork) { + t.Helper() + + vm := mocks.NewMockvmNetworkManager(ctrl) + guest := mocks.NewMockguestNetwork(ctrl) + caps := mocks.NewMockcapabilitiesProvider(ctrl) + caps.EXPECT().Capabilities().Return(&gcs.LCOWGuestDefinedCapabilities{ + GcsGuestCapabilities: prot.GcsGuestCapabilities{ + NamespaceAddRequestSupported: namespaceAddSupported, + }, + }) + + c := New( + &Options{NetworkNamespace: "ns-1"}, + vm, + guest, + caps, + ) + return c, vm, guest +} + +// newLCOWEndpoint returns a synthetic HCN endpoint suitable for unit tests. +// All fields the controller reads downstream are populated so that the +// hcsschema.NetworkAdapter passed to AddNIC/RemoveNIC is fully realistic. +func newLCOWEndpoint(name string) *hcn.HostComputeEndpoint { + return &hcn.HostComputeEndpoint{ + Id: "ep-" + name, + Name: name, + MacAddress: "aa:bb:cc:dd:ee:01", + HostComputeNamespace: "ns-1", + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// Add endpoint tests +// ───────────────────────────────────────────────────────────────────────────── + +// TestLCOW_AddEndpoint_Success_NamespaceSupport verifies the happy path with +// namespace support: host AddNIC runs first, then guest AddNetworkInterface +// receives an adapter built from the same endpoint, and the NIC is tracked. +func TestLCOW_AddEndpoint_Success_NamespaceSupport(t *testing.T) { + ctrl := gomock.NewController(t) + c, vm, guest := newLCOWController(t, ctrl, true) + + ep := newLCOWEndpoint("eth0") + expectedAdapter, err := guestresource.BuildLCOWNetworkAdapter("nic-1", ep, false) + if err != nil { + t.Fatalf("failed to build expected adapter: %v", err) + } + + gomock.InOrder( + vm.EXPECT().AddNIC(gomock.Any(), "nic-1", &hcsschema.NetworkAdapter{ + EndpointId: ep.Id, + MacAddress: ep.MacAddress, + }).Return(nil), + guest.EXPECT().AddNetworkInterface(gomock.Any(), expectedAdapter).Return(nil), + ) + + if err := c.addEndpointToGuestNamespace(context.Background(), "nic-1", ep, false); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got, ok := c.vmEndpoints["nic-1"]; !ok || got != ep { + t.Errorf("expected nic-1 → %+v in vmEndpoints, got: %+v (present=%v)", ep, got, ok) + } +} + +// TestLCOW_AddEndpoint_Success_NoNamespaceSupport verifies that when the +// guest does not advertise namespace-add support, only the host-side NIC +// hot-add is performed; the guest call is skipped. +func TestLCOW_AddEndpoint_Success_NoNamespaceSupport(t *testing.T) { + ctrl := gomock.NewController(t) + c, vm, _ := newLCOWController(t, ctrl, false) + + ep := newLCOWEndpoint("eth0") + + vm.EXPECT().AddNIC(gomock.Any(), "nic-1", &hcsschema.NetworkAdapter{ + EndpointId: ep.Id, + MacAddress: ep.MacAddress, + }).Return(nil) + // guest.AddNetworkInterface is intentionally not expected — gomock will + // fail the test if the controller calls it without namespace support. + + if err := c.addEndpointToGuestNamespace(context.Background(), "nic-1", ep, false); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if _, ok := c.vmEndpoints["nic-1"]; !ok { + t.Error("expected nic-1 to be tracked even when guest call is skipped") + } +} + +// TestLCOW_AddEndpoint_HostFails_NotTracked verifies that when the host-side +// AddNIC fails, the NIC is NOT tracked in vmEndpoints. Tracking a NIC the +// host does not own would lead Teardown to call RemoveNIC on a non-existent +// device, masking the real failure. +func TestLCOW_AddEndpoint_HostFails_NotTracked(t *testing.T) { + ctrl := gomock.NewController(t) + c, vm, _ := newLCOWController(t, ctrl, true) + + ep := newLCOWEndpoint("eth0") + + vm.EXPECT().AddNIC(gomock.Any(), "nic-1", gomock.Any()).Return(errLCOWHostAdd) + // guest.AddNetworkInterface must not be called when host add fails. + + err := c.addEndpointToGuestNamespace(context.Background(), "nic-1", ep, false) + if !errors.Is(err, errLCOWHostAdd) { + t.Fatalf("expected host add error to wrap, got: %v", err) + } + if _, ok := c.vmEndpoints["nic-1"]; ok { + t.Error("expected nic-1 NOT tracked after host AddNIC failure") + } +} + +// TestLCOW_AddEndpoint_HostOK_GuestFails_StillTracked verifies that when the +// guest-side configuration fails AFTER a successful host AddNIC, the NIC is +// still tracked. Teardown must still attempt to unwind the host-side NIC, +// or the host UVM leaks the device. +func TestLCOW_AddEndpoint_HostOK_GuestFails_StillTracked(t *testing.T) { + ctrl := gomock.NewController(t) + c, vm, guest := newLCOWController(t, ctrl, true) + + ep := newLCOWEndpoint("eth0") + + gomock.InOrder( + vm.EXPECT().AddNIC(gomock.Any(), "nic-1", gomock.Any()).Return(nil), + guest.EXPECT().AddNetworkInterface(gomock.Any(), gomock.Any()).Return(errLCOWGuestAdd), + ) + + err := c.addEndpointToGuestNamespace(context.Background(), "nic-1", ep, false) + if !errors.Is(err, errLCOWGuestAdd) { + t.Fatalf("expected guest add error to wrap, got: %v", err) + } + if _, ok := c.vmEndpoints["nic-1"]; !ok { + t.Error("expected nic-1 to be tracked so Teardown can unwind the host NIC") + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// Remove endpoint tests +// ───────────────────────────────────────────────────────────────────────────── + +// TestLCOW_RemoveEndpoint_Success verifies the happy removal path: guest-side +// removal runs first, then host-side hot-remove. +func TestLCOW_RemoveEndpoint_Success(t *testing.T) { + ctrl := gomock.NewController(t) + c, vm, guest := newLCOWController(t, ctrl, true) + + ep := newLCOWEndpoint("eth0") + + gomock.InOrder( + guest.EXPECT().RemoveNetworkInterface(gomock.Any(), &guestresource.LCOWNetworkAdapter{ + NamespaceID: "ns-1", + ID: "nic-1", + }).Return(nil), + vm.EXPECT().RemoveNIC(gomock.Any(), "nic-1", &hcsschema.NetworkAdapter{ + EndpointId: ep.Id, + MacAddress: ep.MacAddress, + }).Return(nil), + ) + + if err := c.removeEndpointFromGuestNamespace(context.Background(), "nic-1", ep); err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +// TestLCOW_RemoveEndpoint_GuestFails_HostNotCalled verifies that if the +// guest-side removal fails, the host-side RemoveNIC is NOT invoked. The +// caller (Teardown) marks the controller Invalid and lets a future +// Teardown retry both halves. +func TestLCOW_RemoveEndpoint_GuestFails_HostNotCalled(t *testing.T) { + ctrl := gomock.NewController(t) + c, _, guest := newLCOWController(t, ctrl, true) + + ep := newLCOWEndpoint("eth0") + + guest.EXPECT().RemoveNetworkInterface(gomock.Any(), gomock.Any()).Return(errLCOWGuestRemove) + // vm.RemoveNIC must not be called. + + err := c.removeEndpointFromGuestNamespace(context.Background(), "nic-1", ep) + if !errors.Is(err, errLCOWGuestRemove) { + t.Fatalf("expected guest remove error to wrap, got: %v", err) + } +} + +// TestLCOW_RemoveEndpoint_NoNamespaceSupport_HostOnly verifies that when the +// guest never received the namespace, the controller skips the guest-side +// removal and only hot-removes the NIC from the host. +func TestLCOW_RemoveEndpoint_NoNamespaceSupport_HostOnly(t *testing.T) { + ctrl := gomock.NewController(t) + c, vm, _ := newLCOWController(t, ctrl, false) + + ep := newLCOWEndpoint("eth0") + + vm.EXPECT().RemoveNIC(gomock.Any(), "nic-1", &hcsschema.NetworkAdapter{ + EndpointId: ep.Id, + MacAddress: ep.MacAddress, + }).Return(nil) + // guest.RemoveNetworkInterface must not be called. + + if err := c.removeEndpointFromGuestNamespace(context.Background(), "nic-1", ep); err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// Teardown tests (LCOW: removeNetNSInsideGuest is a no-op so full Teardown +// is exercisable end-to-end without HNS) +// ───────────────────────────────────────────────────────────────────────────── + +// TestLCOW_Teardown_HappyPath verifies that Teardown removes every tracked +// endpoint, clears the tracking map, and transitions the controller to +// StateTornDown. +func TestLCOW_Teardown_HappyPath(t *testing.T) { + ctrl := gomock.NewController(t) + c, vm, guest := newLCOWController(t, ctrl, true) + + c.netState = StateConfigured + ep1 := newLCOWEndpoint("eth0") + ep2 := newLCOWEndpoint("eth1") + c.vmEndpoints["nic-1"] = ep1 + c.vmEndpoints["nic-2"] = ep2 + + // Two endpoints tracked, so each remove API runs exactly twice. Map + // iteration order is not asserted here; gomock matches by call count. + guest.EXPECT().RemoveNetworkInterface(gomock.Any(), gomock.Any()).Return(nil).Times(2) + vm.EXPECT().RemoveNIC(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).Times(2) + + if err := c.Teardown(context.Background()); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if c.netState != StateTornDown { + t.Errorf("expected state TornDown, got %s", c.netState) + } + if len(c.vmEndpoints) != 0 { + t.Errorf("expected vmEndpoints to be empty, got %d entries", len(c.vmEndpoints)) + } +} + +// TestLCOW_Teardown_PartialFailure_RemainingAttempted verifies the cleanup +// chain: if removing one NIC fails, the controller must still attempt the +// remaining NICs. A leak here means a UVM kept alive with stuck NICs. +// The controller transitions to Invalid, but the surviving NICs are gone. +func TestLCOW_Teardown_PartialFailure_RemainingAttempted(t *testing.T) { + ctrl := gomock.NewController(t) + c, vm, guest := newLCOWController(t, ctrl, true) + + c.netState = StateConfigured + ep1 := newLCOWEndpoint("eth0") + ep2 := newLCOWEndpoint("eth1") + ep3 := newLCOWEndpoint("eth2") + c.vmEndpoints["nic-1"] = ep1 + c.vmEndpoints["nic-2"] = ep2 + c.vmEndpoints["nic-3"] = ep3 + + // Fail nic-2 specifically; nic-1 and nic-3 succeed. + // Map iteration order is random, so use DoAndReturn to branch on nicID. + guest.EXPECT().RemoveNetworkInterface(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, settings *guestresource.LCOWNetworkAdapter) error { + if settings.ID == "nic-2" { + return errLCOWGuestRemove + } + return nil + }).Times(3) + // RemoveNIC is only called when guest removal succeeds — so 2 calls. + vm.EXPECT().RemoveNIC(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).Times(2) + + err := c.Teardown(context.Background()) + if !errors.Is(err, errLCOWGuestRemove) { + t.Fatalf("expected joined error to wrap guest remove failure, got: %v", err) + } + if c.netState != StateInvalid { + t.Errorf("expected state Invalid after partial failure, got %s", c.netState) + } + // nic-2 must remain tracked (so a retry can clean it up); nic-1 and nic-3 + // must be gone. + if _, ok := c.vmEndpoints["nic-2"]; !ok { + t.Error("expected nic-2 to remain tracked after its removal failed") + } + if _, ok := c.vmEndpoints["nic-1"]; ok { + t.Error("expected nic-1 to be removed from tracking") + } + if _, ok := c.vmEndpoints["nic-3"]; ok { + t.Error("expected nic-3 to be removed from tracking") + } +} + +// TestLCOW_Teardown_HostRemoveFails_StateInvalid verifies that a host-side +// RemoveNIC failure also surfaces as Invalid (matching the guest-side path) +// and keeps the failed NIC tracked for a future retry. +func TestLCOW_Teardown_HostRemoveFails_StateInvalid(t *testing.T) { + ctrl := gomock.NewController(t) + c, vm, guest := newLCOWController(t, ctrl, true) + + c.netState = StateConfigured + ep := newLCOWEndpoint("eth0") + c.vmEndpoints["nic-1"] = ep + + guest.EXPECT().RemoveNetworkInterface(gomock.Any(), gomock.Any()).Return(nil) + vm.EXPECT().RemoveNIC(gomock.Any(), "nic-1", gomock.Any()).Return(errLCOWHostRemove) + + err := c.Teardown(context.Background()) + if !errors.Is(err, errLCOWHostRemove) { + t.Fatalf("expected host remove error to wrap, got: %v", err) + } + if c.netState != StateInvalid { + t.Errorf("expected state Invalid, got %s", c.netState) + } + if _, ok := c.vmEndpoints["nic-1"]; !ok { + t.Error("expected nic-1 to remain tracked after host RemoveNIC failure") + } +} + +// ────────────────────────────────────────────────────────────────────────── +// Half-setup recovery: Teardown unwinds host-side state after partial failures +// ────────────────────────────────────────────────────────────────────────── + +// TestLCOW_AddEndpoint_HostOK_GuestFails_TeardownUnwindsHost is the paired +// follow-on to TestLCOW_AddEndpoint_HostOK_GuestFails_StillTracked. It proves +// the contract end-to-end: when the guest-side AddNetworkInterface fails after +// a successful host AddNIC, the NIC stays tracked AND a subsequent Teardown +// removes the host-side device. Without this verification the leak guarantee +// is just an in-memory map assertion. +func TestLCOW_AddEndpoint_HostOK_GuestFails_TeardownUnwindsHost(t *testing.T) { + ctrl := gomock.NewController(t) + c, vm, guest := newLCOWController(t, ctrl, true) + + ep := newLCOWEndpoint("eth0") + + gomock.InOrder( + vm.EXPECT().AddNIC(gomock.Any(), "nic-1", gomock.Any()).Return(nil), + guest.EXPECT().AddNetworkInterface(gomock.Any(), gomock.Any()).Return(errLCOWGuestAdd), + ) + + if err := c.addEndpointToGuestNamespace(context.Background(), "nic-1", ep, false); !errors.Is(err, errLCOWGuestAdd) { + t.Fatalf("expected guest add error to wrap, got: %v", err) + } + if _, ok := c.vmEndpoints["nic-1"]; !ok { + t.Fatal("precondition: nic-1 must be tracked before Teardown") + } + + // Teardown must run both legs: guest remove (best-effort, may no-op) and + // host RemoveNIC (the actual leak-recovery path). + c.netState = StateConfigured + gomock.InOrder( + guest.EXPECT().RemoveNetworkInterface(gomock.Any(), gomock.Any()).Return(nil), + vm.EXPECT().RemoveNIC(gomock.Any(), "nic-1", gomock.Any()).Return(nil), + ) + + if err := c.Teardown(context.Background()); err != nil { + t.Fatalf("Teardown after half-setup: %v", err) + } + if c.netState != StateTornDown { + t.Errorf("expected state TornDown after recovery, got %s", c.netState) + } + if _, ok := c.vmEndpoints["nic-1"]; ok { + t.Error("expected nic-1 to be cleared after successful Teardown") + } +} + +// TestLCOW_Teardown_GuestFails_RetryFromInvalid covers the half-setup, +// failure-in-teardown, then success-teardown sequence. The first Teardown +// fails on guest removal; the controller transitions to StateInvalid and +// keeps the failed NIC tracked. A second Teardown call (which production +// code is allowed to make from StateInvalid) drains the remaining endpoint +// and reaches StateTornDown. +func TestLCOW_Teardown_GuestFails_RetryFromInvalid(t *testing.T) { + ctrl := gomock.NewController(t) + c, vm, guest := newLCOWController(t, ctrl, true) + + c.netState = StateConfigured + ep := newLCOWEndpoint("eth0") + c.vmEndpoints["nic-1"] = ep + + // First Teardown: guest remove fails → state Invalid, NIC still tracked. + guest.EXPECT().RemoveNetworkInterface(gomock.Any(), gomock.Any()).Return(errLCOWGuestRemove) + + if err := c.Teardown(context.Background()); !errors.Is(err, errLCOWGuestRemove) { + t.Fatalf("first Teardown: expected guest remove error, got: %v", err) + } + if c.netState != StateInvalid { + t.Fatalf("first Teardown: expected StateInvalid, got %s", c.netState) + } + if _, ok := c.vmEndpoints["nic-1"]; !ok { + t.Fatal("first Teardown: nic-1 must remain tracked for retry") + } + + // Second Teardown: both legs succeed → state TornDown, map cleared. + gomock.InOrder( + guest.EXPECT().RemoveNetworkInterface(gomock.Any(), gomock.Any()).Return(nil), + vm.EXPECT().RemoveNIC(gomock.Any(), "nic-1", gomock.Any()).Return(nil), + ) + + if err := c.Teardown(context.Background()); err != nil { + t.Fatalf("retry Teardown: %v", err) + } + if c.netState != StateTornDown { + t.Errorf("retry Teardown: expected StateTornDown, got %s", c.netState) + } + if _, ok := c.vmEndpoints["nic-1"]; ok { + t.Error("retry Teardown: expected nic-1 to be cleared") + } +} diff --git a/internal/controller/network/network_test.go b/internal/controller/network/network_test.go new file mode 100644 index 0000000000..02c99fea05 --- /dev/null +++ b/internal/controller/network/network_test.go @@ -0,0 +1,195 @@ +//go:build windows && (lcow || wcow) + +package network + +import ( + "context" + "strings" + "testing" + + "go.uber.org/mock/gomock" + + "github.com/Microsoft/hcsshim/internal/controller/network/mocks" + "github.com/Microsoft/hcsshim/internal/gcs" + "github.com/Microsoft/hcsshim/internal/guest/prot" +) + +// ───────────────────────────────────────────────────────────────────────────── +// Test helpers +// ───────────────────────────────────────────────────────────────────────────── + +// newCapsProvider returns a [mocks.MockcapabilitiesProvider] whose +// Capabilities() returns LCOW capabilities with the given namespace-add flag. +// LCOW is used here because Capabilities() returns the [gcs.GuestDefinedCapabilities] +// interface; the concrete OS does not matter for the platform-agnostic tests. +func newCapsProvider(t *testing.T, ctrl *gomock.Controller, namespaceAddSupported bool) *mocks.MockcapabilitiesProvider { + t.Helper() + caps := mocks.NewMockcapabilitiesProvider(ctrl) + caps.EXPECT().Capabilities().Return(&gcs.LCOWGuestDefinedCapabilities{ + GcsGuestCapabilities: prot.GcsGuestCapabilities{ + NamespaceAddRequestSupported: namespaceAddSupported, + }, + }) + return caps +} + +// newNilCapsProvider returns a provider whose Capabilities() returns nil, +// modelling a guest that has not yet reported its capabilities. +func newNilCapsProvider(t *testing.T, ctrl *gomock.Controller) *mocks.MockcapabilitiesProvider { + t.Helper() + caps := mocks.NewMockcapabilitiesProvider(ctrl) + caps.EXPECT().Capabilities().Return(nil) + return caps +} + +// ───────────────────────────────────────────────────────────────────────────── +// Construction tests +// ───────────────────────────────────────────────────────────────────────────── + +// TestNew_NamespaceCapabilityCached verifies that [New] queries the guest's +// capabilities exactly once and caches the namespace-add flag, so hot paths +// do not re-query on every Setup/Teardown call. +func TestNew_NamespaceCapabilityCached(t *testing.T) { + ctrl := gomock.NewController(t) + c := New( + &Options{NetworkNamespace: "ns-1"}, + mocks.NewMockvmNetworkManager(ctrl), + mocks.NewMockguestNetwork(ctrl), + newCapsProvider(t, ctrl, true), + ) + + if !c.isNamespaceSupportedByGuest { + t.Error("expected isNamespaceSupportedByGuest=true after caps say supported") + } + if c.netState != StateNotConfigured { + t.Errorf("expected initial state NotConfigured, got %s", c.netState) + } + if c.namespaceID != "ns-1" { + t.Errorf("expected namespaceID=ns-1, got %q", c.namespaceID) + } + if c.vmEndpoints == nil { + t.Error("expected vmEndpoints map to be non-nil") + } +} + +// TestNew_NilCapabilities_NoCrash verifies that a guest which reports nil +// capabilities does not panic [New] and leaves the namespace flag unset. +// This matches the guarded read in network.go. +func TestNew_NilCapabilities_NoCrash(t *testing.T) { + ctrl := gomock.NewController(t) + c := New( + &Options{NetworkNamespace: "ns-1"}, + mocks.NewMockvmNetworkManager(ctrl), + mocks.NewMockguestNetwork(ctrl), + newNilCapsProvider(t, ctrl), + ) + + if c.isNamespaceSupportedByGuest { + t.Error("expected isNamespaceSupportedByGuest=false when caps are nil") + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// Setup state-guard tests +// ───────────────────────────────────────────────────────────────────────────── + +// TestSetup_RejectInWrongState verifies that Setup refuses to run when the +// controller is not in StateNotConfigured. The guard must reject the call +// without mutating state, since the deferred Invalid handler is registered +// after the guard. +func TestSetup_RejectInWrongState(t *testing.T) { + ctrl := gomock.NewController(t) + c := New( + &Options{NetworkNamespace: "ns-1"}, + mocks.NewMockvmNetworkManager(ctrl), + mocks.NewMockguestNetwork(ctrl), + newCapsProvider(t, ctrl, true), + ) + c.netState = StateConfigured + + err := c.Setup(context.Background()) + if err == nil { + t.Fatal("expected error from Setup in StateConfigured, got nil") + } + if !strings.Contains(err.Error(), "Configured") { + t.Errorf("expected error to mention current state Configured, got: %v", err) + } + // State must not change on guard rejection — the deferred Invalid + // handler is registered after the guard, so a guard miss must be a no-op. + if c.netState != StateConfigured { + t.Errorf("expected state to remain Configured after guard rejection, got %s", c.netState) + } +} + +// TestSetup_EmptyNamespaceID verifies that Setup fails fast when no +// namespace ID is configured and transitions the controller to Invalid. +// This is the only Setup pre-HCN failure path that we can exercise without +// a live HNS, but it also covers the deferred Invalid-on-error handler. +func TestSetup_EmptyNamespaceID(t *testing.T) { + ctrl := gomock.NewController(t) + c := New( + &Options{NetworkNamespace: ""}, + mocks.NewMockvmNetworkManager(ctrl), + mocks.NewMockguestNetwork(ctrl), + newCapsProvider(t, ctrl, true), + ) + + err := c.Setup(context.Background()) + if err == nil { + t.Fatal("expected error from Setup with empty namespace ID, got nil") + } + // On any Setup failure after the state guard, the controller must + // move to Invalid so subsequent Setup calls keep being rejected. + if c.netState != StateInvalid { + t.Errorf("expected state Invalid after Setup failure, got %s", c.netState) + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// Teardown idempotency tests +// ───────────────────────────────────────────────────────────────────────────── + +// TestTeardown_NoOpFromNotConfigured verifies that calling Teardown before +// Setup is a no-op. The shim invokes Teardown unconditionally on pod +// removal even if Setup never ran (or failed before reaching Configured). +func TestTeardown_NoOpFromNotConfigured(t *testing.T) { + ctrl := gomock.NewController(t) + vm := mocks.NewMockvmNetworkManager(ctrl) + guest := mocks.NewMockguestNetwork(ctrl) + c := New( + &Options{NetworkNamespace: "ns-1"}, + vm, + guest, + newCapsProvider(t, ctrl, true), + ) + + // No EXPECT() on vm or guest — any call would fail the test. + if err := c.Teardown(context.Background()); err != nil { + t.Fatalf("expected nil from Teardown in NotConfigured, got: %v", err) + } + if c.netState != StateNotConfigured { + t.Errorf("expected state to remain NotConfigured, got %s", c.netState) + } +} + +// TestTeardown_NoOpFromTornDown verifies that calling Teardown a second time +// after a successful Teardown is a no-op. Containerd retries StopPodSandbox. +func TestTeardown_NoOpFromTornDown(t *testing.T) { + ctrl := gomock.NewController(t) + vm := mocks.NewMockvmNetworkManager(ctrl) + guest := mocks.NewMockguestNetwork(ctrl) + c := New( + &Options{NetworkNamespace: "ns-1"}, + vm, + guest, + newCapsProvider(t, ctrl, true), + ) + c.netState = StateTornDown + + if err := c.Teardown(context.Background()); err != nil { + t.Fatalf("expected nil from Teardown in TornDown, got: %v", err) + } + if c.netState != StateTornDown { + t.Errorf("expected state to remain TornDown, got %s", c.netState) + } +} diff --git a/internal/controller/network/network_wcow_test.go b/internal/controller/network/network_wcow_test.go new file mode 100644 index 0000000000..bd2b9b6a48 --- /dev/null +++ b/internal/controller/network/network_wcow_test.go @@ -0,0 +1,410 @@ +//go:build windows && wcow + +package network + +import ( + "context" + "errors" + "testing" + + "go.uber.org/mock/gomock" + + "github.com/Microsoft/hcsshim/hcn" + "github.com/Microsoft/hcsshim/internal/controller/network/mocks" + "github.com/Microsoft/hcsshim/internal/gcs" + "github.com/Microsoft/hcsshim/internal/hcs/schema1" + hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2" + "github.com/Microsoft/hcsshim/internal/protocol/guestrequest" +) + +var ( + errWCOWHostAdd = errors.New("host AddNIC failed") + errWCOWGuestPreAdd = errors.New("guest PreAdd failed") + errWCOWGuestAdd = errors.New("guest Add failed") + errWCOWGuestRemove = errors.New("guest Remove failed") +) + +// newWCOWController constructs a Controller wired with WCOW mocks and the +// supplied namespace-add capability flag. State starts at StateNotConfigured; +// callers must override netState before exercising guarded entry points. +func newWCOWController( + t *testing.T, + ctrl *gomock.Controller, + namespaceAddSupported bool, +) (*Controller, *mocks.MockvmNetworkManager, *mocks.MockguestNetwork) { + t.Helper() + + vm := mocks.NewMockvmNetworkManager(ctrl) + guest := mocks.NewMockguestNetwork(ctrl) + caps := mocks.NewMockcapabilitiesProvider(ctrl) + caps.EXPECT().Capabilities().Return(&gcs.WCOWGuestDefinedCapabilities{ + GuestDefinedCapabilities: schema1.GuestDefinedCapabilities{ + NamespaceAddRequestSupported: namespaceAddSupported, + }, + }) + + c := New( + &Options{NetworkNamespace: "ns-1"}, + vm, + guest, + caps, + ) + return c, vm, guest +} + +// newWCOWEndpoint returns a synthetic HCN endpoint suitable for unit tests. +func newWCOWEndpoint(name string) *hcn.HostComputeEndpoint { + return &hcn.HostComputeEndpoint{ + Id: "ep-" + name, + Name: name, + MacAddress: "aa:bb:cc:dd:ee:01", + HostComputeNamespace: "ns-1", + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// Add endpoint tests (WCOW: PreAdd → host AddNIC → guest Add) +// ───────────────────────────────────────────────────────────────────────────── + +// TestWCOW_AddEndpoint_3PhaseSequence_Success verifies the WCOW three-phase +// add sequence: guest PreAdd, then host AddNIC, then guest Add (finalize). +// The order matters — WCOW expects the guest to be informed BEFORE the NIC +// arrives on the bus. +func TestWCOW_AddEndpoint_3PhaseSequence_Success(t *testing.T) { + ctrl := gomock.NewController(t) + c, vm, guest := newWCOWController(t, ctrl, true) + + ep := newWCOWEndpoint("eth0") + + gomock.InOrder( + // 1. PreAdd: tells the WCOW guest a NIC is about to arrive. + guest.EXPECT().AddNetworkInterface( + gomock.Any(), "nic-1", guestrequest.RequestTypePreAdd, ep, + ).Return(nil), + // 2. Host hot-add. + vm.EXPECT().AddNIC(gomock.Any(), "nic-1", &hcsschema.NetworkAdapter{ + EndpointId: ep.Id, + MacAddress: ep.MacAddress, + }).Return(nil), + // 3. Guest finalize add. + guest.EXPECT().AddNetworkInterface( + gomock.Any(), "nic-1", guestrequest.RequestTypeAdd, gomock.Nil(), + ).Return(nil), + ) + + if err := c.addEndpointToGuestNamespace(context.Background(), "nic-1", ep, false); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got, ok := c.vmEndpoints["nic-1"]; !ok || got != ep { + t.Errorf("expected nic-1 → %+v in vmEndpoints, got: %+v (present=%v)", ep, got, ok) + } +} + +// TestWCOW_AddEndpoint_PreAddFails_NotTracked verifies that when the guest +// pre-add fails, neither the host AddNIC nor the guest finalize is invoked, +// and the NIC is not tracked. PreAdd is the WCOW-specific guard against +// presenting an unwanted device to the guest. +func TestWCOW_AddEndpoint_PreAddFails_NotTracked(t *testing.T) { + ctrl := gomock.NewController(t) + c, _, guest := newWCOWController(t, ctrl, true) + + ep := newWCOWEndpoint("eth0") + + guest.EXPECT().AddNetworkInterface( + gomock.Any(), "nic-1", guestrequest.RequestTypePreAdd, ep, + ).Return(errWCOWGuestPreAdd) + // No vm.AddNIC, no second guest.AddNetworkInterface — gomock fails if either is called. + + err := c.addEndpointToGuestNamespace(context.Background(), "nic-1", ep, false) + if !errors.Is(err, errWCOWGuestPreAdd) { + t.Fatalf("expected pre-add error to wrap, got: %v", err) + } + if _, ok := c.vmEndpoints["nic-1"]; ok { + t.Error("expected nic-1 NOT tracked after pre-add failure") + } +} + +// TestWCOW_AddEndpoint_HostFails_NotTracked verifies that a host-side AddNIC +// failure (after a successful PreAdd) leaves the NIC untracked. Tracking +// would lead Teardown to RemoveNIC a device the host does not own. +func TestWCOW_AddEndpoint_HostFails_NotTracked(t *testing.T) { + ctrl := gomock.NewController(t) + c, vm, guest := newWCOWController(t, ctrl, true) + + ep := newWCOWEndpoint("eth0") + + gomock.InOrder( + guest.EXPECT().AddNetworkInterface( + gomock.Any(), "nic-1", guestrequest.RequestTypePreAdd, ep, + ).Return(nil), + vm.EXPECT().AddNIC(gomock.Any(), "nic-1", gomock.Any()).Return(errWCOWHostAdd), + ) + + err := c.addEndpointToGuestNamespace(context.Background(), "nic-1", ep, false) + if !errors.Is(err, errWCOWHostAdd) { + t.Fatalf("expected host add error to wrap, got: %v", err) + } + if _, ok := c.vmEndpoints["nic-1"]; ok { + t.Error("expected nic-1 NOT tracked after host AddNIC failure") + } +} + +// TestWCOW_AddEndpoint_FinalAddFails_StillTracked verifies that a failure of +// the final guest Add (after host AddNIC succeeded) leaves the NIC tracked +// so that Teardown unwinds the host-side device. Otherwise the UVM leaks +// the NIC. +func TestWCOW_AddEndpoint_FinalAddFails_StillTracked(t *testing.T) { + ctrl := gomock.NewController(t) + c, vm, guest := newWCOWController(t, ctrl, true) + + ep := newWCOWEndpoint("eth0") + + gomock.InOrder( + guest.EXPECT().AddNetworkInterface( + gomock.Any(), "nic-1", guestrequest.RequestTypePreAdd, ep, + ).Return(nil), + vm.EXPECT().AddNIC(gomock.Any(), "nic-1", gomock.Any()).Return(nil), + guest.EXPECT().AddNetworkInterface( + gomock.Any(), "nic-1", guestrequest.RequestTypeAdd, gomock.Nil(), + ).Return(errWCOWGuestAdd), + ) + + err := c.addEndpointToGuestNamespace(context.Background(), "nic-1", ep, false) + if !errors.Is(err, errWCOWGuestAdd) { + t.Fatalf("expected guest add error to wrap, got: %v", err) + } + if _, ok := c.vmEndpoints["nic-1"]; !ok { + t.Error("expected nic-1 to be tracked so Teardown can unwind the host NIC") + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// Remove endpoint tests +// ───────────────────────────────────────────────────────────────────────────── + +// TestWCOW_RemoveEndpoint_Success verifies the happy removal path: guest-side +// remove first, then host RemoveNIC. +func TestWCOW_RemoveEndpoint_Success(t *testing.T) { + ctrl := gomock.NewController(t) + c, vm, guest := newWCOWController(t, ctrl, true) + + ep := newWCOWEndpoint("eth0") + + gomock.InOrder( + guest.EXPECT().RemoveNetworkInterface( + gomock.Any(), "nic-1", guestrequest.RequestTypeRemove, gomock.Nil(), + ).Return(nil), + vm.EXPECT().RemoveNIC(gomock.Any(), "nic-1", &hcsschema.NetworkAdapter{ + EndpointId: ep.Id, + MacAddress: ep.MacAddress, + }).Return(nil), + ) + + if err := c.removeEndpointFromGuestNamespace(context.Background(), "nic-1", ep); err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +// TestWCOW_RemoveEndpoint_GuestFails_HostNotCalled verifies that a guest-side +// removal failure short-circuits the host hot-remove. Caller (Teardown) +// marks the controller Invalid and lets a future retry attempt both halves. +func TestWCOW_RemoveEndpoint_GuestFails_HostNotCalled(t *testing.T) { + ctrl := gomock.NewController(t) + c, _, guest := newWCOWController(t, ctrl, true) + + ep := newWCOWEndpoint("eth0") + + guest.EXPECT().RemoveNetworkInterface( + gomock.Any(), "nic-1", guestrequest.RequestTypeRemove, gomock.Nil(), + ).Return(errWCOWGuestRemove) + // vm.RemoveNIC must not be called. + + err := c.removeEndpointFromGuestNamespace(context.Background(), "nic-1", ep) + if !errors.Is(err, errWCOWGuestRemove) { + t.Fatalf("expected guest remove error to wrap, got: %v", err) + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// Teardown tests +// +// WCOW's removeNetNSInsideGuest calls hcn.GetNamespaceByID when the guest +// supports namespace add. To exercise full Teardown without a live HNS, +// these tests construct controllers with namespace support DISABLED; +// removeNetNSInsideGuest then becomes a no-op. The endpoint loop logic +// — the high-value cleanup path — is fully covered. +// ───────────────────────────────────────────────────────────────────────────── + +// TestWCOW_Teardown_HappyPath_NoNamespaceSupport verifies that Teardown +// removes every tracked endpoint, clears the tracking map, and transitions +// to StateTornDown. +func TestWCOW_Teardown_HappyPath_NoNamespaceSupport(t *testing.T) { + ctrl := gomock.NewController(t) + c, vm, guest := newWCOWController(t, ctrl, false) + + c.netState = StateConfigured + ep1 := newWCOWEndpoint("eth0") + ep2 := newWCOWEndpoint("eth1") + c.vmEndpoints["nic-1"] = ep1 + c.vmEndpoints["nic-2"] = ep2 + + guest.EXPECT().RemoveNetworkInterface( + gomock.Any(), gomock.Any(), guestrequest.RequestTypeRemove, gomock.Nil(), + ).Return(nil).Times(2) + vm.EXPECT().RemoveNIC(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).Times(2) + + if err := c.Teardown(context.Background()); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if c.netState != StateTornDown { + t.Errorf("expected state TornDown, got %s", c.netState) + } + if len(c.vmEndpoints) != 0 { + t.Errorf("expected vmEndpoints to be empty, got %d entries", len(c.vmEndpoints)) + } +} + +// TestWCOW_Teardown_PartialFailure_RemainingAttempted verifies the cleanup +// chain: if removing one NIC fails, the controller must still attempt the +// remaining NICs. Surviving NICs leak the host UVM device. +func TestWCOW_Teardown_PartialFailure_RemainingAttempted(t *testing.T) { + ctrl := gomock.NewController(t) + c, vm, guest := newWCOWController(t, ctrl, false) + + c.netState = StateConfigured + ep1 := newWCOWEndpoint("eth0") + ep2 := newWCOWEndpoint("eth1") + ep3 := newWCOWEndpoint("eth2") + c.vmEndpoints["nic-1"] = ep1 + c.vmEndpoints["nic-2"] = ep2 + c.vmEndpoints["nic-3"] = ep3 + + // Fail nic-2; nic-1 and nic-3 succeed. + guest.EXPECT().RemoveNetworkInterface( + gomock.Any(), gomock.Any(), guestrequest.RequestTypeRemove, gomock.Nil(), + ).DoAndReturn( + func(_ context.Context, adapterID string, _ guestrequest.RequestType, _ *hcn.HostComputeEndpoint) error { + if adapterID == "nic-2" { + return errWCOWGuestRemove + } + return nil + }).Times(3) + // vm.RemoveNIC only runs when guest removal succeeds → 2 calls. + vm.EXPECT().RemoveNIC(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).Times(2) + + err := c.Teardown(context.Background()) + if !errors.Is(err, errWCOWGuestRemove) { + t.Fatalf("expected joined error to wrap guest remove failure, got: %v", err) + } + if c.netState != StateInvalid { + t.Errorf("expected state Invalid after partial failure, got %s", c.netState) + } + if _, ok := c.vmEndpoints["nic-2"]; !ok { + t.Error("expected nic-2 to remain tracked after its removal failed") + } + if _, ok := c.vmEndpoints["nic-1"]; ok { + t.Error("expected nic-1 to be removed from tracking") + } + if _, ok := c.vmEndpoints["nic-3"]; ok { + t.Error("expected nic-3 to be removed from tracking") + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// Half-setup recovery: Teardown unwinds host-side state after partial failures +// ───────────────────────────────────────────────────────────────────────────── + +// TestWCOW_AddEndpoint_FinalAddFails_TeardownUnwindsHost is the paired +// follow-on to TestWCOW_AddEndpoint_FinalAddFails_StillTracked. The +// half-setup state (PreAdd ok, host AddNIC ok, guest Add fail) leaves the +// NIC tracked; this test proves Teardown then unwinds the host side, so the +// UVM does not leak the device. +func TestWCOW_AddEndpoint_FinalAddFails_TeardownUnwindsHost(t *testing.T) { + ctrl := gomock.NewController(t) + c, vm, guest := newWCOWController(t, ctrl, false) + + ep := newWCOWEndpoint("eth0") + + gomock.InOrder( + guest.EXPECT().AddNetworkInterface( + gomock.Any(), "nic-1", guestrequest.RequestTypePreAdd, ep, + ).Return(nil), + vm.EXPECT().AddNIC(gomock.Any(), "nic-1", gomock.Any()).Return(nil), + guest.EXPECT().AddNetworkInterface( + gomock.Any(), "nic-1", guestrequest.RequestTypeAdd, gomock.Nil(), + ).Return(errWCOWGuestAdd), + ) + + if err := c.addEndpointToGuestNamespace(context.Background(), "nic-1", ep, false); !errors.Is(err, errWCOWGuestAdd) { + t.Fatalf("expected guest add error to wrap, got: %v", err) + } + if _, ok := c.vmEndpoints["nic-1"]; !ok { + t.Fatal("precondition: nic-1 must be tracked before Teardown") + } + + // Teardown: guest Remove (best-effort), then host RemoveNIC. + c.netState = StateConfigured + gomock.InOrder( + guest.EXPECT().RemoveNetworkInterface( + gomock.Any(), "nic-1", guestrequest.RequestTypeRemove, gomock.Nil(), + ).Return(nil), + vm.EXPECT().RemoveNIC(gomock.Any(), "nic-1", gomock.Any()).Return(nil), + ) + + if err := c.Teardown(context.Background()); err != nil { + t.Fatalf("Teardown after half-setup: %v", err) + } + if c.netState != StateTornDown { + t.Errorf("expected state TornDown after recovery, got %s", c.netState) + } + if _, ok := c.vmEndpoints["nic-1"]; ok { + t.Error("expected nic-1 to be cleared after successful Teardown") + } +} + +// TestWCOW_Teardown_GuestFails_RetryFromInvalid covers the half-setup, +// failure-in-teardown, then success-teardown sequence for WCOW. First +// Teardown fails on guest removal -> state Invalid, NIC stays tracked. Second +// Teardown call (allowed from StateInvalid) drains the endpoint and reaches +// StateTornDown. +func TestWCOW_Teardown_GuestFails_RetryFromInvalid(t *testing.T) { + ctrl := gomock.NewController(t) + c, vm, guest := newWCOWController(t, ctrl, false) + + c.netState = StateConfigured + ep := newWCOWEndpoint("eth0") + c.vmEndpoints["nic-1"] = ep + + // First Teardown: guest remove fails → state Invalid, NIC still tracked. + guest.EXPECT().RemoveNetworkInterface( + gomock.Any(), "nic-1", guestrequest.RequestTypeRemove, gomock.Nil(), + ).Return(errWCOWGuestRemove) + + if err := c.Teardown(context.Background()); !errors.Is(err, errWCOWGuestRemove) { + t.Fatalf("first Teardown: expected guest remove error, got: %v", err) + } + if c.netState != StateInvalid { + t.Fatalf("first Teardown: expected StateInvalid, got %s", c.netState) + } + if _, ok := c.vmEndpoints["nic-1"]; !ok { + t.Fatal("first Teardown: nic-1 must remain tracked for retry") + } + + // Second Teardown: both legs succeed → state TornDown, map cleared. + gomock.InOrder( + guest.EXPECT().RemoveNetworkInterface( + gomock.Any(), "nic-1", guestrequest.RequestTypeRemove, gomock.Nil(), + ).Return(nil), + vm.EXPECT().RemoveNIC(gomock.Any(), "nic-1", gomock.Any()).Return(nil), + ) + + if err := c.Teardown(context.Background()); err != nil { + t.Fatalf("retry Teardown: %v", err) + } + if c.netState != StateTornDown { + t.Errorf("retry Teardown: expected StateTornDown, got %s", c.netState) + } + if _, ok := c.vmEndpoints["nic-1"]; ok { + t.Error("retry Teardown: expected nic-1 to be cleared") + } +} From 13608c15491439149516181d1f4900e45990b39f Mon Sep 17 00:00:00 2001 From: Shreyansh Sancheti Date: Mon, 27 Apr 2026 22:21:30 +0530 Subject: [PATCH 2/2] controller/network: merge AddEndpoint half-setup tests into single contract test Each platform had two tests covering the same half-setup recovery path: TestLCOW_AddEndpoint_HostOK_GuestFails_StillTracked TestLCOW_AddEndpoint_HostOK_GuestFails_TeardownUnwindsHost TestWCOW_AddEndpoint_FinalAddFails_StillTracked TestWCOW_AddEndpoint_FinalAddFails_TeardownUnwindsHost The "_StillTracked" tests asserted the in-memory contract that a NIC remains tracked after a guest-side failure. The "_TeardownUnwindsHost" tests proved the same contract end-to-end by then running Teardown and verifying the host-side device gets unwound. These are not independent contracts: tracking the NIC is the precondition for Teardown unwinding. Splitting them into two tests duplicates setup and hides the relationship. Merge by removing each "_StillTracked" test; the "_TeardownUnwindsHost" tests already perform the tracked-state assertion as their precondition before exercising Teardown. Net result: identical coverage, 2 fewer tests, 1 file with the full half-setup recovery story per platform. Signed-off-by: Shreyansh Sancheti --- .../controller/network/network_lcow_test.go | 37 +++-------------- .../controller/network/network_wcow_test.go | 40 +++---------------- 2 files changed, 11 insertions(+), 66 deletions(-) diff --git a/internal/controller/network/network_lcow_test.go b/internal/controller/network/network_lcow_test.go index 2dc3c9c33d..819e94448b 100644 --- a/internal/controller/network/network_lcow_test.go +++ b/internal/controller/network/network_lcow_test.go @@ -144,30 +144,6 @@ func TestLCOW_AddEndpoint_HostFails_NotTracked(t *testing.T) { } } -// TestLCOW_AddEndpoint_HostOK_GuestFails_StillTracked verifies that when the -// guest-side configuration fails AFTER a successful host AddNIC, the NIC is -// still tracked. Teardown must still attempt to unwind the host-side NIC, -// or the host UVM leaks the device. -func TestLCOW_AddEndpoint_HostOK_GuestFails_StillTracked(t *testing.T) { - ctrl := gomock.NewController(t) - c, vm, guest := newLCOWController(t, ctrl, true) - - ep := newLCOWEndpoint("eth0") - - gomock.InOrder( - vm.EXPECT().AddNIC(gomock.Any(), "nic-1", gomock.Any()).Return(nil), - guest.EXPECT().AddNetworkInterface(gomock.Any(), gomock.Any()).Return(errLCOWGuestAdd), - ) - - err := c.addEndpointToGuestNamespace(context.Background(), "nic-1", ep, false) - if !errors.Is(err, errLCOWGuestAdd) { - t.Fatalf("expected guest add error to wrap, got: %v", err) - } - if _, ok := c.vmEndpoints["nic-1"]; !ok { - t.Error("expected nic-1 to be tracked so Teardown can unwind the host NIC") - } -} - // ───────────────────────────────────────────────────────────────────────────── // Remove endpoint tests // ───────────────────────────────────────────────────────────────────────────── @@ -347,12 +323,11 @@ func TestLCOW_Teardown_HostRemoveFails_StateInvalid(t *testing.T) { // Half-setup recovery: Teardown unwinds host-side state after partial failures // ────────────────────────────────────────────────────────────────────────── -// TestLCOW_AddEndpoint_HostOK_GuestFails_TeardownUnwindsHost is the paired -// follow-on to TestLCOW_AddEndpoint_HostOK_GuestFails_StillTracked. It proves -// the contract end-to-end: when the guest-side AddNetworkInterface fails after -// a successful host AddNIC, the NIC stays tracked AND a subsequent Teardown -// removes the host-side device. Without this verification the leak guarantee -// is just an in-memory map assertion. +// TestLCOW_AddEndpoint_HostOK_GuestFails_TeardownUnwindsHost covers the +// half-setup recovery contract end-to-end: when the guest-side +// AddNetworkInterface fails after a successful host AddNIC, the NIC must +// remain tracked so a subsequent Teardown can remove the host-side device. +// Otherwise the host UVM leaks the NIC. func TestLCOW_AddEndpoint_HostOK_GuestFails_TeardownUnwindsHost(t *testing.T) { ctrl := gomock.NewController(t) c, vm, guest := newLCOWController(t, ctrl, true) @@ -368,7 +343,7 @@ func TestLCOW_AddEndpoint_HostOK_GuestFails_TeardownUnwindsHost(t *testing.T) { t.Fatalf("expected guest add error to wrap, got: %v", err) } if _, ok := c.vmEndpoints["nic-1"]; !ok { - t.Fatal("precondition: nic-1 must be tracked before Teardown") + t.Fatal("expected nic-1 to be tracked after guest-side failure so Teardown can unwind the host NIC") } // Teardown must run both legs: guest remove (best-effort, may no-op) and diff --git a/internal/controller/network/network_wcow_test.go b/internal/controller/network/network_wcow_test.go index bd2b9b6a48..a68e0a8dbe 100644 --- a/internal/controller/network/network_wcow_test.go +++ b/internal/controller/network/network_wcow_test.go @@ -149,35 +149,6 @@ func TestWCOW_AddEndpoint_HostFails_NotTracked(t *testing.T) { } } -// TestWCOW_AddEndpoint_FinalAddFails_StillTracked verifies that a failure of -// the final guest Add (after host AddNIC succeeded) leaves the NIC tracked -// so that Teardown unwinds the host-side device. Otherwise the UVM leaks -// the NIC. -func TestWCOW_AddEndpoint_FinalAddFails_StillTracked(t *testing.T) { - ctrl := gomock.NewController(t) - c, vm, guest := newWCOWController(t, ctrl, true) - - ep := newWCOWEndpoint("eth0") - - gomock.InOrder( - guest.EXPECT().AddNetworkInterface( - gomock.Any(), "nic-1", guestrequest.RequestTypePreAdd, ep, - ).Return(nil), - vm.EXPECT().AddNIC(gomock.Any(), "nic-1", gomock.Any()).Return(nil), - guest.EXPECT().AddNetworkInterface( - gomock.Any(), "nic-1", guestrequest.RequestTypeAdd, gomock.Nil(), - ).Return(errWCOWGuestAdd), - ) - - err := c.addEndpointToGuestNamespace(context.Background(), "nic-1", ep, false) - if !errors.Is(err, errWCOWGuestAdd) { - t.Fatalf("expected guest add error to wrap, got: %v", err) - } - if _, ok := c.vmEndpoints["nic-1"]; !ok { - t.Error("expected nic-1 to be tracked so Teardown can unwind the host NIC") - } -} - // ───────────────────────────────────────────────────────────────────────────── // Remove endpoint tests // ───────────────────────────────────────────────────────────────────────────── @@ -314,11 +285,10 @@ func TestWCOW_Teardown_PartialFailure_RemainingAttempted(t *testing.T) { // Half-setup recovery: Teardown unwinds host-side state after partial failures // ───────────────────────────────────────────────────────────────────────────── -// TestWCOW_AddEndpoint_FinalAddFails_TeardownUnwindsHost is the paired -// follow-on to TestWCOW_AddEndpoint_FinalAddFails_StillTracked. The -// half-setup state (PreAdd ok, host AddNIC ok, guest Add fail) leaves the -// NIC tracked; this test proves Teardown then unwinds the host side, so the -// UVM does not leak the device. +// TestWCOW_AddEndpoint_FinalAddFails_TeardownUnwindsHost covers the +// half-setup recovery contract end-to-end: PreAdd ok, host AddNIC ok, guest +// Add fails. The NIC must remain tracked so a subsequent Teardown can +// unwind the host-side device. Otherwise the UVM leaks the NIC. func TestWCOW_AddEndpoint_FinalAddFails_TeardownUnwindsHost(t *testing.T) { ctrl := gomock.NewController(t) c, vm, guest := newWCOWController(t, ctrl, false) @@ -339,7 +309,7 @@ func TestWCOW_AddEndpoint_FinalAddFails_TeardownUnwindsHost(t *testing.T) { t.Fatalf("expected guest add error to wrap, got: %v", err) } if _, ok := c.vmEndpoints["nic-1"]; !ok { - t.Fatal("precondition: nic-1 must be tracked before Teardown") + t.Fatal("expected nic-1 to be tracked after guest-side failure so Teardown can unwind the host NIC") } // Teardown: guest Remove (best-effort), then host RemoveNIC.