test: add ut for populating network and endpoint hns id in endpoint and endpoint info structs (#2844)

* validate endpoint struct after endpoint creation

* add uts to validate endpoint info and endpoint structs have hns network and endpoint id fields populated

endpoint struct should have the hns endpoint id after new endpoint
endpoint info struct should have the hns endpoint id after new endpoint as well

network struct should have the hns network id after new network
endpoint info struct should have the hns network id after new network as well

endpoint struct should have the hns network id after new endpoint if endpoint info had it

* address linter issue

* address feedback by validating info sent to cns

* modify hnsv2 wrapper behavior to only set hns id if not already set for npm uts

Author: QxBytes

network/endpoint.go

@@ -15,6 +15,7 @@ import (
 	"github.com/Azure/azure-container-networking/netlink"
 	"github.com/Azure/azure-container-networking/network/policy"
 	"github.com/Azure/azure-container-networking/platform"
+	"github.com/pkg/errors"
 	"go.uber.org/zap"
 )
 
@@ -386,3 +387,25 @@ func (epInfo *EndpointInfo) IsEndpointStateIncomplete() bool {
 	}
 	return false
 }
+
+func (ep *endpoint) validateEndpoint() error {
+	if ep.ContainerID == "" || ep.NICType == "" {
+		return errors.New("endpoint struct must contain a container id and nic type")
+	}
+	return nil
+}
+
+func validateEndpoints(eps []*endpoint) error {
+	containerIDs := map[string]bool{}
+	for _, ep := range eps {
+		if err := ep.validateEndpoint(); err != nil {
+			return errors.Wrap(err, "failed to validate endpoint struct")
+		}
+		containerIDs[ep.ContainerID] = true
+
+		if len(containerIDs) != 1 {
+			return errors.New("multiple distinct container ids detected")
+		}
+	}
+	return nil
+}

network/endpoint_test.go

@@ -175,10 +175,11 @@ var _ = Describe("Test Endpoint", func() {
 				Endpoints: map[string]*endpoint{},
 			}
 			epInfo := &EndpointInfo{
-				EndpointID: "768e8deb-eth1",
-				Data:       make(map[string]interface{}),
-				IfName:     eth0IfName,
-				NICType:    cns.InfraNIC,
+				EndpointID:  "768e8deb-eth1",
+				Data:        make(map[string]interface{}),
+				IfName:      eth0IfName,
+				NICType:     cns.InfraNIC,
+				ContainerID: "0ea7476f26d192f067abdc8b3df43ce3cdbe324386e1c010cb48de87eefef480",
 			}
 			epInfo.Data[VlanIDKey] = 100
 
@@ -206,6 +207,7 @@ var _ = Describe("Test Endpoint", func() {
 				Expect(ep.Gateways[0].String()).To(Equal("192.168.0.1"))
 				Expect(ep.VlanID).To(Equal(epInfo.Data[VlanIDKey].(int)))
 				Expect(ep.IfName).To(Equal(epInfo.IfName))
+				Expect(ep.ContainerID).To(Equal(epInfo.ContainerID))
 			})
 			It("Should be not added", func() {
 				// Adding an endpoint with an id.
@@ -374,4 +376,79 @@ var _ = Describe("Test Endpoint", func() {
 			})
 		})
 	})
+
+	// validation when calling add
+	Describe("Test validateEndpoints", func() {
+		Context("When in a single add call we create two endpoint structs", func() {
+			It("Should have the same container id", func() {
+				eps := []*endpoint{
+					{
+						ContainerID: "0ea7476f26d192f067abdc8b3df43ce3cdbe324386e1c010cb48de87eefef480",
+						NICType:     cns.InfraNIC,
+					},
+					{
+						ContainerID: "0ea7476f26d192f067abdc8b3df43ce3cdbe324386e1c010cb48de87eefef480",
+						NICType:     cns.DelegatedVMNIC,
+					},
+				}
+				Expect(validateEndpoints(eps)).To(BeNil())
+			})
+		})
+		Context("When in a single add call we have different container ids", func() {
+			It("Should error", func() {
+				eps := []*endpoint{
+					{
+						ContainerID: "0ea7476f26d192f067abdc8b3df43ce3cdbe324386e1c010cb48de87eefef480",
+						NICType:     cns.InfraNIC,
+					},
+					{
+						ContainerID: "0ea7476f26d192f067abdc8b3df43ce3cdbe324386e1c010cb48de87eefef481",
+						NICType:     cns.DelegatedVMNIC,
+					},
+				}
+				Expect(validateEndpoints(eps)).ToNot(BeNil())
+			})
+		})
+		Context("When no container id", func() {
+			It("Should error", func() {
+				eps := []*endpoint{
+					{
+						ContainerID: "",
+						NICType:     cns.InfraNIC,
+					},
+					{
+						ContainerID: "",
+						NICType:     cns.DelegatedVMNIC,
+					},
+				}
+				Expect(validateEndpoints(eps)).ToNot(BeNil())
+			})
+		})
+		Context("When missing nic type", func() {
+			It("Should error", func() {
+				eps := []*endpoint{
+					{
+						ContainerID: "0ea7476f26d192f067abdc8b3df43ce3cdbe324386e1c010cb48de87eefef480",
+						NICType:     cns.InfraNIC,
+					},
+					{
+						ContainerID: "0ea7476f26d192f067abdc8b3df43ce3cdbe324386e1c010cb48de87eefef480",
+						NICType:     "",
+					},
+				}
+				Expect(validateEndpoints(eps)).ToNot(BeNil())
+			})
+		})
+		Context("When no container id ib nic", func() {
+			It("Should error", func() {
+				eps := []*endpoint{
+					{
+						ContainerID: "",
+						NICType:     cns.NodeNetworkInterfaceBackendNIC,
+					},
+				}
+				Expect(validateEndpoints(eps)).ToNot(BeNil())
+			})
+		})
+	})
 })

network/endpoint_windows_test.go

@@ -35,9 +35,7 @@ func TestNewAndDeleteEndpointImplHnsV2(t *testing.T) {
 
 	// this hnsv2 variable overwrites the package level variable in network
 	// we do this to avoid passing around os specific objects in platform agnostic code
-	Hnsv2 = hnswrapper.Hnsv2wrapperwithtimeout{
-		Hnsv2: hnswrapper.NewHnsv2wrapperFake(),
-	}
+	Hnsv2 = hnswrapper.NewHnsv2wrapperFake()
 
 	epInfo := &EndpointInfo{
 		EndpointID:  "753d3fb6-e9b3-49e2-a109-2acc5dda61f1",
@@ -50,15 +48,35 @@ func TestNewAndDeleteEndpointImplHnsV2(t *testing.T) {
 			Servers: []string{"10.0.0.1, 10.0.0.2"},
 			Options: nil,
 		},
-		MacAddress: net.HardwareAddr("00:00:5e:00:53:01"),
+		MacAddress:   net.HardwareAddr("00:00:5e:00:53:01"),
+		NICType:      cns.InfraNIC,
+		HNSNetworkID: "853d3fb6-e9b3-49e2-a109-2acc5dda61f1",
 	}
-	endpoint, err := nw.newEndpointImplHnsV2(nil, epInfo)
+	ep, err := nw.newEndpointImplHnsV2(nil, epInfo)
 	if err != nil {
 		fmt.Printf("+%v", err)
 		t.Fatal(err)
 	}
 
-	err = nw.deleteEndpointImplHnsV2(endpoint)
+	if err = validateEndpoints([]*endpoint{ep}); err != nil {
+		fmt.Printf("+%v", err)
+		t.Fatal(err)
+	}
+
+	if epInfo.HNSEndpointID == "" {
+		t.Fatal("hns endpoint id not populated inside endpoint info during new endpoint impl call")
+	}
+
+	if ep.HnsId == "" {
+		t.Fatal("hns endpoint id not populated inside endpoint struct during new endpoint impl call")
+	}
+
+	if ep.HNSNetworkID == "" {
+		t.Fatal("hns network id was not copied to the endpoint struct during new endpoint impl call")
+	}
+
+	err = nw.deleteEndpointImplHnsV2(ep)
+
 	if err != nil {
 		fmt.Printf("+%v", err)
 		t.Fatal(err)

network/hnswrapper/hnsv2wrapperfake.go

@@ -50,6 +50,9 @@ func (f Hnsv2wrapperFake) CreateNetwork(network *hcn.HostComputeNetwork) (*hcn.H
 	defer f.Unlock()
 
 	delayHnsCall(f.Delay)
+	if network.Id == "" {
+		network.Id = network.Name // simulate hns creating the network and generating an hns network id
+	}
 	f.Cache.networks[network.Name] = NewFakeHostComputeNetwork(network)
 	return network, nil
 }
@@ -216,6 +219,9 @@ func (f Hnsv2wrapperFake) CreateEndpoint(endpoint *hcn.HostComputeEndpoint) (*hc
 	f.Lock()
 	defer f.Unlock()
 	delayHnsCall(f.Delay)
+	if endpoint.Id == "" {
+		endpoint.Id = endpoint.Name // simulate hns creating the endpoint and generating an hns endpoint id
+	}
 	f.Cache.endpoints[endpoint.Id] = NewFakeHostComputeEndpoint(endpoint)
 	return endpoint, nil
 }

network/manager.go

@@ -423,15 +423,30 @@ func (nm *networkManager) UpdateEndpointState(eps []*endpoint) error {
 		logger.Info("Update endpoint state", zap.String("hnsEndpointID", ipinfo.HnsEndpointID), zap.String("hnsNetworkID", ipinfo.HnsNetworkID),
 			zap.String("hostVethName", ipinfo.HostVethName), zap.String("macAddress", ipinfo.MacAddress), zap.String("nicType", string(ipinfo.NICType)))
 	}
-	// logger.Info("Calling cns updateEndpoint API with ", zap.String("containerID: ", ep.ContainerID), zap.String("HnsId: ", ep.HnsId), zap.String("HostIfName: ", ep.HostIfName))
+
 	// we assume all endpoints have the same container id
-	response, err := nm.CnsClient.UpdateEndpoint(context.TODO(), eps[0].ContainerID, ifnameToIPInfoMap)
+	cnsEndpointID := eps[0].ContainerID
+	if err := validateUpdateEndpointState(cnsEndpointID, ifnameToIPInfoMap); err != nil {
+		return errors.Wrap(err, "failed to validate update endpoint state that will be sent to cns")
+	}
+	response, err := nm.CnsClient.UpdateEndpoint(context.TODO(), cnsEndpointID, ifnameToIPInfoMap)
 	if err != nil {
 		return errors.Wrapf(err, "Update endpoint API returend with error")
 	}
 	logger.Info("Update endpoint API returend ", zap.String("podname: ", response.ReturnCode.String()))
 	return nil
 }
+func validateUpdateEndpointState(endpointID string, ifNameToIPInfoMap map[string]*restserver.IPInfo) error {
+	if endpointID == "" {
+		return errors.New("endpoint id empty while validating update endpoint state")
+	}
+	for ifName := range ifNameToIPInfoMap {
+		if ifName == "" {
+			return errors.New("an interface name is empty while validating update endpoint state")
+		}
+	}
+	return nil
+}
 
 // GetEndpointState will make a call to CNS GetEndpointState API in the stateless CNI mode to fetch the endpointInfo
 // TODO unit tests need to be added, WorkItem: 26606939

network/manager_test.go

@@ -229,6 +229,44 @@ var _ = Describe("Test Manager", func() {
 				Expect(num).To(Equal(3))
 			})
 		})
+
+		Context("When different fields passed to update endpoint state", func() {
+			It("Should error or validate correctly", func() {
+				nm := &networkManager{}
+
+				err := nm.UpdateEndpointState([]*endpoint{
+					{
+						IfName:      "eth0",
+						ContainerID: "2bfc3b23e078f0bea48612d5d081ace587599cdac026d23e4d57bd03c85d357c",
+					},
+					{
+						IfName:      "",
+						ContainerID: "2bfc3b23e078f0bea48612d5d081ace587599cdac026d23e4d57bd03c85d357c",
+					},
+				})
+				Expect(err).To(HaveOccurred())
+
+				err = nm.UpdateEndpointState([]*endpoint{
+					{
+						IfName:      "eth1",
+						ContainerID: "",
+					},
+					{
+						IfName:      "eth0",
+						ContainerID: "",
+					},
+				})
+				Expect(err).To(HaveOccurred())
+
+				err = validateUpdateEndpointState(
+					"2bfc3b23e078f0bea48612d5d081ace587599cdac026d23e4d57bd03c85d357c",
+					map[string]*restserver.IPInfo{
+						"eth1": {},
+						"eth2": {},
+					})
+				Expect(err).To(BeNil())
+			})
+		})
 	})
 	Describe("Test EndpointCreate", func() {
 		Context("When no endpoints provided", func() {

network/network.go

@@ -346,6 +346,10 @@ func (nm *networkManager) EndpointCreate(cnsclient apipaClient, epInfos []*Endpo
 		eps = append(eps, ep)
 	}
 
+	if err := validateEndpoints(eps); err != nil {
+		return err
+	}
+
 	// save endpoints
 	return nm.SaveState(eps)
 }

network/network_test.go

@@ -197,7 +197,7 @@ var _ = Describe("Test Network", func() {
 					Mode:         opModeTransparentVlan,
 				}
 				nw, err := nm.newNetwork(nwInfo)
-				Expect(err).To(MatchError(platform.ErrMockExec))
+				Expect(err).NotTo(BeNil())
 				Expect(nw).To(BeNil())
 			})
 		})

network/network_windows_test.go

@@ -52,6 +52,13 @@ func TestNewAndDeleteNetworkImplHnsV2(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	if network.HnsId == "" {
+		t.Fatal("hns network id not populated in network struct")
+	}
+	if nwInfo.HNSNetworkID == "" {
+		t.Fatal("hns network id not populated")
+	}
+
 	err = nm.deleteNetworkImplHnsV2(network)
 
 	if err != nil {