From 0dac414fa42453808d48907bce6bc4c66e149410 Mon Sep 17 00:00:00 2001 From: rjdenney Date: Tue, 18 Jul 2023 12:10:51 -0400 Subject: [PATCH 1/4] adding fix for requesting IPs with 0 NCs --- cns/restserver/ipam.go | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/cns/restserver/ipam.go b/cns/restserver/ipam.go index af1f9e7057..5773525e5e 100644 --- a/cns/restserver/ipam.go +++ b/cns/restserver/ipam.go @@ -88,8 +88,8 @@ func (service *HTTPRestService) requestIPConfigHandler(w http.ResponseWriter, r return } - // This method can only return 1 IP. If we have more than one NC then we expect to need to return one IP per NC - if len(service.state.ContainerStatus) > 1 { + // This method can only return EXACTLY 1 IP. If we have more than one NC then we expect to need to return one IP per NC + if len(service.state.ContainerStatus) != 1 { // we send a response back saying that this API won't be able to return the amount of IPs needed to fulfill the request reserveResp := &cns.IPConfigResponse{ Response: cns.Response{ @@ -127,7 +127,20 @@ func (service *HTTPRestService) requestIPConfigHandler(w http.ResponseWriter, r logger.ResponseEx(service.Name+operationName, ipconfigsRequest, reserveResp, reserveResp.Response.ReturnCode, err) return } - + // Checks to make sure we return exactly 1 IP + if len(ipConfigsResp.PodIPInfo) != 1 { + // we send a response back saying that this API won't be able to return the amount of IPs needed to fulfill the request + reserveResp := &cns.IPConfigResponse{ + Response: cns.Response{ + ReturnCode: types.UnexpectedError, + Message: fmt.Sprintf("request returned incorrect number of IPs. Expected %d and returned %d", len(service.state.ContainerStatus), len(ipConfigsResp.PodIPInfo)), + }, + } + w.Header().Set(cnsReturnCode, reserveResp.Response.ReturnCode.String()) + err = service.Listener.Encode(w, &reserveResp) + logger.ResponseEx(service.Name+operationName, ipconfigRequest, reserveResp, reserveResp.Response.ReturnCode, err) + return + } // As this API is expected to return IPConfigResponse, generate it from the IPConfigsResponse returned above. reserveResp := &cns.IPConfigResponse{ Response: ipConfigsResp.Response, @@ -661,8 +674,15 @@ func (service *HTTPRestService) GetExistingIPConfig(podInfo cns.PodInfo) ([]cns. // Assigns a pod with all IPs desired func (service *HTTPRestService) AssignDesiredIPConfigs(podInfo cns.PodInfo, desiredIPAddresses []string) ([]cns.PodIpInfo, error) { + // Sets the number of desired IPs equal to the number of NCs so that we can get one IP per NC numDesiredIPAddresses := len(desiredIPAddresses) + // Creates a slice of PodIpInfo with the size as number of NCs to hold the result for assigned IP configs podIPInfo := make([]cns.PodIpInfo, numDesiredIPAddresses) + // if there are no NCs on the NNC there will be no IPs in the pool so return error + if numDesiredIPAddresses == 0 { + //nolint:goerr113 // return error + return podIPInfo, fmt.Errorf("no NCs found on the NNC so no IPs are in the pool") + } // creating a map for the loop to check to see if the IP in the pool is one of the desired IPs desiredIPMap := make(map[string]struct{}) // slice to keep track of IP configs to assign @@ -762,6 +782,11 @@ func (service *HTTPRestService) AssignAvailableIPConfigs(podInfo cns.PodInfo) ([ numIPsNeeded := len(service.state.ContainerStatus) // Creates a slice of PodIpInfo with the size as number of NCs to hold the result for assigned IP configs podIPInfo := make([]cns.PodIpInfo, numIPsNeeded) + // if there are no NCs on the NNC there will be no IPs in the pool so return error + if numIPsNeeded == 0 { + //nolint:goerr113 // return error + return podIPInfo, fmt.Errorf("No NCs found on the NNC so no IPs are in the pool") + } // This map is used to store whether or not we have found an available IP from an NC when looping through the pool ipsToAssign := make(map[string]cns.IPConfigurationStatus) From 3650c4f3687f854ed059ede84400693e53a17be4 Mon Sep 17 00:00:00 2001 From: ryandenney Date: Wed, 19 Jul 2023 13:44:14 -0400 Subject: [PATCH 2/4] addressing comments --- cns/restserver/ipam.go | 35 +++++++++++++++++++---------------- cns/restserver/ipam_test.go | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 16 deletions(-) diff --git a/cns/restserver/ipam.go b/cns/restserver/ipam.go index 5773525e5e..4dbcb21d23 100644 --- a/cns/restserver/ipam.go +++ b/cns/restserver/ipam.go @@ -20,6 +20,7 @@ import ( var ( ErrStoreEmpty = errors.New("empty endpoint state store") ErrParsePodIPFailed = errors.New("failed to parse pod's ip") + ErrNoPoolIPs = errors.New("no NCs found on the NNC so no IPs are in the pool") ) // requestIPConfigHandlerHelper validates the request, assigns IPs, and returns a response @@ -127,7 +128,9 @@ func (service *HTTPRestService) requestIPConfigHandler(w http.ResponseWriter, r logger.ResponseEx(service.Name+operationName, ipconfigsRequest, reserveResp, reserveResp.Response.ReturnCode, err) return } + // Checks to make sure we return exactly 1 IP + // If IPAM assigned more than 1 IP then we need to raise an error since this API can only return one IP and IPAM may have assigned more than one if len(ipConfigsResp.PodIPInfo) != 1 { // we send a response back saying that this API won't be able to return the amount of IPs needed to fulfill the request reserveResp := &cns.IPConfigResponse{ @@ -674,15 +677,16 @@ func (service *HTTPRestService) GetExistingIPConfig(podInfo cns.PodInfo) ([]cns. // Assigns a pod with all IPs desired func (service *HTTPRestService) AssignDesiredIPConfigs(podInfo cns.PodInfo, desiredIPAddresses []string) ([]cns.PodIpInfo, error) { - // Sets the number of desired IPs equal to the number of NCs so that we can get one IP per NC + // gets the number of NCs + numOfNCs := len(service.state.ContainerStatus) + // Check to make sure that the number + if numOfNCs == 0 { + return nil, ErrNoPoolIPs + } + // Sets the number of desired IPs equal to the number of desired IPs passed in numDesiredIPAddresses := len(desiredIPAddresses) // Creates a slice of PodIpInfo with the size as number of NCs to hold the result for assigned IP configs podIPInfo := make([]cns.PodIpInfo, numDesiredIPAddresses) - // if there are no NCs on the NNC there will be no IPs in the pool so return error - if numDesiredIPAddresses == 0 { - //nolint:goerr113 // return error - return podIPInfo, fmt.Errorf("no NCs found on the NNC so no IPs are in the pool") - } // creating a map for the loop to check to see if the IP in the pool is one of the desired IPs desiredIPMap := make(map[string]struct{}) // slice to keep track of IP configs to assign @@ -776,17 +780,16 @@ func (service *HTTPRestService) AssignDesiredIPConfigs(podInfo cns.PodInfo, desi // Assigns an available IP from each NC on the NNC. If there is one NC then we expect to only have one IP return // In the case of dualstack we would expect to have one IPv6 from one NC and one IPv4 from a second NC func (service *HTTPRestService) AssignAvailableIPConfigs(podInfo cns.PodInfo) ([]cns.PodIpInfo, error) { - service.Lock() - defer service.Unlock() // Sets the number of IPs needed equal to the number of NCs so that we can get one IP per NC - numIPsNeeded := len(service.state.ContainerStatus) - // Creates a slice of PodIpInfo with the size as number of NCs to hold the result for assigned IP configs - podIPInfo := make([]cns.PodIpInfo, numIPsNeeded) + numOfNCs := len(service.state.ContainerStatus) // if there are no NCs on the NNC there will be no IPs in the pool so return error - if numIPsNeeded == 0 { - //nolint:goerr113 // return error - return podIPInfo, fmt.Errorf("No NCs found on the NNC so no IPs are in the pool") + if numOfNCs == 0 { + return nil, ErrNoPoolIPs } + service.Lock() + defer service.Unlock() + // Creates a slice of PodIpInfo with the size as number of NCs to hold the result for assigned IP configs + podIPInfo := make([]cns.PodIpInfo, numOfNCs) // This map is used to store whether or not we have found an available IP from an NC when looping through the pool ipsToAssign := make(map[string]cns.IPConfigurationStatus) @@ -802,13 +805,13 @@ func (service *HTTPRestService) AssignAvailableIPConfigs(podInfo cns.PodInfo) ([ } ipsToAssign[ipState.NCID] = ipState // Once one IP per container is found break out of the loop and stop searching - if len(ipsToAssign) == numIPsNeeded { + if len(ipsToAssign) == numOfNCs { break } } // Checks to make sure we found one IP for each NC - if len(ipsToAssign) != numIPsNeeded { + if len(ipsToAssign) != numOfNCs { //nolint:goerr113 // return error return podIPInfo, fmt.Errorf("not enough IPs available, waiting on Azure CNS to allocate more") } diff --git a/cns/restserver/ipam_test.go b/cns/restserver/ipam_test.go index d35b7d3b56..f74ca0ef38 100644 --- a/cns/restserver/ipam_test.go +++ b/cns/restserver/ipam_test.go @@ -1360,6 +1360,38 @@ func IPAMMarkExistingIPConfigAsPending(t *testing.T, ncStates []ncState) { } } +func TestIPAMFailToRequestIPsWithNoNCsSpecificIP(t *testing.T) { + svc := getTestService() + req := cns.IPConfigsRequest{ + PodInterfaceID: testPod1Info.InterfaceID(), + InfraContainerID: testPod1Info.InfraContainerID(), + } + b, _ := testPod1Info.OrchestratorContext() + req.OrchestratorContext = b + req.DesiredIPAddresses = make([]string, 1) + req.DesiredIPAddresses[0] = testIP1 + + _, err := requestIPConfigsHelper(svc, req) + if err == nil { + t.Fatalf("Expected error. Should not be able to request IPs when there are no NCs") + } +} + +func TestIPAMFailToRequestIPsWithNoNCsAnyIP(t *testing.T) { + svc := getTestService() + req := cns.IPConfigsRequest{ + PodInterfaceID: testPod1Info.InterfaceID(), + InfraContainerID: testPod1Info.InfraContainerID(), + } + b, _ := testPod1Info.OrchestratorContext() + req.OrchestratorContext = b + + _, err := requestIPConfigsHelper(svc, req) + if err == nil { + t.Fatalf("Expected error. Should not be able to request IPs when there are no NCs") + } +} + func TestIPAMReleaseOneIPWhenExpectedToHaveTwo(t *testing.T) { svc := getTestService() From e1c3dd3f418ba9866eb6fcf53f7132b9f5444020 Mon Sep 17 00:00:00 2001 From: ryandenney Date: Wed, 19 Jul 2023 14:01:45 -0400 Subject: [PATCH 3/4] error change --- cns/restserver/ipam.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cns/restserver/ipam.go b/cns/restserver/ipam.go index 4dbcb21d23..8dd329ea34 100644 --- a/cns/restserver/ipam.go +++ b/cns/restserver/ipam.go @@ -20,7 +20,7 @@ import ( var ( ErrStoreEmpty = errors.New("empty endpoint state store") ErrParsePodIPFailed = errors.New("failed to parse pod's ip") - ErrNoPoolIPs = errors.New("no NCs found on the NNC so no IPs are in the pool") + ErrNoNCs = errors.New("No NCs found in the CNS internal state") ) // requestIPConfigHandlerHelper validates the request, assigns IPs, and returns a response @@ -681,7 +681,7 @@ func (service *HTTPRestService) AssignDesiredIPConfigs(podInfo cns.PodInfo, desi numOfNCs := len(service.state.ContainerStatus) // Check to make sure that the number if numOfNCs == 0 { - return nil, ErrNoPoolIPs + return nil, ErrNoNCs } // Sets the number of desired IPs equal to the number of desired IPs passed in numDesiredIPAddresses := len(desiredIPAddresses) @@ -784,7 +784,7 @@ func (service *HTTPRestService) AssignAvailableIPConfigs(podInfo cns.PodInfo) ([ numOfNCs := len(service.state.ContainerStatus) // if there are no NCs on the NNC there will be no IPs in the pool so return error if numOfNCs == 0 { - return nil, ErrNoPoolIPs + return nil, ErrNoNCs } service.Lock() defer service.Unlock() From 468605afd54467b99f7b281b5a694491f55938cf Mon Sep 17 00:00:00 2001 From: ryandenney Date: Wed, 19 Jul 2023 16:34:34 -0400 Subject: [PATCH 4/4] fixing comments --- cns/restserver/ipam.go | 16 ++++++++-------- cns/restserver/ipam_test.go | 2 ++ 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/cns/restserver/ipam.go b/cns/restserver/ipam.go index 8dd329ea34..e89da0bf7e 100644 --- a/cns/restserver/ipam.go +++ b/cns/restserver/ipam.go @@ -95,7 +95,7 @@ func (service *HTTPRestService) requestIPConfigHandler(w http.ResponseWriter, r reserveResp := &cns.IPConfigResponse{ Response: cns.Response{ ReturnCode: types.InvalidRequest, - Message: fmt.Sprintf("Called API that can only return 1 IP when expecting %d", len(service.state.ContainerStatus)), + Message: fmt.Sprintf("Expected 1 NC when calling this API but found %d NCs", len(service.state.ContainerStatus)), }, } w.Header().Set(cnsReturnCode, reserveResp.Response.ReturnCode.String()) @@ -136,7 +136,7 @@ func (service *HTTPRestService) requestIPConfigHandler(w http.ResponseWriter, r reserveResp := &cns.IPConfigResponse{ Response: cns.Response{ ReturnCode: types.UnexpectedError, - Message: fmt.Sprintf("request returned incorrect number of IPs. Expected %d and returned %d", len(service.state.ContainerStatus), len(ipConfigsResp.PodIPInfo)), + Message: fmt.Sprintf("request returned incorrect number of IPs. Expected 1 and returned %d", len(ipConfigsResp.PodIPInfo)), }, } w.Header().Set(cnsReturnCode, reserveResp.Response.ReturnCode.String()) @@ -289,12 +289,12 @@ func (service *HTTPRestService) releaseIPConfigHandler(w http.ResponseWriter, r return } - // check to make sure there aren't multiple NCs - if len(service.state.ContainerStatus) > 1 { + // check to make sure there is only one NC + if len(service.state.ContainerStatus) != 1 { reserveResp := &cns.IPConfigResponse{ Response: cns.Response{ ReturnCode: types.InvalidRequest, - Message: fmt.Sprintf("Called API that can only return 1 IP when expecting %d", len(service.state.ContainerStatus)), + Message: fmt.Sprintf("Expected 1 NC when calling this API but found %d NCs", len(service.state.ContainerStatus)), }, } w.Header().Set(cnsReturnCode, reserveResp.Response.ReturnCode.String()) @@ -677,9 +677,9 @@ func (service *HTTPRestService) GetExistingIPConfig(podInfo cns.PodInfo) ([]cns. // Assigns a pod with all IPs desired func (service *HTTPRestService) AssignDesiredIPConfigs(podInfo cns.PodInfo, desiredIPAddresses []string) ([]cns.PodIpInfo, error) { - // gets the number of NCs + // Gets the number of NCs which will determine the number of IPs given to a pod numOfNCs := len(service.state.ContainerStatus) - // Check to make sure that the number + // checks to make sure we have NCs before trying to get IPs if numOfNCs == 0 { return nil, ErrNoNCs } @@ -780,7 +780,7 @@ func (service *HTTPRestService) AssignDesiredIPConfigs(podInfo cns.PodInfo, desi // Assigns an available IP from each NC on the NNC. If there is one NC then we expect to only have one IP return // In the case of dualstack we would expect to have one IPv6 from one NC and one IPv4 from a second NC func (service *HTTPRestService) AssignAvailableIPConfigs(podInfo cns.PodInfo) ([]cns.PodIpInfo, error) { - // Sets the number of IPs needed equal to the number of NCs so that we can get one IP per NC + // Gets the number of NCs which will determine the number of IPs given to a pod numOfNCs := len(service.state.ContainerStatus) // if there are no NCs on the NNC there will be no IPs in the pool so return error if numOfNCs == 0 { diff --git a/cns/restserver/ipam_test.go b/cns/restserver/ipam_test.go index f74ca0ef38..b552b15774 100644 --- a/cns/restserver/ipam_test.go +++ b/cns/restserver/ipam_test.go @@ -1375,6 +1375,7 @@ func TestIPAMFailToRequestIPsWithNoNCsSpecificIP(t *testing.T) { if err == nil { t.Fatalf("Expected error. Should not be able to request IPs when there are no NCs") } + assert.ErrorIs(t, err, ErrNoNCs) } func TestIPAMFailToRequestIPsWithNoNCsAnyIP(t *testing.T) { @@ -1390,6 +1391,7 @@ func TestIPAMFailToRequestIPsWithNoNCsAnyIP(t *testing.T) { if err == nil { t.Fatalf("Expected error. Should not be able to request IPs when there are no NCs") } + assert.ErrorIs(t, err, ErrNoNCs) } func TestIPAMReleaseOneIPWhenExpectedToHaveTwo(t *testing.T) {