From 33bf9a33ac7c8dd654cd7ecbd82e1c9c5269557c Mon Sep 17 00:00:00 2001 From: mehabhalodiya Date: Tue, 14 Jul 2026 15:34:46 +0100 Subject: [PATCH] fix: OCPBUGS-93456: change NodePortPublishingStrategy.Port from int32 to *int32 With this fix, omitempty no longer drops port: 0 during serialization. Signed-off-by: mehabhalodiya --- api/hypershift/v1beta1/hostedcluster_types.go | 2 +- .../v1beta1/zz_generated.deepcopy.go | 7 ++++++- .../hostedcontrolplane/kas/service.go | 8 ++++---- .../hostedcontrolplane/kas/service_test.go | 5 +++-- .../hostedcontrolplane/oauth/service.go | 4 ++-- .../hostedcontrolplane/oauth/service_test.go | 3 ++- .../v2/ignitionserver/service.go | 4 ++-- .../v2/ignitionserver/service_test.go | 3 ++- .../v2/ignitionserver_proxy/service.go | 4 ++-- .../v2/ignitionserver_proxy/service_test.go | 7 ++++--- .../hostedcluster/hostedcluster_controller.go | 2 +- .../hostedcluster_controller_test.go | 18 +++++++++--------- .../hostedcluster/network_policies_test.go | 5 +++-- test/e2e/util/util.go | 4 ++-- .../hypershift/v1beta1/hostedcluster_types.go | 2 +- .../v1beta1/zz_generated.deepcopy.go | 7 ++++++- 16 files changed, 50 insertions(+), 35 deletions(-) diff --git a/api/hypershift/v1beta1/hostedcluster_types.go b/api/hypershift/v1beta1/hostedcluster_types.go index c26677a63aa8..626e54a0c96f 100644 --- a/api/hypershift/v1beta1/hostedcluster_types.go +++ b/api/hypershift/v1beta1/hostedcluster_types.go @@ -1102,7 +1102,7 @@ type NodePortPublishingStrategy struct { // port is the port of the NodePort service. If <=0, the port is dynamically // assigned when the service is created. // +optional - Port int32 `json:"port,omitempty"` + Port *int32 `json:"port,omitempty"` } // LoadBalancerPublishingStrategy specifies setting used to expose a service as a LoadBalancer. diff --git a/api/hypershift/v1beta1/zz_generated.deepcopy.go b/api/hypershift/v1beta1/zz_generated.deepcopy.go index ba481ba6e9b7..a65ee8a1ade6 100644 --- a/api/hypershift/v1beta1/zz_generated.deepcopy.go +++ b/api/hypershift/v1beta1/zz_generated.deepcopy.go @@ -3902,6 +3902,11 @@ func (in *NodePoolStatus) DeepCopy() *NodePoolStatus { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *NodePortPublishingStrategy) DeepCopyInto(out *NodePortPublishingStrategy) { *out = *in + if in.Port != nil { + in, out := &in.Port, &out.Port + *out = new(int32) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new NodePortPublishingStrategy. @@ -4646,7 +4651,7 @@ func (in *ServicePublishingStrategy) DeepCopyInto(out *ServicePublishingStrategy if in.NodePort != nil { in, out := &in.NodePort, &out.NodePort *out = new(NodePortPublishingStrategy) - **out = **in + (*in).DeepCopyInto(*out) } if in.LoadBalancer != nil { in, out := &in.LoadBalancer, &out.LoadBalancer diff --git a/control-plane-operator/controllers/hostedcontrolplane/kas/service.go b/control-plane-operator/controllers/hostedcontrolplane/kas/service.go index ffd30fb0c040..d29d9ca124c4 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/kas/service.go +++ b/control-plane-operator/controllers/hostedcontrolplane/kas/service.go @@ -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 { @@ -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 { diff --git a/control-plane-operator/controllers/hostedcontrolplane/kas/service_test.go b/control-plane-operator/controllers/hostedcontrolplane/kas/service_test.go index 67dfe1e8d4bb..3dc04086bb23 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/kas/service_test.go +++ b/control-plane-operator/controllers/hostedcontrolplane/kas/service_test.go @@ -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) { @@ -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, @@ -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, }}, diff --git a/control-plane-operator/controllers/hostedcontrolplane/oauth/service.go b/control-plane-operator/controllers/hostedcontrolplane/oauth/service.go index 3060fe63c643..217cb439700f 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/oauth/service.go +++ b/control-plane-operator/controllers/hostedcontrolplane/oauth/service.go @@ -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) { diff --git a/control-plane-operator/controllers/hostedcontrolplane/oauth/service_test.go b/control-plane-operator/controllers/hostedcontrolplane/oauth/service_test.go index 68ce94c52929..9ac932665889 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/oauth/service_test.go +++ b/control-plane-operator/controllers/hostedcontrolplane/oauth/service_test.go @@ -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) { @@ -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, }}, diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/service.go b/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/service.go index 4f4f3f9ca5d2..186532743618 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/service.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/service.go @@ -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 { diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/service_test.go b/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/service_test.go index c2df921bf970..c0c59552b0db 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/service_test.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/service_test.go @@ -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" ) @@ -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))}, }, }, }, diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/service.go b/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/service.go index 7427dd48f676..be40775f0ebc 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/service.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/service.go @@ -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 diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/service_test.go b/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/service_test.go index 95f128c5ddd7..208b3e15e83e 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/service_test.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/service_test.go @@ -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) { @@ -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)), }, }, }, @@ -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)), }, }, }, @@ -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)), }, }, }, diff --git a/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go b/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go index 0f497ad5875d..406e08f54f77 100644 --- a/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go +++ b/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go @@ -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)), diff --git a/hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go b/hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go index f9e3fbe895b9..66902ea3cfbd 100644 --- a/hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go +++ b/hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go @@ -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)), }, }, }, @@ -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)), }, }, }, @@ -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)), }, }, }, @@ -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)), }, }, }, @@ -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)), }, }, }, @@ -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)), }, }, }, @@ -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)), }, }, }, @@ -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)), }, }, }, @@ -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)), }, }, }, diff --git a/hypershift-operator/controllers/hostedcluster/network_policies_test.go b/hypershift-operator/controllers/hostedcluster/network_policies_test.go index eac8b9f5b84e..eb279cf07ebf 100644 --- a/hypershift-operator/controllers/hostedcluster/network_policies_test.go +++ b/hypershift-operator/controllers/hostedcluster/network_policies_test.go @@ -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" @@ -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))}, }}, }, }, @@ -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))}, }}, }, }, diff --git a/test/e2e/util/util.go b/test/e2e/util/util.go index e540ad79a276..a3ababf0d7b3 100644 --- a/test/e2e/util/util.go +++ b/test/e2e/util/util.go @@ -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 "" diff --git a/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.go b/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.go index c26677a63aa8..626e54a0c96f 100644 --- a/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.go +++ b/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.go @@ -1102,7 +1102,7 @@ type NodePortPublishingStrategy struct { // port is the port of the NodePort service. If <=0, the port is dynamically // assigned when the service is created. // +optional - Port int32 `json:"port,omitempty"` + Port *int32 `json:"port,omitempty"` } // LoadBalancerPublishingStrategy specifies setting used to expose a service as a LoadBalancer. diff --git a/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.go b/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.go index ba481ba6e9b7..a65ee8a1ade6 100644 --- a/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.go +++ b/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.go @@ -3902,6 +3902,11 @@ func (in *NodePoolStatus) DeepCopy() *NodePoolStatus { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *NodePortPublishingStrategy) DeepCopyInto(out *NodePortPublishingStrategy) { *out = *in + if in.Port != nil { + in, out := &in.Port, &out.Port + *out = new(int32) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new NodePortPublishingStrategy. @@ -4646,7 +4651,7 @@ func (in *ServicePublishingStrategy) DeepCopyInto(out *ServicePublishingStrategy if in.NodePort != nil { in, out := &in.NodePort, &out.NodePort *out = new(NodePortPublishingStrategy) - **out = **in + (*in).DeepCopyInto(*out) } if in.LoadBalancer != nil { in, out := &in.LoadBalancer, &out.LoadBalancer