Skip to content

Commit 3a9ff6c

Browse files
pohlypohly
committed
sanity: fix nil pointer error for IDGen when using Ginkgo suite
When adding IDGen, only one of the two code paths that trigger testing was updated such that it sets a default ID generator when the config doesn't already have one. GinkgoTest was not updated. As a result, existing E2E suites which use GinkgoTest and don't set the new field crash at runtime with a nil pointer error when updated to csi-test 2.2.0. Updating the instance provided by the caller is also a bit questionable because it is an undocumented side effect. It's cleaner to treat the struct as read-only and implement the fallback in an accessor function.
1 parent 82b0519 commit 3a9ff6c

File tree

3 files changed

+26
-21
lines changed

3 files changed

+26
-21
lines changed

‎pkg/sanity/controller.go

+10-10
Original file line numberDiff line numberDiff line change
@@ -825,7 +825,7 @@ var _ = DescribeSanity("Controller Service [Controller Server]", func(sc *Sanity
825825
volReq.VolumeContentSource = &csi.VolumeContentSource{
826826
Type: &csi.VolumeContentSource_Volume{
827827
Volume: &csi.VolumeContentSource_VolumeSource{
828-
VolumeId: sc.Config.IDGen.GenerateUniqueValidVolumeID(),
828+
VolumeId: sc.Config.GetIDGen().GenerateUniqueValidVolumeID(),
829829
},
830830
},
831831
}
@@ -864,7 +864,7 @@ var _ = DescribeSanity("Controller Service [Controller Server]", func(sc *Sanity
864864
_, err := c.DeleteVolume(
865865
context.Background(),
866866
&csi.DeleteVolumeRequest{
867-
VolumeId: sc.Config.IDGen.GenerateInvalidVolumeID(),
867+
VolumeId: sc.Config.GetIDGen().GenerateInvalidVolumeID(),
868868
Secrets: sc.Secrets.DeleteVolumeSecret,
869869
},
870870
)
@@ -1061,7 +1061,7 @@ var _ = DescribeSanity("Controller Service [Controller Server]", func(sc *Sanity
10611061
_, err := c.ValidateVolumeCapabilities(
10621062
context.Background(),
10631063
&csi.ValidateVolumeCapabilitiesRequest{
1064-
VolumeId: sc.Config.IDGen.GenerateUniqueValidVolumeID(),
1064+
VolumeId: sc.Config.GetIDGen().GenerateUniqueValidVolumeID(),
10651065
VolumeCapabilities: []*csi.VolumeCapability{
10661066
{
10671067
AccessType: &csi.VolumeCapability_Mount{
@@ -1110,7 +1110,7 @@ var _ = DescribeSanity("Controller Service [Controller Server]", func(sc *Sanity
11101110
_, err := c.ControllerPublishVolume(
11111111
context.Background(),
11121112
&csi.ControllerPublishVolumeRequest{
1113-
VolumeId: sc.Config.IDGen.GenerateUniqueValidVolumeID(),
1113+
VolumeId: sc.Config.GetIDGen().GenerateUniqueValidVolumeID(),
11141114
Secrets: sc.Secrets.ControllerPublishVolumeSecret,
11151115
},
11161116
)
@@ -1126,8 +1126,8 @@ var _ = DescribeSanity("Controller Service [Controller Server]", func(sc *Sanity
11261126
_, err := c.ControllerPublishVolume(
11271127
context.Background(),
11281128
&csi.ControllerPublishVolumeRequest{
1129-
VolumeId: sc.Config.IDGen.GenerateUniqueValidVolumeID(),
1130-
NodeId: sc.Config.IDGen.GenerateUniqueValidNodeID(),
1129+
VolumeId: sc.Config.GetIDGen().GenerateUniqueValidVolumeID(),
1130+
NodeId: sc.Config.GetIDGen().GenerateUniqueValidNodeID(),
11311131
Secrets: sc.Secrets.ControllerPublishVolumeSecret,
11321132
},
11331133
)
@@ -1287,8 +1287,8 @@ var _ = DescribeSanity("Controller Service [Controller Server]", func(sc *Sanity
12871287
conpubvol, err := c.ControllerPublishVolume(
12881288
context.Background(),
12891289
&csi.ControllerPublishVolumeRequest{
1290-
VolumeId: sc.Config.IDGen.GenerateUniqueValidVolumeID(),
1291-
NodeId: sc.Config.IDGen.GenerateUniqueValidNodeID(),
1290+
VolumeId: sc.Config.GetIDGen().GenerateUniqueValidVolumeID(),
1291+
NodeId: sc.Config.GetIDGen().GenerateUniqueValidNodeID(),
12921292
VolumeCapability: &csi.VolumeCapability{
12931293
AccessType: &csi.VolumeCapability_Mount{
12941294
Mount: &csi.VolumeCapability_MountVolume{},
@@ -1346,7 +1346,7 @@ var _ = DescribeSanity("Controller Service [Controller Server]", func(sc *Sanity
13461346
context.Background(),
13471347
&csi.ControllerPublishVolumeRequest{
13481348
VolumeId: vol.GetVolume().GetVolumeId(),
1349-
NodeId: sc.Config.IDGen.GenerateUniqueValidNodeID(),
1349+
NodeId: sc.Config.GetIDGen().GenerateUniqueValidNodeID(),
13501350
VolumeCapability: &csi.VolumeCapability{
13511351
AccessType: &csi.VolumeCapability_Mount{
13521352
Mount: &csi.VolumeCapability_MountVolume{},
@@ -1711,7 +1711,7 @@ var _ = DescribeSanity("ListSnapshots [Controller Server]", func(sc *SanityConte
17111711

17121712
snapshots, err := c.ListSnapshots(
17131713
context.Background(),
1714-
&csi.ListSnapshotsRequest{SourceVolumeId: sc.Config.IDGen.GenerateUniqueValidVolumeID()})
1714+
&csi.ListSnapshotsRequest{SourceVolumeId: sc.Config.GetIDGen().GenerateUniqueValidVolumeID()})
17151715
Expect(err).NotTo(HaveOccurred())
17161716
Expect(snapshots).NotTo(BeNil())
17171717
Expect(snapshots.GetEntries()).To(BeEmpty())

‎pkg/sanity/node.go

+7-7
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,7 @@ var _ = DescribeSanity("Node Service", func(sc *SanityContext) {
187187
_, err := c.NodePublishVolume(
188188
context.Background(),
189189
&csi.NodePublishVolumeRequest{
190-
VolumeId: sc.Config.IDGen.GenerateUniqueValidVolumeID(),
190+
VolumeId: sc.Config.GetIDGen().GenerateUniqueValidVolumeID(),
191191
Secrets: sc.Secrets.NodePublishVolumeSecret,
192192
},
193193
)
@@ -202,7 +202,7 @@ var _ = DescribeSanity("Node Service", func(sc *SanityContext) {
202202
_, err := c.NodePublishVolume(
203203
context.Background(),
204204
&csi.NodePublishVolumeRequest{
205-
VolumeId: sc.Config.IDGen.GenerateUniqueValidVolumeID(),
205+
VolumeId: sc.Config.GetIDGen().GenerateUniqueValidVolumeID(),
206206
TargetPath: sc.TargetPath + "/target",
207207
Secrets: sc.Secrets.NodePublishVolumeSecret,
208208
},
@@ -233,7 +233,7 @@ var _ = DescribeSanity("Node Service", func(sc *SanityContext) {
233233
_, err := c.NodeUnpublishVolume(
234234
context.Background(),
235235
&csi.NodeUnpublishVolumeRequest{
236-
VolumeId: sc.Config.IDGen.GenerateUniqueValidVolumeID(),
236+
VolumeId: sc.Config.GetIDGen().GenerateUniqueValidVolumeID(),
237237
})
238238
Expect(err).To(HaveOccurred())
239239

@@ -286,7 +286,7 @@ var _ = DescribeSanity("Node Service", func(sc *SanityContext) {
286286
_, err := c.NodeStageVolume(
287287
context.Background(),
288288
&csi.NodeStageVolumeRequest{
289-
VolumeId: sc.Config.IDGen.GenerateUniqueValidVolumeID(),
289+
VolumeId: sc.Config.GetIDGen().GenerateUniqueValidVolumeID(),
290290
VolumeCapability: &csi.VolumeCapability{
291291
AccessType: &csi.VolumeCapability_Mount{
292292
Mount: &csi.VolumeCapability_MountVolume{},
@@ -395,7 +395,7 @@ var _ = DescribeSanity("Node Service", func(sc *SanityContext) {
395395
_, err := c.NodeUnstageVolume(
396396
context.Background(),
397397
&csi.NodeUnstageVolumeRequest{
398-
VolumeId: sc.Config.IDGen.GenerateUniqueValidVolumeID(),
398+
VolumeId: sc.Config.GetIDGen().GenerateUniqueValidVolumeID(),
399399
})
400400
Expect(err).To(HaveOccurred())
401401

@@ -430,7 +430,7 @@ var _ = DescribeSanity("Node Service", func(sc *SanityContext) {
430430
_, err := c.NodeGetVolumeStats(
431431
context.Background(),
432432
&csi.NodeGetVolumeStatsRequest{
433-
VolumeId: sc.Config.IDGen.GenerateUniqueValidVolumeID(),
433+
VolumeId: sc.Config.GetIDGen().GenerateUniqueValidVolumeID(),
434434
},
435435
)
436436
Expect(err).To(HaveOccurred())
@@ -444,7 +444,7 @@ var _ = DescribeSanity("Node Service", func(sc *SanityContext) {
444444
_, err := c.NodeGetVolumeStats(
445445
context.Background(),
446446
&csi.NodeGetVolumeStatsRequest{
447-
VolumeId: sc.Config.IDGen.GenerateUniqueValidVolumeID(),
447+
VolumeId: sc.Config.GetIDGen().GenerateUniqueValidVolumeID(),
448448
VolumePath: "some/path",
449449
},
450450
)

‎pkg/sanity/sanity.go

+9-4
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,15 @@ type Config struct {
127127
IDGen IDGenerator
128128
}
129129

130+
// GetIDGen always returns a valid ID generator. If none is set in the configuration,
131+
// then the DefaultIDGenerator is returned.
132+
func (c Config) GetIDGen() IDGenerator {
133+
if c.IDGen != nil {
134+
return c.IDGen
135+
}
136+
return &DefaultIDGenerator{}
137+
}
138+
130139
// SanityContext holds the variables that each test can depend on. It
131140
// gets initialized before each test block runs.
132141
type SanityContext struct {
@@ -158,10 +167,6 @@ func Test(t *testing.T, reqConfig *Config) {
158167
}
159168
}
160169

161-
if reqConfig.IDGen == nil {
162-
reqConfig.IDGen = &DefaultIDGenerator{}
163-
}
164-
165170
sc := &SanityContext{
166171
Config: reqConfig,
167172
}

0 commit comments

Comments
 (0)