Skip to content

OCPBUGS-85243: Set aws-load-balancer-scheme on public HCP router service #8458

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 1 commit into from
May 28, 2026
Merged

OCPBUGS-85243: Set aws-load-balancer-scheme on public HCP router service #8458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion control-plane-operator/controllers/hostedcontrolplane/infra/infra_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1475,7 +1475,8 @@ func TestReconcileHCPRouterServices(t *testing.T) {
Name: "router",
Namespace: namespace,
Annotations: map[string]string{
"service.beta.kubernetes.io/aws-load-balancer-type": "nlb",
"service.beta.kubernetes.io/aws-load-balancer-type": "nlb",
"service.beta.kubernetes.io/aws-load-balancer-scheme": "internet-facing",
},
Labels: map[string]string{"app": "private-router"},
},
Expand All @@ -1497,6 +1498,7 @@ func TestReconcileHCPRouterServices(t *testing.T) {
return publicService(append(m, func(s *corev1.Service) {
s.Name = "private-router"
s.Annotations["service.beta.kubernetes.io/aws-load-balancer-internal"] = "true"
delete(s.Annotations, "service.beta.kubernetes.io/aws-load-balancer-scheme")
})...)
}
withCrossZoneAnnotation := func(svc *corev1.Service) {
Expand Down
1 change: 1 addition & 0 deletions ...ane/infra/testdata/zz_fixture_TestReconcileInfrastructure_AWS_PublicAndPrivate_Route.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions ...dcontrolplane/infra/testdata/zz_fixture_TestReconcileInfrastructure_AWS_Public_Route.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions control-plane-operator/controllers/hostedcontrolplane/ingress/router.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,15 @@ func ReconcileRouterService(svc *corev1.Service, internal, crossZoneLoadBalancin
svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-type"] = "nlb"
if internal {
svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-internal"] = "true"
delete(svc.Annotations, "service.beta.kubernetes.io/aws-load-balancer-scheme")
} else {
delete(svc.Annotations, "service.beta.kubernetes.io/aws-load-balancer-internal")
// The AWS Load Balancer Controller (used on EKS) defaults to scheme=internal:
// https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.4/guide/service/annotations/
// The in-tree AWS cloud provider (used on OpenShift) defaults to internet-facing:
// https://cloud-provider-aws.sigs.k8s.io/service_controller/
// Set the annotation explicitly so the public router works on both.
svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-scheme"] = "internet-facing"

@jparrill jparrill May 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): The PR description mentions "clean up mutually exclusive annotations" but the production code only adds -- it never deletes the conflicting annotation. While the current architecture uses separate Service objects for public/private (so the transition scenario can't happen today), defensive cleanup costs nothing and follows the pattern already used in oauth/service.go:77:

if internal {
	svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-internal"] = "true"
	delete(svc.Annotations, "service.beta.kubernetes.io/aws-load-balancer-scheme")
} else {
	svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-scheme"] = "internet-facing"
	delete(svc.Annotations, "service.beta.kubernetes.io/aws-load-balancer-internal")
}

This would also make the code match what the infra_test.go fixture helper already expects (the privateService helper does delete(s.Annotations, "...scheme")).

@typeid typeid May 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks! Updated.

@sdminonne sdminonne May 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 thanks both

}
Comment thread
typeid marked this conversation as resolved.
if crossZoneLoadBalancingEnabled {
// In-tree AWS cloud provider annotation for cross-zone load balancing (OpenShift management clusters).
Expand Down
65 changes: 65 additions & 0 deletions control-plane-operator/controllers/hostedcontrolplane/ingress/router_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package ingress
import (
"testing"

. "github.com/onsi/gomega"

hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
"github.com/openshift/hypershift/support/netutil"

Expand Down Expand Up @@ -56,6 +58,69 @@ func TestReconcileRouterServiceAnnotations(t *testing.T) {
}
}

// When reconciling an external (non-internal) AWS router service
// it should set aws-load-balancer-scheme to internet-facing.
func TestReconcileRouterService_WhenExternalAWS_ItShouldSetInternetFacingScheme(t *testing.T) {

@jparrill jparrill May 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Good test -- gomega + Gherkin naming, positive and negative assertions. Consider adding a complementary subtest that pre-populates the conflicting annotation to verify cleanup:

t.Run("When switching from internal to external it should remove the internal annotation", func(t *testing.T) {
	g := NewGomegaWithT(t)
	svc := &corev1.Service{}
	svc.Annotations = map[string]string{
		"service.beta.kubernetes.io/aws-load-balancer-internal": "true",
	}
	err := ReconcileRouterService(svc, false, true, hcp)
	g.Expect(err).ToNot(HaveOccurred())
	g.Expect(svc.Annotations).To(HaveKeyWithValue(
		"service.beta.kubernetes.io/aws-load-balancer-scheme", "internet-facing"))
	g.Expect(svc.Annotations).ToNot(HaveKey(
		"service.beta.kubernetes.io/aws-load-balancer-internal"))
})

This would pair with the defensive delete() calls suggested in router.go.

@typeid typeid May 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test case for the switching back and forth.

g := NewGomegaWithT(t)

hcp := &hyperv1.HostedControlPlane{}
hcp.Spec.Platform.Type = hyperv1.AWSPlatform

svc := &corev1.Service{}

err := ReconcileRouterService(svc, false /* internal */, true /* cross-zone */, hcp)
g.Expect(err).ToNot(HaveOccurred())

g.Expect(svc.Annotations).To(HaveKeyWithValue(
"service.beta.kubernetes.io/aws-load-balancer-scheme", "internet-facing"))
g.Expect(svc.Annotations).ToNot(HaveKey(
"service.beta.kubernetes.io/aws-load-balancer-internal"))
}

func TestReconcileRouterService_WhenSwitchingMode_ItShouldRemoveConflictingAnnotation(t *testing.T) {
tests := []struct {
name string
internal bool
preExisting map[string]string
expectKey string
expectValue string
expectAbsent string
}{
{
name: "When switching from internal to external it should remove the internal annotation",
internal: false,
preExisting: map[string]string{"service.beta.kubernetes.io/aws-load-balancer-internal": "true"},
expectKey: "service.beta.kubernetes.io/aws-load-balancer-scheme",
expectValue: "internet-facing",
expectAbsent: "service.beta.kubernetes.io/aws-load-balancer-internal",
},
{
name: "When switching from external to internal it should remove the scheme annotation",
internal: true,
preExisting: map[string]string{"service.beta.kubernetes.io/aws-load-balancer-scheme": "internet-facing"},
expectKey: "service.beta.kubernetes.io/aws-load-balancer-internal",
expectValue: "true",
expectAbsent: "service.beta.kubernetes.io/aws-load-balancer-scheme",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
g := NewGomegaWithT(t)

hcp := &hyperv1.HostedControlPlane{}
hcp.Spec.Platform.Type = hyperv1.AWSPlatform

svc := &corev1.Service{}
svc.Annotations = tt.preExisting

err := ReconcileRouterService(svc, tt.internal, true /* cross-zone */, hcp)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(svc.Annotations).To(HaveKeyWithValue(tt.expectKey, tt.expectValue))
g.Expect(svc.Annotations).ToNot(HaveKey(tt.expectAbsent))
})
}
}

// Test that LoadBalancerSourceRanges is applied for external router services with allowedCIDRBlocks
func TestReconcileRouterService_AppliesLoadBalancerSourceRanges(t *testing.T) {
// Test case 1: External router service should have LoadBalancerSourceRanges set
Expand Down
Loading