fix: reserve 0th IP as gateway for overlay on Windows (#1968)

* fix: reserve 0th IP as gateway for overlay on Windows

* fix: allow gateway to be updated

cni/network/network_windows.go

@@ -415,6 +415,7 @@ func (plugin *NetPlugin) isDualNicFeatureSupported(netNs string) bool {
 }
 
 func getOverlayGateway(podsubnet *net.IPNet) (net.IP, error) {
+	log.Printf("WARN: No gateway specified for Overlay NC. CNI will choose one, but connectivity may break.")
 	ncgw := podsubnet.IP
 	ncgw[3]++
 	ncgw = net.ParseIP(ncgw.String())

cns/kubecontroller/nodenetworkconfig/conversion.go

@@ -21,6 +21,7 @@ var (
 )
 
 // CreateNCRequestFromDynamicNC generates a CreateNetworkContainerRequest from a dynamic NetworkContainer.
+//
 //nolint:gocritic //ignore hugeparam
 func CreateNCRequestFromDynamicNC(nc v1alpha.NetworkContainer) (*cns.CreateNetworkContainerRequest, error) {
 	primaryIP := nc.PrimaryIP
@@ -69,6 +70,7 @@ func CreateNCRequestFromDynamicNC(nc v1alpha.NetworkContainer) (*cns.CreateNetwo
 }
 
 // CreateNCRequestFromStaticNC generates a CreateNetworkContainerRequest from a static NetworkContainer.
+//
 //nolint:gocritic //ignore hugeparam
 func CreateNCRequestFromStaticNC(nc v1alpha.NetworkContainer) (*cns.CreateNetworkContainerRequest, error) {
 	primaryPrefix, err := netip.ParsePrefix(nc.PrimaryIP)
@@ -85,24 +87,6 @@ func CreateNCRequestFromStaticNC(nc v1alpha.NetworkContainer) (*cns.CreateNetwor
 		PrefixLength: uint8(subnetPrefix.Bits()),
 	}
 
-	secondaryIPConfigs := map[string]cns.SecondaryIPConfig{}
-
-	// iterate through all IP addresses in the subnet described by primaryPrefix and
-	// add them to the request as secondary IPConfigs.
-	for addr := primaryPrefix.Masked().Addr(); primaryPrefix.Contains(addr); addr = addr.Next() {
-		secondaryIPConfigs[addr.String()] = cns.SecondaryIPConfig{
-			IPAddress: addr.String(),
-			NCVersion: int(nc.Version),
-		}
-	}
-	return &cns.CreateNetworkContainerRequest{
-		SecondaryIPConfigs:   secondaryIPConfigs,
-		NetworkContainerid:   nc.ID,
-		NetworkContainerType: cns.Docker,
-		Version:              strconv.FormatInt(nc.Version, 10), //nolint:gomnd // it's decimal
-		IPConfiguration: cns.IPConfiguration{
-			IPSubnet:         subnet,
-			GatewayIPAddress: nc.DefaultGateway,
-		},
-	}, nil
+	req := createNCRequestFromStaticNCHelper(nc, primaryPrefix, subnet)
+	return req, nil
 }

cns/kubecontroller/nodenetworkconfig/conversion_linux.go

@@ -0,0 +1,36 @@
+package nodenetworkconfig
+
+import (
+	"net/netip"
+	"strconv"
+
+	"github.com/Azure/azure-container-networking/cns"
+	"github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha"
+)
+
+// createNCRequestFromStaticNCHelper generates a CreateNetworkContainerRequest from a static NetworkContainer
+// by adding all IPs in the the block to the secondary IP configs list. It does not skip any IPs.
+//
+//nolint:gocritic //ignore hugeparam
+func createNCRequestFromStaticNCHelper(nc v1alpha.NetworkContainer, primaryIPPrefix netip.Prefix, subnet cns.IPSubnet) *cns.CreateNetworkContainerRequest {
+	secondaryIPConfigs := map[string]cns.SecondaryIPConfig{}
+
+	// iterate through all IP addresses in the subnet described by primaryPrefix and
+	// add them to the request as secondary IPConfigs.
+	for addr := primaryIPPrefix.Masked().Addr(); primaryIPPrefix.Contains(addr); addr = addr.Next() {
+		secondaryIPConfigs[addr.String()] = cns.SecondaryIPConfig{
+			IPAddress: addr.String(),
+			NCVersion: int(nc.Version),
+		}
+	}
+	return &cns.CreateNetworkContainerRequest{
+		SecondaryIPConfigs:   secondaryIPConfigs,
+		NetworkContainerid:   nc.ID,
+		NetworkContainerType: cns.Docker,
+		Version:              strconv.FormatInt(nc.Version, 10), //nolint:gomnd // it's decimal
+		IPConfiguration: cns.IPConfiguration{
+			IPSubnet:         subnet,
+			GatewayIPAddress: nc.DefaultGateway,
+		},
+	}
+}

cns/kubecontroller/nodenetworkconfig/conversion_linux_test.go

@@ -0,0 +1,37 @@
+package nodenetworkconfig
+
+import (
+	"strconv"
+
+	"github.com/Azure/azure-container-networking/cns"
+)
+
+var validOverlayRequest = &cns.CreateNetworkContainerRequest{
+	Version: strconv.FormatInt(version, 10),
+	IPConfiguration: cns.IPConfiguration{
+		IPSubnet: cns.IPSubnet{
+			PrefixLength: uint8(subnetPrefixLen),
+			IPAddress:    primaryIP,
+		},
+	},
+	NetworkContainerid:   ncID,
+	NetworkContainerType: cns.Docker,
+	SecondaryIPConfigs: map[string]cns.SecondaryIPConfig{
+		"10.0.0.0": {
+			IPAddress: "10.0.0.0",
+			NCVersion: version,
+		},
+		"10.0.0.1": {
+			IPAddress: "10.0.0.1",
+			NCVersion: version,
+		},
+		"10.0.0.2": {
+			IPAddress: "10.0.0.2",
+			NCVersion: version,
+		},
+		"10.0.0.3": {
+			IPAddress: "10.0.0.3",
+			NCVersion: version,
+		},
+	},
+}

cns/kubecontroller/nodenetworkconfig/conversion_test.go

@@ -87,36 +87,6 @@ var validOverlayNC = v1alpha.NetworkContainer{
 	Version:            version,
 }
 
-var validOverlayRequest = &cns.CreateNetworkContainerRequest{
-	Version: strconv.FormatInt(version, 10),
-	IPConfiguration: cns.IPConfiguration{
-		IPSubnet: cns.IPSubnet{
-			PrefixLength: uint8(subnetPrefixLen),
-			IPAddress:    primaryIP,
-		},
-	},
-	NetworkContainerid:   ncID,
-	NetworkContainerType: cns.Docker,
-	SecondaryIPConfigs: map[string]cns.SecondaryIPConfig{
-		"10.0.0.0": {
-			IPAddress: "10.0.0.0",
-			NCVersion: version,
-		},
-		"10.0.0.1": {
-			IPAddress: "10.0.0.1",
-			NCVersion: version,
-		},
-		"10.0.0.2": {
-			IPAddress: "10.0.0.2",
-			NCVersion: version,
-		},
-		"10.0.0.3": {
-			IPAddress: "10.0.0.3",
-			NCVersion: version,
-		},
-	},
-}
-
 func TestCreateNCRequestFromDynamicNC(t *testing.T) {
 	tests := []struct {
 		name    string

cns/kubecontroller/nodenetworkconfig/conversion_windows.go

@@ -0,0 +1,45 @@
+package nodenetworkconfig
+
+import (
+	"net/netip"
+	"strconv"
+
+	"github.com/Azure/azure-container-networking/cns"
+	"github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha"
+)
+
+// createNCRequestFromStaticNCHelper generates a CreateNetworkContainerRequest from a static NetworkContainer.
+// If the NC's DefaultGateway is empty, it will set the 0th IP as the gateway IP and all remaining IPs as
+// secondary IPs. If the gateway is not empty, it will not reserve the 0th IP and add it as a secondary IP.
+//
+//nolint:gocritic //ignore hugeparam
+func createNCRequestFromStaticNCHelper(nc v1alpha.NetworkContainer, primaryIPPrefix netip.Prefix, subnet cns.IPSubnet) *cns.CreateNetworkContainerRequest {
+	secondaryIPConfigs := map[string]cns.SecondaryIPConfig{}
+
+	// if NC DefaultGateway is empty, set the 0th IP to the gateway and add the rest of the IPs
+	// as secondary IPs
+	startingAddr := primaryIPPrefix.Masked().Addr() // the masked address is the 0th IP in the subnet
+	if nc.DefaultGateway == "" {
+		nc.DefaultGateway = startingAddr.String()
+		startingAddr = startingAddr.Next()
+	}
+
+	// iterate through all IP addresses in the subnet described by primaryPrefix and
+	// add them to the request as secondary IPConfigs.
+	for addr := startingAddr; primaryIPPrefix.Contains(addr); addr = addr.Next() {
+		secondaryIPConfigs[addr.String()] = cns.SecondaryIPConfig{
+			IPAddress: addr.String(),
+			NCVersion: int(nc.Version),
+		}
+	}
+	return &cns.CreateNetworkContainerRequest{
+		SecondaryIPConfigs:   secondaryIPConfigs,
+		NetworkContainerid:   nc.ID,
+		NetworkContainerType: cns.Docker,
+		Version:              strconv.FormatInt(nc.Version, 10), //nolint:gomnd // it's decimal
+		IPConfiguration: cns.IPConfiguration{
+			IPSubnet:         subnet,
+			GatewayIPAddress: nc.DefaultGateway,
+		},
+	}
+}

cns/kubecontroller/nodenetworkconfig/conversion_windows_test.go

@@ -0,0 +1,34 @@
+package nodenetworkconfig
+
+import (
+	"strconv"
+
+	"github.com/Azure/azure-container-networking/cns"
+)
+
+var validOverlayRequest = &cns.CreateNetworkContainerRequest{
+	Version: strconv.FormatInt(version, 10),
+	IPConfiguration: cns.IPConfiguration{
+		IPSubnet: cns.IPSubnet{
+			PrefixLength: uint8(subnetPrefixLen),
+			IPAddress:    primaryIP,
+		},
+		GatewayIPAddress: "10.0.0.0",
+	},
+	NetworkContainerid:   ncID,
+	NetworkContainerType: cns.Docker,
+	SecondaryIPConfigs: map[string]cns.SecondaryIPConfig{
+		"10.0.0.1": {
+			IPAddress: "10.0.0.1",
+			NCVersion: version,
+		},
+		"10.0.0.2": {
+			IPAddress: "10.0.0.2",
+			NCVersion: version,
+		},
+		"10.0.0.3": {
+			IPAddress: "10.0.0.3",
+			NCVersion: version,
+		},
+	},
+}

cns/restserver/internalapi.go

@@ -400,8 +400,13 @@ func (service *HTTPRestService) CreateOrUpdateNetworkContainerInternal(req *cns.
 	existingNCInfo, ok := service.getNetworkContainerDetails(req.NetworkContainerid)
 	if ok {
 		existingReq := existingNCInfo.CreateNetworkContainerRequest
-		if !reflect.DeepEqual(existingReq.IPConfiguration, req.IPConfiguration) {
-			logger.Errorf("[Azure CNS] Error. PrimaryCA is not same, NCId %s, old CA %s, new CA %s", req.NetworkContainerid, existingReq.PrimaryInterfaceIdentifier, req.PrimaryInterfaceIdentifier)
+		if !reflect.DeepEqual(existingReq.IPConfiguration.IPSubnet, req.IPConfiguration.IPSubnet) {
+			logger.Errorf("[Azure CNS] Error. PrimaryCA is not same, NCId %s, old CA %s/%d, new CA %s/%d",
+				req.NetworkContainerid,
+				existingReq.IPConfiguration.IPSubnet.IPAddress,
+				existingReq.IPConfiguration.IPSubnet.PrefixLength,
+				req.IPConfiguration.IPSubnet.IPAddress,
+				req.IPConfiguration.IPSubnet.PrefixLength)
 			return types.PrimaryCANotSame
 		}
 	}

cns/restserver/internalapi_test.go

@@ -46,6 +46,90 @@ func TestCreateOrUpdateNetworkContainerInternal(t *testing.T) {
 	validateCreateOrUpdateNCInternal(t, 2, "-1")
 }
 
+// TestReconcileNCStatePrimaryIPChangeShouldFail tests that reconciling NC state with
+// a NC whose IP has changed should fail
+func TestReconcileNCStatePrimaryIPChangeShouldFail(t *testing.T) {
+	restartService()
+	setEnv(t)
+	setOrchestratorTypeInternal(cns.KubernetesCRD)
+	svc.state.ContainerStatus = make(map[string]containerstatus)
+
+	// start with a NC in state
+	ncID := "555ac5c9-89f2-4b5d-b8d0-616894d6d151"
+	svc.state.ContainerStatus[ncID] = containerstatus{
+		ID:          ncID,
+		VMVersion:   "0",
+		HostVersion: "0",
+		CreateNetworkContainerRequest: cns.CreateNetworkContainerRequest{
+			NetworkContainerid: ncID,
+			IPConfiguration: cns.IPConfiguration{
+				IPSubnet: cns.IPSubnet{
+					IPAddress:    "10.0.1.0",
+					PrefixLength: 24,
+				},
+			},
+		},
+	}
+
+	// now try to reconcile the state where the NC primary IP has changed
+	resp := svc.ReconcileNCState(&cns.CreateNetworkContainerRequest{
+		NetworkContainerid: ncID,
+		IPConfiguration: cns.IPConfiguration{
+			IPSubnet: cns.IPSubnet{
+				IPAddress:    "10.0.2.0", // note this IP has changed
+				PrefixLength: 24,
+			},
+		},
+	}, map[string]cns.PodInfo{}, &v1alpha.NodeNetworkConfig{})
+
+	assert.Equal(t, types.PrimaryCANotSame, resp)
+}
+
+// TestReconcileNCStateGatewayChange tests that NC state gets updated when reconciled
+// if the NC's gateway IP has changed
+func TestReconcileNCStateGatewayChange(t *testing.T) {
+	restartService()
+	setEnv(t)
+	setOrchestratorTypeInternal(cns.KubernetesCRD)
+	svc.state.ContainerStatus = make(map[string]containerstatus)
+
+	// start with a NC in state
+	ncID := "555ac5c9-89f2-4b5d-b8d0-616894d6d151"
+	oldGW := "10.0.0.0"
+	newGW := "10.0.1.0"
+	ncPrimaryIP := cns.IPSubnet{
+		IPAddress:    "10.0.1.1",
+		PrefixLength: 24,
+	}
+	svc.state.ContainerStatus[ncID] = containerstatus{
+		ID:          ncID,
+		VMVersion:   "0",
+		HostVersion: "0",
+		CreateNetworkContainerRequest: cns.CreateNetworkContainerRequest{
+			NetworkContainerid:   ncID,
+			NetworkContainerType: cns.Kubernetes,
+			IPConfiguration: cns.IPConfiguration{
+				IPSubnet:         ncPrimaryIP,
+				GatewayIPAddress: oldGW,
+			},
+		},
+	}
+
+	// now try to reconcile the state where the NC gateway has changed
+	resp := svc.ReconcileNCState(&cns.CreateNetworkContainerRequest{
+		NetworkContainerid:   ncID,
+		NetworkContainerType: cns.Kubernetes,
+		IPConfiguration: cns.IPConfiguration{
+			IPSubnet:         ncPrimaryIP,
+			GatewayIPAddress: newGW, // note this IP has changed
+		},
+	}, map[string]cns.PodInfo{}, &v1alpha.NodeNetworkConfig{})
+
+	assert.Equal(t, types.Success, resp)
+	// assert the new state reflects the gateway update
+	assert.Equal(t, newGW, svc.state.ContainerStatus[ncID].CreateNetworkContainerRequest.IPConfiguration.GatewayIPAddress)
+}
+
 func TestCreateOrUpdateNCWithLargerVersionComparedToNMAgent(t *testing.T) {
 	restartService()