🐛 Fix multiple panics in restore functions

api/v1alpha6/conversion_test.go

@@ -763,3 +763,34 @@ func TestConvert_v1alpha6_OpenStackMachineSpec_To_v1beta1_OpenStackMachineSpec(t
 		})
 	}
 }
+
+func Test_FuzzRestorers(t *testing.T) {
+	/* Cluster */
+	testhelpers.FuzzRestorer(t, "restorev1alpha6ClusterSpec", restorev1alpha6ClusterSpec)
+	testhelpers.FuzzRestorer(t, "restorev1beta1ClusterSpec", restorev1beta1ClusterSpec)
+	testhelpers.FuzzRestorer(t, "restorev1alpha6ClusterStatus", restorev1alpha6ClusterStatus)
+	testhelpers.FuzzRestorer(t, "restorev1beta1ClusterStatus", restorev1beta1ClusterStatus)
+	testhelpers.FuzzRestorer(t, "restorev1beta1Bastion", restorev1beta1Bastion)
+	testhelpers.FuzzRestorer(t, "restorev1beta1BastionStatus", restorev1beta1BastionStatus)
+
+	/* ClusterTemplate */
+	testhelpers.FuzzRestorer(t, "restorev1beta1ClusterTemplateSpec", restorev1beta1ClusterTemplateSpec)
+
+	/* Machine */
+	testhelpers.FuzzRestorer(t, "restorev1alpha6MachineSpec", restorev1alpha6MachineSpec)
+	testhelpers.FuzzRestorer(t, "restorev1beta1MachineSpec", restorev1beta1MachineSpec)
+
+	/* MachineTemplate */
+	testhelpers.FuzzRestorer(t, "restorev1alpha6MachineTemplateMachineSpec", restorev1alpha6MachineTemplateMachineSpec)
+
+	/* Types */
+	testhelpers.FuzzRestorer(t, "restorev1alpha6SecurityGroupFilter", restorev1alpha6SecurityGroupFilter)
+	testhelpers.FuzzRestorer(t, "restorev1beta1SecurityGroupParam", restorev1beta1SecurityGroupParam)
+	testhelpers.FuzzRestorer(t, "restorev1alpha6NetworkFilter", restorev1alpha6NetworkFilter)
+	testhelpers.FuzzRestorer(t, "restorev1beta1NetworkParam", restorev1beta1NetworkParam)
+	testhelpers.FuzzRestorer(t, "restorev1alpha6SubnetFilter", restorev1alpha6SubnetFilter)
+	testhelpers.FuzzRestorer(t, "restorev1alpha6SubnetParam", restorev1alpha6SubnetParam)
+	testhelpers.FuzzRestorer(t, "restorev1beta1SubnetParam", restorev1beta1SubnetParam)
+	testhelpers.FuzzRestorer(t, "restorev1alpha6Port", restorev1alpha6Port)
+	testhelpers.FuzzRestorer(t, "restorev1alpha6SecurityGroup", restorev1alpha6SecurityGroup)
+}

api/v1alpha6/openstackcluster_conversion.go

@@ -120,17 +120,19 @@ func restorev1alpha6ClusterSpec(previous *OpenStackClusterSpec, dst *OpenStackCl
 		return
 	}
 
-	for i := range previous.ExternalRouterIPs {
-		dstIP := &dst.ExternalRouterIPs[i]
-		previousIP := &previous.ExternalRouterIPs[i]
-
-		// Subnet.Filter.ID was overwritten in up-conversion by Subnet.UUID
-		dstIP.Subnet.Filter.ID = previousIP.Subnet.Filter.ID
-
-		// If Subnet.UUID was previously unset, we overwrote it with the value of Subnet.Filter.ID
-		// Don't unset it again if it doesn't have the previous value of Subnet.Filter.ID, because that means it was genuinely changed
-		if previousIP.Subnet.UUID == "" && dstIP.Subnet.UUID == previousIP.Subnet.Filter.ID {
-			dstIP.Subnet.UUID = ""
+	if len(previous.ExternalRouterIPs) == len(dst.ExternalRouterIPs) {
+		for i := range previous.ExternalRouterIPs {
+			dstIP := &dst.ExternalRouterIPs[i]
+			previousIP := &previous.ExternalRouterIPs[i]
+
+			// Subnet.Filter.ID was overwritten in up-conversion by Subnet.UUID
+			dstIP.Subnet.Filter.ID = previousIP.Subnet.Filter.ID
+
+			// If Subnet.UUID was previously unset, we overwrote it with the value of Subnet.Filter.ID
+			// Don't unset it again if it doesn't have the previous value of Subnet.Filter.ID, because that means it was genuinely changed
+			if previousIP.Subnet.UUID == "" && dstIP.Subnet.UUID == previousIP.Subnet.Filter.ID {
+				dstIP.Subnet.UUID = ""
+			}
 		}
 	}
 
@@ -199,7 +201,7 @@ func restorev1beta1ClusterSpec(previous *infrav1.OpenStackClusterSpec, dst *infr
 
 	dst.ManagedSubnets = previous.ManagedSubnets
 
-	if previous.ManagedSecurityGroups != nil {
+	if previous.ManagedSecurityGroups != nil && dst.ManagedSecurityGroups != nil {
 		dst.ManagedSecurityGroups.AllNodesSecurityGroupRules = previous.ManagedSecurityGroups.AllNodesSecurityGroupRules
 	}
 
@@ -345,13 +347,13 @@ func Convert_v1beta1_OpenStackClusterSpec_To_v1alpha6_OpenStackClusterSpec(in *i
 func restorev1alpha6ClusterStatus(previous *OpenStackClusterStatus, dst *OpenStackClusterStatus) {
 	// PortOpts.SecurityGroups have been removed in v1beta1
 	// We restore the whole PortOpts/Networks since they are anyway immutable.
-	if previous.ExternalNetwork != nil {
+	if previous.ExternalNetwork != nil && dst.ExternalNetwork != nil {
 		dst.ExternalNetwork.PortOpts = previous.ExternalNetwork.PortOpts
 	}
 	if previous.Network != nil {
 		dst.Network = previous.Network
 	}
-	if previous.Bastion != nil && previous.Bastion.Networks != nil {
+	if previous.Bastion != nil && previous.Bastion.Networks != nil && dst.Bastion != nil {
 		dst.Bastion.Networks = previous.Bastion.Networks
 	}
 

api/v1alpha6/types_conversion.go

@@ -224,7 +224,7 @@ func restorev1beta1SubnetParam(previous *infrav1.SubnetParam, dst *infrav1.Subne
 
 	optional.RestoreString(&previous.ID, &dst.ID)
 
-	if dst.Filter != nil {
+	if previous.Filter != nil && dst.Filter != nil {
 		dst.Filter.FilterByNeutronTags = previous.Filter.FilterByNeutronTags
 	}
 }

api/v1alpha7/conversion_test.go

@@ -371,3 +371,38 @@ func TestConvert_v1alpha7_OpenStackMachineSpec_To_v1beta1_OpenStackMachineSpec(t
 		})
 	}
 }
+
+func Test_FuzzRestorers(t *testing.T) {
+	/* Cluster */
+	testhelpers.FuzzRestorer(t, "restorev1alpha7ClusterSpec", restorev1alpha7ClusterSpec)
+	testhelpers.FuzzRestorer(t, "restorev1beta1ClusterSpec", restorev1beta1ClusterSpec)
+	testhelpers.FuzzRestorer(t, "restorev1alpha7ClusterStatus", restorev1alpha7ClusterStatus)
+	testhelpers.FuzzRestorer(t, "restorev1beta1ClusterStatus", restorev1beta1ClusterStatus)
+	testhelpers.FuzzRestorer(t, "restorev1alpha7Bastion", restorev1alpha7Bastion)
+	testhelpers.FuzzRestorer(t, "restorev1beta1Bastion", restorev1beta1Bastion)
+	testhelpers.FuzzRestorer(t, "restorev1beta1BastionStatus", restorev1beta1BastionStatus)
+
+	/* ClusterTemplate */
+	testhelpers.FuzzRestorer(t, "restorev1alpha7ClusterTemplateSpec", restorev1alpha7ClusterTemplateSpec)
+	testhelpers.FuzzRestorer(t, "restorev1alpha7ClusterTemplateSpec", restorev1alpha7ClusterTemplateSpec)
+
+	/* Machine */
+	testhelpers.FuzzRestorer(t, "restorev1alpha7MachineSpec", restorev1alpha7MachineSpec)
+	testhelpers.FuzzRestorer(t, "restorev1beta1MachineSpec", restorev1beta1MachineSpec)
+
+	/* MachineTemplate */
+	testhelpers.FuzzRestorer(t, "restorev1alpha7MachineTemplateSpec", restorev1alpha7MachineTemplateSpec)
+
+	/* Types */
+	testhelpers.FuzzRestorer(t, "restorev1alpha7SecurityGroupFilter", restorev1alpha7SecurityGroupFilter)
+	testhelpers.FuzzRestorer(t, "restorev1alpha7SecurityGroup", restorev1alpha7SecurityGroup)
+	testhelpers.FuzzRestorer(t, "restorev1beta1SecurityGroupParam", restorev1beta1SecurityGroupParam)
+	testhelpers.FuzzRestorer(t, "restorev1alpha7NetworkFilter", restorev1alpha7NetworkFilter)
+	testhelpers.FuzzRestorer(t, "restorev1beta1NetworkParam", restorev1beta1NetworkParam)
+	testhelpers.FuzzRestorer(t, "restorev1alpha7SubnetFilter", restorev1alpha7SubnetFilter)
+	testhelpers.FuzzRestorer(t, "restorev1beta1SubnetParam", restorev1beta1SubnetParam)
+	testhelpers.FuzzRestorer(t, "restorev1alpha7RouterFilter", restorev1alpha7RouterFilter)
+	testhelpers.FuzzRestorer(t, "restorev1beta1RouterParam", restorev1beta1RouterParam)
+	testhelpers.FuzzRestorer(t, "restorev1alpha7Port", restorev1alpha7Port)
+	testhelpers.FuzzRestorer(t, "restorev1beta1Port", restorev1beta1Port)
+}

api/v1alpha7/openstackcluster_conversion.go

@@ -207,7 +207,7 @@ func restorev1beta1ClusterSpec(previous *infrav1.OpenStackClusterSpec, dst *infr
 
 	dst.ManagedSubnets = previous.ManagedSubnets
 
-	if previous.ManagedSecurityGroups != nil {
+	if previous.ManagedSecurityGroups != nil && dst.ManagedSecurityGroups != nil {
 		dst.ManagedSecurityGroups.AllNodesSecurityGroupRules = previous.ManagedSecurityGroups.AllNodesSecurityGroupRules
 	}
 
@@ -371,26 +371,24 @@ func Convert_v1beta1_OpenStackClusterStatus_To_v1alpha7_OpenStackClusterStatus(i
 /* Bastion */
 
 func restorev1alpha7Bastion(previous **Bastion, dst **Bastion) {
-	if *previous == nil || *dst == nil {
+	if previous == nil || dst == nil || *previous == nil || *dst == nil {
 		return
 	}
-
 	prevMachineSpec := &(*previous).Instance
 	dstMachineSpec := &(*dst).Instance
 	restorev1alpha7MachineSpec(prevMachineSpec, dstMachineSpec)
 	dstMachineSpec.InstanceID = prevMachineSpec.InstanceID
 }
 
 func restorev1beta1Bastion(previous **infrav1.Bastion, dst **infrav1.Bastion) {
-	if *previous != nil {
-		if *dst != nil && (*previous).Spec != nil && (*dst).Spec != nil {
-			restorev1beta1MachineSpec((*previous).Spec, (*dst).Spec)
-		}
-
-		optional.RestoreString(&(*previous).FloatingIP, &(*dst).FloatingIP)
-		optional.RestoreString(&(*previous).AvailabilityZone, &(*dst).AvailabilityZone)
-		optional.RestoreBool(&(*previous).Enabled, &(*dst).Enabled)
+	if previous == nil || dst == nil || *previous == nil || *dst == nil {
+		return
 	}
+
+	restorev1beta1MachineSpec((*previous).Spec, (*dst).Spec)
+	optional.RestoreString(&(*previous).FloatingIP, &(*dst).FloatingIP)
+	optional.RestoreString(&(*previous).AvailabilityZone, &(*dst).AvailabilityZone)
+	optional.RestoreBool(&(*previous).Enabled, &(*dst).Enabled)
 }
 
 func Convert_v1alpha7_Bastion_To_v1beta1_Bastion(in *Bastion, out *infrav1.Bastion, s apiconversion.Scope) error {

api/v1alpha7/types_conversion.go

@@ -193,7 +193,7 @@ func restorev1beta1SubnetParam(previous *infrav1.SubnetParam, dst *infrav1.Subne
 
 	optional.RestoreString(&previous.ID, &dst.ID)
 
-	if dst.Filter != nil {
+	if previous.Filter != nil && dst.Filter != nil {
 		dst.Filter.FilterByNeutronTags = previous.Filter.FilterByNeutronTags
 	}
 }
@@ -257,7 +257,7 @@ func restorev1beta1RouterParam(previous *infrav1.RouterParam, dst *infrav1.Route
 	}
 
 	optional.RestoreString(&previous.ID, &dst.ID)
-	if dst.Filter != nil {
+	if previous.Filter != nil && dst.Filter != nil {
 		dst.Filter.FilterByNeutronTags = previous.Filter.FilterByNeutronTags
 	}
 }

test/helpers/fuzz_restorer.go

@@ -0,0 +1,54 @@
+/*
+Copyright 2024 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+	http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package helpers
+
+import (
+	"runtime/debug"
+	"testing"
+
+	"github.com/onsi/gomega/format"
+	"k8s.io/client-go/kubernetes/scheme"
+	utilconversion "sigs.k8s.io/cluster-api/util/conversion"
+)
+
+// FuzzRestorer fuzzes the inputs to a restore function and ensures that the function does not panic.
+func FuzzRestorer[T any](t *testing.T, name string, f func(*T, *T)) {
+	t.Helper()
+	fuzz := utilconversion.GetFuzzer(scheme.Scheme)
+
+	t.Run(name, func(t *testing.T) {
+		for i := 0; i < 1000; i++ {
+			previous := new(T)
+			dst := new(T)
+			fuzz.Fuzz(previous)
+			fuzz.Fuzz(dst)
+
+			func() {
+				defer func() {
+					if r := recover(); r != nil {
+						t.Errorf("PANIC with arguments\nPrevious: %s\nDest: %s\nStack: %s",
+							format.Object(previous, 1),
+							format.Object(dst, 1),
+							debug.Stack())
+						t.FailNow()
+					}
+				}()
+				f(previous, dst)
+			}()
+		}
+	})
+}