NEEDS-JIRA: Fix node.kubernetes.io/exclude-from-external-load-balancers on masters by mdbooth · Pull Request #125 · openshift/cloud-provider-gcp

mdbooth · 2026-07-03T09:27:27Z

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.

Manual backport of #121

Additionally backports some small test fixes required for the tests to compile and pass. Required so we can execute the new test added for the bugfix.

/hold until the parent PRs merge

Cherry-picked from upstream kubernetes/cloud-provider-gcp commit d3a674c

…nsureExternalLoadBalancerRBSFinalizer Commit 94e1834 (upstream PR kubernetes#771) added a test case expecting that ensureExternalLoadBalancer returns ImplementedElsewhere when a service carries NetLBFinalizerV3. In upstream this worked because ensureExternalLoadBalancer called usesL4RBS(), which was updated by the same commit to check both V2 and V3 finalizers. On the 4.19 branch, ensureExternalLoadBalancer uses explicit per-condition checks (introduced by c6fdbaf / upstream 97e991b) rather than usesL4RBS(), so only ELBRbsFinalizer (V2) is checked. The V3 test case was therefore brought in without the production code path that makes it pass, causing a runtime test failure. Remove the test case until the corresponding production code fix is backported to this branch.

…akeGCECloud NewFakeGCECloud sets fields on the Cloud struct from TestClusterValues but was missing the mapping from vals.SubnetworkURL to gce.unsafeSubnetworkURL. This caused g.SubnetworkURL() to return an empty string in tests that set vals.SubnetworkURL, which in turn made subnetNameFromURL() fail and skip node filtering entirely in ensureInternalInstanceGroups. This is the minimum required change from upstream commit b8caeb9 (PR kubernetes#647: "Add syncDefaultPodRanges change for Multi-networking default Param and Network"), which introduced SubnetworkURL into TestClusterValues and wired it through NewFakeGCECloud. Only that wiring is extracted here; the rest of PR kubernetes#647 is unrelated to this branch.

…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. Manual backport of 52387cc

coderabbitai · 2026-07-03T09:27:36Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 8d7115fb-3635-498f-98a8-7971add5a2ca

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands.}

openshift-ci · 2026-07-03T09:28:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign nrb for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2026-07-03T12:43:47Z

@mdbooth: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

JoelSpeed · 2026-07-03T13:07:44Z

@@ -215,7 +215,7 @@ func (g *Cloud) UpdateLoadBalancer(ctx context.Context, clusterName string, svc
 	// GCE load balancers do not support services with LoadBalancerClass set. LoadBalancerClass can't be updated for an existing load balancer, so here we don't need to clean any resources.
 	// Check API documentation for .Spec.LoadBalancerClass for details on when this field is allowed to be changed.
 	if svc.Spec.LoadBalancerClass != nil {
-		klog.Infof("Ignoring service %s/%s using load balancer class %s, it is not supported by this controller.", svc.Namespace, svc.Name, svc.Spec.LoadBalancerClass)
+		klog.Infof("Ignoring service %s/%s using load balancer class %s, it is not supported by this controller.", svc.Namespace, svc.Name, *svc.Spec.LoadBalancerClass)


This is fixing up an existing carry?

The tests literally didn't compile upstream either 😬 The repo history around this time is wild.

JoelSpeed · 2026-07-03T13:09:27Z

+			// To avoid this we explicitly exclude the bootstrap machine from
+			// consideration if it is present.


How will this affect API server availability during the bootstrap process?

It doesn't. All this means is that we ignore the presence of the bootstrap server when re-using the instance group for an internal loadbalancer. This code doesn't handle the API server LB. Assume that's CAPI?

Are you sure? There are two API servers, one public, one private. The CCM is the only thing attaching new control plane nodes to either once the cluster is bootstrapped. So if there's a risk that this logic removes the bootstrap node from the instance group before it's time, then that's a possible interference with the bootstrapping process

Well the e2es will tell us for sure. But the effect of this change is to allow the IG containing the bootstrap node to be added to an ILB (by ignoring its presence), not to prevent it.

mdbooth added 4 commits July 3, 2026 10:21

UPSTREAM: 720: Fix mismatched format string for LoadBalancerClass

c4e59db

Cherry-picked from upstream kubernetes/cloud-provider-gcp commit d3a674c

openshift-ci Bot requested review from JoelSpeed and sub-mod July 3, 2026 09:28

openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 3, 2026

mdbooth force-pushed the OCPBUGS-84569-4.19 branch 2 times, most recently from 55e4188 to b616902 Compare July 3, 2026 10:02

UPSTREAM: <drop>: go mod vendor

a3228af

mdbooth force-pushed the OCPBUGS-84569-4.19 branch from b616902 to a3228af Compare July 3, 2026 10:03

JoelSpeed reviewed Jul 3, 2026

View reviewed changes

Navigation Menu

Uh oh!

NEEDS-JIRA: Fix node.kubernetes.io/exclude-from-external-load-balancers on masters - #125

NEEDS-JIRA: Fix node.kubernetes.io/exclude-from-external-load-balancers on masters#125
mdbooth wants to merge 5 commits into
openshift:release-4.19from
mdbooth:OCPBUGS-84569-4.19

mdbooth commented Jul 3, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jul 3, 2026 •

edited

Loading

Review skipped

Uh oh!

openshift-ci Bot commented Jul 3, 2026

Uh oh!

openshift-ci Bot commented Jul 3, 2026

Uh oh!

JoelSpeed Jul 3, 2026

Uh oh!

mdbooth Jul 3, 2026

Uh oh!

JoelSpeed Jul 3, 2026

Uh oh!

mdbooth Jul 3, 2026

Uh oh!

JoelSpeed Jul 6, 2026

Uh oh!

mdbooth Jul 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		// To avoid this we explicitly exclude the bootstrap machine from
		// consideration if it is present.

Uh oh!

Conversation

mdbooth commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

openshift-ci Bot commented Jul 3, 2026

Uh oh!

openshift-ci Bot commented Jul 3, 2026

Uh oh!

JoelSpeed Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

mdbooth Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

mdbooth Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

mdbooth Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mdbooth commented Jul 3, 2026 •

edited

Loading

coderabbitai Bot commented Jul 3, 2026 •

edited

Loading