From a501746611d55dc2966c4513cf2941825aebf45c Mon Sep 17 00:00:00 2001
From: Bryan Venteicher <bryan.venteicher@broadcom.com>
Date: Fri, 13 Dec 2024 14:10:06 -0600
Subject: [PATCH] Fix potential VM validation webhook for Spec.Network

If the VM Spec.Network was changed to/from nil, the validation webhook
would NPE. I think the stems from late in v1a2 we decided to make most
of the top level Spec/Status fields pointers and this was just missed.

Add a few basic update tests for VM Spec.Network.
---
 .../validation/virtualmachine_validator.go    |  9 +-
 .../virtualmachine_validator_unit_test.go     | 96 ++++++++++++++++++-
 2 files changed, 101 insertions(+), 4 deletions(-)

diff --git a/webhooks/virtualmachine/validation/virtualmachine_validator.go b/webhooks/virtualmachine/validation/virtualmachine_validator.go
index 7a6543d56..71ca1cefd 100644
--- a/webhooks/virtualmachine/validation/virtualmachine_validator.go
+++ b/webhooks/virtualmachine/validation/virtualmachine_validator.go
@@ -1230,8 +1230,13 @@ func (v validator) validateImmutableNetwork(ctx *pkgctx.WebhookRequestContext, v
 
 	p := field.NewPath("spec", "network")
 
-	oldInterfaces := oldVM.Spec.Network.Interfaces
-	newInterfaces := vm.Spec.Network.Interfaces
+	var oldInterfaces, newInterfaces []vmopv1.VirtualMachineNetworkInterfaceSpec
+	if oldNetwork != nil {
+		oldInterfaces = oldNetwork.Interfaces
+	}
+	if newNetwork != nil {
+		newInterfaces = newNetwork.Interfaces
+	}
 
 	if len(oldInterfaces) != len(newInterfaces) {
 		return append(allErrs, field.Forbidden(p.Child("interfaces"), "network interfaces cannot be added or removed"))
diff --git a/webhooks/virtualmachine/validation/virtualmachine_validator_unit_test.go b/webhooks/virtualmachine/validation/virtualmachine_validator_unit_test.go
index 4c8f9996f..8251a41ce 100644
--- a/webhooks/virtualmachine/validation/virtualmachine_validator_unit_test.go
+++ b/webhooks/virtualmachine/validation/virtualmachine_validator_unit_test.go
@@ -3148,7 +3148,7 @@ func unitTestsValidateUpdate() {
 				},
 			),
 
-			Entry("alow changing image for privileged users when FSS_WCP_VMSERVICE_INCREMENTAL_RESTORE is enabled, and Registered annotation is present",
+			Entry("allow changing image for privileged users when FSS_WCP_VMSERVICE_INCREMENTAL_RESTORE is enabled, and Registered annotation is present",
 				testParams{
 					setup: func(ctx *unitValidatingWebhookContext) {
 						pkgcfg.SetContext(ctx, func(config *pkgcfg.Config) {
@@ -3687,7 +3687,7 @@ func unitTestsValidateUpdate() {
 				},
 			),
 
-			Entry("alow removing CD-ROM when VM is powered off",
+			Entry("allow removing CD-ROM when VM is powered off",
 				testParams{
 					setup: func(ctx *unitValidatingWebhookContext) {
 						ctx.vm.Spec.Cdrom = []vmopv1.VirtualMachineCdromSpec{}
@@ -4014,6 +4014,98 @@ func unitTestsValidateUpdate() {
 			),
 		)
 	})
+
+	Context("Network", func() {
+
+		DescribeTable("network update", doTest,
+			Entry("allow Network go from nil to not-nil",
+				testParams{
+					setup: func(ctx *unitValidatingWebhookContext) {
+						ctx.oldVM.Spec.Network = nil
+						ctx.vm.Spec.Network = &vmopv1.VirtualMachineNetworkSpec{}
+					},
+					expectAllowed: true,
+				},
+			),
+			Entry("allow Network go from not-nil to nil",
+				testParams{
+					setup: func(ctx *unitValidatingWebhookContext) {
+						ctx.oldVM.Spec.Network = &vmopv1.VirtualMachineNetworkSpec{}
+						ctx.vm.Spec.Network = nil
+					},
+					expectAllowed: true,
+				},
+			),
+			Entry("disallow adding Network Interface",
+				testParams{
+					setup: func(ctx *unitValidatingWebhookContext) {
+						ctx.oldVM.Spec.Network = &vmopv1.VirtualMachineNetworkSpec{
+							Interfaces: []vmopv1.VirtualMachineNetworkInterfaceSpec{
+								{
+									Name: "eth0",
+								},
+							},
+						}
+						ctx.vm.Spec.Network = &vmopv1.VirtualMachineNetworkSpec{
+							Interfaces: []vmopv1.VirtualMachineNetworkInterfaceSpec{
+								{
+									Name: "eth0",
+								},
+								{
+									Name: "eth1",
+								},
+							},
+						}
+					},
+					validate: doValidateWithMsg(`spec.network.interfaces: Forbidden: network interfaces cannot be added or removed`),
+				},
+			),
+			Entry("disallow Network Interface Name change",
+				testParams{
+					setup: func(ctx *unitValidatingWebhookContext) {
+						ctx.oldVM.Spec.Network = &vmopv1.VirtualMachineNetworkSpec{
+							Interfaces: []vmopv1.VirtualMachineNetworkInterfaceSpec{
+								{
+									Name: "eth0",
+								},
+							},
+						}
+						ctx.vm.Spec.Network = &vmopv1.VirtualMachineNetworkSpec{
+							Interfaces: []vmopv1.VirtualMachineNetworkInterfaceSpec{
+								{
+									Name: "eth1",
+								},
+							},
+						}
+					},
+					validate: doValidateWithMsg(`spec.network.interfaces[0].name: Forbidden: field is immutable`),
+				},
+			),
+			Entry("disallow Network Interface Network ref change",
+				testParams{
+					setup: func(ctx *unitValidatingWebhookContext) {
+						ctx.oldVM.Spec.Network = &vmopv1.VirtualMachineNetworkSpec{
+							Interfaces: []vmopv1.VirtualMachineNetworkInterfaceSpec{
+								{
+									Name:    "eth0",
+									Network: &common.PartialObjectRef{Name: "net1"},
+								},
+							},
+						}
+						ctx.vm.Spec.Network = &vmopv1.VirtualMachineNetworkSpec{
+							Interfaces: []vmopv1.VirtualMachineNetworkInterfaceSpec{
+								{
+									Name:    "eth0",
+									Network: &common.PartialObjectRef{Name: "net99"},
+								},
+							},
+						}
+					},
+					validate: doValidateWithMsg(`spec.network.interfaces[0].network: Forbidden: field is immutable`),
+				},
+			),
+		)
+	})
 }
 
 func unitTestsValidateDelete() {