cns_removePrefixLengthFromSecondaryIpconfig (#648)

neaggarwMS · web-flow · commit e1282abd664a · 2020-08-13T14:06:37.000-07:00
* cns_removePrefixLengthFromSecondaryIpconfig
Removed PrefixLength from SecondaryIPConfig struct. We dont need to populate /32 in this struct. When CNI request for SecondaryIPconfig, then add Subnet range prefix length it the IP CIDR format.
diff --git a/cns/NetworkContainerContract.go b/cns/NetworkContainerContract.go
@@ -127,7 +127,7 @@ type IPConfiguration struct {
 
 // SecondaryIPConfig contains IP info of SecondaryIP
 type SecondaryIPConfig struct {
-	IPSubnet IPSubnet
+	IPAddress string
 }
 
 // IPSubnet contains ip subnet.
diff --git a/cns/cnsclient/cnsclient_test.go b/cns/cnsclient/cnsclient_test.go
@@ -46,10 +46,7 @@ func addTestStateToRestServer(t *testing.T, secondaryIps []string) {
 
 	for _, secIpAddress := range secondaryIps {
 		secIpConfig := cns.SecondaryIPConfig{
-			IPSubnet: cns.IPSubnet{
-				IPAddress:    secIpAddress,
-				PrefixLength: 32,
-			},
+			IPAddress: secIpAddress,
 		}
 		ipId := uuid.New()
 		secondaryIPConfigs[ipId.String()] = secIpConfig
diff --git a/cns/requestcontroller/kubecontroller/crdrequestcontroller_test.go b/cns/requestcontroller/kubecontroller/crdrequestcontroller_test.go
@@ -366,17 +366,11 @@ func TestUpdateSpecOnNonExistingNodeNetConfig(t *testing.T) {
 	logger.InitLogger("Azure CNS RequestController", 0, 0, "")
 
 	ipConfig1 := cns.SecondaryIPConfig{
-		IPSubnet: cns.IPSubnet{
-			IPAddress:    "10.0.0.1",
-			PrefixLength: 32,
-		},
+		IPAddress: "10.0.0.1",
 	}
 
 	ipConfig2 := cns.SecondaryIPConfig{
-		IPSubnet: cns.IPSubnet{
-			IPAddress:    "10.0.0.2",
-			PrefixLength: 32,
-		},
+		IPAddress: "10.0.0.2",
 	}
 
 	secondaryIPConfigs := map[string]cns.SecondaryIPConfig{
@@ -421,17 +415,11 @@ func TestUpdateSpecOnExistingNodeNetConfig(t *testing.T) {
 	logger.InitLogger("Azure CNS RequestController", 0, 0, "")
 
 	ipConfig1 := cns.SecondaryIPConfig{
-		IPSubnet: cns.IPSubnet{
-			IPAddress:    "10.0.0.1",
-			PrefixLength: 32,
-		},
+		IPAddress: "10.0.0.1",
 	}
 
 	ipConfig2 := cns.SecondaryIPConfig{
-		IPSubnet: cns.IPSubnet{
-			IPAddress:    "10.0.0.2",
-			PrefixLength: 32,
-		},
+		IPAddress: "10.0.0.2",
 	}
 
 	secondaryIPConfigs := map[string]cns.SecondaryIPConfig{
diff --git a/cns/requestcontroller/kubecontroller/crdtranslator.go b/cns/requestcontroller/kubecontroller/crdtranslator.go
@@ -37,7 +37,8 @@ func CRDStatusToNCRequest(crdStatus nnc.NodeNetworkConfigStatus) (cns.CreateNetw
 		ncRequest.NetworkContainerid = nc.ID
 		ncRequest.NetworkContainerType = cns.Docker
 
-		// Convert "10.0.0.1/32" into "10.0.0.1" and 32
+		// Convert "10.0.0.1/32" into "10.0.0.1" and prefix length
+		// Todo, this will be changed soon and only ipaddress will be passed
 		if ip, ipNet, err = net.ParseCIDR(nc.PrimaryIP); err != nil {
 			return ncRequest, err
 		}
@@ -49,16 +50,12 @@ func CRDStatusToNCRequest(crdStatus nnc.NodeNetworkConfigStatus) (cns.CreateNetw
 		ncRequest.IPConfiguration.GatewayIPAddress = nc.DefaultGateway
 
 		for _, ipAssignment = range nc.IPAssignments {
-			if ip, ipNet, err = net.ParseCIDR(ipAssignment.IP); err != nil {
+			if ip, _, err = net.ParseCIDR(ipAssignment.IP); err != nil {
 				return ncRequest, err
 			}
 
-			_, bits = ipNet.Mask.Size()
-
-			ipSubnet.IPAddress = ip.String()
-			ipSubnet.PrefixLength = uint8(bits)
 			secondaryIPConfig = cns.SecondaryIPConfig{
-				IPSubnet: ipSubnet,
+				IPAddress: ip.String(),
 			}
 			ncRequest.SecondaryIPConfigs[ipAssignment.Name] = secondaryIPConfig
 		}
diff --git a/cns/requestcontroller/kubecontroller/crdtranslator_test.go b/cns/requestcontroller/kubecontroller/crdtranslator_test.go
@@ -195,12 +195,8 @@ func TestStatusToNCRequestSuccess(t *testing.T) {
 		t.Fatalf("Expected there to be a secondary ip with the key %v but found nothing", allocatedUUID)
 	}
 
-	if secondaryIP.IPSubnet.IPAddress != ipCIDRString {
-		t.Fatalf("Expected %v as the secondary IP config but got %v", ipCIDRString, secondaryIP.IPSubnet.IPAddress)
-	}
-
-	if secondaryIP.IPSubnet.PrefixLength != ipCIDRMaskLength {
-		t.Fatalf("Expected %v as the prefix length for the secondary IP config but got %v", ipCIDRMaskLength, secondaryIP.IPSubnet.PrefixLength)
+	if secondaryIP.IPAddress != ipCIDRString {
+		t.Fatalf("Expected %v as the secondary IP config but got %v", ipCIDRString, secondaryIP.IPAddress)
 	}
 }
 
@@ -233,10 +229,7 @@ func TestSecondaryIPsToCRDSpecSuccess(t *testing.T) {
 
 	secondaryIPs = map[string]cns.SecondaryIPConfig{
 		allocatedUUID: {
-			IPSubnet: cns.IPSubnet{
-				IPAddress:    ipCIDRString,
-				PrefixLength: ipCIDRMaskLength,
-			},
+			IPAddress: ipCIDRString,
 		},
 	}
 
diff --git a/cns/restserver/internalapi.go b/cns/restserver/internalapi.go
@@ -167,12 +167,12 @@ func (service *HTTPRestService) ReconcileNCState(ncRequest *cns.CreateNetworkCon
 
 	// now parse the secondaryIP list, if it exists in PodInfo list, then allocate that ip
 	for _, secIpConfig := range ncRequest.SecondaryIPConfigs {
-		if podInfo, exists := podInfoByIp[secIpConfig.IPSubnet.IPAddress]; exists {
+		if podInfo, exists := podInfoByIp[secIpConfig.IPAddress]; exists {
 			log.Logf("SecondaryIP %+v is allocated to Pod. %+v, ncId: %s", secIpConfig, podInfo, ncRequest.NetworkContainerid)
 
 			desiredIPConfig := cns.IPSubnet{
-				IPAddress:    secIpConfig.IPSubnet.IPAddress,
-				PrefixLength: secIpConfig.IPSubnet.PrefixLength,
+				IPAddress:    secIpConfig.IPAddress,
+				PrefixLength: 32, //todo: remove PrefixLenght in
 			}
 
 			kubernetesPodInfo := cns.KubernetesPodInfo{
@@ -220,11 +220,10 @@ func (service *HTTPRestService) CreateOrUpdateNetworkContainerInternal(req cns.C
 	}
 
 	// Validate SecondaryIPConfig
-	for ipId, secIpconfig := range req.SecondaryIPConfigs {
+	for _, secIpconfig := range req.SecondaryIPConfigs {
 		// Validate Ipconfig
-		err := validateIPSubnet(secIpconfig.IPSubnet)
-		if err != nil {
-			logger.Errorf("[Azure CNS] Error. SecondaryIpConfig, Id:%s is invalid, SecondaryIPConfig: %v, ncId: %s", ipId, secIpconfig, req.NetworkContainerid)
+		if secIpconfig.IPAddress == "" {
+			logger.Errorf("Failed to add IPConfig to state: %+v, empty IPSubnet.IPAddress", secIpconfig)
 			return InvalidSecondaryIPConfig
 		}
 	}
diff --git a/cns/restserver/internalapi_test.go b/cns/restserver/internalapi_test.go
@@ -58,7 +58,7 @@ func TestReconcileNCWithExistingState(t *testing.T) {
 	var startingIndex = 6
 	for i := 0; i < 4; i++ {
 		ipaddress := "10.0.0." + strconv.Itoa(startingIndex)
-		secIpConfig := newSecondaryIPConfig(ipaddress, 32)
+		secIpConfig := newSecondaryIPConfig(ipaddress)
 		ipId := uuid.New()
 		secondaryIPConfigs[ipId.String()] = secIpConfig
 		startingIndex++
@@ -95,7 +95,7 @@ func TestReconcileNCWithSystemPods(t *testing.T) {
 	var startingIndex = 6
 	for i := 0; i < 4; i++ {
 		ipaddress := "10.0.0." + strconv.Itoa(startingIndex)
-		secIpConfig := newSecondaryIPConfig(ipaddress, 32)
+		secIpConfig := newSecondaryIPConfig(ipaddress)
 		ipId := uuid.New()
 		secondaryIPConfigs[ipId.String()] = secIpConfig
 		startingIndex++
@@ -136,7 +136,7 @@ func validateCreateOrUpdateNCInternal(t *testing.T, secondaryIpCount int) {
 	var startingIndex = 6
 	for i := 0; i < secondaryIpCount; i++ {
 		ipaddress := "10.0.0." + strconv.Itoa(startingIndex)
-		secIpConfig := newSecondaryIPConfig(ipaddress, 32)
+		secIpConfig := newSecondaryIPConfig(ipaddress)
 		ipId := uuid.New()
 		secondaryIPConfigs[ipId.String()] = secIpConfig
 		startingIndex++
@@ -148,7 +148,7 @@ func validateCreateOrUpdateNCInternal(t *testing.T, secondaryIpCount int) {
 	fmt.Println("Validate Scaleup")
 	for i := 0; i < secondaryIpCount; i++ {
 		ipaddress := "10.0.0." + strconv.Itoa(startingIndex)
-		secIpConfig := newSecondaryIPConfig(ipaddress, 32)
+		secIpConfig := newSecondaryIPConfig(ipaddress)
 		ipId := uuid.New()
 		secondaryIPConfigs[ipId.String()] = secIpConfig
 		startingIndex++
@@ -216,8 +216,8 @@ func validateNetworkRequest(t *testing.T, req cns.CreateNetworkContainerRequest)
 			if secondaryIpConfig, ok := req.SecondaryIPConfigs[ipid]; !ok {
 				t.Fatalf("PodIpConfigState has stale ipId: %s, config: %+v", ipid, ipStatus)
 			} else {
-				if ipStatus.IPSubnet != secondaryIpConfig.IPSubnet {
-					t.Fatalf("IPId: %s IPSubnet doesnt match: expected %+v, actual: %+v", ipid, secondaryIpConfig.IPSubnet, ipStatus.IPSubnet)
+				if ipStatus.IPAddress != secondaryIpConfig.IPAddress {
+					t.Fatalf("IPId: %s IPSubnet doesnt match: expected %+v, actual: %+v", ipid, secondaryIpConfig.IPAddress, ipStatus.IPAddress)
 				}
 
 				// Validate IP state
@@ -239,12 +239,12 @@ func validateNetworkRequest(t *testing.T, req cns.CreateNetworkContainerRequest)
 					t.Fatalf("IPId: %s State is not Available, ipStatus: %+v", ipid, ipStatus)
 				}
 
-				alreadyValidated[ipid] = ipStatus.IPSubnet.IPAddress
+				alreadyValidated[ipid] = ipStatus.IPAddress
 			}
 		} else {
 			// if ipaddress is not same, then fail
-			if ipaddress != ipStatus.IPSubnet.IPAddress {
-				t.Fatalf("Added the same IP guid :%s with different ipaddress, expected:%s, actual %s", ipid, ipStatus.IPSubnet.IPAddress, ipaddress)
+			if ipaddress != ipStatus.IPAddress {
+				t.Fatalf("Added the same IP guid :%s with different ipaddress, expected:%s, actual %s", ipid, ipStatus.IPAddress, ipaddress)
 			}
 		}
 	}
@@ -298,7 +298,7 @@ func validateNCStateAfterReconcile(t *testing.T, ncRequest *cns.CreateNetworkCon
 		}
 
 		// Validate if IPAddress matches
-		if ipConfigstate.IPSubnet.IPAddress != ipaddress {
+		if ipConfigstate.IPAddress != ipaddress {
 			t.Fatalf("IpAddress %s is not same, for Pod: %+v, actual ipState: %+v", ipaddress, podInfo, ipConfigstate)
 		}
 
@@ -319,7 +319,7 @@ func validateNCStateAfterReconcile(t *testing.T, ncRequest *cns.CreateNetworkCon
 	// validate rest of Secondary IPs in Available state
 	if ncRequest != nil {
 		for secIpId, secIpConfig := range ncRequest.SecondaryIPConfigs {
-			if _, exists := expectedAllocatedPods[secIpConfig.IPSubnet.IPAddress]; exists {
+			if _, exists := expectedAllocatedPods[secIpConfig.IPAddress]; exists {
 				continue
 			}
 
diff --git a/cns/restserver/ipam.go b/cns/restserver/ipam.go
@@ -169,8 +169,7 @@ func (service *HTTPRestService) GetExistingIPConfig(podInfo cns.KubernetesPodInf
 	ipID := service.PodIPIDByOrchestratorContext[podInfo.GetOrchestratorContextKey()]
 	if ipID != "" {
 		if ipState, isExist := service.PodIPConfigState[ipID]; isExist {
-			ipConfiguration.IPSubnet = ipState.IPSubnet
-			err := service.populateIpConfigInfoFromNCUntransacted(ipState.NCID, &ipConfiguration)
+			err := service.populateIpConfigInfoUntransacted(ipState, &ipConfiguration)
 			return ipConfiguration, isExist, err
 		}
 
@@ -188,7 +187,7 @@ func (service *HTTPRestService) AllocateDesiredIPConfig(podInfo cns.KubernetesPo
 
 	found := false
 	for _, ipState := range service.PodIPConfigState {
-		if ipState.IPSubnet.IPAddress == desiredIPAddress {
+		if ipState.IPAddress == desiredIPAddress {
 			if ipState.State == cns.Allocated {
 				// This IP has already been allocated, if it is allocated to same pod, then return the same
 				// IPconfiguration
@@ -207,13 +206,8 @@ func (service *HTTPRestService) AllocateDesiredIPConfig(podInfo cns.KubernetesPo
 			}
 
 			if found {
-				err := service.populateIpConfigInfoFromNCUntransacted(ipState.NCID, &ipConfiguration)
-				if err != nil {
-					return ipConfiguration, err
-				}
-
-				ipConfiguration.IPSubnet = ipState.IPSubnet
-				return ipConfiguration, nil
+				err := service.populateIpConfigInfoUntransacted(ipState, &ipConfiguration)
+				return ipConfiguration, err
 			}
 		}
 	}
@@ -228,10 +222,10 @@ func (service *HTTPRestService) AllocateAnyAvailableIPConfig(podInfo cns.Kuberne
 
 	for _, ipState := range service.PodIPConfigState {
 		if ipState.State == cns.Available {
-			service.setIPConfigAsAllocated(ipState, podInfo, orchestratorContext)
-
-			ipConfiguration.IPSubnet = ipState.IPSubnet
-			err := service.populateIpConfigInfoFromNCUntransacted(ipState.NCID, &ipConfiguration)
+			err := service.populateIpConfigInfoUntransacted(ipState, &ipConfiguration)
+			if err == nil {
+				service.setIPConfigAsAllocated(ipState, podInfo, orchestratorContext)
+			}
 			return ipConfiguration, err
 		}
 	}
diff --git a/cns/restserver/ipam_test.go b/cns/restserver/ipam_test.go
@@ -47,23 +47,20 @@ func getTestService() *HTTPRestService {
 	return svc
 }
 
-func newSecondaryIPConfig(ipAddress string, prefixLength uint8) cns.SecondaryIPConfig {
+func newSecondaryIPConfig(ipAddress string) cns.SecondaryIPConfig {
 	return cns.SecondaryIPConfig{
-		IPSubnet: cns.IPSubnet{
-			IPAddress:    ipAddress,
-			PrefixLength: prefixLength,
-		},
+		IPAddress: ipAddress,
 	}
 }
 
 func NewPodState(ipaddress string, prefixLength uint8, id, ncid, state string) ipConfigurationStatus {
-	ipconfig := newSecondaryIPConfig(ipaddress, prefixLength)
+	ipconfig := newSecondaryIPConfig(ipaddress)
 
 	return ipConfigurationStatus{
-		IPSubnet: ipconfig.IPSubnet,
-		ID:       id,
-		NCID:     ncid,
-		State:    state,
+		IPAddress: ipconfig.IPAddress,
+		ID:        id,
+		NCID:      ncid,
+		State:     state,
 	}
 }
 
@@ -88,6 +85,7 @@ func requestIpAddressAndGetState(t *testing.T, req cns.GetIPConfigRequest) (ipCo
 	if reflect.DeepEqual(ipConfig.GatewayIPAddress, gatewayIp) != true {
 		t.Fatalf("Gateway is not added as expected ipConfig %+v, expected GatewayIp: %+v", ipConfig, gatewayIp)
 	}
+
 	// retrieve podinfo from orchestrator context
 	if err := json.Unmarshal(req.OrchestratorContext, &podInfo); err != nil {
 		return ipState, err
@@ -100,10 +98,10 @@ func requestIpAddressAndGetState(t *testing.T, req cns.GetIPConfigRequest) (ipCo
 }
 
 func NewPodStateWithOrchestratorContext(ipaddress string, prefixLength uint8, id, ncid, state string, orchestratorContext cns.KubernetesPodInfo) (ipConfigurationStatus, error) {
-	ipconfig := newSecondaryIPConfig(ipaddress, prefixLength)
+	ipconfig := newSecondaryIPConfig(ipaddress)
 	b, err := json.Marshal(orchestratorContext)
 	return ipConfigurationStatus{
-		IPSubnet:            ipconfig.IPSubnet,
+		IPAddress:           ipconfig.IPAddress,
 		ID:                  id,
 		NCID:                ncid,
 		State:               state,
@@ -117,10 +115,7 @@ func UpdatePodIpConfigState(t *testing.T, svc *HTTPRestService, ipconfigs map[st
 	secondaryIPConfigs := make(map[string]cns.SecondaryIPConfig)
 	for _, ipconfig := range ipconfigs {
 		secIpConfig := cns.SecondaryIPConfig{
-			IPSubnet: cns.IPSubnet{
-				IPAddress:    ipconfig.IPSubnet.IPAddress,
-				PrefixLength: ipconfig.IPSubnet.PrefixLength,
-			},
+			IPAddress: ipconfig.IPAddress,
 		}
 
 		ipId := ipconfig.ID
@@ -406,7 +401,7 @@ func TestIPAMRequestThenReleaseThenRequestAgain(t *testing.T) {
 
 	desiredState, _ := NewPodStateWithOrchestratorContext(testIP1, 24, testPod1GUID, testNCID, cns.Allocated, testPod1Info)
 	// want first available Pod IP State
-	desiredState.IPSubnet = desiredIPConfig
+	desiredState.IPAddress = desiredIPConfig.IPAddress
 	desiredState.OrchestratorContext = b
 
 	if reflect.DeepEqual(desiredState, actualstate) != true {
@@ -489,7 +484,10 @@ func TestAvailableIPConfigs(t *testing.T) {
 	req := cns.GetIPConfigRequest{}
 	b, _ := json.Marshal(testPod1Info)
 	req.OrchestratorContext = b
-	req.DesiredIPConfig = state1.IPSubnet
+	req.DesiredIPConfig = cns.IPSubnet{
+		IPAddress:    state1.IPAddress,
+		PrefixLength: 32,
+	}
 
 	_, err := requestIpAddressAndGetState(t, req)
 	if err != nil {
diff --git a/cns/restserver/restserver.go b/cns/restserver/restserver.go
@@ -59,7 +59,7 @@ type allocatedIPCount struct {
 type ipConfigurationStatus struct {
 	NCID                string
 	ID                  string //uuid
-	IPSubnet            cns.IPSubnet
+	IPAddress           string
 	State               string
 	OrchestratorContext json.RawMessage
 }
diff --git a/cns/restserver/util.go b/cns/restserver/util.go

Original file line number	Diff line number	Diff line change
`@@ -127,7 +127,7 @@ type IPConfiguration struct {`
`127`	`127`
`128`	`128`	`// SecondaryIPConfig contains IP info of SecondaryIP`
`129`	`129`	`type SecondaryIPConfig struct {`
`130`		`- IPSubnet IPSubnet`
	`130`	`+ IPAddress string`
`131`	`131`	`}`
`132`	`132`
`133`	`133`	`// IPSubnet contains ip subnet.`
Original file line number	Diff line number	Diff line change
`@@ -195,12 +195,8 @@ func TestStatusToNCRequestSuccess(t *testing.T) {`
`195`	`195`	`t.Fatalf("Expected there to be a secondary ip with the key %v but found nothing", allocatedUUID)`
`196`	`196`	`}`
`197`	`197`
`198`		`- if secondaryIP.IPSubnet.IPAddress != ipCIDRString {`
`199`		`- t.Fatalf("Expected %v as the secondary IP config but got %v", ipCIDRString, secondaryIP.IPSubnet.IPAddress)`
`200`		`- }`
`201`		`-`
`202`		`- if secondaryIP.IPSubnet.PrefixLength != ipCIDRMaskLength {`
`203`		`- t.Fatalf("Expected %v as the prefix length for the secondary IP config but got %v", ipCIDRMaskLength, secondaryIP.IPSubnet.PrefixLength)`
	`198`	`+ if secondaryIP.IPAddress != ipCIDRString {`
	`199`	`+ t.Fatalf("Expected %v as the secondary IP config but got %v", ipCIDRString, secondaryIP.IPAddress)`
`204`	`200`	`}`
`205`	`201`	`}`
`206`	`202`
`@@ -233,10 +229,7 @@ func TestSecondaryIPsToCRDSpecSuccess(t *testing.T) {`
`233`	`229`
`234`	`230`	`secondaryIPs = map[string]cns.SecondaryIPConfig{`
`235`	`231`	`allocatedUUID: {`
`236`		`- IPSubnet: cns.IPSubnet{`
`237`		`- IPAddress: ipCIDRString,`
`238`		`- PrefixLength: ipCIDRMaskLength,`
`239`		`- },`
	`232`	`+ IPAddress: ipCIDRString,`
`240`	`233`	`},`
`241`	`234`	`}`
`242`	`235`
Original file line number	Diff line number	Diff line change
`@@ -59,7 +59,7 @@ type allocatedIPCount struct {`
`59`	`59`	`type ipConfigurationStatus struct {`
`60`	`60`	`NCID string`
`61`	`61`	`ID string //uuid`
`62`		`- IPSubnet cns.IPSubnet`
	`62`	`+ IPAddress string`
`63`	`63`	`State string`
`64`	`64`	`OrchestratorContext json.RawMessage`
`65`	`65`	`}`