From fe5af9e8a66585a401cc48e78992595405818bfc Mon Sep 17 00:00:00 2001 From: rjdenney Date: Wed, 12 Apr 2023 13:54:51 -0400 Subject: [PATCH 1/3] Update for previous comments --- cns/client/client.go | 6 +++--- cns/restserver/ipam.go | 42 ++++++++++++++++++------------------------ 2 files changed, 21 insertions(+), 27 deletions(-) diff --git a/cns/client/client.go b/cns/client/client.go index b01cac99e7..82e9e33318 100644 --- a/cns/client/client.go +++ b/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: %w", 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: %w", ErrAPINotFound, err) //nolint:errorlint // multiple %w not supported in 1.19 } if err != nil { diff --git a/cns/restserver/ipam.go b/cns/restserver/ipam.go index 8b68b8030b..107e465e98 100644 --- a/cns/restserver/ipam.go +++ b/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{ + 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, } } @@ -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 From 28de34d06660d17cc592796eb039358357d59592 Mon Sep 17 00:00:00 2001 From: rjdenney Date: Wed, 12 Apr 2023 18:23:03 -0400 Subject: [PATCH 2/3] adding tests --- cns/restserver/ipam_test.go | 71 +++++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/cns/restserver/ipam_test.go b/cns/restserver/ipam_test.go index d825fe1407..6fa3c6f49a 100644 --- a/cns/restserver/ipam_test.go +++ b/cns/restserver/ipam_test.go @@ -1425,3 +1425,74 @@ 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") + } +} From 416c514007711941d127326f9d5cbab93a1283e4 Mon Sep 17 00:00:00 2001 From: rjdenney Date: Wed, 12 Apr 2023 19:33:56 -0400 Subject: [PATCH 3/3] fix linter issues --- cns/client/client.go | 4 ++-- cns/restserver/ipam.go | 4 ++-- cns/restserver/ipam_test.go | 1 - 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/cns/client/client.go b/cns/client/client.go index 82e9e33318..f7e41fba23 100644 --- a/cns/client/client.go +++ b/cns/client/client.go @@ -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: %w", 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: %w", 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 { diff --git a/cns/restserver/ipam.go b/cns/restserver/ipam.go index 107e465e98..b5a091d4cb 100644 --- a/cns/restserver/ipam.go +++ b/cns/restserver/ipam.go @@ -109,10 +109,10 @@ func (service *HTTPRestService) requestIPConfigHandler(w http.ResponseWriter, r InfraContainerID: ipconfigRequest.InfraContainerID, OrchestratorContext: ipconfigRequest.OrchestratorContext, Ifname: ipconfigRequest.Ifname, - } + } if ipconfigRequest.DesiredIPAddress != "" { ipconfigsRequest.DesiredIPAddresses = []string{ - ipconfigRequest.DesiredIPAddress, + ipconfigRequest.DesiredIPAddress, } } diff --git a/cns/restserver/ipam_test.go b/cns/restserver/ipam_test.go index 6fa3c6f49a..d35b7d3b56 100644 --- a/cns/restserver/ipam_test.go +++ b/cns/restserver/ipam_test.go @@ -1454,7 +1454,6 @@ func TestIPAMFailToReleasePartialIPsInPool(t *testing.T) { 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) {