From c1024c47cbc24804611d555264719dd7632fbd36 Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Tue, 30 Jun 2026 13:23:14 +0100 Subject: [PATCH 1/2] UPSTREAM: : Replace use of deprecated sets.String --- providers/gce/gce_loadbalancer_internal_openshift.go | 12 ++++++------ .../gce/gce_loadbalancer_internal_openshift_test.go | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/providers/gce/gce_loadbalancer_internal_openshift.go b/providers/gce/gce_loadbalancer_internal_openshift.go index 23ee03485c..8a2d0df92b 100644 --- a/providers/gce/gce_loadbalancer_internal_openshift.go +++ b/providers/gce/gce_loadbalancer_internal_openshift.go @@ -65,7 +65,7 @@ func (g *Cloud) filterNodesWithExistingExternalInstanceGroups(name string, nodes } // Track instances that are already managed by existing external instance groups - instancesInExistingInstanceGroups := sets.NewString() + instancesInExistingInstanceGroups := sets.New[string]() candidateExternalInstanceGroups, err := g.candidateExternalInstanceGroups(zone) if err != nil { @@ -122,8 +122,8 @@ func filterNodeObjectFromName(nodesInZone []*v1.Node, nodeNames []string) []*v1. } // extractInstanceNamesFromGroup extracts instance names from a list of instances in an instance group. -func extractInstanceNamesFromGroup(instances []*compute.InstanceWithNamedPorts) sets.String { - instanceNames := sets.NewString() +func extractInstanceNamesFromGroup(instances []*compute.InstanceWithNamedPorts) sets.Set[string] { + instanceNames := sets.New[string]() for _, ins := range instances { // Extract instance name from URL path (e.g., ".../instances/node-name") parts := strings.Split(ins.Instance, "/") @@ -134,7 +134,7 @@ func extractInstanceNamesFromGroup(instances []*compute.InstanceWithNamedPorts) // evaluateExternalInstanceGroup determines if an external instance group can be reused. // It returns whether the group should be reused and the set of instance names in the group. -func (g *Cloud) evaluateExternalInstanceGroup(ig *compute.InstanceGroup, zone string, gceHostNamesInZone sets.String) (shouldReuse bool, instanceNames sets.String, err error) { +func (g *Cloud) evaluateExternalInstanceGroup(ig *compute.InstanceGroup, zone string, gceHostNamesInZone sets.Set[string]) (shouldReuse bool, instanceNames sets.Set[string], err error) { // Get all instances in this external instance group instances, err := g.ListInstancesInInstanceGroup(ig.Name, zone, allInstances) if err != nil { @@ -187,14 +187,14 @@ func (g *Cloud) candidateExternalInstanceGroups(zone string) ([]*compute.Instanc } // gceInstanceNamesInZone returns a set of GCE Host names from the list of nodes provided -func (g *Cloud) gceInstanceNamesInZone(zoneNodes []*v1.Node) (sets.String, error) { +func (g *Cloud) gceInstanceNamesInZone(zoneNodes []*v1.Node) (sets.Set[string], error) { // hosts is a list of GCE instances matching the zone's node names. hosts, err := g.getFoundInstanceByNames(nodeNames(zoneNodes)) if err != nil { return nil, err } - names := sets.NewString() + names := sets.New[string]() for _, h := range hosts { names.Insert(h.Name) } diff --git a/providers/gce/gce_loadbalancer_internal_openshift_test.go b/providers/gce/gce_loadbalancer_internal_openshift_test.go index 50090f8266..261f64159e 100644 --- a/providers/gce/gce_loadbalancer_internal_openshift_test.go +++ b/providers/gce/gce_loadbalancer_internal_openshift_test.go @@ -118,7 +118,7 @@ func TestEvaluateExternalInstanceGroup(t *testing.T) { masterIG, err := gce.GetInstanceGroup(masterIGName, zone) require.NoError(t, err) - gceHostNames := sets.NewString( + gceHostNames := sets.New( testInfraName+"-master-0", testInfraName+"-worker-a-wnjp7", testInfraName+"-infra-a-zztd5", From 14028110ed519fc7e681dab78c095b05f9b1de96 Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Tue, 30 Jun 2026 17:37:27 +0100 Subject: [PATCH 2/2] UPSTREAM: : Fix node.kubernetes.io/exclude-from-external-load-balancers on masters We previously used allHaveNodePrefix() to ensure that if a master instance group contained the bootstrap machine this would not cause it to be excluded from re-use. This worked because the bootstrap machine's name has the same prefix as all other machines in the cluster. However, this logic unconditionally always included the master instance group. If the masters are labelled with node.kubernetes.io/exclude-from-external-load-balancers the CCM framework code will have filtered the Nodes before passing them to provider-GCP. But this logic meant we added them anyway, sending unintended traffic to the control plane machines. --- .../gce_loadbalancer_internal_openshift.go | 46 ++-- ...ce_loadbalancer_internal_openshift_test.go | 213 ++++++++++++++++++ 2 files changed, 230 insertions(+), 29 deletions(-) diff --git a/providers/gce/gce_loadbalancer_internal_openshift.go b/providers/gce/gce_loadbalancer_internal_openshift.go index 8a2d0df92b..1a5ea259aa 100644 --- a/providers/gce/gce_loadbalancer_internal_openshift.go +++ b/providers/gce/gce_loadbalancer_internal_openshift.go @@ -17,6 +17,7 @@ limitations under the License. package gce import ( + "fmt" "slices" "strings" @@ -144,39 +145,26 @@ func (g *Cloud) evaluateExternalInstanceGroup(ig *compute.InstanceGroup, zone st // Extract instance names from the group instanceNames = extractInstanceNamesFromGroup(instances) - // If all instances in this external instance group are also in our zone's node list, - // or they all have the node instance prefix, we can reuse this instance group instead - // of creating our own internal instance group - hasAll := gceHostNamesInZone.HasAll(instanceNames.UnsortedList()...) - allHavePrefix := g.allHaveNodePrefix(instanceNames.UnsortedList()) - shouldReuse = hasAll || allHavePrefix - klog.V(2).Infof("evaluateExternalInstanceGroup(%v): shouldReuse=%v (hasAll=%v, allHavePrefix=%v), instances=%v", ig.Name, shouldReuse, hasAll, allHavePrefix, instanceNames.UnsortedList()) + // During bootstrap the bootstrap machine is placed in one of the master + // instance groups. However, the bootstrap machine does not have an + // associated Node. This will prevent us from considering this instance + // group for reuse because the instance group contains an instance which is + // not in the service's Node list. Consequently we will attempt to add the + // masters to a second instance group, which will fail with + // INSTANCE_IN_MULTIPLE_LOAD_BALANCED_IGS. + // + // To avoid this we explicitly exclude the bootstrap machine from + // consideration if it is present. + bootstrapInstanceName := fmt.Sprintf("%s-bootstrap", g.nodeInstancePrefix) + instanceNames.Delete(bootstrapInstanceName) + + // If all instances in this external instance group are also in our zone's node list + shouldReuse = gceHostNamesInZone.HasAll(instanceNames.UnsortedList()...) && instanceNames.Len() > 0 + klog.V(2).Infof("evaluateExternalInstanceGroup(%v): shouldReuse=%v, instances=%v", ig.Name, shouldReuse, instanceNames.UnsortedList()) return shouldReuse, instanceNames, nil } -// allHaveNodePrefix checks if all instances have the cluster's node instance prefix. -// -// DO NOT REMOVE. This is load-bearing for all OCP GCP clusters. -// -// The OpenShift installer creates per-zone master instance groups (e.g. -// -master-). During CAPG installs (OCPBUGS-35256), the bootstrap -// node is placed in the same master IG as a control plane node. The bootstrap -// node is not a k8s node, so HasAll rejects the master IG. Without this prefix -// fallback the CCM then tries to add the master to its own k8s-ig, which fails -// with INSTANCE_IN_MULTIPLE_LOAD_BALANCED_IGS (master is already in the -// installer's IG). It then tries to add the worker to that same k8s-ig -// alongside the master, which fails with wrongSubnetwork (masters and workers -// are on different subnets). -func (g *Cloud) allHaveNodePrefix(instances []string) bool { - for _, instance := range instances { - if !strings.HasPrefix(instance, g.nodeInstancePrefix) { - return false - } - } - return true -} - // candidateExternalInstanceGroups returns instance groups with the external instance groups prefix, if defined. func (g *Cloud) candidateExternalInstanceGroups(zone string) ([]*compute.InstanceGroup, error) { if g.externalInstanceGroupsPrefix == "" { diff --git a/providers/gce/gce_loadbalancer_internal_openshift_test.go b/providers/gce/gce_loadbalancer_internal_openshift_test.go index 261f64159e..bbc4d3db13 100644 --- a/providers/gce/gce_loadbalancer_internal_openshift_test.go +++ b/providers/gce/gce_loadbalancer_internal_openshift_test.go @@ -20,6 +20,7 @@ package gce import ( "fmt" + "slices" "testing" "github.com/stretchr/testify/assert" @@ -40,6 +41,10 @@ func fqdnName(short string) string { return fmt.Sprintf("%s.c.%s.internal", short, "test-project") } +func igSelfLink(zone, role string) string { + return fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/test-project/zones/%s/instanceGroups/%s", zone, testInfraName+"-"+role+"-"+zone) +} + func TestFilterNodeObjectFromName(t *testing.T) { t.Parallel() for name, tc := range map[string]struct { @@ -95,6 +100,214 @@ func TestFilterNodeObjectFromName(t *testing.T) { } } +func TestFilterNodesWithExistingExternalInstanceGroups(t *testing.T) { + t.Parallel() + + vals := DefaultTestClusterValues() + zoneA := vals.ZoneName // us-central1-b + zoneB := vals.SecondaryZoneName // us-central1-c + + type testNode struct{ name, zone string } + + testNodes := []testNode{ + {name: "master-0", zone: zoneA}, + {name: "worker-a-wnjp7", zone: zoneA}, + {name: "infra-a-zztd5", zone: zoneA}, + {name: "master-1", zone: zoneB}, + {name: "worker-b-s48dq", zone: zoneB}, + {name: "infra-b-2bn6x", zone: zoneB}, + } + + nodesFn := func(fqdn bool) []*v1.Node { + r := make([]*v1.Node, len(testNodes)) + for i, t := range testNodes { + r[i] = &v1.Node{ObjectMeta: metav1.ObjectMeta{ + Name: testInfraName + "-" + t.name, + Labels: map[string]string{v1.LabelTopologyZone: t.zone}}} + if fqdn { + r[i].Name = fqdnName(r[i].Name) + } + } + + return r + } + + // setupFake creates a fake GCE cloud with instances and master IGs in both zones. + // igZoneAInstances allows the set of instances in the zoneA master IG to be overridden. + setupFake := func(t *testing.T, prefix string, igZoneAInstances []string) *Cloud { + t.Helper() + gce, err := fakeGCECloud(vals) + require.NoError(t, err) + gce.nodeInstancePrefix = testInfraName + gce.externalInstanceGroupsPrefix = prefix + + // Create GCE instances for the test nodes + for _, testNode := range testNodes { + require.NoError(t, gce.InsertInstance(gce.ProjectID(), testNode.zone, &compute.Instance{ + Name: testInfraName + "-" + testNode.name, Tags: &compute.Tags{Items: []string{testNode.name}}, Zone: testNode.zone, + })) + } + + // Create GCE instances for any additional instances specified for the first instance group. + for _, inst := range igZoneAInstances { + if !slices.ContainsFunc(testNodes, func(t testNode) bool { return testInfraName+"-"+t.name == inst }) { + require.NoError(t, gce.InsertInstance(gce.ProjectID(), zoneA, &compute.Instance{ + Name: inst, Tags: &compute.Tags{Items: []string{inst}}, Zone: zoneA, + })) + } + } + + // By default the first instance group contains the first master + // instance, but it can be overridden. + if igZoneAInstances == nil { + igZoneAInstances = []string{testInfraName + "-master-0"} + } + + // Create a GCE instance group for each test zone + for _, ig := range []struct { + zone, name string + members []string + }{ + {zoneA, testInfraName + "-master-" + zoneA, igZoneAInstances}, + {zoneB, testInfraName + "-master-" + zoneB, []string{testInfraName + "-master-1"}}, + } { + require.NoError(t, gce.CreateInstanceGroup(&compute.InstanceGroup{Name: ig.name}, ig.zone)) + for _, member := range ig.members { + require.NoError(t, gce.AddInstancesToInstanceGroup(ig.name, ig.zone, []*compute.InstanceReference{ + {Instance: fmt.Sprintf("zones/%s/instances/%s", ig.zone, member)}, + })) + } + } + + return gce + } + + nodeNames := func(nodes []*v1.Node) sets.Set[string] { + names := sets.New[string]() + for _, n := range nodes { + names.Insert(n.Name) + } + return names + } + + for name, tc := range map[string]struct { + useFQDNNodeNames bool + externalInstanceGroupsPrefix string + lbIGName string + igZoneAInstances []string // instances in zoneA master IG; nil = default (master-0) + excludeNodeNames []string // nodes to remove before calling the function, simulating CCM pre-filtering + wantIGs []string + wantExcludedNodes []string + }{ + "FQDN nodes covered by external master IGs are filtered out and workers and infra remain": { + useFQDNNodeNames: true, + externalInstanceGroupsPrefix: testInfraName, + lbIGName: "k8s-ig--test-lb", + wantIGs: []string{ + igSelfLink(zoneA, "master"), + igSelfLink(zoneB, "master"), + }, + wantExcludedNodes: []string{fqdnName(testInfraName + "-master-0"), fqdnName(testInfraName + "-master-1")}, + }, + "no external instance groups prefix configured so all nodes remain": { + externalInstanceGroupsPrefix: "", + lbIGName: "k8s-ig--test-lb", + wantIGs: nil, + }, + // The LB IG name matches an existing external IG name in zoneA. + // That IG is skipped; only the zoneB master IG is reused. + "external IG with same name as LB IG is skipped": { + externalInstanceGroupsPrefix: testInfraName, + lbIGName: testInfraName + "-master-" + zoneA, + wantIGs: []string{igSelfLink(zoneB, "master")}, + wantExcludedNodes: []string{testInfraName + "-master-1"}, + }, + "bootstrap instance in master IG is ignored and IG is still reused": { + externalInstanceGroupsPrefix: testInfraName, + lbIGName: "k8s-ig--test-lb", + igZoneAInstances: []string{testInfraName + "-master-0", testInfraName + "-bootstrap"}, + wantIGs: []string{ + igSelfLink(zoneA, "master"), + igSelfLink(zoneB, "master"), + }, + wantExcludedNodes: []string{testInfraName + "-master-0", testInfraName + "-master-1"}, + }, + "bootstrap-only instance group is not reused": { + externalInstanceGroupsPrefix: testInfraName, + lbIGName: "k8s-ig--test-lb", + igZoneAInstances: []string{testInfraName + "-bootstrap"}, + wantIGs: []string{igSelfLink(zoneB, "master")}, + wantExcludedNodes: []string{testInfraName + "-master-1"}, + }, + "empty instance group is not reused": { + externalInstanceGroupsPrefix: testInfraName, + lbIGName: "k8s-ig--test-lb", + igZoneAInstances: []string{}, + wantIGs: []string{igSelfLink(zoneB, "master")}, + wantExcludedNodes: []string{testInfraName + "-master-1"}, + }, + "IG with unknown instance is not reused": { + externalInstanceGroupsPrefix: testInfraName, + lbIGName: "k8s-ig--test-lb", + igZoneAInstances: []string{"other-cluster-master-0"}, + wantIGs: []string{igSelfLink(zoneB, "master")}, + wantExcludedNodes: []string{testInfraName + "-master-1"}, + }, + "all IG instances are known nodes so IG is reused": { + externalInstanceGroupsPrefix: testInfraName, + lbIGName: "k8s-ig--test-lb", + wantIGs: []string{ + igSelfLink(zoneA, "master"), + igSelfLink(zoneB, "master"), + }, + wantExcludedNodes: []string{testInfraName + "-master-0", testInfraName + "-master-1"}, + }, + // Masters labelled node.kubernetes.io/exclude-from-external-load-balancers are + // filtered by the CCM framework before the cloud provider is called. The master + // instance groups still exist on GCP and contain the master instances, but because + // gceHostNamesInZone is derived only from the nodes passed in (workers+infra), + // HasAll fails for those IGs and they must not be reused. The old allHaveNodePrefix + // fallback would have returned shouldReuse=true here (all masters share the infra + // prefix), incorrectly sending traffic to the control plane. + "masters pre-filtered by CCM label: master IGs not reused, workers and infra get internal IGs": { + externalInstanceGroupsPrefix: testInfraName, + lbIGName: "k8s-ig--test-lb", + excludeNodeNames: []string{ + testInfraName + "-master-0", + testInfraName + "-master-1", + }, + wantIGs: nil, // master IGs must not be reused + wantExcludedNodes: nil, // all remaining nodes (workers+infra) need internal IGs + }, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + gce := setupFake(t, tc.externalInstanceGroupsPrefix, tc.igZoneAInstances) + nodes := nodesFn(tc.useFQDNNodeNames) + if len(tc.excludeNodeNames) > 0 { + excludeSet := sets.New(tc.excludeNodeNames...) + var kept []*v1.Node + for _, n := range nodes { + if !excludeSet.Has(n.Name) { + kept = append(kept, n) + } + } + nodes = kept + } + filteredNodes, existingIGLinks, err := gce.filterNodesWithExistingExternalInstanceGroups(tc.lbIGName, nodes) + require.NoError(t, err) + + assert.ElementsMatch(t, tc.wantIGs, existingIGLinks) + + // Assert that filteredNodes contains all nodes except the excluded ones. + allNodeNames := nodeNames(nodes) + wantIncluded := allNodeNames.Difference(sets.New(tc.wantExcludedNodes...)) + assert.ElementsMatch(t, wantIncluded.UnsortedList(), nodeNames(filteredNodes).UnsortedList()) + }) + } +} + func TestEvaluateExternalInstanceGroup(t *testing.T) { t.Parallel() vals := DefaultTestClusterValues()