Skip to content

Commit 5874b7e

Browse files
akutzakutz
authored
Merge pull request vmware-tanzu#1387 from lubronzhan/topic/lubron/fix_validate_volumes
🐛Fix a NPE when validating PVC
2 parents 27c854b + 726b11f commit 5874b7e

File tree

2 files changed

+171
-122
lines changed

2 files changed

+171
-122
lines changed

‎webhooks/virtualmachine/validation/virtualmachine_validator.go‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1188,14 +1188,16 @@ func (v validator) validateVolumes(
11881188
}
11891189

11901190
// Validate that no two volumes have the same PVC claim name.
1191-
if claimName := vol.PersistentVolumeClaim.ClaimName; claimName != "" {
1192-
if volumePVCNamesSet.Has(claimName) {
1193-
allErrs = append(allErrs, field.Duplicate(
1194-
volPath.Child("persistentVolumeClaim", "claimName"),
1195-
claimName,
1196-
))
1197-
} else {
1198-
volumePVCNamesSet.Insert(claimName)
1191+
if pvc := vol.PersistentVolumeClaim; pvc != nil {
1192+
if claimName := pvc.ClaimName; claimName != "" {
1193+
if volumePVCNamesSet.Has(claimName) {
1194+
allErrs = append(allErrs, field.Duplicate(
1195+
volPath.Child("persistentVolumeClaim", "claimName"),
1196+
claimName,
1197+
))
1198+
} else {
1199+
volumePVCNamesSet.Insert(claimName)
1200+
}
11991201
}
12001202
}
12011203

‎webhooks/virtualmachine/validation/virtualmachine_validator_unit_test.go‎

Lines changed: 161 additions & 114 deletions
Original file line numberDiff line numberDiff line change
@@ -293,6 +293,7 @@ func setControllerForPVCWithBusNumber(vm *vmopv1.VirtualMachine, defaultBusNum i
293293
}
294294
}
295295

296+
// Validate Create operations.
296297
func unitTestsValidateCreate() {
297298

298299
var (
@@ -3967,6 +3968,85 @@ func unitTestsValidateCreate() {
39673968
)
39683969
})
39693970

3971+
DescribeTable("Volume PVC Name conflicts with other volumes", doTest,
3972+
Entry("should deny volumes with duplicate ClaimName",
3973+
testParams{
3974+
setup: func(ctx *unitValidatingWebhookContext) {
3975+
// New VM adds volumes with duplicate ClaimName.
3976+
ctx.vm.Spec.Volumes = []vmopv1.VirtualMachineVolume{
3977+
{
3978+
Name: "existing-volume",
3979+
VirtualMachineVolumeSource: vmopv1.VirtualMachineVolumeSource{
3980+
PersistentVolumeClaim: &vmopv1.PersistentVolumeClaimVolumeSource{
3981+
PersistentVolumeClaimVolumeSource: corev1.PersistentVolumeClaimVolumeSource{
3982+
ClaimName: "existing-pvc-1",
3983+
},
3984+
},
3985+
},
3986+
},
3987+
{
3988+
Name: "new-volume",
3989+
VirtualMachineVolumeSource: vmopv1.VirtualMachineVolumeSource{
3990+
PersistentVolumeClaim: &vmopv1.PersistentVolumeClaimVolumeSource{
3991+
PersistentVolumeClaimVolumeSource: corev1.PersistentVolumeClaimVolumeSource{
3992+
ClaimName: "existing-pvc-1",
3993+
},
3994+
},
3995+
},
3996+
},
3997+
}
3998+
},
3999+
validate: doValidateWithMsg(
4000+
"spec.volumes[1].persistentVolumeClaim.claimName: Duplicate value: \"existing-pvc-1\"",
4001+
),
4002+
},
4003+
),
4004+
Entry("should allow volmes with different ClaimName",
4005+
testParams{
4006+
setup: func(ctx *unitValidatingWebhookContext) {
4007+
// New VM adds volumes with duplicate ClaimName.
4008+
ctx.vm.Spec.Volumes = []vmopv1.VirtualMachineVolume{
4009+
{
4010+
Name: "existing-volume",
4011+
VirtualMachineVolumeSource: vmopv1.VirtualMachineVolumeSource{
4012+
PersistentVolumeClaim: &vmopv1.PersistentVolumeClaimVolumeSource{
4013+
PersistentVolumeClaimVolumeSource: corev1.PersistentVolumeClaimVolumeSource{
4014+
ClaimName: "existing-pvc-1",
4015+
},
4016+
},
4017+
},
4018+
},
4019+
{
4020+
Name: "new-volume",
4021+
VirtualMachineVolumeSource: vmopv1.VirtualMachineVolumeSource{
4022+
PersistentVolumeClaim: &vmopv1.PersistentVolumeClaimVolumeSource{
4023+
PersistentVolumeClaimVolumeSource: corev1.PersistentVolumeClaimVolumeSource{
4024+
ClaimName: "existing-pvc-2", // different from existing-pvc-1.
4025+
},
4026+
},
4027+
},
4028+
},
4029+
}
4030+
},
4031+
expectAllowed: true,
4032+
},
4033+
),
4034+
Entry("should allow volmes without any PVC",
4035+
testParams{
4036+
setup: func(ctx *unitValidatingWebhookContext) {
4037+
ctx.oldVM = nil
4038+
ctx.vm.Spec.Volumes = []vmopv1.VirtualMachineVolume{
4039+
{
4040+
Name: "new-volume",
4041+
VirtualMachineVolumeSource: vmopv1.VirtualMachineVolumeSource{},
4042+
},
4043+
}
4044+
},
4045+
expectAllowed: true,
4046+
},
4047+
),
4048+
)
4049+
39704050
commonCreateAndUpdateValidations(doTest)
39714051
}
39724052

@@ -4095,6 +4175,7 @@ func setupNewVMForUpdate(ctx *unitValidatingWebhookContext, args updateArgs) {
40954175
setControllerForPVC(ctx.vm)
40964176
}
40974177

4178+
// Validate Update operations.
40984179
func unitTestsValidateUpdate() { //nolint:gocyclo
40994180
var (
41004181
ctx *unitValidatingWebhookContext
@@ -8138,143 +8219,109 @@ func unitTestsValidateUpdate() { //nolint:gocyclo
81388219
)
81398220
})
81408221

8141-
Context("Volume PVC ClaimName conflicts with other volumes", func() {
8142-
DescribeTable("Create", doTest,
8143-
Entry("should deny volumes with duplicate ClaimName",
8144-
testParams{
8145-
setup: func(ctx *unitValidatingWebhookContext) {
8146-
// New VM adds volumes with duplicate ClaimName
8147-
ctx.vm.Spec.Volumes = []vmopv1.VirtualMachineVolume{
8148-
{
8149-
Name: "existing-volume",
8150-
VirtualMachineVolumeSource: vmopv1.VirtualMachineVolumeSource{
8151-
PersistentVolumeClaim: &vmopv1.PersistentVolumeClaimVolumeSource{
8152-
PersistentVolumeClaimVolumeSource: corev1.PersistentVolumeClaimVolumeSource{
8153-
ClaimName: "existing-pvc-1",
8154-
},
8155-
},
8156-
},
8157-
},
8158-
{
8159-
Name: "new-volume",
8160-
VirtualMachineVolumeSource: vmopv1.VirtualMachineVolumeSource{
8161-
PersistentVolumeClaim: &vmopv1.PersistentVolumeClaimVolumeSource{
8162-
PersistentVolumeClaimVolumeSource: corev1.PersistentVolumeClaimVolumeSource{
8163-
ClaimName: "existing-pvc-1",
8164-
},
8165-
},
8166-
},
8167-
},
8168-
}
8169-
},
8170-
validate: doValidateWithMsg(
8171-
"spec.volumes[1].persistentVolumeClaim.claimName: Duplicate value: \"existing-pvc-1\"",
8172-
),
8173-
},
8174-
),
8175-
Entry("should allow volmes with different ClaimName",
8176-
testParams{
8177-
setup: func(ctx *unitValidatingWebhookContext) {
8178-
// New VM adds volumes with duplicate ClaimName
8179-
ctx.vm.Spec.Volumes = []vmopv1.VirtualMachineVolume{
8180-
{
8181-
Name: "existing-volume",
8182-
VirtualMachineVolumeSource: vmopv1.VirtualMachineVolumeSource{
8183-
PersistentVolumeClaim: &vmopv1.PersistentVolumeClaimVolumeSource{
8184-
PersistentVolumeClaimVolumeSource: corev1.PersistentVolumeClaimVolumeSource{
8185-
ClaimName: "existing-pvc-1",
8186-
},
8187-
},
8188-
},
8189-
},
8190-
{
8191-
Name: "new-volume",
8192-
VirtualMachineVolumeSource: vmopv1.VirtualMachineVolumeSource{
8193-
PersistentVolumeClaim: &vmopv1.PersistentVolumeClaimVolumeSource{
8194-
PersistentVolumeClaimVolumeSource: corev1.PersistentVolumeClaimVolumeSource{
8195-
ClaimName: "existing-pvc-2", // different from existing-pvc-1.
8196-
},
8222+
DescribeTable("Volume PVC Name conflicts with other volumes", doTest,
8223+
Entry("should deny new volume with same ClaimName",
8224+
testParams{
8225+
setup: func(ctx *unitValidatingWebhookContext) {
8226+
// Setup old VM with one volume.
8227+
ctx.oldVM.Spec.Volumes = []vmopv1.VirtualMachineVolume{
8228+
{
8229+
Name: "existing-volume",
8230+
VirtualMachineVolumeSource: vmopv1.VirtualMachineVolumeSource{
8231+
PersistentVolumeClaim: &vmopv1.PersistentVolumeClaimVolumeSource{
8232+
PersistentVolumeClaimVolumeSource: corev1.PersistentVolumeClaimVolumeSource{
8233+
ClaimName: "existing-pvc-1",
81978234
},
81988235
},
81998236
},
8200-
}
8201-
},
8202-
expectAllowed: true,
8203-
},
8204-
),
8205-
)
8237+
},
8238+
}
82068239

8207-
DescribeTable("Update", doTest,
8208-
Entry("should deny new volume with same ClaimName",
8209-
testParams{
8210-
setup: func(ctx *unitValidatingWebhookContext) {
8211-
// Setup old VM with one volume
8212-
ctx.oldVM.Spec.Volumes = []vmopv1.VirtualMachineVolume{
8213-
{
8214-
Name: "existing-volume",
8215-
VirtualMachineVolumeSource: vmopv1.VirtualMachineVolumeSource{
8216-
PersistentVolumeClaim: &vmopv1.PersistentVolumeClaimVolumeSource{
8217-
PersistentVolumeClaimVolumeSource: corev1.PersistentVolumeClaimVolumeSource{
8218-
ClaimName: "existing-pvc-1",
8219-
},
8220-
},
8240+
// New VM adds a second volume with different ClaimName.
8241+
ctx.vm.Spec.Volumes = slices.Clone(ctx.oldVM.Spec.Volumes)
8242+
ctx.vm.Spec.Volumes = append(ctx.vm.Spec.Volumes, vmopv1.VirtualMachineVolume{
8243+
Name: "new-volume",
8244+
VirtualMachineVolumeSource: vmopv1.VirtualMachineVolumeSource{
8245+
PersistentVolumeClaim: &vmopv1.PersistentVolumeClaimVolumeSource{
8246+
PersistentVolumeClaimVolumeSource: corev1.PersistentVolumeClaimVolumeSource{
8247+
ClaimName: "existing-pvc-1",
82218248
},
82228249
},
8223-
}
8224-
8225-
// New VM adds a second volume with different ClaimName
8226-
ctx.vm.Spec.Volumes = slices.Clone(ctx.oldVM.Spec.Volumes)
8227-
ctx.vm.Spec.Volumes = append(ctx.vm.Spec.Volumes, vmopv1.VirtualMachineVolume{
8228-
Name: "new-volume",
8250+
},
8251+
})
8252+
},
8253+
validate: doValidateWithMsg(
8254+
"spec.volumes[1].persistentVolumeClaim.claimName: Duplicate value: \"existing-pvc-1\"",
8255+
),
8256+
},
8257+
),
8258+
Entry("should allow new volume with different ClaimName",
8259+
testParams{
8260+
setup: func(ctx *unitValidatingWebhookContext) {
8261+
// Setup old VM with one volume.
8262+
ctx.oldVM.Spec.Volumes = []vmopv1.VirtualMachineVolume{
8263+
{
8264+
Name: "existing-volume",
82298265
VirtualMachineVolumeSource: vmopv1.VirtualMachineVolumeSource{
82308266
PersistentVolumeClaim: &vmopv1.PersistentVolumeClaimVolumeSource{
82318267
PersistentVolumeClaimVolumeSource: corev1.PersistentVolumeClaimVolumeSource{
82328268
ClaimName: "existing-pvc-1",
82338269
},
82348270
},
82358271
},
8236-
})
8237-
},
8238-
validate: doValidateWithMsg(
8239-
"spec.volumes[1].persistentVolumeClaim.claimName: Duplicate value: \"existing-pvc-1\"",
8240-
),
8241-
},
8242-
),
8243-
Entry("should allow new volume with different ClaimName",
8244-
testParams{
8245-
setup: func(ctx *unitValidatingWebhookContext) {
8246-
// Setup old VM with one volume
8247-
ctx.oldVM.Spec.Volumes = []vmopv1.VirtualMachineVolume{
8248-
{
8249-
Name: "existing-volume",
8250-
VirtualMachineVolumeSource: vmopv1.VirtualMachineVolumeSource{
8251-
PersistentVolumeClaim: &vmopv1.PersistentVolumeClaimVolumeSource{
8252-
PersistentVolumeClaimVolumeSource: corev1.PersistentVolumeClaimVolumeSource{
8253-
ClaimName: "existing-pvc-1",
8254-
},
8255-
},
8272+
},
8273+
}
8274+
8275+
// New VM adds a second volume with different ClaimName.
8276+
ctx.vm.Spec.Volumes = slices.Clone(ctx.oldVM.Spec.Volumes)
8277+
ctx.vm.Spec.Volumes = append(ctx.vm.Spec.Volumes, vmopv1.VirtualMachineVolume{
8278+
Name: "new-volume",
8279+
VirtualMachineVolumeSource: vmopv1.VirtualMachineVolumeSource{
8280+
PersistentVolumeClaim: &vmopv1.PersistentVolumeClaimVolumeSource{
8281+
PersistentVolumeClaimVolumeSource: corev1.PersistentVolumeClaimVolumeSource{
8282+
ClaimName: "new-pvc-2",
82568283
},
82578284
},
8258-
}
8285+
},
8286+
})
8287+
},
8288+
expectAllowed: true,
8289+
},
8290+
),
82598291

8260-
// New VM adds a second volume with different ClaimName
8261-
ctx.vm.Spec.Volumes = slices.Clone(ctx.oldVM.Spec.Volumes)
8262-
ctx.vm.Spec.Volumes = append(ctx.vm.Spec.Volumes, vmopv1.VirtualMachineVolume{
8263-
Name: "new-volume",
8292+
Entry("should allow new volume with empty PVC",
8293+
testParams{
8294+
setup: func(ctx *unitValidatingWebhookContext) {
8295+
// Setup old VM with one volume.
8296+
ctx.oldVM.Spec.Volumes = []vmopv1.VirtualMachineVolume{
8297+
{
8298+
Name: "existing-volume",
82648299
VirtualMachineVolumeSource: vmopv1.VirtualMachineVolumeSource{
82658300
PersistentVolumeClaim: &vmopv1.PersistentVolumeClaimVolumeSource{
82668301
PersistentVolumeClaimVolumeSource: corev1.PersistentVolumeClaimVolumeSource{
8267-
ClaimName: "new-pvc-2",
8302+
ClaimName: "existing-pvc-1",
82688303
},
82698304
},
82708305
},
8271-
})
8272-
},
8273-
expectAllowed: true,
8306+
},
8307+
}
8308+
8309+
// New VM adds a second volume without claim name
8310+
ctx.vm.Spec.Volumes = slices.Clone(ctx.oldVM.Spec.Volumes)
8311+
ctx.vm.Spec.Volumes = append(ctx.vm.Spec.Volumes,
8312+
vmopv1.VirtualMachineVolume{
8313+
Name: "vol-2",
8314+
// Controller info is required.
8315+
UnitNumber: ptr.To(int32(10)),
8316+
ControllerType: vmopv1.VirtualControllerTypeSCSI,
8317+
ControllerBusNumber: ptr.To(int32(0)),
8318+
},
8319+
)
82748320
},
8275-
),
8276-
)
8277-
})
8321+
expectAllowed: true,
8322+
},
8323+
),
8324+
)
82788325

82798326
unitTestsValidateVolumeUnitNumber(doTest)
82808327

0 commit comments

Comments
 (0)