subnetpool: make prefixes immutable

Author: winiciusallan

api/v1alpha1/subnetpool_types.go

@@ -55,6 +55,7 @@ type SubnetPoolResourceSpec struct {
 	// +kubebuilder:validation:MaxItems:=64
 	// +listType=set
 	// +required
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="prefixes is immutable"
 	Prefixes []CIDR `json:"prefixes,omitempty"`
 
 	// minPrefixLength is the smallest prefix that can be allocated

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

@@ -19,7 +19,6 @@ package subnetpool
 import (
 	"context"
 	"iter"
-	"slices"
 
 	"github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/subnetpools"
 	corev1 "k8s.io/api/core/v1"
@@ -222,7 +221,6 @@ func (actuator subnetpoolActuator) updateResource(ctx context.Context, obj orcOb
 
 	handleNameUpdate(&updateOpts, obj, osResource)
 	handleDescriptionUpdate(&updateOpts, resource, osResource)
-	handlePrefixesUpdate(&updateOpts, resource, osResource)
 	handleMinPrefixLengthUpdate(&updateOpts, resource, osResource)
 	handleMaxPrefixLengthUpdate(&updateOpts, resource, osResource)
 	handleIsDefaultUpdate(&updateOpts, resource, osResource)
@@ -291,22 +289,6 @@ func handleMaxPrefixLengthUpdate(updateOpts *subnetpools.UpdateOpts, resource *r
 	}
 }
 
-func handlePrefixesUpdate(updateOpts *subnetpools.UpdateOpts, resource *resourceSpecT, osResource *osResourceT) {
-	desiredPrefixes := make([]string, len(resource.Prefixes))
-	for i := range resource.Prefixes {
-		desiredPrefixes[i] = string(resource.Prefixes[i])
-	}
-	slices.Sort(desiredPrefixes)
-
-	currentPrefixes := make([]string, len(osResource.Prefixes))
-	copy(currentPrefixes, osResource.Prefixes)
-	slices.Sort(currentPrefixes)
-
-	if !slices.Equal(desiredPrefixes, currentPrefixes) {
-		updateOpts.Prefixes = desiredPrefixes
-	}
-}
-
 func handleIsDefaultUpdate(updateOpts *subnetpools.UpdateOpts, resource *resourceSpecT, osResource *osResourceT) {
 	// fallback to the default value if unset.
 	isDefault := ptr.Deref(resource.IsDefault, false)

internal/controllers/subnetpool/actuator.go

@@ -174,67 +174,6 @@ func TestHandleMaxPrefixLengthUpdate(t *testing.T) {
 	}
 }
 
-func TestHandlePrefixesUpdate(t *testing.T) {
-	testCases := []struct {
-		name             string
-		newPrefixes      []orcv1alpha1.CIDR
-		existingPrefixes []string
-		expectChange     bool
-	}{
-		{
-			name:             "Identical prefixes",
-			newPrefixes:      []orcv1alpha1.CIDR{"192.168.0.0/24", "10.0.0.0/16"},
-			existingPrefixes: []string{"192.168.0.0/24", "10.0.0.0/16"},
-			expectChange:     false,
-		},
-		{
-			name:             "Different prefixes",
-			newPrefixes:      []orcv1alpha1.CIDR{"192.168.0.0/24"},
-			existingPrefixes: []string{"10.0.0.0/16"},
-			expectChange:     true,
-		},
-		{
-			name:             "Prefixes out of order",
-			newPrefixes:      []orcv1alpha1.CIDR{"10.0.0.0/16", "192.168.0.0/24"},
-			existingPrefixes: []string{"192.168.0.0/24", "10.0.0.0/16"},
-			expectChange:     false,
-		},
-		{
-			name:             "Extra prefix in existing",
-			newPrefixes:      []orcv1alpha1.CIDR{"192.168.0.0/24"},
-			existingPrefixes: []string{"192.168.0.0/24", "10.0.0.0/16"},
-			expectChange:     true,
-		},
-		{
-			name:             "Extra prefix in new",
-			newPrefixes:      []orcv1alpha1.CIDR{"192.168.0.0/24", "10.0.0.0/16"},
-			existingPrefixes: []string{"192.168.0.0/24"},
-			expectChange:     true,
-		},
-		{
-			name:             "Empty prefixes",
-			newPrefixes:      []orcv1alpha1.CIDR{},
-			existingPrefixes: []string{},
-			expectChange:     false,
-		},
-	}
-
-	for _, tt := range testCases {
-		t.Run(tt.name, func(t *testing.T) {
-			resource := &orcv1alpha1.SubnetPoolResourceSpec{Prefixes: tt.newPrefixes}
-			osResource := &osResourceT{Prefixes: tt.existingPrefixes}
-
-			updateOpts := subnetpools.UpdateOpts{}
-			handlePrefixesUpdate(&updateOpts, resource, osResource)
-
-			got, _ := needsUpdate(updateOpts)
-			if got != tt.expectChange {
-				t.Errorf("Expected change: %v, got: %v", tt.expectChange, got)
-			}
-		})
-	}
-}
-
 func TestHandleIsDefaultUpdate(t *testing.T) {
 	ptrToBool := ptr.To[bool]
 	testCases := []struct {

internal/controllers/subnetpool/actuator_test.go

@@ -8,7 +8,6 @@ status:
     name: subnetpool-update-updated
     description: subnetpool-update-updated
     prefixes:
-    - 10.0.0.0/16
     - 192.168.10.0/24
     minPrefixLength: 7
     maxPrefixLength: 30

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

@@ -7,9 +7,6 @@ spec:
   resource:
     name: subnetpool-update-updated
     description: subnetpool-update-updated
-    prefixes:
-    - 10.0.0.0/16
-    - 192.168.10.0/24
     minPrefixLength: 7
     maxPrefixLength: 30
 

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

@@ -18,7 +18,6 @@ status:
   resource:
     name: subnetpool-update
     prefixes:
-    - 10.0.0.0/16
     - 192.168.10.0/24
     minPrefixLength: 8
     maxPrefixLength: 32

internal/controllers/subnetpool/tests/subnetpool-update/02-assert.yaml

@@ -1,9 +1,7 @@
 # NOTE: kuttl only does patch updates, which means we can't delete a field.
 # We have to use a kubectl apply command instead.
-# NOTE: prefixes must be a superset of the previous value because OpenStack
-# does not allow removing existing prefixes from a subnet pool.
 apiVersion: kuttl.dev/v1beta1
 kind: TestStep
 commands:
-  - command: kubectl replace -f 02-reverted-subnetpool.yaml
+  - command: kubectl replace -f 00-minimal-resource.yaml
     namespaced: true

internal/controllers/subnetpool/tests/subnetpool-update/02-reverted-resource.yaml

@@ -124,12 +124,15 @@ var _ = Describe("ORC SubnetPool API validations", func() {
 		Expect(applyObj(ctx, obj, patch)).To(MatchError(ContainSubstring("addressScopeRef is immutable")))
 	})
 
-	// TODO(scaffolding): Add more resource-specific validation tests.
-	// Some common things to test:
-	// - Immutability of fields with `self == oldSelf` validation
-	// - Enum validation (valid and invalid values)
-	// - Numeric range validation (min/max bounds)
-	// - Tag uniqueness (if the resource has tags with listType=set)
-	// - Format validation (CIDR, UUID, etc.)
-	// - Cross-field validation rules
+	It("should have immutable prefixes", func(ctx context.Context) {
+		obj := subnetpoolStub(namespace)
+		patch := baseSubnetPoolPatch(obj)
+		patch.Spec.WithResource(testSubnetPoolResource())
+		Expect(applyObj(ctx, obj, patch)).To(Succeed())
+
+		patch.Spec.WithResource(testSubnetPoolResource().
+			WithPrefixes(orcv1alpha1.CIDR("10.0.0.0/8")))
+
+		Expect(applyObj(ctx, obj, patch)).To(MatchError(ContainSubstring("prefixes is immutable")))
+	})
 })