Skip to content

✨ allow switching from filter.name to id of network and subnets in OSC spec #2518

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
k8s-ci-robot merged 1 commit into from
May 9, 2025
Merged

✨ allow switching from filter.name to id of network and subnets in OSC spec #2518

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions pkg/webhooks/openstackcluster_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,60 @@ func (*openStackClusterWebhook) ValidateCreate(_ context.Context, objRaw runtime
return aggregateObjErrors(newObj.GroupVersionKind().GroupKind(), newObj.Name, allErrs)
}

// allowSubnetFilterToIDTransition checks if changes to OpenStackCluster.Spec.Subnets
// are transitioning from a Filter-based definition to an ID-based one, and whether
// those transitions are valid based on the current status.network.subnets.
//
// This function only allows Filter → ID transitions when the filter name in the old
// spec matches the subnet name in status, and the new ID matches the corresponding subnet ID.
//
// Returns true if all such transitions are valid; false otherwise.
func allowSubnetFilterToIDTransition(oldObj, newObj *infrav1.OpenStackCluster) bool {
if newObj.Spec.Network == nil || oldObj.Spec.Network == nil || oldObj.Status.Network == nil {
return false
}

if len(newObj.Spec.Subnets) != len(oldObj.Spec.Subnets) || len(oldObj.Status.Network.Subnets) == 0 {
return false
}

for i := range newObj.Spec.Subnets {
oldSubnet := oldObj.Spec.Subnets[i]
newSubnet := newObj.Spec.Subnets[i]

// Allow Filter → ID only if both values match a known subnet in status
if oldSubnet.Filter != nil && newSubnet.ID != nil && newSubnet.Filter == nil {
matchFound := false
for _, statusSubnet := range oldObj.Status.Network.Subnets {
if oldSubnet.Filter.Name == statusSubnet.Name && *newSubnet.ID == statusSubnet.ID {
matchFound = true
break
}
}
if !matchFound {
return false
}
}

// Reject any change from ID → Filter
if oldSubnet.ID != nil && newSubnet.Filter != nil {
return false
}

// Reject changes to Filter or ID if they do not match the old values
if oldSubnet.Filter != nil && newSubnet.Filter != nil &&
oldSubnet.Filter.Name != newSubnet.Filter.Name {
return false
}
if oldSubnet.ID != nil && newSubnet.ID != nil &&
*oldSubnet.ID != *newSubnet.ID {
return false
}
}

return true
}

// ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type.
func (*openStackClusterWebhook) ValidateUpdate(_ context.Context, oldObjRaw, newObjRaw runtime.Object) (admission.Warnings, error) {
var allErrs field.ErrorList
Expand Down Expand Up @@ -160,6 +214,20 @@ func (*openStackClusterWebhook) ValidateUpdate(_ context.Context, oldObjRaw, new
oldObj.Spec.APIServerFloatingIP = nil
}

// Allow changes from filter to id for spec.network and spec.subnets
if newObj.Spec.Network != nil && oldObj.Spec.Network != nil && oldObj.Status.Network != nil {
// Allow change from spec.network.subnets from filter to id if it matches the current subnets.
if allowSubnetFilterToIDTransition(oldObj, newObj) {
oldObj.Spec.Subnets = nil
newObj.Spec.Subnets = nil
}
// Allow change from spec.network.filter to spec.network.id only if it matches the current network.
if ptr.Deref(newObj.Spec.Network.ID, "") == oldObj.Status.Network.ID {
newObj.Spec.Network = nil
oldObj.Spec.Network = nil
}
}

if !reflect.DeepEqual(oldObj.Spec, newObj.Spec) {
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec"), "cannot be modified"))
}
Expand Down
304 changes: 304 additions & 0 deletions pkg/webhooks/openstackcluster_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -598,6 +598,310 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) {
},
wantErr: false,
},
{
name: "Switching OpenStackCluster.Spec.Network from filter.name to id is allowed when they refer to the same network",
oldTemplate: &infrav1.OpenStackCluster{
Spec: infrav1.OpenStackClusterSpec{
IdentityRef: infrav1.OpenStackIdentityReference{
Name: "foobar",
CloudName: "foobar",
},
Network: &infrav1.NetworkParam{
Filter: &infrav1.NetworkFilter{
Name: "testnetworkname",
},
},
},
Status: infrav1.OpenStackClusterStatus{
Network: &infrav1.NetworkStatusWithSubnets{
NetworkStatus: infrav1.NetworkStatus{
ID: "testnetworkid",
Name: "testnetworkname",
},
},
},
},
newTemplate: &infrav1.OpenStackCluster{
Spec: infrav1.OpenStackClusterSpec{
IdentityRef: infrav1.OpenStackIdentityReference{
Name: "foobar",
CloudName: "foobar",
},
Network: &infrav1.NetworkParam{
ID: ptr.To("testnetworkid"),
},
},
Status: infrav1.OpenStackClusterStatus{
Network: &infrav1.NetworkStatusWithSubnets{
NetworkStatus: infrav1.NetworkStatus{
ID: "testnetworkid",
Name: "testnetworkname",
},
},
},
},
wantErr: false,
},
{
name: "Switching OpenStackCluster.Spec.Network from filter.name to id is not allowed when they refer to different networks",
oldTemplate: &infrav1.OpenStackCluster{
Spec: infrav1.OpenStackClusterSpec{
IdentityRef: infrav1.OpenStackIdentityReference{
Name: "foobar",
CloudName: "foobar",
},
Network: &infrav1.NetworkParam{
Filter: &infrav1.NetworkFilter{
Name: "testetworkname",
},
},
},
Status: infrav1.OpenStackClusterStatus{
Network: &infrav1.NetworkStatusWithSubnets{
NetworkStatus: infrav1.NetworkStatus{
ID: "testetworkid1",
Name: "testnetworkname",
},
},
},
},
newTemplate: &infrav1.OpenStackCluster{
Spec: infrav1.OpenStackClusterSpec{
IdentityRef: infrav1.OpenStackIdentityReference{
Name: "foobar",
CloudName: "foobar",
},
Network: &infrav1.NetworkParam{
ID: ptr.To("testetworkid2"),
},
},
Status: infrav1.OpenStackClusterStatus{
Network: &infrav1.NetworkStatusWithSubnets{
NetworkStatus: infrav1.NetworkStatus{
ID: "testetworkid1",
Name: "testnetworkname",
},
},
},
},
wantErr: true,
},
{
name: "Switching OpenStackCluster.Spec.Subnets from filter.name to id is allowed when they refer to the same subnet",
oldTemplate: &infrav1.OpenStackCluster{
Spec: infrav1.OpenStackClusterSpec{
IdentityRef: infrav1.OpenStackIdentityReference{
Name: "foobar",
CloudName: "foobar",
},
Network: &infrav1.NetworkParam{
ID: ptr.To("net-123"),
},
Subnets: []infrav1.SubnetParam{
{
Filter: &infrav1.SubnetFilter{
Name: "test-subnet",
},
},
},
},
Status: infrav1.OpenStackClusterStatus{
Network: &infrav1.NetworkStatusWithSubnets{
NetworkStatus: infrav1.NetworkStatus{
ID: "net-123",
Name: "testnetwork",
},
Subnets: []infrav1.Subnet{
{
ID: "subnet-123",
Name: "test-subnet",
},
},
},
},
},
newTemplate: &infrav1.OpenStackCluster{
Spec: infrav1.OpenStackClusterSpec{
IdentityRef: infrav1.OpenStackIdentityReference{
Name: "foobar",
CloudName: "foobar",
},
Network: &infrav1.NetworkParam{
ID: ptr.To("net-123"),
},
Subnets: []infrav1.SubnetParam{
{
ID: ptr.To("subnet-123"),
},
},
},
Status: infrav1.OpenStackClusterStatus{
Network: &infrav1.NetworkStatusWithSubnets{
NetworkStatus: infrav1.NetworkStatus{
ID: "net-123",
Name: "testnetwork",
},
Subnets: []infrav1.Subnet{
{
ID: "subnet-123",
Name: "test-subnet",
},
},
},
},
},
wantErr: false,
},
{
name: "Switching OpenStackCluster.Spec.Subnets from filter.name to id is not allowed when they refer to different subnets",
oldTemplate: &infrav1.OpenStackCluster{
Spec: infrav1.OpenStackClusterSpec{
IdentityRef: infrav1.OpenStackIdentityReference{
Name: "foobar",
CloudName: "foobar",
},
Network: &infrav1.NetworkParam{
ID: ptr.To("net-123"),
},
Subnets: []infrav1.SubnetParam{
{
Filter: &infrav1.SubnetFilter{
Name: "test-subnet",
},
},
},
},
Status: infrav1.OpenStackClusterStatus{
Network: &infrav1.NetworkStatusWithSubnets{
NetworkStatus: infrav1.NetworkStatus{
ID: "net-123",
Name: "testnetwork",
},
Subnets: []infrav1.Subnet{
{
ID: "subnet-123",
Name: "test-subnet",
},
},
},
},
},
newTemplate: &infrav1.OpenStackCluster{
Spec: infrav1.OpenStackClusterSpec{
IdentityRef: infrav1.OpenStackIdentityReference{
Name: "foobar",
CloudName: "foobar",
},
Network: &infrav1.NetworkParam{
ID: ptr.To("net-123"),
},
Subnets: []infrav1.SubnetParam{
{
ID: ptr.To("wrong-subnet"),
},
},
},
Status: infrav1.OpenStackClusterStatus{
Network: &infrav1.NetworkStatusWithSubnets{
NetworkStatus: infrav1.NetworkStatus{
ID: "net-123",
Name: "testnetwork",
},
Subnets: []infrav1.Subnet{
{
ID: "subnet-123",
Name: "test-subnet",
},
},
},
},
},
wantErr: true,
},
{
name: "Switching one OpenStackCluster.Spec.Subnets entry from filter to a mismatched ID (from another subnet) should be rejected, even if other subnets remain unchanged",
oldTemplate: &infrav1.OpenStackCluster{
Spec: infrav1.OpenStackClusterSpec{
IdentityRef: infrav1.OpenStackIdentityReference{
Name: "foobar",
CloudName: "foobar",
},
Network: &infrav1.NetworkParam{
ID: ptr.To("net-123"),
},
Subnets: []infrav1.SubnetParam{
{
Filter: &infrav1.SubnetFilter{
Name: "test-subnet-1",
},
},
{
Filter: &infrav1.SubnetFilter{
Name: "test-subnet-2",
},
},
},
},
Status: infrav1.OpenStackClusterStatus{
Network: &infrav1.NetworkStatusWithSubnets{
NetworkStatus: infrav1.NetworkStatus{
ID: "net-123",
Name: "testnetwork",
},
Subnets: []infrav1.Subnet{
{
ID: "test-subnet-id-1",
Name: "test-subnet-1",
},
{
ID: "test-subnet-id-2",
Name: "test-subnet-2",
},
},
},
},
},
newTemplate: &infrav1.OpenStackCluster{
Spec: infrav1.OpenStackClusterSpec{
IdentityRef: infrav1.OpenStackIdentityReference{
Name: "foobar",
CloudName: "foobar",
},
Network: &infrav1.NetworkParam{
ID: ptr.To("net-123"),
},
Subnets: []infrav1.SubnetParam{
{
ID: ptr.To("test-subnet-id-2"),
},
{
Filter: &infrav1.SubnetFilter{
Name: "test-subnet-2",
},
},
},
},
Status: infrav1.OpenStackClusterStatus{
Network: &infrav1.NetworkStatusWithSubnets{
NetworkStatus: infrav1.NetworkStatus{
ID: "net-123",
Name: "testnetwork",
},
Subnets: []infrav1.Subnet{
{
ID: "test-subnet-id-1",
Name: "test-subnet-1",
},
{
ID: "test-subnet-id-2",
Name: "test-subnet-2",
},
},
},
},
},
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down