Skip to content

Commit bdfba15

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes-sigs#2541 from k8s-infra-cherrypick-robot/cherry-pick-2540-to-release-0.11
🐛 allow switching from filter.name to id of network and subnets in OSC spec
2 parents 4bd158a + 60f1805 commit bdfba15

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
@@ -154,6 +208,20 @@ func (*openStackClusterWebhook) ValidateUpdate(_ context.Context, oldObjRaw, new
154208
oldObj.Spec.APIServerFloatingIP = nil
155209
}
156210

211+
// Allow changes from filter to id for spec.network and spec.subnets
212+
if newObj.Spec.Network != nil && oldObj.Spec.Network != nil && oldObj.Status.Network != nil {
213+
// Allow change from spec.network.subnets from filter to id if it matches the current subnets.
214+
if allowSubnetFilterToIDTransition(oldObj, newObj) {
215+
oldObj.Spec.Subnets = nil
216+
newObj.Spec.Subnets = nil
217+
}
218+
// Allow change from spec.network.filter to spec.network.id only if it matches the current network.
219+
if ptr.Deref(newObj.Spec.Network.ID, "") == oldObj.Status.Network.ID {
220+
newObj.Spec.Network = nil
221+
oldObj.Spec.Network = nil
222+
}
223+
}
224+
157225
if !reflect.DeepEqual(oldObj.Spec, newObj.Spec) {
158226
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec"), "cannot be modified"))
159227
}

‎pkg/webhooks/openstackcluster_webhook_test.go

Lines changed: 304 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -510,6 +510,310 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) {
510510
},
511511
wantErr: false,
512512
},
513+
{
514+
name: "Switching OpenStackCluster.Spec.Network from filter.name to id is allowed when they refer to the same network",
515+
oldTemplate: &infrav1.OpenStackCluster{
516+
Spec: infrav1.OpenStackClusterSpec{
517+
IdentityRef: infrav1.OpenStackIdentityReference{
518+
Name: "foobar",
519+
CloudName: "foobar",
520+
},
521+
Network: &infrav1.NetworkParam{
522+
Filter: &infrav1.NetworkFilter{
523+
Name: "testnetworkname",
524+
},
525+
},
526+
},
527+
Status: infrav1.OpenStackClusterStatus{
528+
Network: &infrav1.NetworkStatusWithSubnets{
529+
NetworkStatus: infrav1.NetworkStatus{
530+
ID: "testnetworkid",
531+
Name: "testnetworkname",
532+
},
533+
},
534+
},
535+
},
536+
newTemplate: &infrav1.OpenStackCluster{
537+
Spec: infrav1.OpenStackClusterSpec{
538+
IdentityRef: infrav1.OpenStackIdentityReference{
539+
Name: "foobar",
540+
CloudName: "foobar",
541+
},
542+
Network: &infrav1.NetworkParam{
543+
ID: ptr.To("testnetworkid"),
544+
},
545+
},
546+
Status: infrav1.OpenStackClusterStatus{
547+
Network: &infrav1.NetworkStatusWithSubnets{
548+
NetworkStatus: infrav1.NetworkStatus{
549+
ID: "testnetworkid",
550+
Name: "testnetworkname",
551+
},
552+
},
553+
},
554+
},
555+
wantErr: false,
556+
},
557+
{
558+
name: "Switching OpenStackCluster.Spec.Network from filter.name to id is not allowed when they refer to different networks",
559+
oldTemplate: &infrav1.OpenStackCluster{
560+
Spec: infrav1.OpenStackClusterSpec{
561+
IdentityRef: infrav1.OpenStackIdentityReference{
562+
Name: "foobar",
563+
CloudName: "foobar",
564+
},
565+
Network: &infrav1.NetworkParam{
566+
Filter: &infrav1.NetworkFilter{
567+
Name: "testetworkname",
568+
},
569+
},
570+
},
571+
Status: infrav1.OpenStackClusterStatus{
572+
Network: &infrav1.NetworkStatusWithSubnets{
573+
NetworkStatus: infrav1.NetworkStatus{
574+
ID: "testetworkid1",
575+
Name: "testnetworkname",
576+
},
577+
},
578+
},
579+
},
580+
newTemplate: &infrav1.OpenStackCluster{
581+
Spec: infrav1.OpenStackClusterSpec{
582+
IdentityRef: infrav1.OpenStackIdentityReference{
583+
Name: "foobar",
584+
CloudName: "foobar",
585+
},
586+
Network: &infrav1.NetworkParam{
587+
ID: ptr.To("testetworkid2"),
588+
},
589+
},
590+
Status: infrav1.OpenStackClusterStatus{
591+
Network: &infrav1.NetworkStatusWithSubnets{
592+
NetworkStatus: infrav1.NetworkStatus{
593+
ID: "testetworkid1",
594+
Name: "testnetworkname",
595+
},
596+
},
597+
},
598+
},
599+
wantErr: true,
600+
},
601+
{
602+
name: "Switching OpenStackCluster.Spec.Subnets from filter.name to id is allowed when they refer to the same subnet",
603+
oldTemplate: &infrav1.OpenStackCluster{
604+
Spec: infrav1.OpenStackClusterSpec{
605+
IdentityRef: infrav1.OpenStackIdentityReference{
606+
Name: "foobar",
607+
CloudName: "foobar",
608+
},
609+
Network: &infrav1.NetworkParam{
610+
ID: ptr.To("net-123"),
611+
},
612+
Subnets: []infrav1.SubnetParam{
613+
{
614+
Filter: &infrav1.SubnetFilter{
615+
Name: "test-subnet",
616+
},
617+
},
618+
},
619+
},
620+
Status: infrav1.OpenStackClusterStatus{
621+
Network: &infrav1.NetworkStatusWithSubnets{
622+
NetworkStatus: infrav1.NetworkStatus{
623+
ID: "net-123",
624+
Name: "testnetwork",
625+
},
626+
Subnets: []infrav1.Subnet{
627+
{
628+
ID: "subnet-123",
629+
Name: "test-subnet",
630+
},
631+
},
632+
},
633+
},
634+
},
635+
newTemplate: &infrav1.OpenStackCluster{
636+
Spec: infrav1.OpenStackClusterSpec{
637+
IdentityRef: infrav1.OpenStackIdentityReference{
638+
Name: "foobar",
639+
CloudName: "foobar",
640+
},
641+
Network: &infrav1.NetworkParam{
642+
ID: ptr.To("net-123"),
643+
},
644+
Subnets: []infrav1.SubnetParam{
645+
{
646+
ID: ptr.To("subnet-123"),
647+
},
648+
},
649+
},
650+
Status: infrav1.OpenStackClusterStatus{
651+
Network: &infrav1.NetworkStatusWithSubnets{
652+
NetworkStatus: infrav1.NetworkStatus{
653+
ID: "net-123",
654+
Name: "testnetwork",
655+
},
656+
Subnets: []infrav1.Subnet{
657+
{
658+
ID: "subnet-123",
659+
Name: "test-subnet",
660+
},
661+
},
662+
},
663+
},
664+
},
665+
wantErr: false,
666+
},
667+
{
668+
name: "Switching OpenStackCluster.Spec.Subnets from filter.name to id is not allowed when they refer to different subnets",
669+
oldTemplate: &infrav1.OpenStackCluster{
670+
Spec: infrav1.OpenStackClusterSpec{
671+
IdentityRef: infrav1.OpenStackIdentityReference{
672+
Name: "foobar",
673+
CloudName: "foobar",
674+
},
675+
Network: &infrav1.NetworkParam{
676+
ID: ptr.To("net-123"),
677+
},
678+
Subnets: []infrav1.SubnetParam{
679+
{
680+
Filter: &infrav1.SubnetFilter{
681+
Name: "test-subnet",
682+
},
683+
},
684+
},
685+
},
686+
Status: infrav1.OpenStackClusterStatus{
687+
Network: &infrav1.NetworkStatusWithSubnets{
688+
NetworkStatus: infrav1.NetworkStatus{
689+
ID: "net-123",
690+
Name: "testnetwork",
691+
},
692+
Subnets: []infrav1.Subnet{
693+
{
694+
ID: "subnet-123",
695+
Name: "test-subnet",
696+
},
697+
},
698+
},
699+
},
700+
},
701+
newTemplate: &infrav1.OpenStackCluster{
702+
Spec: infrav1.OpenStackClusterSpec{
703+
IdentityRef: infrav1.OpenStackIdentityReference{
704+
Name: "foobar",
705+
CloudName: "foobar",
706+
},
707+
Network: &infrav1.NetworkParam{
708+
ID: ptr.To("net-123"),
709+
},
710+
Subnets: []infrav1.SubnetParam{
711+
{
712+
ID: ptr.To("wrong-subnet"),
713+
},
714+
},
715+
},
716+
Status: infrav1.OpenStackClusterStatus{
717+
Network: &infrav1.NetworkStatusWithSubnets{
718+
NetworkStatus: infrav1.NetworkStatus{
719+
ID: "net-123",
720+
Name: "testnetwork",
721+
},
722+
Subnets: []infrav1.Subnet{
723+
{
724+
ID: "subnet-123",
725+
Name: "test-subnet",
726+
},
727+
},
728+
},
729+
},
730+
},
731+
wantErr: true,
732+
},
733+
{
734+
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",
735+
oldTemplate: &infrav1.OpenStackCluster{
736+
Spec: infrav1.OpenStackClusterSpec{
737+
IdentityRef: infrav1.OpenStackIdentityReference{
738+
Name: "foobar",
739+
CloudName: "foobar",
740+
},
741+
Network: &infrav1.NetworkParam{
742+
ID: ptr.To("net-123"),
743+
},
744+
Subnets: []infrav1.SubnetParam{
745+
{
746+
Filter: &infrav1.SubnetFilter{
747+
Name: "test-subnet-1",
748+
},
749+
},
750+
{
751+
Filter: &infrav1.SubnetFilter{
752+
Name: "test-subnet-2",
753+
},
754+
},
755+
},
756+
},
757+
Status: infrav1.OpenStackClusterStatus{
758+
Network: &infrav1.NetworkStatusWithSubnets{
759+
NetworkStatus: infrav1.NetworkStatus{
760+
ID: "net-123",
761+
Name: "testnetwork",
762+
},
763+
Subnets: []infrav1.Subnet{
764+
{
765+
ID: "test-subnet-id-1",
766+
Name: "test-subnet-1",
767+
},
768+
{
769+
ID: "test-subnet-id-2",
770+
Name: "test-subnet-2",
771+
},
772+
},
773+
},
774+
},
775+
},
776+
newTemplate: &infrav1.OpenStackCluster{
777+
Spec: infrav1.OpenStackClusterSpec{
778+
IdentityRef: infrav1.OpenStackIdentityReference{
779+
Name: "foobar",
780+
CloudName: "foobar",
781+
},
782+
Network: &infrav1.NetworkParam{
783+
ID: ptr.To("net-123"),
784+
},
785+
Subnets: []infrav1.SubnetParam{
786+
{
787+
ID: ptr.To("test-subnet-id-2"),
788+
},
789+
{
790+
Filter: &infrav1.SubnetFilter{
791+
Name: "test-subnet-2",
792+
},
793+
},
794+
},
795+
},
796+
Status: infrav1.OpenStackClusterStatus{
797+
Network: &infrav1.NetworkStatusWithSubnets{
798+
NetworkStatus: infrav1.NetworkStatus{
799+
ID: "net-123",
800+
Name: "testnetwork",
801+
},
802+
Subnets: []infrav1.Subnet{
803+
{
804+
ID: "test-subnet-id-1",
805+
Name: "test-subnet-1",
806+
},
807+
{
808+
ID: "test-subnet-id-2",
809+
Name: "test-subnet-2",
810+
},
811+
},
812+
},
813+
},
814+
},
815+
wantErr: true,
816+
},
513817
}
514818
for _, tt := range tests {
515819
t.Run(tt.name, func(t *testing.T) {

0 commit comments

Comments
 (0)