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)