Merge pull request #2174 from shiftstack/apiserverport

⚠️ Convert APIServerPort to uint16

Author: k8s-ci-robot

api/v1alpha6/openstackcluster_conversion.go

@@ -17,9 +17,11 @@ limitations under the License.
 package v1alpha6
 
 import (
+	"math"
 	"reflect"
 
 	apiconversion "k8s.io/apimachinery/pkg/conversion"
+	"k8s.io/utils/ptr"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	ctrlconversion "sigs.k8s.io/controller-runtime/pkg/conversion"
 
@@ -178,6 +180,11 @@ func restorev1alpha6ClusterSpec(previous *OpenStackClusterSpec, dst *OpenStackCl
 	restorev1alpha6SubnetFilter(&previous.Subnet, &dst.Subnet)
 
 	restorev1alpha6NetworkFilter(&previous.Network, &dst.Network)
+
+	// APIServerPort is now uint16
+	if previous.APIServerPort > math.MaxUint16 {
+		dst.APIServerPort = previous.APIServerPort
+	}
 }
 
 func restorev1beta1ClusterSpec(previous *infrav1.OpenStackClusterSpec, dst *infrav1.OpenStackClusterSpec) {
@@ -241,7 +248,7 @@ func restorev1beta1ClusterSpec(previous *infrav1.OpenStackClusterSpec, dst *infr
 
 	optional.RestoreString(&previous.APIServerFloatingIP, &dst.APIServerFloatingIP)
 	optional.RestoreString(&previous.APIServerFixedIP, &dst.APIServerFixedIP)
-	optional.RestoreInt(&previous.APIServerPort, &dst.APIServerPort)
+	optional.RestoreUInt16(&previous.APIServerPort, &dst.APIServerPort)
 	optional.RestoreBool(&previous.DisableAPIServerFloatingIP, &dst.DisableAPIServerFloatingIP)
 	optional.RestoreBool(&previous.ControlPlaneOmitAvailabilityZone, &dst.ControlPlaneOmitAvailabilityZone)
 	optional.RestoreBool(&previous.DisablePortSecurity, &dst.DisablePortSecurity)
@@ -313,6 +320,10 @@ func Convert_v1alpha6_OpenStackClusterSpec_To_v1beta1_OpenStackClusterSpec(in *O
 		out.APIServerLoadBalancer = apiServerLoadBalancer
 	}
 
+	if in.APIServerPort > 0 && in.APIServerPort < math.MaxUint16 {
+		out.APIServerPort = ptr.To(uint16(in.APIServerPort)) //nolint:gosec
+	}
+
 	return nil
 }
 
@@ -376,6 +387,8 @@ func Convert_v1beta1_OpenStackClusterSpec_To_v1alpha6_OpenStackClusterSpec(in *i
 		}
 	}
 
+	out.APIServerPort = int(ptr.Deref(in.APIServerPort, 0))
+
 	return nil
 }
 

api/v1alpha6/zz_generated.conversion.go

@@ -17,9 +17,11 @@ limitations under the License.
 package v1alpha7
 
 import (
+	"math"
 	"reflect"
 
 	apiconversion "k8s.io/apimachinery/pkg/conversion"
+	"k8s.io/utils/ptr"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	ctrlconversion "sigs.k8s.io/controller-runtime/pkg/conversion"
 
@@ -168,6 +170,11 @@ func restorev1alpha7ClusterSpec(previous *OpenStackClusterSpec, dst *OpenStackCl
 	}
 
 	restorev1alpha7NetworkFilter(&previous.Network, &dst.Network)
+
+	// APIServerPort is not uint16
+	if previous.APIServerPort > math.MaxUint16 {
+		dst.APIServerPort = previous.APIServerPort
+	}
 }
 
 func restorev1beta1ClusterSpec(previous *infrav1.OpenStackClusterSpec, dst *infrav1.OpenStackClusterSpec) {
@@ -238,7 +245,7 @@ func restorev1beta1ClusterSpec(previous *infrav1.OpenStackClusterSpec, dst *infr
 
 	optional.RestoreString(&previous.APIServerFloatingIP, &dst.APIServerFloatingIP)
 	optional.RestoreString(&previous.APIServerFixedIP, &dst.APIServerFixedIP)
-	optional.RestoreInt(&previous.APIServerPort, &dst.APIServerPort)
+	optional.RestoreUInt16(&previous.APIServerPort, &dst.APIServerPort)
 	optional.RestoreBool(&previous.DisableAPIServerFloatingIP, &dst.DisableAPIServerFloatingIP)
 	optional.RestoreBool(&previous.ControlPlaneOmitAvailabilityZone, &dst.ControlPlaneOmitAvailabilityZone)
 	optional.RestoreBool(&previous.DisablePortSecurity, &dst.DisablePortSecurity)
@@ -310,6 +317,10 @@ func Convert_v1alpha7_OpenStackClusterSpec_To_v1beta1_OpenStackClusterSpec(in *O
 		out.APIServerLoadBalancer = apiServerLoadBalancer
 	}
 
+	if in.APIServerPort > 0 && in.APIServerPort < math.MaxUint16 {
+		out.APIServerPort = ptr.To(uint16(in.APIServerPort)) //nolint:gosec
+	}
+
 	return nil
 }
 
@@ -373,6 +384,8 @@ func Convert_v1beta1_OpenStackClusterSpec_To_v1alpha7_OpenStackClusterSpec(in *i
 		}
 	}
 
+	out.APIServerPort = int(ptr.Deref(in.APIServerPort, 0))
+
 	return nil
 }
 

api/v1alpha7/openstackcluster_conversion.go

@@ -133,9 +133,9 @@ type OpenStackClusterSpec struct {
 	APIServerFixedIP optional.String `json:"apiServerFixedIP,omitempty"`
 
 	// APIServerPort is the port on which the listener on the APIServer
-	// will be created
+	// will be created. If specified, it must be an integer between 0 and 65535.
 	// +optional
-	APIServerPort optional.Int `json:"apiServerPort,omitempty"`
+	APIServerPort optional.UInt16 `json:"apiServerPort,omitempty"`
 
 	// ManagedSecurityGroups determines whether OpenStack security groups for the cluster
 	// will be managed by the OpenStack provider or whether pre-existing security groups will

api/v1alpha7/zz_generated.conversion.go

@@ -20,7 +20,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"math"
 	"time"
 
 	"github.com/gophercloud/gophercloud/v2/openstack/networking/v2/networks"
@@ -760,10 +759,7 @@ func reconcileProvisionedNetworkComponents(networkingService *networking.Service
 // cluster spec.
 func reconcileControlPlaneEndpoint(scope *scope.WithLogger, networkingService *networking.Service, openStackCluster *infrav1.OpenStackCluster, clusterResourceName string) error {
 	// Calculate the port that we will use for the API server
-	apiServerPort, err := getAPIServerPort(openStackCluster)
-	if err != nil {
-		return err
-	}
+	apiServerPort := getAPIServerPort(openStackCluster)
 
 	// host must be set by a matching control plane endpoint provider below
 	var host string
@@ -830,20 +826,14 @@ func reconcileControlPlaneEndpoint(scope *scope.WithLogger, networkingService *n
 }
 
 // getAPIServerPort returns the port to use for the API server based on the cluster spec.
-func getAPIServerPort(openStackCluster *infrav1.OpenStackCluster) (int32, error) {
+func getAPIServerPort(openStackCluster *infrav1.OpenStackCluster) int32 {
 	switch {
 	case openStackCluster.Spec.ControlPlaneEndpoint != nil && openStackCluster.Spec.ControlPlaneEndpoint.IsValid():
-		return openStackCluster.Spec.ControlPlaneEndpoint.Port, nil
+		return openStackCluster.Spec.ControlPlaneEndpoint.Port
 	case openStackCluster.Spec.APIServerPort != nil:
-		// XXX: This highlights that we have missing validation on
-		// APIServerPort. We should ideally add validation and change its type
-		// to int32.
-		if *openStackCluster.Spec.APIServerPort > math.MaxInt32 {
-			return 0, fmt.Errorf("value of apiServerPort is larger than %d", math.MaxInt32)
-		}
-		return int32(*openStackCluster.Spec.APIServerPort), nil //nolint:gosec
+		return int32(*openStackCluster.Spec.APIServerPort)
 	}
-	return 6443, nil
+	return 6443
 }
 
 func (r *OpenStackClusterReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {

api/v1beta1/openstackcluster_types.go

@@ -490,15 +490,15 @@ func Test_getAPIServerPort(t *testing.T) {
 			name: "with API server port",
 			openStackCluster: &infrav1.OpenStackCluster{
 				Spec: infrav1.OpenStackClusterSpec{
-					APIServerPort: ptr.To(6445),
+					APIServerPort: ptr.To(uint16(6445)),
 				},
 			},
 			want: 6445,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got, _ := getAPIServerPort(tt.openStackCluster); got != tt.want {
+			if got := getAPIServerPort(tt.openStackCluster); got != tt.want {
 				t.Errorf("getAPIServerPort() = %v, want %v", got, tt.want)
 			}
 		})

api/v1beta1/zz_generated.deepcopy.go

@@ -32,6 +32,12 @@ func RestoreInt(previous, dst *Int) {
 	}
 }
 
+func RestoreUInt16(previous, dst *UInt16) {
+	if *dst == nil || **dst == 0 {
+		*dst = *previous
+	}
+}
+
 func RestoreBool(previous, dst *Bool) {
 	if *dst == nil || !**dst {
 		*dst = *previous
@@ -78,6 +84,24 @@ func Convert_optional_Int_To_int(in *Int, out *int, _ conversion.Scope) error {
 	return nil
 }
 
+func Convert_uint16_To_optional_UInt16(in *uint16, out *UInt16, _ conversion.Scope) error {
+	if *in == 0 {
+		*out = nil
+	} else {
+		*out = in
+	}
+	return nil
+}
+
+func Convert_optional_UInt16_To_uint16(in *UInt16, out *uint16, _ conversion.Scope) error {
+	if *in == nil {
+		*out = 0
+	} else {
+		*out = **in
+	}
+	return nil
+}
+
 func Convert_bool_To_optional_Bool(in *bool, out *Bool, _ conversion.Scope) error {
 	if !*in {
 		*out = nil

config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml

@@ -28,6 +28,14 @@ type String *string
 // +optional.
 type Int *int
 
+// UInt16 is a uint16 that can be unspecified. uint16s which are converted to
+// optional.UInt16 during API conversion will be converted to nil if the value
+// was previously 0.
+// +kubebuilder:validation:Minimum:=0
+// +kubebuilder:validation:Maximum:=65535
+// +optional.
+type UInt16 *uint16
+
 // Bool is a bool that can be unspecified. bools which are converted to
 // optional.Bool during API conversion will be converted to nil if the value
 // was previously false.