Merge pull request #678 from shiftstack/port-binding-immutable

port: make hostID immutable

Author: mandre

api/v1alpha1/port_types.go

@@ -202,7 +202,13 @@ type PortResourceSpec struct {
 	MACAddress string `json:"macAddress,omitempty"`
 
 	// hostID specifies the host where the port will be bound.
+	// Note that when the port is attached to a server, OpenStack may
+	// rebind the port to the server's actual compute host, which may
+	// differ from the specified hostID if no matching scheduler hint
+	// is used. In this case the port's status will reflect the actual
+	// binding host, not the value specified here.
 	// +optional
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="hostID is immutable"
 	HostID *HostID `json:"hostID,omitempty"`
 }
 

cmd/models-schema/zz_generated.openapi.go

@@ -300,8 +300,13 @@ spec:
                     minLength: 1
                     type: string
                   hostID:
-                    description: hostID specifies the host where the port will be
-                      bound.
+                    description: |-
+                      hostID specifies the host where the port will be bound.
+                      Note that when the port is attached to a server, OpenStack may
+                      rebind the port to the server's actual compute host, which may
+                      differ from the specified hostID if no matching scheduler hint
+                      is used. In this case the port's status will reflect the actual
+                      binding host, not the value specified here.
                     maxProperties: 1
                     minProperties: 1
                     properties:
@@ -322,6 +327,8 @@ spec:
                         type: string
                     type: object
                     x-kubernetes-validations:
+                    - message: hostID is immutable
+                      rule: self == oldSelf
                     - message: exactly one of id or serverRef must be set
                       rule: (has(self.id) && size(self.id) > 0) != (has(self.serverRef)
                         && size(self.serverRef) > 0)

config/crd/bases/openstack.k-orc.cloud_ports.yaml

@@ -377,14 +377,6 @@ func (actuator portActuator) updateResource(ctx context.Context, obj orcObjectPT
 	reconcileStatus := progress.NewReconcileStatus().
 		WithReconcileStatus(secGroupDepRS)
 
-	// Resolve hostID if specified
-	var resolvedHostID string
-	if resource.HostID != nil {
-		var hostIDReconcileStatus progress.ReconcileStatus
-		resolvedHostID, hostIDReconcileStatus = resolveHostID(ctx, actuator.k8sClient, obj, resource.HostID)
-		reconcileStatus = reconcileStatus.WithReconcileStatus(hostIDReconcileStatus)
-	}
-
 	needsReschedule, _ := reconcileStatus.NeedsReschedule()
 	if needsReschedule {
 		return reconcileStatus
@@ -403,7 +395,7 @@ func (actuator portActuator) updateResource(ctx context.Context, obj orcObjectPT
 		updateOpts = baseUpdateOpts
 	}
 
-	updateOpts = handlePortBindingUpdate(updateOpts, resource, osResource, resolvedHostID)
+	updateOpts = handlePortBindingUpdate(updateOpts, resource, osResource)
 	updateOpts = handlePortSecurityUpdate(updateOpts, resource, osResource)
 
 	needsUpdate, err := needsUpdate(updateOpts)
@@ -529,7 +521,7 @@ func handleSecurityGroupRefsUpdate(updateOpts *ports.UpdateOpts, resource *resou
 	}
 }
 
-func handlePortBindingUpdate(updateOpts ports.UpdateOptsBuilder, resource *resourceSpecT, osResource *osResourceT, resolvedHostID string) ports.UpdateOptsBuilder {
+func handlePortBindingUpdate(updateOpts ports.UpdateOptsBuilder, resource *resourceSpecT, osResource *osResourceT) ports.UpdateOptsBuilder {
 	if resource.VNICType != "" {
 		if resource.VNICType != osResource.VNICType {
 			updateOpts = &portsbinding.UpdateOptsExt{
@@ -539,15 +531,6 @@ func handlePortBindingUpdate(updateOpts ports.UpdateOptsBuilder, resource *resou
 		}
 	}
 
-	if resolvedHostID != "" {
-		if resolvedHostID != osResource.HostID {
-			updateOpts = &portsbinding.UpdateOptsExt{
-				UpdateOptsBuilder: updateOpts,
-				HostID:            &resolvedHostID,
-			}
-		}
-	}
-
 	return updateOpts
 }
 

internal/controllers/port/actuator.go

@@ -359,7 +359,7 @@ func TestHandlePortBindingUpdate(t *testing.T) {
 				},
 			}
 
-			updateOpts := handlePortBindingUpdate(&ports.UpdateOpts{}, resource, osResource, "")
+			updateOpts := handlePortBindingUpdate(&ports.UpdateOpts{}, resource, osResource)
 
 			got, _ := needsUpdate(updateOpts)
 			if got != tt.expectChange {

internal/controllers/port/actuator_test.go

@@ -6,20 +6,13 @@ resourceRefs:
       kind: port
       name: port-update
       ref: port
-    - apiVersion: openstack.k-orc.cloud/v1alpha1
-      kind: port
-      name: port-update-admin
-      ref: portAdmin
 assertAll:
     - celExpr: "port.status.id != ''"
     - celExpr: "port.status.resource.createdAt != ''"
     - celExpr: "port.status.resource.updatedAt != ''"
     - celExpr: "port.status.resource.macAddress != ''"
     - celExpr: "!has(port.status.resource.fixedIPs)"
     - celExpr: "!has(port.status.resource.description)"
-    # Following the network API reference, the default value for
-    # hostID field is an empty string.
-    - celExpr: "portAdmin.status.resource.hostID == ''"
 ---
 apiVersion: openstack.k-orc.cloud/v1alpha1
 kind: Port
@@ -43,26 +36,3 @@ status:
       message: OpenStack resource is up to date
       status: "False"
       reason: Success
----
-apiVersion: openstack.k-orc.cloud/v1alpha1
-kind: Port
-metadata:
-  name: port-update-admin
-status:
-  resource:
-    name: port-update-admin
-    adminStateUp: true
-    portSecurityEnabled: true
-    propagateUplinkStatus: false
-    revisionNumber: 1
-    status: DOWN
-    vnicType: normal
-  conditions:
-    - type: Available
-      message: OpenStack resource is available
-      status: "True"
-      reason: Success
-    - type: Progressing
-      message: OpenStack resource is up to date
-      status: "False"
-      reason: Success

internal/controllers/port/tests/port-update/00-assert.yaml

@@ -12,17 +12,3 @@ spec:
     portSecurity: Disabled
     # Need to set the default values to revert them correctly in the 02-revert-resource step.
     vnicType: normal
----
-# This port is intended to be used only to test fields editable
-# by admin users
-apiVersion: openstack.k-orc.cloud/v1alpha1
-kind: Port
-metadata:
-  name: port-update-admin
-spec:
-  cloudCredentialsRef:
-    cloudName: openstack-admin
-    secretName: openstack-clouds
-  managementPolicy: managed
-  resource:
-    networkRef: port-update

internal/controllers/port/tests/port-update/00-minimal-resource.yaml

@@ -49,18 +49,3 @@ status:
   - type: Progressing
     status: "False"
     reason: Success 
----
-apiVersion: openstack.k-orc.cloud/v1alpha1
-kind: Port
-metadata:
-  name: port-update-admin
-status:
-  resource:
-    hostID: devstack
-  conditions:
-  - type: Available
-    status: "True"
-    reason: Success
-  - type: Progressing
-    status: "False"
-    reason: Success 

internal/controllers/port/tests/port-update/01-assert.yaml

@@ -20,16 +20,3 @@ spec:
     - tag1
     vnicType: direct
     portSecurity: Enabled
----
-apiVersion: openstack.k-orc.cloud/v1alpha1
-kind: Port
-metadata:
-  name: port-update-admin
-spec:
-  cloudCredentialsRef:
-    cloudName: openstack-admin
-    secretName: openstack-clouds
-  managementPolicy: managed
-  resource:
-    hostID:
-      id: devstack

internal/controllers/port/tests/port-update/01-updated-resource.yaml

@@ -123,6 +123,20 @@ var _ = Describe("ORC Port API validations", func() {
 		Expect(applyObj(ctx, port, patch)).To(MatchError(ContainSubstring("spec.resource.vnicType: Too long: may not be longer than 64")))
 	})
 
+	It("should not allow hostID to be modified", func(ctx context.Context) {
+		port := portStub(namespace)
+		patch := basePortPatch(port)
+		patch.Spec.WithResource(applyconfigv1alpha1.PortResourceSpec().
+			WithNetworkRef(networkName).
+			WithHostID(applyconfigv1alpha1.HostID().WithID("host-a")))
+		Expect(applyObj(ctx, port, patch)).To(Succeed())
+
+		patch.Spec.WithResource(applyconfigv1alpha1.PortResourceSpec().
+			WithNetworkRef(networkName).
+			WithHostID(applyconfigv1alpha1.HostID().WithID("host-b")))
+		Expect(applyObj(ctx, port, patch)).To(MatchError(ContainSubstring("hostID is immutable")))
+	})
+
 	// Note: we can't create a test for when the portSecurity is set to Inherit and the securityGroupRefs are set, because
 	// the validation is done in the OpenStack API and not in the ORC API. The OpenStack API will return an error if
 	// the network has port security disabled and the port has security group references.