Add validation for provisionserver port range

This validates both user provided and auto assigned ports to be in the expected range. Signed-off-by: rabi <[email protected]>
openstack-k8s-operators · Nov 18, 2024 · 9608e97 · 9608e97
1 parent a50471a
commit 9608e97
Show file tree

Hide file tree

Showing 6 changed files with 51 additions and 41 deletions.
diff --git a/api/bases/baremetal.openstack.org_openstackprovisionservers.yaml b/api/bases/baremetal.openstack.org_openstackprovisionservers.yaml
@@ -82,6 +82,8 @@ spec:
               port:
                 description: Port - The port on which the Apache server should listen
                 format: int32
+                maximum: 6220
+                minimum: 6190
                 type: integer
               preserveJobs:
                 default: false
@@ -143,7 +145,6 @@ spec:
             - osContainerImageUrl
             - osImage
             - osImageDir
-            - port
             type: object
           status:
             description: OpenStackProvisionServerStatus defines the observed state

diff --git a/api/v1beta1/openstackprovisionserver.go b/api/v1beta1/openstackprovisionserver.go
@@ -37,6 +37,7 @@ func AssignProvisionServerPort(
 	c goClient.Client,
 	instance *OpenStackProvisionServer,
 	portStart int32,
+	portEnd int32,
 ) error {
 	existingPorts, err := GetExistingProvServerPorts(ctx, c, instance)
 	if err != nil {
@@ -45,31 +46,36 @@ func AssignProvisionServerPort(
 
 	// It's possible that this prov server already exists and we are just dealing with
 	// a minimized version of it (only its ObjectMeta is set, etc)
-	instance.Spec.Port = existingPorts[instance.GetName()]
-
-	// If we get this far, no port has been previously assigned, so we pick one
-	if instance.Spec.Port == 0 {
-		cur := portStart
-
-		for {
-			found := false
+	cur := existingPorts[instance.GetName()]
+	if cur == 0 {
+		cur = portStart
+	}
 
-			for _, port := range existingPorts {
-				if port == cur {
-					found = true
-					break
-				}
+	for ; ; cur++ {
+		if cur > portEnd {
+			return fmt.Errorf("slected port is out of range %v-%v-%v", cur, portStart, portEnd)
+		}
+		found := false
+		for _, port := range existingPorts {
+			if port == cur {
+				found = true
+				break
 			}
+		}
 
-			if !found {
+		if found {
+			if existingPorts[instance.GetName()] != cur {
+				return fmt.Errorf("%v port already used by another OpeStackProvisionServer", cur)
+			} else {
 				break
 			}
+		}
 
-			cur++
+		if !found {
+			break
 		}
 
-		instance.Spec.Port = cur
 	}
-
+	instance.Spec.Port = cur
 	return nil
 }
diff --git a/api/v1beta1/openstackprovisionserver_types.go b/api/v1beta1/openstackprovisionserver_types.go
@@ -36,7 +36,8 @@ const (
 	// Checksum job hash
 	ChecksumHash = "checksum"
 	// DefaultProvisionPort - The starting default port for the OpenStackProvisionServer's Apache container
-	DefaultProvisionPort = 6190
+	ProvisionServerPortStart = 6190
+	ProvisionServerPortEnd   = 6210
 )
 
 const (
@@ -53,7 +54,10 @@ const (
 // OpenStackProvisionServerSpec defines the desired state of OpenStackProvisionServer
 type OpenStackProvisionServerSpec struct {
 	// Port - The port on which the Apache server should listen
-	Port int32 `json:"port"`
+	// +kubebuilder:validation:Optional
+	// +kubebuilder:validation:Minimum=6190
+	// +kubebuilder:validation:Maximum=6220
+	Port int32 `json:"port,omitempty"`
 	// +kubebuilder:validation:Optional
 	// Interface - An optional interface to use instead of the cluster's default provisioning interface (if any)
 	Interface string `json:"interface,omitempty"`

diff --git a/api/v1beta1/openstackprovisionserver_webhook.go b/api/v1beta1/openstackprovisionserver_webhook.go
@@ -115,14 +115,13 @@ func (r *OpenStackProvisionServer) Default() {
 	if r.Spec.OSImage == "" {
 		r.Spec.OSImage = openstackProvisionServerDefaults.OSImage
 	}
-	if r.Spec.Port == 0 {
-		err := AssignProvisionServerPort(context.TODO(), webhookClient, r, DefaultProvisionPort)
-		if err != nil {
-			// If this occurs, it will also be caught just after this defaulting webhook in the
-			// validating webhook, because that webhook calls the same underlying function that
-			// checks for the availability of ports.  That will cause the create/update of the
-			// CR to fail and halt moving forward.
-			openstackprovisionserverlog.Error(err, "Cannot assign port for OpenStackProvisionServer", "OpenStackProvisionServer", r)
-		}
+	err := AssignProvisionServerPort(context.TODO(), webhookClient, r,
+		ProvisionServerPortStart, ProvisionServerPortEnd)
+	if err != nil {
+		// If this occurs, it will also be caught just after this defaulting webhook in the
+		// validating webhook, because that webhook calls the same underlying function that
+		// checks for the availability of ports.  That will cause the create/update of the
+		// CR to fail and halt moving forward.
+		openstackprovisionserverlog.Error(err, "Cannot assign port for OpenStackProvisionServer", "OpenStackProvisionServer", r)
 	}
 }
diff --git a/config/crd/bases/baremetal.openstack.org_openstackprovisionservers.yaml b/config/crd/bases/baremetal.openstack.org_openstackprovisionservers.yaml
@@ -82,6 +82,8 @@ spec:
               port:
                 description: Port - The port on which the Apache server should listen
                 format: int32
+                maximum: 6220
+                minimum: 6190
                 type: integer
               preserveJobs:
                 default: false
@@ -143,7 +145,6 @@ spec:
             - osContainerImageUrl
             - osImage
             - osImageDir
-            - port
             type: object
           status:
             description: OpenStackProvisionServerStatus defines the observed state

diff --git a/controllers/openstackbaremetalset_controller.go b/controllers/openstackbaremetalset_controller.go
@@ -504,24 +504,23 @@ func (r *OpenStackBaremetalSetReconciler) provisionServerCreateOrUpdate(
 	op, err := controllerutil.CreateOrPatch(ctx, helper.GetClient(), provisionServer, func() error {
 		// Leave the prov server's existing port as-is if this is an update, otherwise pick a new one
 		// based on what is available
-		if provisionServer.Spec.Port == 0 {
-			err := baremetalv1.AssignProvisionServerPort(
-				ctx,
-				helper.GetClient(),
-				provisionServer,
-				baremetalv1.DefaultProvisionPort,
-			)
-			if err != nil {
-				return err
-			}
+		err := baremetalv1.AssignProvisionServerPort(
+			ctx,
+			helper.GetClient(),
+			provisionServer,
+			baremetalv1.ProvisionServerPortStart,
+			baremetalv1.ProvisionServerPortEnd,
+		)
+		if err != nil {
+			return err
 		}
 		provisionServer.Spec.OSImage = instance.Spec.OSImage
 		provisionServer.Spec.OSContainerImageURL = instance.Spec.OSContainerImageURL
 		provisionServer.Spec.ApacheImageURL = instance.Spec.ApacheImageURL
 		provisionServer.Spec.AgentImageURL = instance.Spec.AgentImageURL
 		provisionServer.Spec.Interface = instance.Spec.ProvisioningInterface
 
-		err := controllerutil.SetControllerReference(instance, provisionServer, helper.GetScheme())
+		err = controllerutil.SetControllerReference(instance, provisionServer, helper.GetScheme())
 		if err != nil {
 			return err
 		}