Merge pull request #679 from mmamczur/multi-subnet

L4 ILB - skip nodes from the non default network if multi subnet is t…

Author: k8s-ci-robot

providers/gce/gce_loadbalancer_internal.go

@@ -50,6 +50,8 @@ const (
 	maxInstancesPerInstanceGroup = 1000
 	// maxL4ILBPorts is the maximum number of ports that can be specified in an L4 ILB Forwarding Rule. Beyond this, "AllPorts" field should be used.
 	maxL4ILBPorts = 5
+	// labelGKESubnetworkName is the key of the label that contains the subnet name the node is connected to.
+	labelGKESubnetworkName = "cloud.google.com/gke-node-pool-subnet"
 )
 
 func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v1.Service, existingFwdRule *compute.ForwardingRule, nodes []*v1.Node) (*v1.LoadBalancerStatus, error) {
@@ -254,6 +256,54 @@ func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v
 	return status, nil
 }
 
+func removeNodesInNonDefaultNetworks(nodes []*v1.Node, defaultSubnetName string) []*v1.Node {
+	var newList []*v1.Node
+	var skippedNodes []string
+	for _, node := range nodes {
+		subnetLabel, ok := node.Labels[labelGKESubnetworkName]
+		// nodes that have no label and no PodCIDR should be filtered out.
+		// This translates to: if node doesn't have the label but has PodCIDR then it is assumed to be in the default network.
+		// This check is a safeguard for situations when the cluster might be running older node controller that does not know multi-subnet or multi-subnet feature misbehaves.
+		if !ok && node.Spec.PodCIDR == "" {
+			skippedNodes = append(skippedNodes, node.Name)
+			continue
+		}
+		// For clusters that become multi-subnet the label on existing nodes from the default network can be present with an emtpy value.
+		if ok && subnetLabel != "" && subnetLabel != defaultSubnetName {
+			skippedNodes = append(skippedNodes, node.Name)
+			continue
+		}
+		newList = append(newList, node)
+	}
+	countOfSkippedNodes := len(skippedNodes)
+	if len(skippedNodes) > 0 {
+		klog.V(2).Infof("Skipped %d nodes from non default subnetworks. First skipped nodes: %v", countOfSkippedNodes, truncateList(skippedNodes, 10))
+	}
+	return newList
+}
+
+// Extract the subnet name from the URL.
+// example: for `https://www.googleapis.com/compute/v1/projects/project/regions/us-central1/subnetworks/defaultSubnet`
+// this will return `defaultSubnet`.
+func subnetNameFromURL(url string) (string, error) {
+	resource, err := cloud.ParseResourceURL(url)
+	if err != nil {
+		return "", err
+	}
+	if resource.Key == nil {
+		return "", fmt.Errorf("subnet URL %s is missing the name", url)
+	}
+	return resource.Key.Name, nil
+}
+
+// truncates a list - will return a sublist of at most max elements.
+func truncateList[T any](l []T, max int) []T {
+	if len(l) <= max {
+		return l
+	}
+	return l[:max]
+}
+
 func (g *Cloud) clearPreviousInternalResources(svc *v1.Service, loadBalancerName string, existingBackendService *compute.BackendService, expectedBSName, expectedHCName string) {
 	// If a new backend service was created, delete the old one.
 	if existingBackendService.Name != expectedBSName {
@@ -626,6 +676,17 @@ func (g *Cloud) ensureInternalInstanceGroup(name, zone string, nodes []*v1.Node)
 // ensureInternalInstanceGroups generates an unmanaged instance group for every zone
 // where a K8s node exists. It also ensures that each node belongs to an instance group
 func (g *Cloud) ensureInternalInstanceGroups(name string, nodes []*v1.Node) ([]string, error) {
+	defaultSubnetName, err := subnetNameFromURL(g.SubnetworkURL())
+	// Perform node filtering only if the subnet URL is valid. Do not stop execution in case some clusters have invalid SubnetworkURL configured.
+	if err == nil {
+		// Filter out any node that is not from the default network. This is required for multi-subnet feature.
+		// This should not change behavior for nodes that are in the default network.
+		// This can't be done earlier when listing node since the code is shared between internal and external LBs.
+		nodes = removeNodesInNonDefaultNetworks(nodes, defaultSubnetName)
+	} else {
+		klog.Errorf("invalid subnetwork URL configured for the controller, assuming all nodes are in the default subnetwork %s, err: %v", g.SubnetworkURL(), err)
+	}
+
 	zonedNodes := splitNodesByZone(nodes)
 	klog.V(2).Infof("ensureInternalInstanceGroups(%v): %d nodes over %d zones in region %v", name, len(nodes), len(zonedNodes), g.region)
 	var igLinks []string

providers/gce/gce_loadbalancer_internal_test.go

@@ -199,6 +199,177 @@ func TestEnsureMultipleInstanceGroups(t *testing.T) {
 	}
 }
 
+func TestEnsureInstanceGroupFromDefaultNetworkMultiSubnetClusterMode(t *testing.T) {
+	t.Parallel()
+
+	vals := DefaultTestClusterValues()
+	vals.SubnetworkURL = "https://www.googleapis.com/compute/v1/projects/project/regions/us-central1/subnetworks/defaultSubnet"
+	gce, err := fakeGCECloud(vals)
+	require.NoError(t, err)
+
+	nodes, err := createAndInsertNodes(gce, []string{"n1", "n2", "n3", "n4", "n5"}, vals.ZoneName)
+	require.NoError(t, err)
+	// node with a matching subnet
+	nodes[0].Labels[labelGKESubnetworkName] = "defaultSubnet"
+	// node with a label of a non-matching subnet
+	nodes[1].Labels[labelGKESubnetworkName] = "anotherSubnet"
+	// node with no label but a PodCIDR
+	nodes[2].Spec.PodCIDR = "10.0.5.0/24"
+	// node[3] has no label nor PodCIDR
+	nodes[3].Spec.PodCIDR = ""
+	// node[4] has a label that contains an empty string (this indicates the default network).
+	nodes[4].Spec.PodCIDR = ""
+	nodes[4].Labels[labelGKESubnetworkName] = ""
+
+	baseName := makeInstanceGroupName(vals.ClusterID)
+
+	igsFromCloud, err := gce.ensureInternalInstanceGroups(baseName, nodes)
+	require.NoError(t, err)
+
+	url, err := cloud.ParseResourceURL(igsFromCloud[0])
+	require.NoError(t, err)
+	instances, err := gce.ListInstancesInInstanceGroup(url.Key.Name, url.Key.Zone, "ALL")
+	require.NoError(t, err)
+	assert.Len(t, instances, 3, "Incorrect number of Instances in the group")
+	var instanceURLs []string
+	for _, inst := range instances {
+		instanceURLs = append(instanceURLs, inst.Instance)
+	}
+	if !hasInstanceForNode(instances, nodes[0]) {
+		t.Errorf("expected n1 to be in instances but it contained %+v", instanceURLs)
+	}
+	if hasInstanceForNode(instances, nodes[1]) {
+		t.Errorf("expected n2 to NOT be in instances but it was included %+v", instanceURLs)
+	}
+	if !hasInstanceForNode(instances, nodes[2]) {
+		t.Errorf("expected n3 to be in instances but it contained %+v", instanceURLs)
+	}
+	if hasInstanceForNode(instances, nodes[3]) {
+		t.Errorf("expected n4 to NOT be in instances but it was included %+v", instanceURLs)
+	}
+	if !hasInstanceForNode(instances, nodes[4]) {
+		t.Errorf("expected n5 to be in instances but it contained %+v", instanceURLs)
+	}
+}
+
+// Test that the node filtering will not be done if the cluster subnetwork value is not set properly.
+func TestEnsureInstanceGroupFromDefaultNetworkMultiSubnetClusterModeIfSubnetworkValueIsInvalid(t *testing.T) {
+	t.Parallel()
+
+	vals := DefaultTestClusterValues()
+	vals.SubnetworkURL = "invalid"
+	gce, err := fakeGCECloud(vals)
+	require.NoError(t, err)
+
+	nodes, err := createAndInsertNodes(gce, []string{"n1", "n2"}, vals.ZoneName)
+	require.NoError(t, err)
+	// node with a matching subnet
+	nodes[0].Labels[labelGKESubnetworkName] = "defaultSubnet"
+	// node with a label of a non-matching subnet
+	nodes[1].Labels[labelGKESubnetworkName] = "anotherSubnet"
+
+	baseName := makeInstanceGroupName(vals.ClusterID)
+
+	igsFromCloud, err := gce.ensureInternalInstanceGroups(baseName, nodes)
+	require.NoError(t, err)
+
+	url, err := cloud.ParseResourceURL(igsFromCloud[0])
+	require.NoError(t, err)
+	instances, err := gce.ListInstancesInInstanceGroup(url.Key.Name, url.Key.Zone, "ALL")
+	require.NoError(t, err)
+	assert.Len(t, instances, 2, "Incorrect number of Instances in the group")
+	var instanceURLs []string
+	for _, inst := range instances {
+		instanceURLs = append(instanceURLs, inst.Instance)
+	}
+	if !hasInstanceForNode(instances, nodes[0]) {
+		t.Errorf("expected n1 to be in instances but it contained %+v", instanceURLs)
+	}
+	if !hasInstanceForNode(instances, nodes[1]) {
+		t.Errorf("expected n2 to be in instances but it contained %+v", instanceURLs)
+	}
+}
+
+func hasInstanceForNode(instances []*compute.InstanceWithNamedPorts, node *v1.Node) bool {
+	for _, instance := range instances {
+		if strings.HasSuffix(instance.Instance, node.Name) {
+			return true
+		}
+	}
+	return false
+}
+
+func TestRemoveNodesInNonDefaultNetworks(t *testing.T) {
+	t.Parallel()
+
+	testInput := []struct {
+		node                    *v1.Node
+		shouldBeInDefaultSubnet bool
+	}{
+		{
+			node: &v1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:   "defaultSubnetNodeWithLabel",
+					Labels: map[string]string{labelGKESubnetworkName: "defaultSubnet"},
+				},
+			},
+			shouldBeInDefaultSubnet: true,
+		},
+		{
+			node: &v1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:   "nonDefaultSubnetNode",
+					Labels: map[string]string{labelGKESubnetworkName: "secondarySubnet"},
+				},
+			},
+			shouldBeInDefaultSubnet: false,
+		},
+		{
+			node: &v1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:   "defaultSubnetNodeWithEmptyLabel",
+					Labels: map[string]string{labelGKESubnetworkName: ""},
+				},
+			},
+			shouldBeInDefaultSubnet: true,
+		},
+		{
+			node: &v1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "defaultSubnetNodeWithoutLabel",
+				},
+				Spec: v1.NodeSpec{PodCIDR: "10.0.0.0/28"},
+			},
+			shouldBeInDefaultSubnet: true,
+		},
+		{
+			node: &v1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "nodeInUnknownSubnet",
+				},
+			},
+			shouldBeInDefaultSubnet: false,
+		},
+	}
+	var nodes []*v1.Node
+	for _, testNode := range testInput {
+		nodes = append(nodes, testNode.node)
+	}
+
+	onlyDefaultSubnetNodes := removeNodesInNonDefaultNetworks(nodes, "defaultSubnet")
+
+	defaultSubnetNodesSet := make(map[string]struct{})
+	for _, node := range onlyDefaultSubnetNodes {
+		defaultSubnetNodesSet[node.Name] = struct{}{}
+	}
+	for _, testNode := range testInput {
+		_, hasNode := defaultSubnetNodesSet[testNode.node.Name]
+		if testNode.shouldBeInDefaultSubnet != hasNode {
+			t.Errorf("Node %s should not be in the default subnet but it was present in %v", testNode.node.Name, defaultSubnetNodesSet)
+		}
+	}
+}
+
 func TestEnsureInternalLoadBalancer(t *testing.T) {
 	t.Parallel()
 
@@ -2017,3 +2188,48 @@ func TestEnsureInternalLoadBalancerAllPorts(t *testing.T) {
 	}
 	assertInternalLbResourcesDeleted(t, gce, svc, vals, true)
 }
+
+func TestSubnetNameFromURL(t *testing.T) {
+	cases := []struct {
+		desc     string
+		url      string
+		wantName string
+		wantErr  bool
+	}{
+		{
+			desc:     "full URL",
+			url:      "https://www.googleapis.com/compute/v1/projects/project/regions/us-central1/subnetworks/defaultSubnet",
+			wantName: "defaultSubnet",
+		},
+		{
+			desc:     "project path",
+			url:      "projects/project/regions/us-central1/subnetworks/defaultSubnet",
+			wantName: "defaultSubnet",
+		},
+		{
+			desc:    "missing name",
+			url:     "projects/project/regions/us-central1/subnetworks",
+			wantErr: true,
+		},
+		{
+			desc:    "invalid",
+			url:     "invalid",
+			wantErr: true,
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.desc, func(t *testing.T) {
+			subnetName, err := subnetNameFromURL(tc.url)
+			if err != nil && !tc.wantErr {
+				t.Errorf("unexpected error %v", err)
+			}
+			if err == nil && tc.wantErr {
+				t.Errorf("wanted an error but got none")
+			}
+			if !tc.wantErr && subnetName != tc.wantName {
+				t.Errorf("invalid name extracted from URL %s, want=%s, got=%s", tc.url, tc.wantName, subnetName)
+			}
+		})
+	}
+
+}

providers/gce/gce_loadbalancer_utils_test.go

@@ -114,6 +114,10 @@ func createAndInsertNodes(gce *Cloud, nodeNames []string, zoneName string) ([]*v
 						v1.LabelTopologyZone: zoneName,
 					},
 				},
+				Spec: v1.NodeSpec{
+					// Add PodCIDR for multi subnet purpose. Nodes need to have PodCIDR to be regarded as nodes from the default subnet (unless they have the subnet label).
+					PodCIDR: "192.168.0.0/0",
+				},
 			},
 		)