From d524163f9139be2604fa28b932814a0ab586394a Mon Sep 17 00:00:00 2001 From: Kathryn Baldauf Date: Wed, 29 Sep 2021 14:36:12 -0700 Subject: [PATCH 1/2] Add unit tests for computeagent Signed-off-by: Kathryn Baldauf --- cmd/ncproxy/ncproxy.go | 2 - internal/uvm/computeagent.go | 20 +- internal/uvm/computeagent_test.go | 229 ++++++++++++++++++ .../hcsshim/internal/uvm/computeagent.go | 20 +- 4 files changed, 261 insertions(+), 10 deletions(-) create mode 100644 internal/uvm/computeagent_test.go diff --git a/cmd/ncproxy/ncproxy.go b/cmd/ncproxy/ncproxy.go index ae7b4acf86..50e893ab0b 100644 --- a/cmd/ncproxy/ncproxy.go +++ b/cmd/ncproxy/ncproxy.go @@ -84,7 +84,6 @@ func (s *grpcService) AddNIC(ctx context.Context, req *ncproxygrpc.AddNICRequest } if agent, ok := s.containerIDToComputeAgent.get(req.ContainerID); ok { caReq := &computeagent.AddNICInternalRequest{ - ContainerID: req.ContainerID, NicID: req.NicID, EndpointName: req.EndpointName, } @@ -190,7 +189,6 @@ func (s *grpcService) DeleteNIC(ctx context.Context, req *ncproxygrpc.DeleteNICR } if agent, ok := s.containerIDToComputeAgent.get(req.ContainerID); ok { caReq := &computeagent.DeleteNICInternalRequest{ - ContainerID: req.ContainerID, NicID: req.NicID, EndpointName: req.EndpointName, } diff --git a/internal/uvm/computeagent.go b/internal/uvm/computeagent.go index 3ccacd878f..a97842104a 100644 --- a/internal/uvm/computeagent.go +++ b/internal/uvm/computeagent.go @@ -23,10 +23,22 @@ import ( const ComputeAgentAddrFmt = "\\\\.\\pipe\\computeagent-%s" +// create an interface here so we can mock out calls to the UtilityVM in our tests +type agentComputeSystem interface { + AddEndpointToNSWithID(context.Context, string, string, *hns.HNSEndpoint) error + UpdateNIC(context.Context, string, *hcsschema.NetworkAdapter) error + RemoveEndpointFromNS(context.Context, string, *hns.HNSEndpoint) error +} + +var _ agentComputeSystem = &UtilityVM{} + +// mock hcn function for tests +var hnsGetHNSEndpointByName = hns.GetHNSEndpointByName + // computeAgent implements the ComputeAgent ttrpc service for adding and deleting NICs to a // Utility VM. type computeAgent struct { - uvm *UtilityVM + uvm agentComputeSystem } var _ computeagent.ComputeAgentService = &computeAgent{} @@ -43,7 +55,7 @@ func (ca *computeAgent) AddNIC(ctx context.Context, req *computeagent.AddNICInte return nil, status.Error(codes.InvalidArgument, "received empty field in request") } - endpoint, err := hns.GetHNSEndpointByName(req.EndpointName) + endpoint, err := hnsGetHNSEndpointByName(req.EndpointName) if err != nil { return nil, errors.Wrapf(err, "failed to get endpoint with name %q", req.EndpointName) } @@ -64,7 +76,7 @@ func (ca *computeAgent) ModifyNIC(ctx context.Context, req *computeagent.ModifyN return nil, status.Error(codes.InvalidArgument, "received empty field in request") } - endpoint, err := hns.GetHNSEndpointByName(req.EndpointName) + endpoint, err := hnsGetHNSEndpointByName(req.EndpointName) if err != nil { return nil, errors.Wrapf(err, "failed to get endpoint with name `%s`", req.EndpointName) } @@ -103,7 +115,7 @@ func (ca *computeAgent) DeleteNIC(ctx context.Context, req *computeagent.DeleteN return nil, status.Error(codes.InvalidArgument, "received empty field in request") } - endpoint, err := hns.GetHNSEndpointByName(req.EndpointName) + endpoint, err := hnsGetHNSEndpointByName(req.EndpointName) if err != nil { return nil, errors.Wrapf(err, "failed to get endpoint with name %q", req.EndpointName) } diff --git a/internal/uvm/computeagent_test.go b/internal/uvm/computeagent_test.go new file mode 100644 index 0000000000..96caa3c2be --- /dev/null +++ b/internal/uvm/computeagent_test.go @@ -0,0 +1,229 @@ +package uvm + +import ( + "context" + "testing" + + "github.com/Microsoft/hcsshim/internal/computeagent" + hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2" + "github.com/Microsoft/hcsshim/internal/hns" +) + +type testUtilityVM struct{} + +func (t *testUtilityVM) AddEndpointToNSWithID(ctx context.Context, nsID, nicID string, endpoint *hns.HNSEndpoint) error { + return nil +} + +func (t *testUtilityVM) RemoveEndpointFromNS(ctx context.Context, id string, endpoint *hns.HNSEndpoint) error { + return nil +} + +func (t *testUtilityVM) UpdateNIC(ctx context.Context, id string, settings *hcsschema.NetworkAdapter) error { + return nil +} + +func TestAddNIC(t *testing.T) { + ctx := context.Background() + + agent := &computeAgent{ + uvm: &testUtilityVM{}, + } + + hnsGetHNSEndpointByName = func(endpointName string) (*hns.HNSEndpoint, error) { + return &hns.HNSEndpoint{ + Namespace: &hns.Namespace{ID: "test-namespace-ID"}, + }, nil + } + + var ( + testNICID = "test-NIC-ID" + testEndpointName = "test-endpoint-name" + ) + + type config struct { + name string + nicID string + endpointName string + errorExpected bool + } + tests := []config{ + { + name: "AddNIC returns no error", + nicID: testNICID, + endpointName: testEndpointName, + errorExpected: false, + }, + { + name: "AddNIC returns error with blank nic ID", + nicID: "", + endpointName: testEndpointName, + errorExpected: true, + }, + { + name: "AddNIC returns error with blank endpoint name", + nicID: testNICID, + endpointName: "", + errorExpected: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(_ *testing.T) { + + req := &computeagent.AddNICInternalRequest{ + NicID: test.nicID, + EndpointName: test.endpointName, + } + + _, err := agent.AddNIC(ctx, req) + if test.errorExpected && err == nil { + t.Fatalf("expected AddNIC to return an error") + } + if !test.errorExpected && err != nil { + t.Fatalf("expected AddNIC to return no error, instead got %v", err) + } + }) + } +} + +func TestModifyNIC(t *testing.T) { + ctx := context.Background() + + agent := &computeAgent{ + uvm: &testUtilityVM{}, + } + + hnsGetHNSEndpointByName = func(endpointName string) (*hns.HNSEndpoint, error) { + return &hns.HNSEndpoint{ + Id: "test-ID", + MacAddress: "00-00-00-00-00-00", + }, nil + } + + var ( + testNICID = "test-NIC-ID" + testEndpointName = "test-endpoint-name" + ) + + iovSettingsOn := &computeagent.IovSettings{ + IovOffloadWeight: 100, + } + + type config struct { + name string + nicID string + endpointName string + iovSettings *computeagent.IovSettings + errorExpected bool + } + tests := []config{ + { + name: "ModifyNIC returns no error", + nicID: testNICID, + endpointName: testEndpointName, + iovSettings: iovSettingsOn, + errorExpected: false, + }, + { + name: "ModifyNIC returns error with blank nic ID", + nicID: "", + endpointName: testEndpointName, + iovSettings: iovSettingsOn, + errorExpected: true, + }, + { + name: "ModifyNIC returns error with blank endpoint name", + nicID: testNICID, + endpointName: "", + iovSettings: iovSettingsOn, + errorExpected: true, + }, + { + name: "ModifyNIC returns error with nil IOV settings", + nicID: testNICID, + endpointName: testEndpointName, + iovSettings: nil, + errorExpected: true, + }, + } + for _, test := range tests { + t.Run(test.name, func(_ *testing.T) { + req := &computeagent.ModifyNICInternalRequest{ + NicID: test.nicID, + EndpointName: test.endpointName, + IovPolicySettings: test.iovSettings, + } + + _, err := agent.ModifyNIC(ctx, req) + if test.errorExpected && err == nil { + t.Fatalf("expected ModifyNIC to return an error") + } + if !test.errorExpected && err != nil { + t.Fatalf("expected ModifyNIC to return no error, instead got %v", err) + } + }) + } +} + +func TestDeleteNIC(t *testing.T) { + ctx := context.Background() + + agent := &computeAgent{ + uvm: &testUtilityVM{}, + } + + hnsGetHNSEndpointByName = func(endpointName string) (*hns.HNSEndpoint, error) { + return &hns.HNSEndpoint{ + Namespace: &hns.Namespace{ID: "test-namespace-ID"}, + }, nil + } + + var ( + testNICID = "test-NIC-ID" + testEndpointName = "test-endpoint-name" + ) + + type config struct { + name string + nicID string + endpointName string + errorExpected bool + } + tests := []config{ + { + name: "DeleteNIC returns no error", + nicID: testNICID, + endpointName: testEndpointName, + errorExpected: false, + }, + { + name: "DeleteNIC returns error with blank nic ID", + nicID: "", + endpointName: testEndpointName, + errorExpected: true, + }, + { + name: "DeleteNIC returns error with blank endpoint name", + nicID: testNICID, + endpointName: "", + errorExpected: true, + }, + } + for _, test := range tests { + t.Run(test.name, func(_ *testing.T) { + req := &computeagent.DeleteNICInternalRequest{ + NicID: test.nicID, + EndpointName: test.endpointName, + } + + _, err := agent.DeleteNIC(ctx, req) + if test.errorExpected && err == nil { + t.Fatalf("expected DeleteNIC to return an error") + } + if !test.errorExpected && err != nil { + t.Fatalf("expected DeleteNIC to return no error, instead got %v", err) + } + }) + } +} diff --git a/test/vendor/github.com/Microsoft/hcsshim/internal/uvm/computeagent.go b/test/vendor/github.com/Microsoft/hcsshim/internal/uvm/computeagent.go index 3ccacd878f..a97842104a 100644 --- a/test/vendor/github.com/Microsoft/hcsshim/internal/uvm/computeagent.go +++ b/test/vendor/github.com/Microsoft/hcsshim/internal/uvm/computeagent.go @@ -23,10 +23,22 @@ import ( const ComputeAgentAddrFmt = "\\\\.\\pipe\\computeagent-%s" +// create an interface here so we can mock out calls to the UtilityVM in our tests +type agentComputeSystem interface { + AddEndpointToNSWithID(context.Context, string, string, *hns.HNSEndpoint) error + UpdateNIC(context.Context, string, *hcsschema.NetworkAdapter) error + RemoveEndpointFromNS(context.Context, string, *hns.HNSEndpoint) error +} + +var _ agentComputeSystem = &UtilityVM{} + +// mock hcn function for tests +var hnsGetHNSEndpointByName = hns.GetHNSEndpointByName + // computeAgent implements the ComputeAgent ttrpc service for adding and deleting NICs to a // Utility VM. type computeAgent struct { - uvm *UtilityVM + uvm agentComputeSystem } var _ computeagent.ComputeAgentService = &computeAgent{} @@ -43,7 +55,7 @@ func (ca *computeAgent) AddNIC(ctx context.Context, req *computeagent.AddNICInte return nil, status.Error(codes.InvalidArgument, "received empty field in request") } - endpoint, err := hns.GetHNSEndpointByName(req.EndpointName) + endpoint, err := hnsGetHNSEndpointByName(req.EndpointName) if err != nil { return nil, errors.Wrapf(err, "failed to get endpoint with name %q", req.EndpointName) } @@ -64,7 +76,7 @@ func (ca *computeAgent) ModifyNIC(ctx context.Context, req *computeagent.ModifyN return nil, status.Error(codes.InvalidArgument, "received empty field in request") } - endpoint, err := hns.GetHNSEndpointByName(req.EndpointName) + endpoint, err := hnsGetHNSEndpointByName(req.EndpointName) if err != nil { return nil, errors.Wrapf(err, "failed to get endpoint with name `%s`", req.EndpointName) } @@ -103,7 +115,7 @@ func (ca *computeAgent) DeleteNIC(ctx context.Context, req *computeagent.DeleteN return nil, status.Error(codes.InvalidArgument, "received empty field in request") } - endpoint, err := hns.GetHNSEndpointByName(req.EndpointName) + endpoint, err := hnsGetHNSEndpointByName(req.EndpointName) if err != nil { return nil, errors.Wrapf(err, "failed to get endpoint with name %q", req.EndpointName) } From 8ab28b159b45a2753bd4a7c5c9512bacf24a32fd Mon Sep 17 00:00:00 2001 From: Kathryn Baldauf Date: Fri, 1 Oct 2021 14:00:15 -0700 Subject: [PATCH 2/2] Add back containerID to computeagent requests Signed-off-by: Kathryn Baldauf --- cmd/ncproxy/ncproxy.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cmd/ncproxy/ncproxy.go b/cmd/ncproxy/ncproxy.go index 50e893ab0b..ae7b4acf86 100644 --- a/cmd/ncproxy/ncproxy.go +++ b/cmd/ncproxy/ncproxy.go @@ -84,6 +84,7 @@ func (s *grpcService) AddNIC(ctx context.Context, req *ncproxygrpc.AddNICRequest } if agent, ok := s.containerIDToComputeAgent.get(req.ContainerID); ok { caReq := &computeagent.AddNICInternalRequest{ + ContainerID: req.ContainerID, NicID: req.NicID, EndpointName: req.EndpointName, } @@ -189,6 +190,7 @@ func (s *grpcService) DeleteNIC(ctx context.Context, req *ncproxygrpc.DeleteNICR } if agent, ok := s.containerIDToComputeAgent.get(req.ContainerID); ok { caReq := &computeagent.DeleteNICInternalRequest{ + ContainerID: req.ContainerID, NicID: req.NicID, EndpointName: req.EndpointName, }