Skip to content

Commit cf5d99b

Browse files
rjdenneyrjdenney
authored
CNS ipam and client changes to address comments (#1913)
* Update for previous comments * adding tests * fix linter issues
1 parent d9104f0 commit cf5d99b

File tree

3 files changed

+92
-28
lines changed

3 files changed

+92
-28
lines changed

‎cns/client/client.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ var clientPaths = []string{
4747
cns.GetHomeAz,
4848
}
4949

50-
var errAPINotFound error = errors.New("api not found")
50+
var ErrAPINotFound error = errors.New("api not found")
5151

5252
type do interface {
5353
Do(*http.Request) (*http.Response, error)
@@ -405,7 +405,7 @@ func (c *Client) RequestIPs(ctx context.Context, ipconfig cns.IPConfigsRequest)
405405

406406
// if we get a 404 error
407407
if res.StatusCode == http.StatusNotFound {
408-
return nil, fmt.Errorf("cannot find API RequestIPs %w: %v", errAPINotFound, err) //nolint:errorlint // multiple %w not supported in 1.19
408+
return nil, fmt.Errorf("cannot find API RequestIPs %w: %v", ErrAPINotFound, err) //nolint:errorlint // multiple %w not supported in 1.19
409409
}
410410

411411
if err != nil {
@@ -448,7 +448,7 @@ func (c *Client) ReleaseIPs(ctx context.Context, ipconfig cns.IPConfigsRequest)
448448

449449
// if we get a 404 error
450450
if res.StatusCode == http.StatusNotFound {
451-
return fmt.Errorf("cannot find API ReleaseIPs %w: %v", errAPINotFound, err) //nolint:errorlint // multiple %w not supported in 1.19
451+
return fmt.Errorf("cannot find API ReleaseIPs %w: %v", ErrAPINotFound, err) //nolint:errorlint // multiple %w not supported in 1.19
452452
}
453453

454454
if err != nil {

‎cns/restserver/ipam.go

+19-25
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,8 @@ import (
1818
)
1919

2020
var (
21-
errStoreEmpty = errors.New("empty endpoint state store")
22-
errParsePodIPFailed = errors.New("failed to parse pod's ip")
21+
ErrStoreEmpty = errors.New("empty endpoint state store")
22+
ErrParsePodIPFailed = errors.New("failed to parse pod's ip")
2323
)
2424

2525
// requestIPConfigHandlerHelper validates the request, assigns IPs, and returns a response
@@ -103,24 +103,16 @@ func (service *HTTPRestService) requestIPConfigHandler(w http.ResponseWriter, r
103103
return
104104
}
105105

106-
var ipconfigsRequest cns.IPConfigsRequest
107106
// doesn't fill in DesiredIPAddresses if it is empty in the original request
107+
ipconfigsRequest := cns.IPConfigsRequest{
108+
PodInterfaceID: ipconfigRequest.PodInterfaceID,
109+
InfraContainerID: ipconfigRequest.InfraContainerID,
110+
OrchestratorContext: ipconfigRequest.OrchestratorContext,
111+
Ifname: ipconfigRequest.Ifname,
112+
}
108113
if ipconfigRequest.DesiredIPAddress != "" {
109-
ipconfigsRequest = cns.IPConfigsRequest{
110-
DesiredIPAddresses: []string{
111-
ipconfigRequest.DesiredIPAddress,
112-
},
113-
PodInterfaceID: ipconfigRequest.PodInterfaceID,
114-
InfraContainerID: ipconfigRequest.InfraContainerID,
115-
OrchestratorContext: ipconfigRequest.OrchestratorContext,
116-
Ifname: ipconfigRequest.Ifname,
117-
}
118-
} else {
119-
ipconfigsRequest = cns.IPConfigsRequest{
120-
PodInterfaceID: ipconfigRequest.PodInterfaceID,
121-
InfraContainerID: ipconfigRequest.InfraContainerID,
122-
OrchestratorContext: ipconfigRequest.OrchestratorContext,
123-
Ifname: ipconfigRequest.Ifname,
114+
ipconfigsRequest.DesiredIPAddresses = []string{
115+
ipconfigRequest.DesiredIPAddress,
124116
}
125117
}
126118

@@ -171,7 +163,7 @@ func (service *HTTPRestService) requestIPConfigsHandler(w http.ResponseWriter, r
171163

172164
func (service *HTTPRestService) updateEndpointState(ipconfigsRequest cns.IPConfigsRequest, podInfo cns.PodInfo, podIPInfo []cns.PodIpInfo) error {
173165
if service.EndpointStateStore == nil {
174-
return errStoreEmpty
166+
return ErrStoreEmpty
175167
}
176168
service.Lock()
177169
defer service.Unlock()
@@ -182,7 +174,7 @@ func (service *HTTPRestService) updateEndpointState(ipconfigsRequest cns.IPConfi
182174
ip := net.ParseIP(podIPInfo[i].PodIPConfig.IPAddress)
183175
if ip == nil {
184176
logger.Errorf("failed to parse pod ip address %s", podIPInfo[i].PodIPConfig.IPAddress)
185-
return errParsePodIPFailed
177+
return ErrParsePodIPFailed
186178
}
187179
if ip.To4() == nil { // is an ipv6 address
188180
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
209201
ip := net.ParseIP(podIPInfo[i].PodIPConfig.IPAddress)
210202
if ip == nil {
211203
logger.Errorf("failed to parse pod ip address %s", podIPInfo[i].PodIPConfig.IPAddress)
212-
return errParsePodIPFailed
204+
return ErrParsePodIPFailed
213205
}
214206
ipInfo := &IPInfo{}
215207
if ip.To4() == nil { // is an ipv6 address
@@ -348,7 +340,7 @@ func (service *HTTPRestService) releaseIPConfigsHandler(w http.ResponseWriter, r
348340

349341
func (service *HTTPRestService) removeEndpointState(podInfo cns.PodInfo) error {
350342
if service.EndpointStateStore == nil {
351-
return errStoreEmpty
343+
return ErrStoreEmpty
352344
}
353345
service.Lock()
354346
defer service.Unlock()
@@ -775,9 +767,11 @@ func (service *HTTPRestService) AssignAvailableIPConfigs(podInfo cns.PodInfo) ([
775767
// Searches for available IPs in the pool
776768
for _, ipState := range service.PodIPConfigState {
777769
// check if an IP from this NC is already set side for assignment.
778-
_, ncAlreadyMarkedForAssignment := ipsToAssign[ipState.NCID]
779-
// Checks if we haven't already found an IP from that NC and checks if the current IP is available
780-
if ncAlreadyMarkedForAssignment || ipState.GetState() != types.Available {
770+
if _, ncAlreadyMarkedForAssignment := ipsToAssign[ipState.NCID]; ncAlreadyMarkedForAssignment {
771+
continue
772+
}
773+
// Checks if the current IP is available
774+
if ipState.GetState() != types.Available {
781775
continue
782776
}
783777
ipsToAssign[ipState.NCID] = ipState

‎cns/restserver/ipam_test.go

+70
Original file line numberDiff line numberDiff line change
@@ -1425,3 +1425,73 @@ func TestIPAMFailToRequestOneIPWhenExpectedToHaveTwo(t *testing.T) {
14251425
t.Fatal("Expected available ips to be one since we expect the IP to not be assigned")
14261426
}
14271427
}
1428+
1429+
func TestIPAMFailToReleasePartialIPsInPool(t *testing.T) {
1430+
svc := getTestService()
1431+
1432+
// set state as already assigned
1433+
testState, _ := NewPodStateWithOrchestratorContext(testIP1, testIPID1, testNCID, types.Assigned, 24, 0, testPod1Info)
1434+
ipconfigs := map[string]cns.IPConfigurationStatus{
1435+
testState.ID: testState,
1436+
}
1437+
testStatev6, _ := NewPodStateWithOrchestratorContext(testIP1v6, testIPID1v6, testNCIDv6, types.Assigned, 120, 0, testPod1Info)
1438+
ipconfigsv6 := map[string]cns.IPConfigurationStatus{
1439+
testStatev6.ID: testStatev6,
1440+
}
1441+
1442+
err := UpdatePodIPConfigState(t, svc, ipconfigs, testNCID)
1443+
if err != nil {
1444+
t.Fatalf("Expected to not fail adding IPs to state: %+v", err)
1445+
}
1446+
err = UpdatePodIPConfigState(t, svc, ipconfigsv6, testNCIDv6)
1447+
if err != nil {
1448+
t.Fatalf("Expected to not fail adding empty NC to state: %+v", err)
1449+
}
1450+
// remove the IP from the from the ipconfig map so that it throws an error when trying to release one of the IPs
1451+
delete(svc.PodIPConfigState, testStatev6.ID)
1452+
1453+
err = svc.releaseIPConfigs(testPod1Info)
1454+
if err == nil {
1455+
t.Fatalf("Expected fail releasing IP due to only having one in the ipconfig map, IPs will be reassigned back to the pod")
1456+
}
1457+
}
1458+
1459+
func TestIPAMFailToRequestPartialIPsInPool(t *testing.T) {
1460+
svc := getTestService()
1461+
1462+
// set state as already assigned
1463+
testState := NewPodState(testIP1, testIPID1, testNCID, types.Available, 0)
1464+
ipconfigs := map[string]cns.IPConfigurationStatus{
1465+
testState.ID: testState,
1466+
}
1467+
testStatev6 := NewPodState(testIP1v6, testIPID1v6, testNCIDv6, types.Available, 0)
1468+
ipconfigsv6 := map[string]cns.IPConfigurationStatus{
1469+
testStatev6.ID: testStatev6,
1470+
}
1471+
1472+
err := UpdatePodIPConfigState(t, svc, ipconfigs, testNCID)
1473+
if err != nil {
1474+
t.Fatalf("Expected to not fail adding IPs to state: %+v", err)
1475+
}
1476+
err = UpdatePodIPConfigState(t, svc, ipconfigsv6, testNCIDv6)
1477+
if err != nil {
1478+
t.Fatalf("Expected to not fail adding empty NC to state: %+v", err)
1479+
}
1480+
// remove the IP from the from the ipconfig map so that it throws an error when trying to release one of the IPs
1481+
delete(svc.PodIPConfigState, testStatev6.ID)
1482+
1483+
req := cns.IPConfigsRequest{
1484+
PodInterfaceID: testPod1Info.InterfaceID(),
1485+
InfraContainerID: testPod1Info.InfraContainerID(),
1486+
}
1487+
b, _ := testPod1Info.OrchestratorContext()
1488+
req.OrchestratorContext = b
1489+
req.DesiredIPAddresses = make([]string, 2)
1490+
req.DesiredIPAddresses[0] = testIP1
1491+
req.DesiredIPAddresses[1] = testIP1v6
1492+
1493+
_, err = requestIPAddressAndGetState(t, req)
1494+
if err == nil {
1495+
t.Fatalf("Expected fail requesting IPs due to only having one in the ipconfig map, IPs in the pool will not be assigned")
1496+
}
1497+
}

0 commit comments

Comments
 (0)