CNS ipam and client changes to address comments

cns/client/client.go

@@ -47,7 +47,7 @@ var clientPaths = []string{
 	cns.GetHomeAz,
 }
 
-var errAPINotFound error = errors.New("api not found")
+var ErrAPINotFound error = errors.New("api not found")
 
 type do interface {
 	Do(*http.Request) (*http.Response, error)
@@ -405,7 +405,7 @@ func (c *Client) RequestIPs(ctx context.Context, ipconfig cns.IPConfigsRequest)
 
 	// if we get a 404 error
 	if res.StatusCode == http.StatusNotFound {
-		return nil, fmt.Errorf("cannot find API RequestIPs %w: %v", errAPINotFound, err) //nolint:errorlint // multiple %w not supported in 1.19
+		return nil, fmt.Errorf("cannot find API RequestIPs %w: %v", ErrAPINotFound, err) //nolint:errorlint // multiple %w not supported in 1.19
 	}
 
 	if err != nil {
@@ -448,7 +448,7 @@ func (c *Client) ReleaseIPs(ctx context.Context, ipconfig cns.IPConfigsRequest)
 
 	// if we get a 404 error
 	if res.StatusCode == http.StatusNotFound {
-		return fmt.Errorf("cannot find API ReleaseIPs %w: %v", errAPINotFound, err) //nolint:errorlint // multiple %w not supported in 1.19
+		return fmt.Errorf("cannot find API ReleaseIPs %w: %v", ErrAPINotFound, err) //nolint:errorlint // multiple %w not supported in 1.19
 	}
 
 	if err != nil {

cns/restserver/ipam.go

@@ -18,8 +18,8 @@ import (
 )
 
 var (
-	errStoreEmpty       = errors.New("empty endpoint state store")
-	errParsePodIPFailed = errors.New("failed to parse pod's ip")
+	ErrStoreEmpty       = errors.New("empty endpoint state store")
+	ErrParsePodIPFailed = errors.New("failed to parse pod's ip")
 )
 
 // requestIPConfigHandlerHelper validates the request, assigns IPs, and returns a response
@@ -103,24 +103,16 @@ func (service *HTTPRestService) requestIPConfigHandler(w http.ResponseWriter, r
 		return
 	}
 
-	var ipconfigsRequest cns.IPConfigsRequest
 	// doesn't fill in DesiredIPAddresses if it is empty in the original request
+	ipconfigsRequest := cns.IPConfigsRequest{
+		PodInterfaceID:      ipconfigRequest.PodInterfaceID,
+		InfraContainerID:    ipconfigRequest.InfraContainerID,
+		OrchestratorContext: ipconfigRequest.OrchestratorContext,
+		Ifname:              ipconfigRequest.Ifname,
+	}
 	if ipconfigRequest.DesiredIPAddress != "" {
-		ipconfigsRequest = cns.IPConfigsRequest{
-			DesiredIPAddresses: []string{
-				ipconfigRequest.DesiredIPAddress,
-			},
-			PodInterfaceID:      ipconfigRequest.PodInterfaceID,
-			InfraContainerID:    ipconfigRequest.InfraContainerID,
-			OrchestratorContext: ipconfigRequest.OrchestratorContext,
-			Ifname:              ipconfigRequest.Ifname,
-		}
-	} else {
-		ipconfigsRequest = cns.IPConfigsRequest{
-			PodInterfaceID:      ipconfigRequest.PodInterfaceID,
-			InfraContainerID:    ipconfigRequest.InfraContainerID,
-			OrchestratorContext: ipconfigRequest.OrchestratorContext,
-			Ifname:              ipconfigRequest.Ifname,
+		ipconfigsRequest.DesiredIPAddresses = []string{
+			ipconfigRequest.DesiredIPAddress,
 		}
 	}
 
@@ -171,7 +163,7 @@ func (service *HTTPRestService) requestIPConfigsHandler(w http.ResponseWriter, r
 
 func (service *HTTPRestService) updateEndpointState(ipconfigsRequest cns.IPConfigsRequest, podInfo cns.PodInfo, podIPInfo []cns.PodIpInfo) error {
 	if service.EndpointStateStore == nil {
-		return errStoreEmpty
+		return ErrStoreEmpty
 	}
 	service.Lock()
 	defer service.Unlock()
@@ -182,7 +174,7 @@ func (service *HTTPRestService) updateEndpointState(ipconfigsRequest cns.IPConfi
 			ip := net.ParseIP(podIPInfo[i].PodIPConfig.IPAddress)
 			if ip == nil {
 				logger.Errorf("failed to parse pod ip address %s", podIPInfo[i].PodIPConfig.IPAddress)
-				return errParsePodIPFailed
+				return ErrParsePodIPFailed
 			}
 			if ip.To4() == nil { // is an ipv6 address
 				ipconfig := net.IPNet{IP: ip, Mask: net.CIDRMask(int(podIPInfo[i].PodIPConfig.PrefixLength), 128)} // nolint
@@ -209,7 +201,7 @@ func (service *HTTPRestService) updateEndpointState(ipconfigsRequest cns.IPConfi
 			ip := net.ParseIP(podIPInfo[i].PodIPConfig.IPAddress)
 			if ip == nil {
 				logger.Errorf("failed to parse pod ip address %s", podIPInfo[i].PodIPConfig.IPAddress)
-				return errParsePodIPFailed
+				return ErrParsePodIPFailed
 			}
 			ipInfo := &IPInfo{}
 			if ip.To4() == nil { // is an ipv6 address
@@ -348,7 +340,7 @@ func (service *HTTPRestService) releaseIPConfigsHandler(w http.ResponseWriter, r
 
 func (service *HTTPRestService) removeEndpointState(podInfo cns.PodInfo) error {
 	if service.EndpointStateStore == nil {
-		return errStoreEmpty
+		return ErrStoreEmpty
 	}
 	service.Lock()
 	defer service.Unlock()
@@ -775,9 +767,11 @@ func (service *HTTPRestService) AssignAvailableIPConfigs(podInfo cns.PodInfo) ([
 	// Searches for available IPs in the pool
 	for _, ipState := range service.PodIPConfigState {
 		// check if an IP from this NC is already set side for assignment.
-		_, ncAlreadyMarkedForAssignment := ipsToAssign[ipState.NCID]
-		// Checks if we haven't already found an IP from that NC and checks if the current IP is available
-		if ncAlreadyMarkedForAssignment || ipState.GetState() != types.Available {
+		if _, ncAlreadyMarkedForAssignment := ipsToAssign[ipState.NCID]; ncAlreadyMarkedForAssignment {
+			continue
+		}
+		// Checks if the current IP is available
+		if ipState.GetState() != types.Available {
 			continue
 		}
 		ipsToAssign[ipState.NCID] = ipState

cns/restserver/ipam_test.go

@@ -1425,3 +1425,73 @@ func TestIPAMFailToRequestOneIPWhenExpectedToHaveTwo(t *testing.T) {
 		t.Fatal("Expected available ips to be one since we expect the IP to not be assigned")
 	}
 }
+
+func TestIPAMFailToReleasePartialIPsInPool(t *testing.T) {
+	svc := getTestService()
+
+	// set state as already assigned
+	testState, _ := NewPodStateWithOrchestratorContext(testIP1, testIPID1, testNCID, types.Assigned, 24, 0, testPod1Info)
+	ipconfigs := map[string]cns.IPConfigurationStatus{
+		testState.ID: testState,
+	}
+	testStatev6, _ := NewPodStateWithOrchestratorContext(testIP1v6, testIPID1v6, testNCIDv6, types.Assigned, 120, 0, testPod1Info)
+	ipconfigsv6 := map[string]cns.IPConfigurationStatus{
+		testStatev6.ID: testStatev6,
+	}
+
+	err := UpdatePodIPConfigState(t, svc, ipconfigs, testNCID)
+	if err != nil {
+		t.Fatalf("Expected to not fail adding IPs to state: %+v", err)
+	}
+	err = UpdatePodIPConfigState(t, svc, ipconfigsv6, testNCIDv6)
+	if err != nil {
+		t.Fatalf("Expected to not fail adding empty NC to state: %+v", err)
+	}
+	// remove the IP from the from the ipconfig map so that it throws an error when trying to release one of the IPs
+	delete(svc.PodIPConfigState, testStatev6.ID)
+
+	err = svc.releaseIPConfigs(testPod1Info)
+	if err == nil {
+		t.Fatalf("Expected fail releasing IP due to only having one in the ipconfig map, IPs will be reassigned back to the pod")
+	}
+}
+
+func TestIPAMFailToRequestPartialIPsInPool(t *testing.T) {
+	svc := getTestService()
+
+	// set state as already assigned
+	testState := NewPodState(testIP1, testIPID1, testNCID, types.Available, 0)
+	ipconfigs := map[string]cns.IPConfigurationStatus{
+		testState.ID: testState,
+	}
+	testStatev6 := NewPodState(testIP1v6, testIPID1v6, testNCIDv6, types.Available, 0)
+	ipconfigsv6 := map[string]cns.IPConfigurationStatus{
+		testStatev6.ID: testStatev6,
+	}
+
+	err := UpdatePodIPConfigState(t, svc, ipconfigs, testNCID)
+	if err != nil {
+		t.Fatalf("Expected to not fail adding IPs to state: %+v", err)
+	}
+	err = UpdatePodIPConfigState(t, svc, ipconfigsv6, testNCIDv6)
+	if err != nil {
+		t.Fatalf("Expected to not fail adding empty NC to state: %+v", err)
+	}
+	// remove the IP from the from the ipconfig map so that it throws an error when trying to release one of the IPs
+	delete(svc.PodIPConfigState, testStatev6.ID)
+
+	req := cns.IPConfigsRequest{
+		PodInterfaceID:   testPod1Info.InterfaceID(),
+		InfraContainerID: testPod1Info.InfraContainerID(),
+	}
+	b, _ := testPod1Info.OrchestratorContext()
+	req.OrchestratorContext = b
+	req.DesiredIPAddresses = make([]string, 2)
+	req.DesiredIPAddresses[0] = testIP1
+	req.DesiredIPAddresses[1] = testIP1v6
+
+	_, err = requestIPAddressAndGetState(t, req)
+	if err == nil {
+		t.Fatalf("Expected fail requesting IPs due to only having one in the ipconfig map, IPs in the pool will not be assigned")
+	}
+}