Skip to content

fix: OCPBUGS-93456: changes NodePortPublishingStrategy.Port #9002

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

Open
mehabhalodiya wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: OCPBUGS-93456: changes NodePortPublishingStrategy.Port #9002

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
2 changes: 1 addition & 1 deletion api/hypershift/v1beta1/hostedcluster_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1102,7 +1102,7 @@ type NodePortPublishingStrategy struct {
// port is the port of the NodePort service. If <=0, the port is dynamically

@clebs clebs Jul 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should also document what a nil value here means with the new type.

// assigned when the service is created.
// +optional
Port int32 `json:"port,omitempty"`
Port *int32 `json:"port,omitempty"`

@JoelSpeed JoelSpeed Jul 16, 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.

What is the expected behaviour of this field if the field is omitted? E.g. in the future when this is a nil pointer, what would you expect to happen?

@mehabhalodiya mehabhalodiya Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is what Claude is suggesting to me (new area, took this bug as an opportunity to learn) -

When port is omitted (nil pointer), the behavior is the same as port: 0 — Kubernetes dynamically assigns a NodePort when the service is created.

All consumers already handle this correctly:

  • KAS/OAuth/Konnectivity service controllers: check strategy.NodePort.Port != nil before using the value — when nil, they skip setting portSpec.NodePort, so Kubernetes auto-assigns.
  • Ignition service controllers: same nil guard — skip setting the port, let Kubernetes choose.
  • Port range validation: uses ptr.Deref(svc.NodePort.Port, 0) and only validates when port > 0 — nil defaults to 0 which is excluded from range checks.

The reason this field needs a pointer is not to distinguish nil from 0 semantically (they're equivalent), but to fix a serialization bug: with the previous int32 + omitempty, Go's JSON encoder dropped port: 0 from the serialized output. When the reconciler read a HostedCluster with port: 0, serialized it back (now missing the field), and called client.Update(), the CEL rule self.services == oldSelf.services rejected the update as "Services is immutable" — creating a permanent deadlock where the reconciler couldn't even add a finalizer.

With *int32 + omitempty, only nil is omitted; an explicit port: 0 is preserved in the JSON, so the round-trip is stable and the immutability check passes.

I've updated the godoc to: "If not specified or set to 0, the port is dynamically assigned when the service is created." (not yet pushed, will do)

@JoelSpeed JoelSpeed Jul 17, 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.

The immutability of services implemented by this CEL rule makes this round tripping issue a genuine problem.

Walking through the examples and possible solutions.

Today, an unstructured client can set port: 0. There is no restriction on this and the API server accepts. A structured client would see this as int32, the omitempty causes the port field to be dropped. This breaks immutability.

If we instead changed the field to drop the omitempty. An unstructured client may not set port at all. But when a structured client reads the value, they'll see a 0 int32 value, but without omit empty, they'll add port: 0 to the object. This breaks immutability.

We could prevent the 0 value from being committed (+kubebuilder:validation:XValidation:=1). In the future, no 0 value would be allowed and when the key is omitted, the behaviour is the same as setting 0 today - dynamic allocation. There is no explicit 0 option and there's only one way in the API to represent dynamic assignment. This avoid the round trip issue without changing the port field to a pointer. This is favourable, but it does not account for existing resources. Any resource that exists today would need a manual fix from an unstructured client. This may be acceptable because, AFAICT, any resource that has 0 today would never have a finalizer added and therefore would not progress to become an actual cluster. Users would probably have already deleted this resource? This IS a breaking change, but a breaking change that prevents an otherwise invalid/useless object I think we can permit.

Or we continue to allow the 0 value as an explicit "I want a dynamically allocated port". If we want to do that, we must fix the round trip, and AFAICT the only way to do that is to change the field to *int32 as laid out in this PR. It does correctly maintain whatever the unstructured client applied and would solve the immutability issue. The cons of changing this to a pointer is that we do not control every usage of this field. There will be end users out there who build tooling atop our client libraries. Those who write to this field will need to update their code to account for the change, the compiler will complain if they don't. Those who read this field will be exposed to NPEs as a result of this change, that's a pretty nasty change and not something the compiler would necessarily complain about, so this could break their code in the wild.

So, do we, or do we not want to allow unstructured users to explicitly set port: 0?

}

// LoadBalancerPublishingStrategy specifies setting used to expose a service as a LoadBalancer.
Expand Down
7 changes: 6 additions & 1 deletion api/hypershift/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

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

8 changes: 4 additions & 4 deletions control-plane-operator/controllers/hostedcontrolplane/kas/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,8 @@ func ReconcileService(svc *corev1.Service, strategy *hyperv1.ServicePublishingSt
}
case hyperv1.NodePort:
svc.Spec.Type = corev1.ServiceTypeNodePort
if portSpec.NodePort == 0 && strategy.NodePort != nil {
portSpec.NodePort = strategy.NodePort.Port
if portSpec.NodePort == 0 && strategy.NodePort != nil && strategy.NodePort.Port != nil {
portSpec.NodePort = *strategy.NodePort.Port
}
case hyperv1.Route:
if hcp.Spec.Platform.Type != hyperv1.IBMCloudPlatform || svc.Spec.Type != corev1.ServiceTypeNodePort {
Expand Down Expand Up @@ -313,8 +313,8 @@ func ReconcileKonnectivityServerService(svc *corev1.Service, ownerRef config.Own
}
case hyperv1.NodePort:
svc.Spec.Type = corev1.ServiceTypeNodePort
if portSpec.NodePort == 0 && strategy.NodePort != nil {
portSpec.NodePort = strategy.NodePort.Port
if portSpec.NodePort == 0 && strategy.NodePort != nil && strategy.NodePort.Port != nil {
portSpec.NodePort = *strategy.NodePort.Port
}
case hyperv1.Route:
if hcp.Spec.Platform.Type != hyperv1.IBMCloudPlatform || svc.Spec.Type != corev1.ServiceTypeNodePort {
Expand Down
5 changes: 3 additions & 2 deletions control-plane-operator/controllers/hostedcontrolplane/kas/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
corev1 "k8s.io/api/core/v1"
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/utils/ptr"
)

func TestReconcileService(t *testing.T) {
Expand All @@ -29,7 +30,7 @@ func TestReconcileService(t *testing.T) {
{
name: "IBM Cloud, NodePort strategy, NodePort service, expected to fill port number from strategy",
platform: hyperv1.IBMCloudPlatform,
strategy: hyperv1.ServicePublishingStrategy{Type: hyperv1.NodePort, NodePort: &hyperv1.NodePortPublishingStrategy{Port: 31125}},
strategy: hyperv1.ServicePublishingStrategy{Type: hyperv1.NodePort, NodePort: &hyperv1.NodePortPublishingStrategy{Port: ptr.To(int32(31125))}},
apiServerPort: 1125,
svc_in: corev1.Service{Spec: corev1.ServiceSpec{
Type: corev1.ServiceTypeNodePort,
Expand Down Expand Up @@ -351,7 +352,7 @@ func TestKonnectivityServiceReconcile(t *testing.T) {
{
name: "IBM Cloud, NodePort strategy, NodePort service, expected to fill port number from strategy",
platform: hyperv1.IBMCloudPlatform,
strategy: hyperv1.ServicePublishingStrategy{Type: hyperv1.NodePort, NodePort: &hyperv1.NodePortPublishingStrategy{Port: 1125}},
strategy: hyperv1.ServicePublishingStrategy{Type: hyperv1.NodePort, NodePort: &hyperv1.NodePortPublishingStrategy{Port: ptr.To(int32(1125))}},
svc_in: corev1.Service{Spec: corev1.ServiceSpec{
Type: corev1.ServiceTypeNodePort,
}},
Expand Down
4 changes: 2 additions & 2 deletions control-plane-operator/controllers/hostedcontrolplane/oauth/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ func ReconcileService(svc *corev1.Service, ownerRef config.OwnerRef, strategy *h
switch strategy.Type {
case hyperv1.NodePort:
svc.Spec.Type = corev1.ServiceTypeNodePort
if portSpec.NodePort == 0 && strategy.NodePort != nil {
portSpec.NodePort = strategy.NodePort.Port
if portSpec.NodePort == 0 && strategy.NodePort != nil && strategy.NodePort.Port != nil {
portSpec.NodePort = *strategy.NodePort.Port
}
case hyperv1.Route:
if ((platformType == hyperv1.IBMCloudPlatform) && (svc.Spec.Type != corev1.ServiceTypeNodePort)) || (platformType != hyperv1.IBMCloudPlatform) {
Expand Down
3 changes: 2 additions & 1 deletion control-plane-operator/controllers/hostedcontrolplane/oauth/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/utils/ptr"
)

func TestOauthServiceReconcile(t *testing.T) {
Expand All @@ -32,7 +33,7 @@ func TestOauthServiceReconcile(t *testing.T) {
{
name: "When IBM Cloud platform uses NodePort strategy with a port, it should populate the NodePort from the strategy",
platform: v1beta1.IBMCloudPlatform,
strategy: v1beta1.ServicePublishingStrategy{Type: v1beta1.NodePort, NodePort: &v1beta1.NodePortPublishingStrategy{Port: 1125}},
strategy: v1beta1.ServicePublishingStrategy{Type: v1beta1.NodePort, NodePort: &v1beta1.NodePortPublishingStrategy{Port: ptr.To(int32(1125))}},
svc_in: corev1.Service{Spec: corev1.ServiceSpec{
Type: corev1.ServiceTypeNodePort,
}},
Expand Down
4 changes: 2 additions & 2 deletions control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ func adaptService(cpContext component.WorkloadContext, svc *corev1.Service) erro
switch strategy.Type {
case hyperv1.NodePort:
svc.Spec.Type = corev1.ServiceTypeNodePort
if strategy.NodePort != nil {
svc.Spec.Ports[0].NodePort = strategy.NodePort.Port
if strategy.NodePort != nil && strategy.NodePort.Port != nil {
svc.Spec.Ports[0].NodePort = *strategy.NodePort.Port
}
case hyperv1.Route:
if existingServiceUsesNodePort {
Expand Down
3 changes: 2 additions & 1 deletion control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/utils/ptr"

"sigs.k8s.io/controller-runtime/pkg/client/fake"
)
Expand All @@ -33,7 +34,7 @@ func TestIgnitionServiceReconcile(t *testing.T) {
Service: hyperv1.Ignition,
ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{
Type: hyperv1.NodePort,
NodePort: &hyperv1.NodePortPublishingStrategy{Port: 1125},
NodePort: &hyperv1.NodePortPublishingStrategy{Port: ptr.To(int32(1125))},
},
},
},
Expand Down
4 changes: 2 additions & 2 deletions control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ func adaptService(cpContext component.WorkloadContext, svc *corev1.Service) erro
switch strategy.Type {
case hyperv1.NodePort:
svc.Spec.Type = corev1.ServiceTypeNodePort
if strategy.NodePort != nil {
svc.Spec.Ports[0].NodePort = strategy.NodePort.Port
if strategy.NodePort != nil && strategy.NodePort.Port != nil {
svc.Spec.Ports[0].NodePort = *strategy.NodePort.Port
}
case hyperv1.Route:
svc.Spec.Type = corev1.ServiceTypeClusterIP
Expand Down
7 changes: 4 additions & 3 deletions ...rol-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (

corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/utils/ptr"
)

func TestAdaptService(t *testing.T) {
Expand Down Expand Up @@ -46,7 +47,7 @@ func TestAdaptService(t *testing.T) {
ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{
Type: hyperv1.NodePort,
NodePort: &hyperv1.NodePortPublishingStrategy{
Port: 30123,
Port: ptr.To(int32(30123)),
},
},
},
Expand Down Expand Up @@ -186,7 +187,7 @@ func TestAdaptServicePreservesNodePort(t *testing.T) {
ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{
Type: hyperv1.NodePort,
NodePort: &hyperv1.NodePortPublishingStrategy{
Port: 30456,
Port: ptr.To(int32(30456)),
},
},
},
Expand Down Expand Up @@ -237,7 +238,7 @@ func TestAdaptServiceMultiplePorts(t *testing.T) {
ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{
Type: hyperv1.NodePort,
NodePort: &hyperv1.NodePortPublishingStrategy{
Port: 30789,
Port: ptr.To(int32(30789)),
},
},
},
Expand Down
2 changes: 1 addition & 1 deletion hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4721,7 +4721,7 @@ func validateNodePortPortRange(hc *hyperv1.HostedCluster) field.ErrorList {
// Validate NodePort.Port against the configured range
for idx, svc := range hc.Spec.Services {
if svc.Service == hyperv1.APIServer && svc.Type == hyperv1.NodePort && svc.NodePort != nil {
port := svc.NodePort.Port
port := ptr.Deref(svc.NodePort.Port, 0)
if port > 0 && (port < minPort || port > maxPort) {
errs = append(errs, field.Invalid(
field.NewPath(fmt.Sprintf("spec.services[%d].nodePort.port", idx)),
Expand Down
18 changes: 9 additions & 9 deletions hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2537,7 +2537,7 @@ func TestValidateConfigAndClusterCapabilities(t *testing.T) {
Type: hyperv1.NodePort,
NodePort: &hyperv1.NodePortPublishingStrategy{
Address: "172.16.3.3",
Port: 30443,
Port: ptr.To(int32(30443)),
},
},
},
Expand Down Expand Up @@ -5938,7 +5938,7 @@ func TestValidateNodePortPortRange(t *testing.T) {
Type: hyperv1.NodePort,
NodePort: &hyperv1.NodePortPublishingStrategy{
Address: "1.1.1.1",
Port: 31000,
Port: ptr.To(int32(31000)),
},
},
},
Expand All @@ -5957,7 +5957,7 @@ func TestValidateNodePortPortRange(t *testing.T) {
Type: hyperv1.NodePort,
NodePort: &hyperv1.NodePortPublishingStrategy{
Address: "1.1.1.1",
Port: 0,
Port: ptr.To(int32(0)),
},
},
},
Expand All @@ -5976,7 +5976,7 @@ func TestValidateNodePortPortRange(t *testing.T) {
Type: hyperv1.NodePort,
NodePort: &hyperv1.NodePortPublishingStrategy{
Address: "1.1.1.1",
Port: 10000,
Port: ptr.To(int32(10000)),
},
},
},
Expand All @@ -5998,7 +5998,7 @@ func TestValidateNodePortPortRange(t *testing.T) {
Type: hyperv1.NodePort,
NodePort: &hyperv1.NodePortPublishingStrategy{
Address: "1.1.1.1",
Port: 65000,
Port: ptr.To(int32(65000)),
},
},
},
Expand All @@ -6025,7 +6025,7 @@ func TestValidateNodePortPortRange(t *testing.T) {
Type: hyperv1.NodePort,
NodePort: &hyperv1.NodePortPublishingStrategy{
Address: "1.1.1.1",
Port: 28000,
Port: ptr.To(int32(28000)),
},
},
},
Expand All @@ -6049,7 +6049,7 @@ func TestValidateNodePortPortRange(t *testing.T) {
Type: hyperv1.NodePort,
NodePort: &hyperv1.NodePortPublishingStrategy{
Address: "1.1.1.1",
Port: 40000,
Port: ptr.To(int32(40000)),
},
},
},
Expand All @@ -6076,7 +6076,7 @@ func TestValidateNodePortPortRange(t *testing.T) {
Type: hyperv1.NodePort,
NodePort: &hyperv1.NodePortPublishingStrategy{
Address: "1.1.1.1",
Port: 31000,
Port: ptr.To(int32(31000)),
},
},
},
Expand All @@ -6103,7 +6103,7 @@ func TestValidateNodePortPortRange(t *testing.T) {
Type: hyperv1.NodePort,
NodePort: &hyperv1.NodePortPublishingStrategy{
Address: "1.1.1.1",
Port: 31000,
Port: ptr.To(int32(31000)),
},
},
},
Expand Down
5 changes: 3 additions & 2 deletions hypershift-operator/controllers/hostedcluster/network_policies_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/utils/ptr"

ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -1391,7 +1392,7 @@ func TestReconcileNetworkPolicies_LoadBalancerOauth(t *testing.T) {
{Service: hyperv1.APIServer, ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{Type: hyperv1.Route}},
{Service: hyperv1.OAuthServer, ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{
Type: hyperv1.NodePort,
NodePort: &hyperv1.NodePortPublishingStrategy{Address: "10.0.0.1", Port: 31000},
NodePort: &hyperv1.NodePortPublishingStrategy{Address: "10.0.0.1", Port: ptr.To(int32(31000))},
}},
},
},
Expand All @@ -1403,7 +1404,7 @@ func TestReconcileNetworkPolicies_LoadBalancerOauth(t *testing.T) {
{Service: hyperv1.APIServer, ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{Type: hyperv1.Route}},
{Service: hyperv1.OAuthServer, ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{
Type: hyperv1.NodePort,
NodePort: &hyperv1.NodePortPublishingStrategy{Address: "10.0.0.1", Port: 31000},
NodePort: &hyperv1.NodePortPublishingStrategy{Address: "10.0.0.1", Port: ptr.To(int32(31000))},
}},
},
},
Expand Down
4 changes: 2 additions & 2 deletions test/e2e/util/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2644,8 +2644,8 @@ func apiServerExternalName(service hyperv1.ServicePublishingStrategyMapping) str
return service.LoadBalancer.Hostname
}
case hyperv1.NodePort:
if service.NodePort != nil && len(service.NodePort.Address) > 0 && service.NodePort.Port != 0 {
return fmt.Sprintf("%s:%d", service.NodePort.Address, service.NodePort.Port)
if service.NodePort != nil && len(service.NodePort.Address) > 0 && service.NodePort.Port != nil && *service.NodePort.Port != 0 {
return fmt.Sprintf("%s:%d", service.NodePort.Address, *service.NodePort.Port)
}
}
return ""
Expand Down
2 changes: 1 addition & 1 deletion vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.go
View file
Open in desktop

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

7 changes: 6 additions & 1 deletion vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

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