Skip to content

Commit d884dd5

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #2518 from okozachenko1203/allow-openstackcluster-network-id-change
✨ allow switching from filter.name to id of network and subnets in OSC spec
2 parents 6d07257 + 417ac30 commit d884dd5

File tree

2 files changed

+372
-0
lines changed

2 files changed

+372
-0
lines changed

‎pkg/webhooks/openstackcluster_webhook.go

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,60 @@ func (*openStackClusterWebhook) ValidateCreate(_ context.Context, objRaw runtime
7272
return aggregateObjErrors(newObj.GroupVersionKind().GroupKind(), newObj.Name, allErrs)
7373
}
7474

75+
// allowSubnetFilterToIDTransition checks if changes to OpenStackCluster.Spec.Subnets
76+
// are transitioning from a Filter-based definition to an ID-based one, and whether
77+
// those transitions are valid based on the current status.network.subnets.
78+
//
79+
// This function only allows Filter → ID transitions when the filter name in the old
80+
// spec matches the subnet name in status, and the new ID matches the corresponding subnet ID.
81+
//
82+
// Returns true if all such transitions are valid; false otherwise.
83+
func allowSubnetFilterToIDTransition(oldObj, newObj *infrav1.OpenStackCluster) bool {
84+
if newObj.Spec.Network == nil || oldObj.Spec.Network == nil || oldObj.Status.Network == nil {
85+
return false
86+
}
87+
88+
if len(newObj.Spec.Subnets) != len(oldObj.Spec.Subnets) || len(oldObj.Status.Network.Subnets) == 0 {
89+
return false
90+
}
91+
92+
for i := range newObj.Spec.Subnets {
93+
oldSubnet := oldObj.Spec.Subnets[i]
94+
newSubnet := newObj.Spec.Subnets[i]
95+
96+
// Allow Filter → ID only if both values match a known subnet in status
97+
if oldSubnet.Filter != nil && newSubnet.ID != nil && newSubnet.Filter == nil {
98+
matchFound := false
99+
for _, statusSubnet := range oldObj.Status.Network.Subnets {
100+
if oldSubnet.Filter.Name == statusSubnet.Name && *newSubnet.ID == statusSubnet.ID {
101+
matchFound = true
102+
break
103+
}
104+
}
105+
if !matchFound {
106+
return false
107+
}
108+
}
109+
110+
// Reject any change from ID → Filter
111+
if oldSubnet.ID != nil && newSubnet.Filter != nil {
112+
return false
113+
}
114+
115+
// Reject changes to Filter or ID if they do not match the old values
116+
if oldSubnet.Filter != nil && newSubnet.Filter != nil &&
117+
oldSubnet.Filter.Name != newSubnet.Filter.Name {
118+
return false
119+
}
120+
if oldSubnet.ID != nil && newSubnet.ID != nil &&
121+
*oldSubnet.ID != *newSubnet.ID {
122+
return false
123+
}
124+
}
125+
126+
return true
127+
}
128+
75129
// ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type.
76130
func (*openStackClusterWebhook) ValidateUpdate(_ context.Context, oldObjRaw, newObjRaw runtime.Object) (admission.Warnings, error) {
77131
var allErrs field.ErrorList
@@ -160,6 +214,20 @@ func (*openStackClusterWebhook) ValidateUpdate(_ context.Context, oldObjRaw, new
160214
oldObj.Spec.APIServerFloatingIP = nil
161215
}
162216

217+
// Allow changes from filter to id for spec.network and spec.subnets
218+
if newObj.Spec.Network != nil && oldObj.Spec.Network != nil && oldObj.Status.Network != nil {
219+
// Allow change from spec.network.subnets from filter to id if it matches the current subnets.
220+
if allowSubnetFilterToIDTransition(oldObj, newObj) {
221+
oldObj.Spec.Subnets = nil
222+
newObj.Spec.Subnets = nil
223+
}
224+
// Allow change from spec.network.filter to spec.network.id only if it matches the current network.
225+
if ptr.Deref(newObj.Spec.Network.ID, "") == oldObj.Status.Network.ID {
226+
newObj.Spec.Network = nil
227+
oldObj.Spec.Network = nil
228+
}
229+
}
230+
163231
if !reflect.DeepEqual(oldObj.Spec, newObj.Spec) {
164232
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec"), "cannot be modified"))
165233
}

‎pkg/webhooks/openstackcluster_webhook_test.go

Lines changed: 304 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -598,6 +598,310 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) {
598598
},
599599
wantErr: false,
600600
},
601+
{
602+
name: "Switching OpenStackCluster.Spec.Network from filter.name to id is allowed when they refer to the same network",
603+
oldTemplate: &infrav1.OpenStackCluster{
604+
Spec: infrav1.OpenStackClusterSpec{
605+
IdentityRef: infrav1.OpenStackIdentityReference{
606+
Name: "foobar",
607+
CloudName: "foobar",
608+
},
609+
Network: &infrav1.NetworkParam{
610+
Filter: &infrav1.NetworkFilter{
611+
Name: "testnetworkname",
612+
},
613+
},
614+
},
615+
Status: infrav1.OpenStackClusterStatus{
616+
Network: &infrav1.NetworkStatusWithSubnets{
617+
NetworkStatus: infrav1.NetworkStatus{
618+
ID: "testnetworkid",
619+
Name: "testnetworkname",
620+
},
621+
},
622+
},
623+
},
624+
newTemplate: &infrav1.OpenStackCluster{
625+
Spec: infrav1.OpenStackClusterSpec{
626+
IdentityRef: infrav1.OpenStackIdentityReference{
627+
Name: "foobar",
628+
CloudName: "foobar",
629+
},
630+
Network: &infrav1.NetworkParam{
631+
ID: ptr.To("testnetworkid"),
632+
},
633+
},
634+
Status: infrav1.OpenStackClusterStatus{
635+
Network: &infrav1.NetworkStatusWithSubnets{
636+
NetworkStatus: infrav1.NetworkStatus{
637+
ID: "testnetworkid",
638+
Name: "testnetworkname",
639+
},
640+
},
641+
},
642+
},
643+
wantErr: false,
644+
},
645+
{
646+
name: "Switching OpenStackCluster.Spec.Network from filter.name to id is not allowed when they refer to different networks",
647+
oldTemplate: &infrav1.OpenStackCluster{
648+
Spec: infrav1.OpenStackClusterSpec{
649+
IdentityRef: infrav1.OpenStackIdentityReference{
650+
Name: "foobar",
651+
CloudName: "foobar",
652+
},
653+
Network: &infrav1.NetworkParam{
654+
Filter: &infrav1.NetworkFilter{
655+
Name: "testetworkname",
656+
},
657+
},
658+
},
659+
Status: infrav1.OpenStackClusterStatus{
660+
Network: &infrav1.NetworkStatusWithSubnets{
661+
NetworkStatus: infrav1.NetworkStatus{
662+
ID: "testetworkid1",
663+
Name: "testnetworkname",
664+
},
665+
},
666+
},
667+
},
668+
newTemplate: &infrav1.OpenStackCluster{
669+
Spec: infrav1.OpenStackClusterSpec{
670+
IdentityRef: infrav1.OpenStackIdentityReference{
671+
Name: "foobar",
672+
CloudName: "foobar",
673+
},
674+
Network: &infrav1.NetworkParam{
675+
ID: ptr.To("testetworkid2"),
676+
},
677+
},
678+
Status: infrav1.OpenStackClusterStatus{
679+
Network: &infrav1.NetworkStatusWithSubnets{
680+
NetworkStatus: infrav1.NetworkStatus{
681+
ID: "testetworkid1",
682+
Name: "testnetworkname",
683+
},
684+
},
685+
},
686+
},
687+
wantErr: true,
688+
},
689+
{
690+
name: "Switching OpenStackCluster.Spec.Subnets from filter.name to id is allowed when they refer to the same subnet",
691+
oldTemplate: &infrav1.OpenStackCluster{
692+
Spec: infrav1.OpenStackClusterSpec{
693+
IdentityRef: infrav1.OpenStackIdentityReference{
694+
Name: "foobar",
695+
CloudName: "foobar",
696+
},
697+
Network: &infrav1.NetworkParam{
698+
ID: ptr.To("net-123"),
699+
},
700+
Subnets: []infrav1.SubnetParam{
701+
{
702+
Filter: &infrav1.SubnetFilter{
703+
Name: "test-subnet",
704+
},
705+
},
706+
},
707+
},
708+
Status: infrav1.OpenStackClusterStatus{
709+
Network: &infrav1.NetworkStatusWithSubnets{
710+
NetworkStatus: infrav1.NetworkStatus{
711+
ID: "net-123",
712+
Name: "testnetwork",
713+
},
714+
Subnets: []infrav1.Subnet{
715+
{
716+
ID: "subnet-123",
717+
Name: "test-subnet",
718+
},
719+
},
720+
},
721+
},
722+
},
723+
newTemplate: &infrav1.OpenStackCluster{
724+
Spec: infrav1.OpenStackClusterSpec{
725+
IdentityRef: infrav1.OpenStackIdentityReference{
726+
Name: "foobar",
727+
CloudName: "foobar",
728+
},
729+
Network: &infrav1.NetworkParam{
730+
ID: ptr.To("net-123"),
731+
},
732+
Subnets: []infrav1.SubnetParam{
733+
{
734+
ID: ptr.To("subnet-123"),
735+
},
736+
},
737+
},
738+
Status: infrav1.OpenStackClusterStatus{
739+
Network: &infrav1.NetworkStatusWithSubnets{
740+
NetworkStatus: infrav1.NetworkStatus{
741+
ID: "net-123",
742+
Name: "testnetwork",
743+
},
744+
Subnets: []infrav1.Subnet{
745+
{
746+
ID: "subnet-123",
747+
Name: "test-subnet",
748+
},
749+
},
750+
},
751+
},
752+
},
753+
wantErr: false,
754+
},
755+
{
756+
name: "Switching OpenStackCluster.Spec.Subnets from filter.name to id is not allowed when they refer to different subnets",
757+
oldTemplate: &infrav1.OpenStackCluster{
758+
Spec: infrav1.OpenStackClusterSpec{
759+
IdentityRef: infrav1.OpenStackIdentityReference{
760+
Name: "foobar",
761+
CloudName: "foobar",
762+
},
763+
Network: &infrav1.NetworkParam{
764+
ID: ptr.To("net-123"),
765+
},
766+
Subnets: []infrav1.SubnetParam{
767+
{
768+
Filter: &infrav1.SubnetFilter{
769+
Name: "test-subnet",
770+
},
771+
},
772+
},
773+
},
774+
Status: infrav1.OpenStackClusterStatus{
775+
Network: &infrav1.NetworkStatusWithSubnets{
776+
NetworkStatus: infrav1.NetworkStatus{
777+
ID: "net-123",
778+
Name: "testnetwork",
779+
},
780+
Subnets: []infrav1.Subnet{
781+
{
782+
ID: "subnet-123",
783+
Name: "test-subnet",
784+
},
785+
},
786+
},
787+
},
788+
},
789+
newTemplate: &infrav1.OpenStackCluster{
790+
Spec: infrav1.OpenStackClusterSpec{
791+
IdentityRef: infrav1.OpenStackIdentityReference{
792+
Name: "foobar",
793+
CloudName: "foobar",
794+
},
795+
Network: &infrav1.NetworkParam{
796+
ID: ptr.To("net-123"),
797+
},
798+
Subnets: []infrav1.SubnetParam{
799+
{
800+
ID: ptr.To("wrong-subnet"),
801+
},
802+
},
803+
},
804+
Status: infrav1.OpenStackClusterStatus{
805+
Network: &infrav1.NetworkStatusWithSubnets{
806+
NetworkStatus: infrav1.NetworkStatus{
807+
ID: "net-123",
808+
Name: "testnetwork",
809+
},
810+
Subnets: []infrav1.Subnet{
811+
{
812+
ID: "subnet-123",
813+
Name: "test-subnet",
814+
},
815+
},
816+
},
817+
},
818+
},
819+
wantErr: true,
820+
},
821+
{
822+
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",
823+
oldTemplate: &infrav1.OpenStackCluster{
824+
Spec: infrav1.OpenStackClusterSpec{
825+
IdentityRef: infrav1.OpenStackIdentityReference{
826+
Name: "foobar",
827+
CloudName: "foobar",
828+
},
829+
Network: &infrav1.NetworkParam{
830+
ID: ptr.To("net-123"),
831+
},
832+
Subnets: []infrav1.SubnetParam{
833+
{
834+
Filter: &infrav1.SubnetFilter{
835+
Name: "test-subnet-1",
836+
},
837+
},
838+
{
839+
Filter: &infrav1.SubnetFilter{
840+
Name: "test-subnet-2",
841+
},
842+
},
843+
},
844+
},
845+
Status: infrav1.OpenStackClusterStatus{
846+
Network: &infrav1.NetworkStatusWithSubnets{
847+
NetworkStatus: infrav1.NetworkStatus{
848+
ID: "net-123",
849+
Name: "testnetwork",
850+
},
851+
Subnets: []infrav1.Subnet{
852+
{
853+
ID: "test-subnet-id-1",
854+
Name: "test-subnet-1",
855+
},
856+
{
857+
ID: "test-subnet-id-2",
858+
Name: "test-subnet-2",
859+
},
860+
},
861+
},
862+
},
863+
},
864+
newTemplate: &infrav1.OpenStackCluster{
865+
Spec: infrav1.OpenStackClusterSpec{
866+
IdentityRef: infrav1.OpenStackIdentityReference{
867+
Name: "foobar",
868+
CloudName: "foobar",
869+
},
870+
Network: &infrav1.NetworkParam{
871+
ID: ptr.To("net-123"),
872+
},
873+
Subnets: []infrav1.SubnetParam{
874+
{
875+
ID: ptr.To("test-subnet-id-2"),
876+
},
877+
{
878+
Filter: &infrav1.SubnetFilter{
879+
Name: "test-subnet-2",
880+
},
881+
},
882+
},
883+
},
884+
Status: infrav1.OpenStackClusterStatus{
885+
Network: &infrav1.NetworkStatusWithSubnets{
886+
NetworkStatus: infrav1.NetworkStatus{
887+
ID: "net-123",
888+
Name: "testnetwork",
889+
},
890+
Subnets: []infrav1.Subnet{
891+
{
892+
ID: "test-subnet-id-1",
893+
Name: "test-subnet-1",
894+
},
895+
{
896+
ID: "test-subnet-id-2",
897+
Name: "test-subnet-2",
898+
},
899+
},
900+
},
901+
},
902+
},
903+
wantErr: true,
904+
},
601905
}
602906
for _, tt := range tests {
603907
t.Run(tt.name, func(t *testing.T) {

0 commit comments

Comments
 (0)