Merge pull request #644 from yvetteli0314/master

Update Node pod CIDR in ipam controller for for Multi Pod CIDR suppor…

Author: k8s-ci-robot

pkg/controller/nodeipam/ipam/BUILD

@@ -70,6 +70,7 @@ go_test(
         "//pkg/controller/nodeipam/ipam/cidrset",
         "//pkg/controller/nodeipam/ipam/test",
         "//pkg/controller/testutil",
+        "//pkg/util/node",
         "//providers/gce",
         "//vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta",
         "//vendor/github.com/google/go-cmp/cmp",

pkg/controller/nodeipam/ipam/cloud_cidr_allocator.go

@@ -311,70 +311,40 @@ func (ca *cloudCIDRAllocator) updateCIDRAllocation(nodeName string) error {
 
 	cidrStrings := make([]string, 0)
 
-	// if there are no interfaces or there is 1 interface
-	// but does not have IP alias or IPv6 ranges no CIDR
-	// can be allocated
-	if len(instance.NetworkInterfaces) == 0 ||
-		(len(instance.NetworkInterfaces) == 1 &&
-			len(instance.NetworkInterfaces[0].AliasIpRanges) == 0 &&
-			ca.cloud.GetIPV6Address(instance.NetworkInterfaces[0]) == nil) {
+	if len(instance.NetworkInterfaces) == 0 || (len(instance.NetworkInterfaces) == 1 && len(instance.NetworkInterfaces[0].AliasIpRanges) == 0) {
 		nodeutil.RecordNodeStatusChange(ca.recorder, node, "CIDRNotAvailable")
 		return fmt.Errorf("failed to allocate cidr: Node %v has no ranges from which CIDRs can be allocated", node.Name)
 	}
 
 	// sets the v1.NodeNetworkUnavailable condition to False
 	ca.setNetworkCondition(node)
 
-	// nodes in clusters WITHOUT multi-networking are expected to have
-	// only 1 network-interface and 1 alias IPv4 range or/and 1 IPv6 address
-	// multi-network cluster may have 1 interface with multiple alias
-	if len(instance.NetworkInterfaces) == 1 &&
-		(len(instance.NetworkInterfaces[0].AliasIpRanges) == 1 ||
-			ca.cloud.GetIPV6Address(instance.NetworkInterfaces[0]) != nil) {
-		// with 1 alias IPv4 range on single IPv4 or dual stack clusters
-		if len(instance.NetworkInterfaces[0].AliasIpRanges) == 1 {
-			cidrStrings = append(cidrStrings, instance.NetworkInterfaces[0].AliasIpRanges[0].IpCidrRange)
-		}
-		// with 1 IPv6 range on single IPv6 or dual stack cluster
+	// nodes in clusters WITHOUT multi-networking are expected to have only 1 network-interface with 1 alias IP range.
+	if len(instance.NetworkInterfaces) == 1 && len(instance.NetworkInterfaces[0].AliasIpRanges) == 1 {
+		cidrStrings = append(cidrStrings, instance.NetworkInterfaces[0].AliasIpRanges[0].IpCidrRange)
 		ipv6Addr := ca.cloud.GetIPV6Address(instance.NetworkInterfaces[0])
 		if ipv6Addr != nil {
 			cidrStrings = append(cidrStrings, ipv6Addr.String())
 		}
 	} else {
 		// multi-networking enabled clusters
-		cidrStrings, err = ca.performMultiNetworkCIDRAllocation(node, instance.NetworkInterfaces)
+		hasNodeLabels, defaultSubnet, defaultPodRange := getNodeDefaultLabels(node)
+		// if there's no node label get the cidrStrings with the old way by comparing the default Network and GNP
+		cidrStrings, err = ca.performMultiNetworkCIDRAllocation(node, instance.NetworkInterfaces, hasNodeLabels)
 		if err != nil {
-			nodeutil.RecordNodeStatusChange(ca.recorder, node, "CIDRNotAvailable")
-			return fmt.Errorf("failed to get cidr(s) from provider: %v", err)
+			nodeutil.RecordNodeStatusChange(ca.recorder, node, "AnnotationsNotAvailable")
+			return fmt.Errorf("failed to perform node annotations for multi-networking: %v", err)
 		}
-	}
-	if len(cidrStrings) == 0 {
-		nodeutil.RecordNodeStatusChange(ca.recorder, node, "CIDRNotAvailable")
-		return fmt.Errorf("failed to allocate cidr: Node %v has no CIDRs", node.Name)
-	}
-	// Can have at most 2 ips (one for v4 and one for v6)
-	if len(cidrStrings) > 2 {
-		klog.InfoS("Got more than 2 ips, truncating to 2", "cidrStrings", cidrStrings)
-		cidrStrings = cidrStrings[:2]
-	}
-
-	cidrs, err := netutils.ParseCIDRs(cidrStrings)
-	if err != nil {
-		return fmt.Errorf("failed to parse strings %v as CIDRs: %v", cidrStrings, err)
-	}
-	if len(cidrs) > 1 {
-		if dualStack, _ := netutils.IsDualStackCIDRs(cidrs); !dualStack {
-			return fmt.Errorf("err: IPs are not dual stack, CIDRS: %v", cidrStrings)
+		if hasNodeLabels {
+			cidrStrings = ca.extractDefaultNwCIDRs(instance.NetworkInterfaces, defaultSubnet, defaultPodRange)
 		}
 	}
 
-	node.Spec.PodCIDR = cidrStrings[0]
-	node.Spec.PodCIDRs = cidrStrings
-
-	err = ca.updateNodeCIDR(node, oldNode)
-	if err != nil {
+	// update Node.Spec.PodCIDR(s)
+	if err = ca.updateNodePodCIDRWithCidrStrings(oldNode, node, cidrStrings); err != nil {
 		return err
 	}
+
 	if !reflect.DeepEqual(node.Annotations, oldNode.Annotations) {
 		// retain old north interfaces annotation
 		var oldNorthInterfacesAnnotation networkv1.NorthInterfacesAnnotation
@@ -409,6 +379,34 @@ func (ca *cloudCIDRAllocator) updateCIDRAllocation(nodeName string) error {
 	return err
 }
 
+// updateNodePodCIDRWithCidrStrings update the Node object with Spec.PodCIDR(s),
+// returns error if cidrStrings is not valid or fails to update the Node object
+func (ca *cloudCIDRAllocator) updateNodePodCIDRWithCidrStrings(oldNode *v1.Node, node *v1.Node, cidrStrings []string) error {
+	if len(cidrStrings) == 0 {
+		nodeutil.RecordNodeStatusChange(ca.recorder, node, "CIDRNotAvailable")
+		return fmt.Errorf("failed to allocate cidr: Node %v has no CIDRs", node.Name)
+	}
+	// Can have at most 2 ips (one for v4 and one for v6)
+	if len(cidrStrings) > 2 {
+		klog.InfoS("Got more than 2 ips, truncating to 2", "cidrStrings", cidrStrings)
+		cidrStrings = cidrStrings[:2]
+	}
+
+	cidrs, err := netutils.ParseCIDRs(cidrStrings)
+	if err != nil {
+		return fmt.Errorf("failed to parse strings %v as CIDRs: %v", cidrStrings, err)
+	}
+	if len(cidrs) > 1 {
+		if dualStack, _ := netutils.IsDualStackCIDRs(cidrs); !dualStack {
+			return fmt.Errorf("err: IPs are not dual stack, CIDRS: %v", cidrStrings)
+		}
+	}
+	node.Spec.PodCIDR = cidrStrings[0]
+	node.Spec.PodCIDRs = cidrStrings
+
+	return ca.updateNodeCIDR(node, oldNode)
+}
+
 func (ca *cloudCIDRAllocator) setNetworkCondition(node *v1.Node) {
 	cond := v1.NodeCondition{
 		Type:               v1.NodeNetworkUnavailable,

pkg/controller/nodeipam/ipam/cloud_cidr_allocator_test.go

@@ -40,6 +40,7 @@ import (
 	clSetFake "k8s.io/cloud-provider-gcp/crd/client/network/clientset/versioned/fake"
 	networkinformers "k8s.io/cloud-provider-gcp/crd/client/network/informers/externalversions"
 	"k8s.io/cloud-provider-gcp/pkg/controller/testutil"
+	utilnode "k8s.io/cloud-provider-gcp/pkg/util/node"
 	"k8s.io/cloud-provider-gcp/providers/gce"
 	metricsUtil "k8s.io/component-base/metrics/testutil"
 )
@@ -281,45 +282,6 @@ func TestUpdateCIDRAllocation(t *testing.T) {
 			},
 			expectedUpdate: true,
 		},
-		{
-			name: "ipv6 single stack",
-			fakeNodeHandler: &testutil.FakeNodeHandler{
-				Existing: []*v1.Node{
-					{
-						ObjectMeta: metav1.ObjectMeta{
-							Name: "test",
-						},
-						Spec: v1.NodeSpec{
-							ProviderID: "gce://test-project/us-central1-b/test",
-						},
-					},
-				},
-				Clientset: fake.NewSimpleClientset(),
-			},
-			gceInstance: []*compute.Instance{
-				{
-					Name: "test",
-					NetworkInterfaces: []*compute.NetworkInterface{
-						{
-							Ipv6Address: "2001:db9::110",
-						},
-					},
-				},
-			},
-			nodeChanges: func(node *v1.Node) {
-				node.Spec.PodCIDR = "2001:db9::/112"
-				node.Spec.PodCIDRs = []string{"2001:db9::/112"}
-				node.Status.Conditions = []v1.NodeCondition{
-					{
-						Type:    "NetworkUnavailable",
-						Status:  "False",
-						Reason:  "RouteCreated",
-						Message: "NodeController create implicit route",
-					},
-				}
-			},
-			expectedUpdate: true,
-		},
 		{
 			name: "want error - incorrect cidr",
 			fakeNodeHandler: &testutil.FakeNodeHandler{
@@ -531,6 +493,64 @@ func TestUpdateCIDRAllocation(t *testing.T) {
 			expectedUpdate:  true,
 			expectedMetrics: map[string]float64{},
 		},
+		{
+			name: "[mn] default network only, get PodCIDR with node labels",
+			networks: []*networkv1.Network{
+				network(networkv1.DefaultPodNetworkName, defaultGKENetworkParamsName, false),
+			},
+			gkeNwParams: []*networkv1.GKENetworkParamSet{
+				gkeNetworkParams(defaultGKENetworkParamsName, defaultVPCName, defaultVPCSubnetName, nil),
+			},
+			fakeNodeHandler: &testutil.FakeNodeHandler{
+				Existing: []*v1.Node{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "test",
+							Labels: map[string]string{
+								utilnode.NodePoolSubnetLabelPrefix:   "default",
+								utilnode.NodePoolPodRangeLabelPrefix: defaultSecondaryRangeA,
+							},
+						},
+						Spec: v1.NodeSpec{
+							ProviderID: "gce://test-project/us-central1-b/test",
+						},
+						Status: v1.NodeStatus{
+							Capacity: v1.ResourceList{},
+						},
+					},
+				},
+				Clientset: fake.NewSimpleClientset(),
+			},
+			gceInstance: []*compute.Instance{
+				{
+					Name: "test",
+					NetworkInterfaces: []*compute.NetworkInterface{
+						interfaces(defaultVPCName, defaultVPCSubnetName, "80.1.172.1", []*compute.AliasIpRange{
+							{IpCidrRange: "192.168.1.0/24", SubnetworkRangeName: defaultSecondaryRangeA},
+							{IpCidrRange: "10.11.1.0/24", SubnetworkRangeName: defaultSecondaryRangeB},
+						}),
+					},
+				},
+			},
+			nodeChanges: func(node *v1.Node) {
+				node.Spec.PodCIDR = "192.168.1.0/24"
+				node.Spec.PodCIDRs = []string{"192.168.1.0/24"}
+				node.Status.Conditions = []v1.NodeCondition{
+					{
+						Type:    "NetworkUnavailable",
+						Status:  "False",
+						Reason:  "RouteCreated",
+						Message: "NodeController create implicit route",
+					},
+				}
+				node.Annotations = map[string]string{
+					networkv1.NorthInterfacesAnnotationKey: "[]",
+					networkv1.MultiNetworkAnnotationKey:    "[]",
+				}
+			},
+			expectedUpdate:  true,
+			expectedMetrics: map[string]float64{},
+		},
 		{
 			name: "[mn] one additional network along with default network",
 			networks: []*networkv1.Network{

pkg/controller/nodeipam/ipam/multinetwork_cloud_cidr_allocator.go

@@ -11,18 +11,19 @@ import (
 	"k8s.io/apimachinery/pkg/api/resource"
 	"k8s.io/apimachinery/pkg/labels"
 	networkv1 "k8s.io/cloud-provider-gcp/crd/apis/network/v1"
+	utilnode "k8s.io/cloud-provider-gcp/pkg/util/node"
 	"k8s.io/klog/v2"
 	netutils "k8s.io/utils/net"
 )
 
 // performMultiNetworkCIDRAllocation receives the existing Node object and its
 // GCE interfaces, and is updated with the corresponding annotations for
 // MultiNetwork and NorthInterfaces and the capacity for the additional networks.
-// It also returns calculated cidrs for default Network.
+// It also returns calculated cidrs for default Network if there're no node labels.
 //
 // NorthInterfacesAnnotationKey is modified on Network Ready condition changes.
 // MultiNetworkAnnotationKey is modified on Node's NodeNetworkAnnotationKey changes.
-func (ca *cloudCIDRAllocator) performMultiNetworkCIDRAllocation(node *v1.Node, interfaces []*compute.NetworkInterface) (defaultNwCIDRs []string, err error) {
+func (ca *cloudCIDRAllocator) performMultiNetworkCIDRAllocation(node *v1.Node, interfaces []*compute.NetworkInterface, hasNodeLabels bool) (defaultNwCIDRs []string, err error) {
 	northInterfaces := networkv1.NorthInterfacesAnnotation{}
 	additionalNodeNetworks := networkv1.MultiNetworkAnnotation{}
 
@@ -96,13 +97,16 @@ func (ca *cloudCIDRAllocator) performMultiNetworkCIDRAllocation(node *v1.Node, i
 				}
 				klog.V(2).InfoS("found an allocatable secondary range for the interface on network", "nodeName", node.Name, "networkName", network.Name)
 				processedNetworks[network.Name] = struct{}{}
-				if networkv1.IsDefaultNetwork(network.Name) {
+				// for defaultNwCIDRs, if there're no NodeLabels keep this,
+				// otherwise get the CIDR with labels
+				if networkv1.IsDefaultNetwork(network.Name) && !hasNodeLabels {
 					defaultNwCIDRs = append(defaultNwCIDRs, ipRange.IpCidrRange)
 					ipv6Addr := ca.cloud.GetIPV6Address(inf)
 					if ipv6Addr != nil {
 						defaultNwCIDRs = append(defaultNwCIDRs, ipv6Addr.String())
 					}
-				} else {
+				}
+				if !networkv1.IsDefaultNetwork(network.Name) {
 					northInterfaces = append(northInterfaces, networkv1.NorthInterface{Network: network.Name, IpAddress: inf.NetworkIP})
 					if _, ok := upStatusNetworks[network.Name]; ok {
 						additionalNodeNetworks = append(additionalNodeNetworks, networkv1.NodeNetwork{Name: network.Name, Scope: "host-local", Cidrs: []string{ipRange.IpCidrRange}})
@@ -118,6 +122,42 @@ func (ca *cloudCIDRAllocator) performMultiNetworkCIDRAllocation(node *v1.Node, i
 	return defaultNwCIDRs, nil
 }
 
+// getNodeDefaultLabels returns true if the node has labels for subnet and Pod range
+func getNodeDefaultLabels(node *v1.Node) (bool, string, string) {
+	defaultSubnet, foundSubnet := node.Labels[utilnode.NodePoolSubnetLabelPrefix]
+	defaultPodRange, foundRange := node.Labels[utilnode.NodePoolPodRangeLabelPrefix]
+	if !foundSubnet || defaultSubnet == "" || !foundRange || defaultPodRange == "" {
+		return false, "", ""
+	}
+	return true, defaultSubnet, defaultPodRange
+}
+
+// extractDefaultNwCIDRs returns the Pod CIDRs for default Network.
+// Different subnet can have the same secondary range name, here uses the subnet and range name
+// to find the matching CIDR(s)
+func (ca *cloudCIDRAllocator) extractDefaultNwCIDRs(interfaces []*compute.NetworkInterface, defaultSubnet, defaultPodRange string) (defaultNwCIDRs []string) {
+out:
+	for _, inf := range interfaces {
+		// extra the subnetwork name from the URL
+		parts := strings.Split(inf.Subnetwork, "/subnetworks/")
+		if parts[1] != defaultSubnet {
+			continue
+		}
+		for _, ipRange := range inf.AliasIpRanges {
+			if ipRange.SubnetworkRangeName != defaultPodRange {
+				continue
+			}
+			defaultNwCIDRs = append(defaultNwCIDRs, ipRange.IpCidrRange)
+			ipv6Addr := ca.cloud.GetIPV6Address(inf)
+			if ipv6Addr != nil {
+				defaultNwCIDRs = append(defaultNwCIDRs, ipv6Addr.String())
+			}
+			break out
+		}
+	}
+	return defaultNwCIDRs
+}
+
 func updateAnnotations(node *v1.Node, northInterfaces networkv1.NorthInterfacesAnnotation, additionalNodeNetworks networkv1.MultiNetworkAnnotation) error {
 	northInterfaceAnn, err := networkv1.MarshalNorthInterfacesAnnotation(northInterfaces)
 	if err != nil {

pkg/util/node/node.go

@@ -30,6 +30,16 @@ import (
 	"k8s.io/klog/v2"
 )
 
+// Labels definitions.
+const (
+	// NodePoolPodRangeLabelPrefix is the prefix for the default Pod range
+	// name for the node and it can be different with cluster Pod range
+	NodePoolPodRangeLabelPrefix = "cloud.google.com/gke-np-default-pod-range"
+	// NodePoolSubnetLabelPrefix is the prefix for the default subnet
+	// name for the node
+	NodePoolSubnetLabelPrefix = "cloud.google.com/gke-np-default-subnet"
+)
+
 type nodeForConditionPatch struct {
 	Status nodeStatusForPatch `json:"status"`
 }