Skip to content

Commit f5b0181

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #1989 from shiftstack/morefuzz
🐛 Fix multiple panics in restore functions
2 parents 4c162b6 + de0dffa commit f5b0181

File tree

7 files changed

+148
-28
lines changed

7 files changed

+148
-28
lines changed

‎api/v1alpha6/conversion_test.go

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -763,3 +763,34 @@ func TestConvert_v1alpha6_OpenStackMachineSpec_To_v1beta1_OpenStackMachineSpec(t
763763
})
764764
}
765765
}
766+
767+
func Test_FuzzRestorers(t *testing.T) {
768+
/* Cluster */
769+
testhelpers.FuzzRestorer(t, "restorev1alpha6ClusterSpec", restorev1alpha6ClusterSpec)
770+
testhelpers.FuzzRestorer(t, "restorev1beta1ClusterSpec", restorev1beta1ClusterSpec)
771+
testhelpers.FuzzRestorer(t, "restorev1alpha6ClusterStatus", restorev1alpha6ClusterStatus)
772+
testhelpers.FuzzRestorer(t, "restorev1beta1ClusterStatus", restorev1beta1ClusterStatus)
773+
testhelpers.FuzzRestorer(t, "restorev1beta1Bastion", restorev1beta1Bastion)
774+
testhelpers.FuzzRestorer(t, "restorev1beta1BastionStatus", restorev1beta1BastionStatus)
775+
776+
/* ClusterTemplate */
777+
testhelpers.FuzzRestorer(t, "restorev1beta1ClusterTemplateSpec", restorev1beta1ClusterTemplateSpec)
778+
779+
/* Machine */
780+
testhelpers.FuzzRestorer(t, "restorev1alpha6MachineSpec", restorev1alpha6MachineSpec)
781+
testhelpers.FuzzRestorer(t, "restorev1beta1MachineSpec", restorev1beta1MachineSpec)
782+
783+
/* MachineTemplate */
784+
testhelpers.FuzzRestorer(t, "restorev1alpha6MachineTemplateMachineSpec", restorev1alpha6MachineTemplateMachineSpec)
785+
786+
/* Types */
787+
testhelpers.FuzzRestorer(t, "restorev1alpha6SecurityGroupFilter", restorev1alpha6SecurityGroupFilter)
788+
testhelpers.FuzzRestorer(t, "restorev1beta1SecurityGroupParam", restorev1beta1SecurityGroupParam)
789+
testhelpers.FuzzRestorer(t, "restorev1alpha6NetworkFilter", restorev1alpha6NetworkFilter)
790+
testhelpers.FuzzRestorer(t, "restorev1beta1NetworkParam", restorev1beta1NetworkParam)
791+
testhelpers.FuzzRestorer(t, "restorev1alpha6SubnetFilter", restorev1alpha6SubnetFilter)
792+
testhelpers.FuzzRestorer(t, "restorev1alpha6SubnetParam", restorev1alpha6SubnetParam)
793+
testhelpers.FuzzRestorer(t, "restorev1beta1SubnetParam", restorev1beta1SubnetParam)
794+
testhelpers.FuzzRestorer(t, "restorev1alpha6Port", restorev1alpha6Port)
795+
testhelpers.FuzzRestorer(t, "restorev1alpha6SecurityGroup", restorev1alpha6SecurityGroup)
796+
}

‎api/v1alpha6/openstackcluster_conversion.go

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -120,17 +120,19 @@ func restorev1alpha6ClusterSpec(previous *OpenStackClusterSpec, dst *OpenStackCl
120120
return
121121
}
122122

123-
for i := range previous.ExternalRouterIPs {
124-
dstIP := &dst.ExternalRouterIPs[i]
125-
previousIP := &previous.ExternalRouterIPs[i]
126-
127-
// Subnet.Filter.ID was overwritten in up-conversion by Subnet.UUID
128-
dstIP.Subnet.Filter.ID = previousIP.Subnet.Filter.ID
129-
130-
// If Subnet.UUID was previously unset, we overwrote it with the value of Subnet.Filter.ID
131-
// Don't unset it again if it doesn't have the previous value of Subnet.Filter.ID, because that means it was genuinely changed
132-
if previousIP.Subnet.UUID == "" && dstIP.Subnet.UUID == previousIP.Subnet.Filter.ID {
133-
dstIP.Subnet.UUID = ""
123+
if len(previous.ExternalRouterIPs) == len(dst.ExternalRouterIPs) {
124+
for i := range previous.ExternalRouterIPs {
125+
dstIP := &dst.ExternalRouterIPs[i]
126+
previousIP := &previous.ExternalRouterIPs[i]
127+
128+
// Subnet.Filter.ID was overwritten in up-conversion by Subnet.UUID
129+
dstIP.Subnet.Filter.ID = previousIP.Subnet.Filter.ID
130+
131+
// If Subnet.UUID was previously unset, we overwrote it with the value of Subnet.Filter.ID
132+
// Don't unset it again if it doesn't have the previous value of Subnet.Filter.ID, because that means it was genuinely changed
133+
if previousIP.Subnet.UUID == "" && dstIP.Subnet.UUID == previousIP.Subnet.Filter.ID {
134+
dstIP.Subnet.UUID = ""
135+
}
134136
}
135137
}
136138

@@ -199,7 +201,7 @@ func restorev1beta1ClusterSpec(previous *infrav1.OpenStackClusterSpec, dst *infr
199201

200202
dst.ManagedSubnets = previous.ManagedSubnets
201203

202-
if previous.ManagedSecurityGroups != nil {
204+
if previous.ManagedSecurityGroups != nil && dst.ManagedSecurityGroups != nil {
203205
dst.ManagedSecurityGroups.AllNodesSecurityGroupRules = previous.ManagedSecurityGroups.AllNodesSecurityGroupRules
204206
}
205207

@@ -345,13 +347,13 @@ func Convert_v1beta1_OpenStackClusterSpec_To_v1alpha6_OpenStackClusterSpec(in *i
345347
func restorev1alpha6ClusterStatus(previous *OpenStackClusterStatus, dst *OpenStackClusterStatus) {
346348
// PortOpts.SecurityGroups have been removed in v1beta1
347349
// We restore the whole PortOpts/Networks since they are anyway immutable.
348-
if previous.ExternalNetwork != nil {
350+
if previous.ExternalNetwork != nil && dst.ExternalNetwork != nil {
349351
dst.ExternalNetwork.PortOpts = previous.ExternalNetwork.PortOpts
350352
}
351353
if previous.Network != nil {
352354
dst.Network = previous.Network
353355
}
354-
if previous.Bastion != nil && previous.Bastion.Networks != nil {
356+
if previous.Bastion != nil && previous.Bastion.Networks != nil && dst.Bastion != nil {
355357
dst.Bastion.Networks = previous.Bastion.Networks
356358
}
357359

‎api/v1alpha6/types_conversion.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ func restorev1beta1SubnetParam(previous *infrav1.SubnetParam, dst *infrav1.Subne
224224

225225
optional.RestoreString(&previous.ID, &dst.ID)
226226

227-
if dst.Filter != nil {
227+
if previous.Filter != nil && dst.Filter != nil {
228228
dst.Filter.FilterByNeutronTags = previous.Filter.FilterByNeutronTags
229229
}
230230
}

‎api/v1alpha7/conversion_test.go

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -371,3 +371,38 @@ func TestConvert_v1alpha7_OpenStackMachineSpec_To_v1beta1_OpenStackMachineSpec(t
371371
})
372372
}
373373
}
374+
375+
func Test_FuzzRestorers(t *testing.T) {
376+
/* Cluster */
377+
testhelpers.FuzzRestorer(t, "restorev1alpha7ClusterSpec", restorev1alpha7ClusterSpec)
378+
testhelpers.FuzzRestorer(t, "restorev1beta1ClusterSpec", restorev1beta1ClusterSpec)
379+
testhelpers.FuzzRestorer(t, "restorev1alpha7ClusterStatus", restorev1alpha7ClusterStatus)
380+
testhelpers.FuzzRestorer(t, "restorev1beta1ClusterStatus", restorev1beta1ClusterStatus)
381+
testhelpers.FuzzRestorer(t, "restorev1alpha7Bastion", restorev1alpha7Bastion)
382+
testhelpers.FuzzRestorer(t, "restorev1beta1Bastion", restorev1beta1Bastion)
383+
testhelpers.FuzzRestorer(t, "restorev1beta1BastionStatus", restorev1beta1BastionStatus)
384+
385+
/* ClusterTemplate */
386+
testhelpers.FuzzRestorer(t, "restorev1alpha7ClusterTemplateSpec", restorev1alpha7ClusterTemplateSpec)
387+
testhelpers.FuzzRestorer(t, "restorev1alpha7ClusterTemplateSpec", restorev1alpha7ClusterTemplateSpec)
388+
389+
/* Machine */
390+
testhelpers.FuzzRestorer(t, "restorev1alpha7MachineSpec", restorev1alpha7MachineSpec)
391+
testhelpers.FuzzRestorer(t, "restorev1beta1MachineSpec", restorev1beta1MachineSpec)
392+
393+
/* MachineTemplate */
394+
testhelpers.FuzzRestorer(t, "restorev1alpha7MachineTemplateSpec", restorev1alpha7MachineTemplateSpec)
395+
396+
/* Types */
397+
testhelpers.FuzzRestorer(t, "restorev1alpha7SecurityGroupFilter", restorev1alpha7SecurityGroupFilter)
398+
testhelpers.FuzzRestorer(t, "restorev1alpha7SecurityGroup", restorev1alpha7SecurityGroup)
399+
testhelpers.FuzzRestorer(t, "restorev1beta1SecurityGroupParam", restorev1beta1SecurityGroupParam)
400+
testhelpers.FuzzRestorer(t, "restorev1alpha7NetworkFilter", restorev1alpha7NetworkFilter)
401+
testhelpers.FuzzRestorer(t, "restorev1beta1NetworkParam", restorev1beta1NetworkParam)
402+
testhelpers.FuzzRestorer(t, "restorev1alpha7SubnetFilter", restorev1alpha7SubnetFilter)
403+
testhelpers.FuzzRestorer(t, "restorev1beta1SubnetParam", restorev1beta1SubnetParam)
404+
testhelpers.FuzzRestorer(t, "restorev1alpha7RouterFilter", restorev1alpha7RouterFilter)
405+
testhelpers.FuzzRestorer(t, "restorev1beta1RouterParam", restorev1beta1RouterParam)
406+
testhelpers.FuzzRestorer(t, "restorev1alpha7Port", restorev1alpha7Port)
407+
testhelpers.FuzzRestorer(t, "restorev1beta1Port", restorev1beta1Port)
408+
}

‎api/v1alpha7/openstackcluster_conversion.go

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,7 @@ func restorev1beta1ClusterSpec(previous *infrav1.OpenStackClusterSpec, dst *infr
207207

208208
dst.ManagedSubnets = previous.ManagedSubnets
209209

210-
if previous.ManagedSecurityGroups != nil {
210+
if previous.ManagedSecurityGroups != nil && dst.ManagedSecurityGroups != nil {
211211
dst.ManagedSecurityGroups.AllNodesSecurityGroupRules = previous.ManagedSecurityGroups.AllNodesSecurityGroupRules
212212
}
213213

@@ -371,26 +371,24 @@ func Convert_v1beta1_OpenStackClusterStatus_To_v1alpha7_OpenStackClusterStatus(i
371371
/* Bastion */
372372

373373
func restorev1alpha7Bastion(previous **Bastion, dst **Bastion) {
374-
if *previous == nil || *dst == nil {
374+
if previous == nil || dst == nil || *previous == nil || *dst == nil {
375375
return
376376
}
377-
378377
prevMachineSpec := &(*previous).Instance
379378
dstMachineSpec := &(*dst).Instance
380379
restorev1alpha7MachineSpec(prevMachineSpec, dstMachineSpec)
381380
dstMachineSpec.InstanceID = prevMachineSpec.InstanceID
382381
}
383382

384383
func restorev1beta1Bastion(previous **infrav1.Bastion, dst **infrav1.Bastion) {
385-
if *previous != nil {
386-
if *dst != nil && (*previous).Spec != nil && (*dst).Spec != nil {
387-
restorev1beta1MachineSpec((*previous).Spec, (*dst).Spec)
388-
}
389-
390-
optional.RestoreString(&(*previous).FloatingIP, &(*dst).FloatingIP)
391-
optional.RestoreString(&(*previous).AvailabilityZone, &(*dst).AvailabilityZone)
392-
optional.RestoreBool(&(*previous).Enabled, &(*dst).Enabled)
384+
if previous == nil || dst == nil || *previous == nil || *dst == nil {
385+
return
393386
}
387+
388+
restorev1beta1MachineSpec((*previous).Spec, (*dst).Spec)
389+
optional.RestoreString(&(*previous).FloatingIP, &(*dst).FloatingIP)
390+
optional.RestoreString(&(*previous).AvailabilityZone, &(*dst).AvailabilityZone)
391+
optional.RestoreBool(&(*previous).Enabled, &(*dst).Enabled)
394392
}
395393

396394
func Convert_v1alpha7_Bastion_To_v1beta1_Bastion(in *Bastion, out *infrav1.Bastion, s apiconversion.Scope) error {

‎api/v1alpha7/types_conversion.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ func restorev1beta1SubnetParam(previous *infrav1.SubnetParam, dst *infrav1.Subne
193193

194194
optional.RestoreString(&previous.ID, &dst.ID)
195195

196-
if dst.Filter != nil {
196+
if previous.Filter != nil && dst.Filter != nil {
197197
dst.Filter.FilterByNeutronTags = previous.Filter.FilterByNeutronTags
198198
}
199199
}
@@ -257,7 +257,7 @@ func restorev1beta1RouterParam(previous *infrav1.RouterParam, dst *infrav1.Route
257257
}
258258

259259
optional.RestoreString(&previous.ID, &dst.ID)
260-
if dst.Filter != nil {
260+
if previous.Filter != nil && dst.Filter != nil {
261261
dst.Filter.FilterByNeutronTags = previous.Filter.FilterByNeutronTags
262262
}
263263
}

‎test/helpers/fuzz_restorer.go

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
/*
2+
Copyright 2024 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package helpers
18+
19+
import (
20+
"runtime/debug"
21+
"testing"
22+
23+
"github.com/onsi/gomega/format"
24+
"k8s.io/client-go/kubernetes/scheme"
25+
utilconversion "sigs.k8s.io/cluster-api/util/conversion"
26+
)
27+
28+
// FuzzRestorer fuzzes the inputs to a restore function and ensures that the function does not panic.
29+
func FuzzRestorer[T any](t *testing.T, name string, f func(*T, *T)) {
30+
t.Helper()
31+
fuzz := utilconversion.GetFuzzer(scheme.Scheme)
32+
33+
t.Run(name, func(t *testing.T) {
34+
for i := 0; i < 1000; i++ {
35+
previous := new(T)
36+
dst := new(T)
37+
fuzz.Fuzz(previous)
38+
fuzz.Fuzz(dst)
39+
40+
func() {
41+
defer func() {
42+
if r := recover(); r != nil {
43+
t.Errorf("PANIC with arguments\nPrevious: %s\nDest: %s\nStack: %s",
44+
format.Object(previous, 1),
45+
format.Object(dst, 1),
46+
debug.Stack())
47+
t.FailNow()
48+
}
49+
}()
50+
f(previous, dst)
51+
}()
52+
}
53+
})
54+
}

0 commit comments

Comments
 (0)