Merge pull request #3056 from jfpucheu/feat/allowedaddresspairs-mutable

✨ Make allowedAddressPairs on OpenStackMachine ports mutable

Author: k8s-ci-robot

config/rbac/role.yaml

@@ -671,9 +671,17 @@ func openStackMachineSpecToOpenStackServerSpec(openStackMachineSpec *infrav1.Ope
 
 	// If not ports are provided we create one.
 	// Ports must have a network so if none is provided we use the default network.
-	serverPorts := openStackMachineSpec.Ports
+	// Deep-copy the ports so in-place mutations below (Network injection, SecurityGroups
+	// merge) do not modify the original OpenStackMachineSpec referenced by the patchHelper
+	// baseline, which would cause a spurious spec-immutability webhook rejection.
+	var serverPorts []infrav1.PortOpts
 	if len(openStackMachineSpec.Ports) == 0 {
 		serverPorts = make([]infrav1.PortOpts, 1)
+	} else {
+		serverPorts = make([]infrav1.PortOpts, len(openStackMachineSpec.Ports))
+		for i, p := range openStackMachineSpec.Ports {
+			serverPorts[i] = *p.DeepCopy()
+		}
 	}
 
 	var clusterSubnets []infrav1.FixedIP

controllers/openstackmachine_controller.go

@@ -17,6 +17,7 @@ package controllers
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"fmt"
 
@@ -26,6 +27,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/client-go/tools/events"
+	clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2"
 	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/annotations"
 	conditions "sigs.k8s.io/cluster-api/util/conditions"
@@ -35,16 +37,28 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 
+	infrav1alpha1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha1"
 	infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta2"
 	"sigs.k8s.io/cluster-api-provider-openstack/pkg/cloud/services/compute"
+	"sigs.k8s.io/cluster-api-provider-openstack/pkg/cloud/services/networking"
 	"sigs.k8s.io/cluster-api-provider-openstack/pkg/scope"
 	controllers "sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/controllers"
 )
 
-const imagePropertyForOS = "os_type"
+const (
+	imagePropertyForOS = "os_type"
+
+	// annotationAllowedAddressPairs tracks the last-applied allowedAddressPairs per
+	// OpenStackMachine (stored as JSON). Written as a metadata annotation so we never
+	// touch the immutable OSM spec, which would trigger the spec-immutability webhook.
+	annotationAllowedAddressPairs = "infrastructure.cluster.x-k8s.io/osmt-allowed-address-pairs"
+)
 
 // Set here so we can easily mock it in tests.
-var newComputeService = compute.NewService
+var (
+	newComputeService    = compute.NewService
+	newNetworkingService = networking.NewService
+)
 
 // OpenStackMachineTemplateReconciler reconciles a OpenStackMachineTemplate object.
 // it only updates the .status field to allow auto-scaling.
@@ -58,6 +72,10 @@ type OpenStackMachineTemplateReconciler struct {
 
 // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=openstackmachinetemplates,verbs=get;list;watch
 // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=openstackmachinetemplates/status,verbs=get;update;patch
+// +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=openstackmachines,verbs=get;list;watch;patch
+// +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=openstackservers,verbs=get;list;watch
+// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machinesets,verbs=get;list;watch
+// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machines,verbs=get;list;watch
 
 func (r *OpenStackMachineTemplateReconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ctrl.Result, reterr error) {
 	log := ctrl.LoggerFrom(ctx)
@@ -142,14 +160,14 @@ func (r *OpenStackMachineTemplateReconciler) Reconcile(ctx context.Context, req
 	scope := scope.NewWithLogger(clientScope, log)
 
 	// Handle non-deleted OpenStackMachineTemplates
-	if err := r.reconcileNormal(ctx, scope, openStackMachineTemplate); err != nil {
+	if err := r.reconcileNormal(ctx, scope, cluster.Name, openStackMachineTemplate); err != nil {
 		return ctrl.Result{}, err
 	}
 	log.V(4).Info("Successfully reconciled OpenStackMachineTemplate")
 	return ctrl.Result{}, nil
 }
 
-func (r *OpenStackMachineTemplateReconciler) reconcileNormal(ctx context.Context, scope *scope.WithLogger, openStackMachineTemplate *infrav1.OpenStackMachineTemplate) (reterr error) {
+func (r *OpenStackMachineTemplateReconciler) reconcileNormal(ctx context.Context, scope *scope.WithLogger, clusterName string, openStackMachineTemplate *infrav1.OpenStackMachineTemplate) (reterr error) {
 	log := scope.Logger()
 
 	computeService, err := newComputeService(scope)
@@ -201,10 +219,19 @@ func (r *OpenStackMachineTemplateReconciler) reconcileNormal(ctx context.Context
 		openStackMachineTemplate.Status.Capacity[corev1.ResourceStorage] = *resource.NewQuantity(storageBytes, resource.BinarySI)
 	}
 
+	// reconcileAllowedAddressPairs is called independently of the image/flavor logic so that
+	// it is never skipped by an early return (e.g. when imageID is not yet resolvable).
+	if err := r.reconcileAllowedAddressPairs(ctx, scope, clusterName, openStackMachineTemplate); err != nil {
+		return err
+	}
+
 	imageID, err := computeService.GetImageID(ctx, r.Client, openStackMachineTemplate.Namespace, openStackMachineTemplate.Spec.Template.Spec.Image)
 	if err != nil {
 		return err
 	}
+	if imageID == nil {
+		return nil
+	}
 
 	image, err := computeService.GetImageDetails(*imageID)
 	if err != nil {
@@ -224,6 +251,144 @@ func (r *OpenStackMachineTemplateReconciler) reconcileNormal(ctx context.Context
 	return nil
 }
 
+// reconcileAllowedAddressPairs updates the allowedAddressPairs on existing Neutron ports
+// to match what is defined in the OpenStackMachineTemplate.
+// Idempotency is tracked via an annotation on each OpenStackMachine so that only a
+// metadata-only patch is needed — this avoids touching the immutable OSM spec.
+func (r *OpenStackMachineTemplateReconciler) reconcileAllowedAddressPairs(ctx context.Context, scope *scope.WithLogger, clusterName string, openStackMachineTemplate *infrav1.OpenStackMachineTemplate) error {
+	log := scope.Logger()
+
+	if len(openStackMachineTemplate.Spec.Template.Spec.Ports) == 0 || clusterName == "" {
+		return nil
+	}
+
+	// Build the desired state as JSON for idempotency comparison.
+	type portPairs = []infrav1.AddressPair
+	templatePorts := openStackMachineTemplate.Spec.Template.Spec.Ports
+	desired := make([]portPairs, len(templatePorts))
+	for i, p := range templatePorts {
+		desired[i] = p.AllowedAddressPairs
+	}
+	desiredJSON, err := json.Marshal(desired)
+	if err != nil {
+		return err
+	}
+	desiredStr := string(desiredJSON)
+
+	// List MachineSets in the namespace for this cluster.
+	machineSetList := &clusterv1.MachineSetList{}
+	if err := r.Client.List(ctx, machineSetList,
+		client.InNamespace(openStackMachineTemplate.Namespace),
+		client.MatchingLabels{clusterv1.ClusterNameLabel: clusterName},
+	); err != nil {
+		return err
+	}
+
+	// List Machines in the namespace for this cluster.
+	machineList := &clusterv1.MachineList{}
+	if err := r.Client.List(ctx, machineList,
+		client.InNamespace(openStackMachineTemplate.Namespace),
+		client.MatchingLabels{clusterv1.ClusterNameLabel: clusterName},
+	); err != nil {
+		return err
+	}
+
+	// Networking service is initialised lazily on first actual port update.
+	var networkingService *networking.Service
+
+	for i := range machineSetList.Items {
+		ms := &machineSetList.Items[i]
+		if ms.Spec.Template.Spec.InfrastructureRef.Name != openStackMachineTemplate.Name {
+			continue
+		}
+
+		for j := range machineList.Items {
+			machine := &machineList.Items[j]
+			if !isOwnedByMachineSet(machine, ms) {
+				continue
+			}
+			infraName := machine.Spec.InfrastructureRef.Name
+			if infraName == "" {
+				continue
+			}
+
+			osm := &infrav1.OpenStackMachine{}
+			if err := r.Client.Get(ctx, client.ObjectKey{
+				Namespace: openStackMachineTemplate.Namespace,
+				Name:      infraName,
+			}, osm); err != nil {
+				if apierrors.IsNotFound(err) {
+					continue
+				}
+				return err
+			}
+
+			// Skip if annotation already reflects the desired state.
+			if osm.Annotations[annotationAllowedAddressPairs] == desiredStr {
+				continue
+			}
+
+			// Port IDs are stored in the OpenStackServer status (same name as the OSM).
+			openStackServer := &infrav1alpha1.OpenStackServer{}
+			if err := r.Client.Get(ctx, client.ObjectKey{
+				Namespace: openStackMachineTemplate.Namespace,
+				Name:      infraName,
+			}, openStackServer); err != nil {
+				if apierrors.IsNotFound(err) {
+					continue
+				}
+				return err
+			}
+
+			if openStackServer.Status.Resources == nil || len(openStackServer.Status.Resources.Ports) == 0 {
+				continue
+			}
+
+			if networkingService == nil {
+				networkingService, err = newNetworkingService(scope)
+				if err != nil {
+					return err
+				}
+			}
+
+			for portIdx, portStatus := range openStackServer.Status.Resources.Ports {
+				if portIdx >= len(templatePorts) {
+					break
+				}
+				pairs := templatePorts[portIdx].AllowedAddressPairs
+				log.Info("Updating allowedAddressPairs on port", "portID", portStatus.ID,
+					"machine", osm.Name, "portIndex", portIdx)
+				if err := networkingService.UpdateAllowedAddressPairs(portStatus.ID, pairs); err != nil {
+					log.Error(err, "Failed to update allowedAddressPairs", "portID", portStatus.ID)
+					return err
+				}
+			}
+
+			// Record the applied state in an annotation (metadata-only patch).
+			osmCopy := osm.DeepCopy()
+			if osm.Annotations == nil {
+				osm.Annotations = map[string]string{}
+			}
+			osm.Annotations[annotationAllowedAddressPairs] = desiredStr
+			if err := r.Client.Patch(ctx, osm, client.MergeFrom(osmCopy)); err != nil {
+				return err
+			}
+		}
+	}
+
+	return nil
+}
+
+// isOwnedByMachineSet returns true if the Machine has an owner reference pointing to ms.
+func isOwnedByMachineSet(machine *clusterv1.Machine, ms *clusterv1.MachineSet) bool {
+	for _, ref := range machine.OwnerReferences {
+		if ref.Kind == "MachineSet" && ref.Name == ms.Name {
+			return true
+		}
+	}
+	return false
+}
+
 func (r *OpenStackMachineTemplateReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
 	log := ctrl.LoggerFrom(ctx)