From de0dffa5ff0fa07a82b81769e342f8d8a953ce91 Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Mon, 1 Apr 2024 12:29:36 +0100 Subject: [PATCH] Fuzz test conversion restore functions --- api/v1alpha6/conversion_test.go | 31 ++++++++++++ api/v1alpha6/openstackcluster_conversion.go | 30 ++++++------ api/v1alpha6/types_conversion.go | 2 +- api/v1alpha7/conversion_test.go | 35 +++++++++++++ api/v1alpha7/openstackcluster_conversion.go | 20 ++++---- api/v1alpha7/types_conversion.go | 4 +- test/helpers/fuzz_restorer.go | 54 +++++++++++++++++++++ 7 files changed, 148 insertions(+), 28 deletions(-) create mode 100644 test/helpers/fuzz_restorer.go diff --git a/api/v1alpha6/conversion_test.go b/api/v1alpha6/conversion_test.go index 91fdeb7c3e..67a394d801 100644 --- a/api/v1alpha6/conversion_test.go +++ b/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) +} diff --git a/api/v1alpha6/openstackcluster_conversion.go b/api/v1alpha6/openstackcluster_conversion.go index 55fdcddf76..1da12e3587 100644 --- a/api/v1alpha6/openstackcluster_conversion.go +++ b/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 } diff --git a/api/v1alpha6/types_conversion.go b/api/v1alpha6/types_conversion.go index 7b99803d3f..b855b9e473 100644 --- a/api/v1alpha6/types_conversion.go +++ b/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 } } diff --git a/api/v1alpha7/conversion_test.go b/api/v1alpha7/conversion_test.go index f7e33a8a70..d44eabf604 100644 --- a/api/v1alpha7/conversion_test.go +++ b/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) +} diff --git a/api/v1alpha7/openstackcluster_conversion.go b/api/v1alpha7/openstackcluster_conversion.go index 69b2f58164..7ad0ed7cee 100644 --- a/api/v1alpha7/openstackcluster_conversion.go +++ b/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,10 +371,9 @@ 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) @@ -382,15 +381,14 @@ func restorev1alpha7Bastion(previous **Bastion, dst **Bastion) { } 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 { diff --git a/api/v1alpha7/types_conversion.go b/api/v1alpha7/types_conversion.go index d7c33ec3ef..aeb1c756a6 100644 --- a/api/v1alpha7/types_conversion.go +++ b/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 } } diff --git a/test/helpers/fuzz_restorer.go b/test/helpers/fuzz_restorer.go new file mode 100644 index 0000000000..bfe3775006 --- /dev/null +++ b/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) + }() + } + }) +}