From e4f817738375abe55ae01d7a6f3fc3db2abd2eb9 Mon Sep 17 00:00:00 2001 From: Theo Barber-Bany Date: Thu, 14 Aug 2025 16:36:15 +0100 Subject: [PATCH] UPSTREAM: : Reuse instance-groups for internal load balancers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit rewrites 49f5389e47bae94d1baa3aff39e32cfb61ef3775. Work around GCP internal load balancer restrictions for multi-subnet clusters. GCP internal load balancers have specific restrictions that prevent straightforward load balancing across multiple subnets: 1. "Don't put a VM in more than one load-balanced instance group" 2. Instance groups can "only select VMs that are in the same zone, VPC network, and subnet" 3. "All VMs in an instance group must have their primary network interface in the same VPC network" 4. Internal LBs can load balance to VMs in same region but different subnets For clusters with nodes across multiple subnets, the previous implementation would fail to create internal load balancers. This change implements a two-pass approach: 1. Find existing external instance groups (matching externalInstanceGroupsPrefix) that contain ONLY cluster nodes and reuse them for the backend service 2. Create internal instance groups only for remaining nodes not covered by external groups This ensures compliance with GCP restrictions while enabling multi-subnet load balancing for Kubernetes clusters. References: - Internal LB docs: https://cloud.google.com/load-balancing/docs/internal - Backend service restrictions: https://cloud.google.com/load-balancing/docs/backend-service#restrictions_and_guidance - Instance group constraints: https://cloud.google.com/compute/docs/instance-groups/creating-groups-of-unmanaged-instances#addinstances 🤖 Commit message & comments Generated with [Claude Code](https://claude.ai/code) --- providers/gce/gce.go | 75 ++++--- providers/gce/gce_fake.go | 2 +- providers/gce/gce_instancegroup.go | 21 ++ providers/gce/gce_loadbalancer_internal.go | 11 +- .../gce_loadbalancer_internal_openshift.go | 184 ++++++++++++++++++ .../gce/gce_loadbalancer_internal_test.go | 69 +++++++ 6 files changed, 328 insertions(+), 34 deletions(-) create mode 100644 providers/gce/gce_loadbalancer_internal_openshift.go diff --git a/providers/gce/gce.go b/providers/gce/gce.go index 85f7b9c3f0..d99d89abe2 100644 --- a/providers/gce/gce.go +++ b/providers/gce/gce.go @@ -206,6 +206,10 @@ type Cloud struct { // enableDiscretePortForwarding enables forwarding of individual ports // instead of port ranges in Forwarding Rules for external load balancers. enableDiscretePortForwarding bool + + // externalInstanceGroupsPrefix if set, finds instance groups with + // the provided prefix and considers them for ILB backends. + externalInstanceGroupsPrefix string } // ConfigGlobal is the in memory representation of the gce.conf config data @@ -243,6 +247,10 @@ type ConfigGlobal struct { // Default to none. // For example: MyFeatureFlag AlphaFeatures []string `gcfg:"alpha-features"` + + // ExternalInstanceGroupsPrefix, when not-empty, is used to filter instance groups (from an external GCP Project) + // and include them in the backend for ILB. + ExternalInstanceGroupsPrefix string `gcfg:"external-instance-groups-prefix"` } // ConfigFile is the struct used to parse the /etc/gce.conf configuration file. @@ -270,13 +278,14 @@ type CloudConfig struct { SubnetworkName string SubnetworkURL string // DEPRECATED: Do not rely on this value as it may be incorrect. - SecondaryRangeName string - NodeTags []string - NodeInstancePrefix string - TokenSource oauth2.TokenSource - UseMetadataServer bool - AlphaFeatureGate *AlphaFeatureGate - StackType string + SecondaryRangeName string + NodeTags []string + NodeInstancePrefix string + TokenSource oauth2.TokenSource + UseMetadataServer bool + AlphaFeatureGate *AlphaFeatureGate + StackType string + ExternalInstanceGroupsPrefix string } func init() { @@ -367,6 +376,7 @@ func generateCloudConfig(configFile *ConfigFile) (cloudConfig *CloudConfig, err cloudConfig.NodeTags = configFile.Global.NodeTags cloudConfig.NodeInstancePrefix = configFile.Global.NodeInstancePrefix cloudConfig.AlphaFeatureGate = NewAlphaFeatureGate(configFile.Global.AlphaFeatures) + cloudConfig.ExternalInstanceGroupsPrefix = configFile.Global.ExternalInstanceGroupsPrefix } // retrieve projectID and zone @@ -545,31 +555,32 @@ func CreateGCECloud(config *CloudConfig) (*Cloud, error) { operationPollRateLimiter := flowcontrol.NewTokenBucketRateLimiter(5, 5) // 5 qps, 5 burst. gce := &Cloud{ - service: service, - serviceAlpha: serviceAlpha, - serviceBeta: serviceBeta, - containerService: containerService, - tpuService: tpuService, - projectID: projID, - networkProjectID: netProjID, - onXPN: onXPN, - region: config.Region, - regional: config.Regional, - localZone: config.Zone, - managedZones: config.ManagedZones, - networkURL: networkURL, - unsafeIsLegacyNetwork: isLegacyNetwork, - unsafeSubnetworkURL: subnetURL, - secondaryRangeName: config.SecondaryRangeName, - nodeTags: config.NodeTags, - nodeInstancePrefix: config.NodeInstancePrefix, - useMetadataServer: config.UseMetadataServer, - operationPollRateLimiter: operationPollRateLimiter, - AlphaFeatureGate: config.AlphaFeatureGate, - nodeZones: map[string]sets.String{}, - metricsCollector: newLoadBalancerMetrics(), - projectsBasePath: getProjectsBasePath(service.BasePath), - stackType: StackType(config.StackType), + service: service, + serviceAlpha: serviceAlpha, + serviceBeta: serviceBeta, + containerService: containerService, + tpuService: tpuService, + projectID: projID, + networkProjectID: netProjID, + onXPN: onXPN, + region: config.Region, + regional: config.Regional, + localZone: config.Zone, + managedZones: config.ManagedZones, + networkURL: networkURL, + unsafeIsLegacyNetwork: isLegacyNetwork, + unsafeSubnetworkURL: subnetURL, + secondaryRangeName: config.SecondaryRangeName, + nodeTags: config.NodeTags, + nodeInstancePrefix: config.NodeInstancePrefix, + useMetadataServer: config.UseMetadataServer, + operationPollRateLimiter: operationPollRateLimiter, + AlphaFeatureGate: config.AlphaFeatureGate, + nodeZones: map[string]sets.String{}, + metricsCollector: newLoadBalancerMetrics(), + projectsBasePath: getProjectsBasePath(service.BasePath), + stackType: StackType(config.StackType), + externalInstanceGroupsPrefix: config.ExternalInstanceGroupsPrefix, } gce.manager = &gceServiceManager{gce} diff --git a/providers/gce/gce_fake.go b/providers/gce/gce_fake.go index dfafcc216d..1c27eec0f0 100644 --- a/providers/gce/gce_fake.go +++ b/providers/gce/gce_fake.go @@ -77,7 +77,7 @@ func NewFakeGCECloud(vals TestClusterValues) *Cloud { gce := &Cloud{ region: vals.Region, service: service, - managedZones: []string{vals.ZoneName}, + managedZones: []string{vals.ZoneName, vals.SecondaryZoneName}, localZone: vals.ZoneName, projectID: vals.ProjectID, networkProjectID: vals.ProjectID, diff --git a/providers/gce/gce_instancegroup.go b/providers/gce/gce_instancegroup.go index 46c58792b6..7b96b21e72 100644 --- a/providers/gce/gce_instancegroup.go +++ b/providers/gce/gce_instancegroup.go @@ -20,6 +20,8 @@ limitations under the License. package gce import ( + "fmt" + compute "google.golang.org/api/compute/v1" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud" @@ -71,6 +73,25 @@ func (g *Cloud) ListInstanceGroups(zone string) ([]*compute.InstanceGroup, error return v, mc.Observe(err) } +// ListInstanceGroupsWithPrefix lists all InstanceGroups in the project and +// zone with given prefix. +// When the prefix is empty it lists all the instance groups. +func (g *Cloud) ListInstanceGroupsWithPrefix(zone string, prefix string) ([]*compute.InstanceGroup, error) { + ctx, cancel := cloud.ContextWithCallTimeout() + defer cancel() + + mc := newInstanceGroupMetricContext("list", zone) + f := filter.None + + if prefix != "" { + f = filter.Regexp("name", fmt.Sprintf("%s.*", prefix)) + } + + v, err := g.c.InstanceGroups().List(ctx, zone, f) + + return v, mc.Observe(err) +} + // ListInstancesInInstanceGroup lists all the instances in a given // instance group and state. func (g *Cloud) ListInstancesInInstanceGroup(name string, zone string, state string) ([]*compute.InstanceWithNamedPorts, error) { diff --git a/providers/gce/gce_loadbalancer_internal.go b/providers/gce/gce_loadbalancer_internal.go index 562d417127..800871d688 100644 --- a/providers/gce/gce_loadbalancer_internal.go +++ b/providers/gce/gce_loadbalancer_internal.go @@ -720,7 +720,14 @@ func (g *Cloud) ensureInternalInstanceGroups(name string, nodes []*v1.Node) ([]s 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) + // Filter nodes that are already in existing external instance groups. + // Returns nodes needing internal instance groups and existing IG links to reuse. + filteredNodes, existingIgLinks, err := g.filterNodesWithExistingExternalInstanceGroups(name, nodes) + if err != nil { + return nil, err + } + + zonedNodes := splitNodesByZone(filteredNodes) klog.V(2).Infof("ensureInternalInstanceGroups(%v): %d nodes over %d zones in region %v", name, len(nodes), len(zonedNodes), g.region) emptyZoneNodesNames := sets.NewString() @@ -733,6 +740,8 @@ func (g *Cloud) ensureInternalInstanceGroups(name string, nodes []*v1.Node) ([]s } var igLinks []string + igLinks = append(igLinks, existingIgLinks...) + for zone, nodes := range zonedNodes { if zone == "" { continue // skip ensuring nodes with empty zone diff --git a/providers/gce/gce_loadbalancer_internal_openshift.go b/providers/gce/gce_loadbalancer_internal_openshift.go new file mode 100644 index 0000000000..e0a3b69c77 --- /dev/null +++ b/providers/gce/gce_loadbalancer_internal_openshift.go @@ -0,0 +1,184 @@ +/* +Copyright 2024 Red Hat, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package gce + +import ( + "slices" + "strings" + + "google.golang.org/api/compute/v1" + v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/sets" +) + +// filterNodesWithExistingExternalInstanceGroups filters out nodes that are already managed by +// existing external instance groups matching externalInstanceGroupsPrefix. This implements +// the OpenShift-specific logic for reusing external instance groups to work around GCP +// internal load balancer restrictions for multi-subnet clusters. +// +// Algorithm: Work around GCP internal load balancer restrictions for multi-subnet clusters. +// GCP documentation: +// - Internal LBs can load balance to VMs in same region but different subnets +// - VMs cannot be in more than one instance group, regardless of LB backend. (backend service restrictions) +// - Instance groups can "only select VMs that are in the same zone, VPC network, and subnet" +// - "All VMs in an instance group must have their primary network interface in the same VPC network" +// +// For clusters with nodes across multiple subnets, we use a two-pass approach: +// Pass 1: Find existing external instance groups (matching externalInstanceGroupsPrefix) +// +// that contain ONLY our cluster nodes and reuse them for the backend service. +// +// Pass 2: Create internal instance groups only for remaining nodes not covered by external groups. +// This ensures compliance with GCP restrictions while enabling multi-subnet load balancing. +func (g *Cloud) filterNodesWithExistingExternalInstanceGroups(name string, nodes []*v1.Node) ([]*v1.Node, []string, error) { + zonedNodes := splitNodesByZone(nodes) + var existingIGLinks []string + var filteredNodes []*v1.Node + + // instancesNeedingInternalInstanceGroups tracks instances that don't have + // pre-existing external instance groups defined by the externalInstanceGroupsPrefix. + // They should map to nodes 1:1. + + // This separation is necessary to comply with GCP's restriction that VMs cannot be in + // more than one load-balanced instance group. + instancesNeedingInternalInstanceGroups := map[string][]string{} + + for zone, nodesInZone := range zonedNodes { + gceHostNamesInZone, err := g.gceInstanceNamesInZone(nodesInZone) + if err != nil { + return nil, nil, err + } + + // Track instances that are already managed by existing external instance groups + instancesInExistingInstanceGroups := sets.NewString() + + candidateExternalInstanceGroups, err := g.candidateExternalInstanceGroups(zone) + if err != nil { + return nil, nil, err + } + + for _, ig := range candidateExternalInstanceGroups { + if strings.EqualFold(ig.Name, name) { + continue + } + + shouldReuse, instanceNames, err := g.evaluateExternalInstanceGroup(ig, zone, gceHostNamesInZone) + if err != nil { + return nil, nil, err + } + + if shouldReuse { + existingIGLinks = append(existingIGLinks, ig.SelfLink) + instancesInExistingInstanceGroups.Insert(instanceNames.UnsortedList()...) + } + } + + // Determine which instances (nodes) in this zone need internal instance groups created. + // These are instances that exist in the zone but are not already managed by external instance groups. + if remainingInstances := gceHostNamesInZone.Difference(instancesInExistingInstanceGroups).UnsortedList(); len(remainingInstances) > 0 { + instancesNeedingInternalInstanceGroups[zone] = remainingInstances + } + } + + // Build the filtered node list from instances that need internal instance groups + for zone, nodeNames := range instancesNeedingInternalInstanceGroups { + allNodesInZone := zonedNodes[zone] + nodesForInternalInstanceGroup := filterNodeObjectFromName(allNodesInZone, nodeNames) + filteredNodes = append(filteredNodes, nodesForInternalInstanceGroup...) + } + + return filteredNodes, existingIGLinks, nil +} + +// filterNodeObjectFromName takes a list of *v1.Node and a list of node names, +// and returns the *v1.Node objects that match the provided names. +func filterNodeObjectFromName(nodesInZone []*v1.Node, nodeNames []string) []*v1.Node { + filteredNodes := []*v1.Node{} + + for _, node := range nodesInZone { + if slices.Contains(nodeNames, node.Name) { + filteredNodes = append(filteredNodes, node) + } + } + return filteredNodes +} + +// extractInstanceNamesFromGroup extracts instance names from a list of instances in an instance group. +func extractInstanceNamesFromGroup(instances []*compute.InstanceWithNamedPorts) sets.String { + instanceNames := sets.NewString() + for _, ins := range instances { + // Extract instance name from URL path (e.g., ".../instances/node-name") + parts := strings.Split(ins.Instance, "/") + instanceNames.Insert(parts[len(parts)-1]) + } + return instanceNames +} + +// 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) { + // Get all instances in this external instance group + instances, err := g.ListInstancesInInstanceGroup(ig.Name, zone, allInstances) + if err != nil { + return false, nil, err + } + + // 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 + shouldReuse = gceHostNamesInZone.HasAll(instanceNames.UnsortedList()...) || g.allHaveNodePrefix(instanceNames.UnsortedList()) + + return shouldReuse, instanceNames, nil +} + +// allHaveNodePrefix checks if all instances have the cluster's node instance prefix. +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 == "" { + return nil, nil + } + + return g.ListInstanceGroupsWithPrefix(zone, g.externalInstanceGroupsPrefix) +} + +// gceInstanceNamesInZone returns a set of GCE Host names from the list of nodes provided +func (g *Cloud) gceInstanceNamesInZone(zoneNodes []*v1.Node) (sets.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() + for _, h := range hosts { + names.Insert(h.Name) + } + + return names, nil +} diff --git a/providers/gce/gce_loadbalancer_internal_test.go b/providers/gce/gce_loadbalancer_internal_test.go index 94f1a52636..b45d646493 100644 --- a/providers/gce/gce_loadbalancer_internal_test.go +++ b/providers/gce/gce_loadbalancer_internal_test.go @@ -1099,6 +1099,75 @@ func TestEnsureLoadBalancerDeletedSucceedsOnXPN(t *testing.T) { checkEvent(t, recorder, FirewallChangeMsg, true) } +func TestEnsureInternalInstanceGroupsReuseGroups(t *testing.T) { + vals := DefaultTestClusterValues() + gce, err := fakeGCECloud(vals) + require.NoError(t, err) + gce.externalInstanceGroupsPrefix = "pre-existing" + + igName := makeInstanceGroupName(vals.ClusterID) + nodesA, err := createAndInsertNodes(gce, []string{"test-node-1", "test-node-2"}, vals.ZoneName) + require.NoError(t, err) + nodesB, err := createAndInsertNodes(gce, []string{"test-node-3"}, vals.SecondaryZoneName) + require.NoError(t, err) + + preIGName := "pre-existing-ig" + err = gce.CreateInstanceGroup(&compute.InstanceGroup{Name: preIGName}, vals.ZoneName) + require.NoError(t, err) + err = gce.CreateInstanceGroup(&compute.InstanceGroup{Name: preIGName}, vals.SecondaryZoneName) + require.NoError(t, err) + err = gce.AddInstancesToInstanceGroup(preIGName, vals.ZoneName, gce.ToInstanceReferences(vals.ZoneName, []string{"test-node-1"})) + require.NoError(t, err) + err = gce.AddInstancesToInstanceGroup(preIGName, vals.SecondaryZoneName, gce.ToInstanceReferences(vals.SecondaryZoneName, []string{"test-node-3"})) + require.NoError(t, err) + + anotherPreIGName := "another-existing-ig" + err = gce.CreateInstanceGroup(&compute.InstanceGroup{Name: anotherPreIGName}, vals.ZoneName) + require.NoError(t, err) + err = gce.AddInstancesToInstanceGroup(anotherPreIGName, vals.ZoneName, gce.ToInstanceReferences(vals.ZoneName, []string{"test-node-2"})) + require.NoError(t, err) + + svc := fakeLoadbalancerService(string(LBTypeInternal)) + svc, err = gce.client.CoreV1().Services(svc.Namespace).Create(context.TODO(), svc, metav1.CreateOptions{}) + assert.NoError(t, err) + _, err = gce.ensureInternalLoadBalancer( + vals.ClusterName, vals.ClusterID, + svc, + nil, + append(nodesA, nodesB...), + ) + assert.NoError(t, err) + + backendServiceName := makeBackendServiceName(gce.GetLoadBalancerName(context.TODO(), "", svc), vals.ClusterID, shareBackendService(svc), cloud.SchemeInternal, "TCP", svc.Spec.SessionAffinity) + bs, err := gce.GetRegionBackendService(backendServiceName, gce.region) + require.NoError(t, err) + assert.Equal(t, 3, len(bs.Backends), "Want three backends referencing three instances groups") + + igRef := func(zone, name string) string { + return fmt.Sprintf("zones/%s/instanceGroups/%s", zone, name) + } + for _, name := range []string{igRef(vals.ZoneName, preIGName), igRef(vals.SecondaryZoneName, preIGName), igRef(vals.ZoneName, igName)} { + var found bool + for _, be := range bs.Backends { + if strings.Contains(be.Group, name) { + found = true + break + } + } + assert.True(t, found, "Expected list of backends to have group %q", name) + } + + // Expect initial zone to have test-node-2 + instances, err := gce.ListInstancesInInstanceGroup(igName, vals.ZoneName, "ALL") + require.NoError(t, err) + assert.Equal(t, 1, len(instances)) + assert.Contains( + t, + instances[0].Instance, + fmt.Sprintf("%s/zones/%s/instances/%s", vals.ProjectID, vals.ZoneName, "test-node-2"), + ) +} + func TestEnsureInternalInstanceGroupsDeleted(t *testing.T) { vals := DefaultTestClusterValues() gce, err := fakeGCECloud(vals)