Finalize network v1a2 naming convention (#253)

This change checks for v1a1 network name'd interfaces before creating or updating the new ones. Also includes validation webhook changes for verifying that the combined `network-vm-interface` name is a valid DNS1123 label.

pkg/vmprovider/providers/vsphere2/network/network.go

@@ -16,6 +16,7 @@ import (
 	"github.com/vmware/govmomi/vim25"
 	vimtypes "github.com/vmware/govmomi/vim25/types"
 	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/wait"
@@ -261,12 +262,23 @@ func createNetOPNetworkInterface(
 
 	// If empty, NetOP will try to select a namespace default.
 	networkName := interfaceSpec.Network.Name
+	netIf := &netopv1alpha1.NetworkInterface{}
+	netIfKey := types.NamespacedName{
+		Namespace: vmCtx.VM.Namespace,
+		Name:      NetOPCRName(vmCtx.VM.Name, networkName, interfaceSpec.Name, true),
+	}
+
+	// check if a networkIf object exists with the older (v1a1) naming convention
+	if err := client.Get(vmCtx, netIfKey, netIf); err != nil {
+		if !apierrors.IsNotFound(err) {
+			return nil, err
+		}
 
-	netIf := &netopv1alpha1.NetworkInterface{
-		ObjectMeta: metav1.ObjectMeta{
+		// if notFound set the netIf to the new v1a2 naming convention
+		netIf.ObjectMeta = metav1.ObjectMeta{
 			Name:      NetOPCRName(vmCtx.VM.Name, networkName, interfaceSpec.Name, false),
 			Namespace: vmCtx.VM.Namespace,
-		},
+		}
 	}
 
 	_, err := controllerutil.CreateOrUpdate(vmCtx, client, netIf, func() error {
@@ -404,12 +416,23 @@ func createNCPNetworkInterface(
 
 	// If empty, NCP will use the namespace default.
 	networkName := interfaceSpec.Network.Name
+	vnetIf := &ncpv1alpha1.VirtualNetworkInterface{}
+	vnetIfKey := types.NamespacedName{
+		Namespace: vmCtx.VM.Namespace,
+		Name:      NCPCRName(vmCtx.VM.Name, networkName, interfaceSpec.Name, true),
+	}
+
+	// check if a networkIf object exists with the older (v1a1) naming convention
+	if err := client.Get(vmCtx, vnetIfKey, vnetIf); err != nil {
+		if !apierrors.IsNotFound(err) {
+			return nil, err
+		}
 
-	vnetIf := &ncpv1alpha1.VirtualNetworkInterface{
-		ObjectMeta: metav1.ObjectMeta{
+		// if notFound set the vnetIf to use the new v1a2 naming convention
+		vnetIf.ObjectMeta = metav1.ObjectMeta{
 			Name:      NCPCRName(vmCtx.VM.Name, networkName, interfaceSpec.Name, false),
 			Namespace: vmCtx.VM.Namespace,
-		},
+		}
 	}
 
 	_, err := controllerutil.CreateOrUpdate(vmCtx, client, vnetIf, func() error {

pkg/vmprovider/providers/vsphere2/network/network_test.go

@@ -35,8 +35,9 @@ var _ = Describe("CreateAndWaitForNetworkInterfaces", func() {
 		vm             *vmopv1.VirtualMachine
 		interfaceSpecs []vmopv1.VirtualMachineNetworkInterfaceSpec
 
-		results network.NetworkInterfaceResults
-		err     error
+		results     network.NetworkInterfaceResults
+		err         error
+		initObjects []client.Object
 	)
 
 	BeforeEach(func() {
@@ -59,7 +60,7 @@ var _ = Describe("CreateAndWaitForNetworkInterfaces", func() {
 	})
 
 	JustBeforeEach(func() {
-		ctx = suite.NewTestContextForVCSim(testConfig)
+		ctx = suite.NewTestContextForVCSim(testConfig, initObjects...)
 
 		results, err = network.CreateAndWaitForNetworkInterfaces(
 			vmCtx,
@@ -73,6 +74,7 @@ var _ = Describe("CreateAndWaitForNetworkInterfaces", func() {
 	AfterEach(func() {
 		ctx.AfterEach()
 		ctx = nil
+		initObjects = nil
 	})
 
 	Context("Named Network", func() {
@@ -225,6 +227,95 @@ var _ = Describe("CreateAndWaitForNetworkInterfaces", func() {
 				Expect(ipConfig.IsIPv4).To(BeFalse())
 				Expect(ipConfig.Gateway).To(Equal("fd1a:6c85:79fe:7c98:0000:0000:0000:0001"))
 			})
+
+			When("v1a1 network interface exists", func() {
+				BeforeEach(func() {
+					netIf := &netopv1alpha1.NetworkInterface{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      network.NetOPCRName(vm.Name, networkName, interfaceName, true),
+							Namespace: vm.Namespace,
+						},
+						Spec: netopv1alpha1.NetworkInterfaceSpec{
+							NetworkName: networkName,
+							Type:        netopv1alpha1.NetworkInterfaceTypeVMXNet3,
+						},
+					}
+
+					initObjects = append(initObjects, netIf)
+				})
+
+				It("returns success", func() {
+					// Assert test env is what we expect.
+					Expect(ctx.NetworkRef.Reference().Type).To(Equal("DistributedVirtualPortgroup"))
+
+					Expect(err).To(HaveOccurred())
+					Expect(err.Error()).To(ContainSubstring("network interface is not ready yet"))
+					Expect(results.Results).To(BeEmpty())
+
+					By("simulate successful NetOP reconcile", func() {
+						netInterface := &netopv1alpha1.NetworkInterface{
+							ObjectMeta: metav1.ObjectMeta{
+								Name:      network.NetOPCRName(vm.Name, networkName, interfaceName, true),
+								Namespace: vm.Namespace,
+							},
+						}
+						Expect(ctx.Client.Get(ctx, client.ObjectKeyFromObject(netInterface), netInterface)).To(Succeed())
+						Expect(netInterface.Spec.NetworkName).To(Equal(networkName))
+
+						netInterface.Status.NetworkID = ctx.NetworkRef.Reference().Value
+						netInterface.Status.MacAddress = "" // NetOP doesn't set this.
+						netInterface.Status.IPConfigs = []netopv1alpha1.IPConfig{
+							{
+								IP:         "192.168.1.110",
+								IPFamily:   netopv1alpha1.IPv4Protocol,
+								Gateway:    "192.168.1.1",
+								SubnetMask: "255.255.255.0",
+							},
+							{
+								IP:         "fd1a:6c85:79fe:7c98:0000:0000:0000:000f",
+								IPFamily:   netopv1alpha1.IPv6Protocol,
+								Gateway:    "fd1a:6c85:79fe:7c98:0000:0000:0000:0001",
+								SubnetMask: "ffff:ffff:ffff:ff00:0000:0000:0000:0000",
+							},
+						}
+						netInterface.Status.Conditions = []netopv1alpha1.NetworkInterfaceCondition{
+							{
+								Type:   netopv1alpha1.NetworkInterfaceReady,
+								Status: corev1.ConditionTrue,
+							},
+						}
+						Expect(ctx.Client.Status().Update(ctx, netInterface)).To(Succeed())
+					})
+
+					results, err = network.CreateAndWaitForNetworkInterfaces(
+						vmCtx,
+						ctx.Client,
+						ctx.VCClient.Client,
+						ctx.Finder,
+						nil,
+						interfaceSpecs)
+					Expect(err).ToNot(HaveOccurred())
+
+					Expect(results.Results).To(HaveLen(1))
+					result := results.Results[0]
+					Expect(result.MacAddress).To(BeEmpty())
+					Expect(result.ExternalID).To(BeEmpty())
+					Expect(result.NetworkID).To(Equal(ctx.NetworkRef.Reference().Value))
+					Expect(result.Backing).ToNot(BeNil())
+					Expect(result.Backing.Reference()).To(Equal(ctx.NetworkRef.Reference()))
+					Expect(result.Name).To(Equal(interfaceName))
+
+					Expect(result.IPConfigs).To(HaveLen(2))
+					ipConfig := result.IPConfigs[0]
+					Expect(ipConfig.IPCIDR).To(Equal("192.168.1.110/24"))
+					Expect(ipConfig.IsIPv4).To(BeTrue())
+					Expect(ipConfig.Gateway).To(Equal("192.168.1.1"))
+					ipConfig = result.IPConfigs[1]
+					Expect(ipConfig.IPCIDR).To(Equal("fd1a:6c85:79fe:7c98::f/56"))
+					Expect(ipConfig.IsIPv4).To(BeFalse())
+					Expect(ipConfig.Gateway).To(Equal("fd1a:6c85:79fe:7c98:0000:0000:0000:0001"))
+				})
+			})
 		})
 	})
 
@@ -339,6 +430,109 @@ var _ = Describe("CreateAndWaitForNetworkInterfaces", func() {
 				Expect(results.Results[0].Backing).ToNot(BeNil())
 				Expect(results.Results[0].Backing.Reference()).To(Equal(ctx.NetworkRef.Reference()))
 			})
+
+			When("v1a1 NCP network interface exists", func() {
+				BeforeEach(func() {
+					vnetIf := &ncpv1alpha1.VirtualNetworkInterface{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      network.NCPCRName(vm.Name, networkName, interfaceName, true),
+							Namespace: vm.Namespace,
+						},
+						Spec: ncpv1alpha1.VirtualNetworkInterfaceSpec{
+							VirtualNetwork: networkName,
+						},
+					}
+
+					initObjects = append(initObjects, vnetIf)
+				})
+
+				It("returns success", func() {
+					// Assert test env is what we expect.
+					Expect(ctx.NetworkRef.Reference().Type).To(Equal("DistributedVirtualPortgroup"))
+
+					Expect(err).To(HaveOccurred())
+					Expect(err.Error()).To(ContainSubstring("network interface is not ready yet"))
+					Expect(results.Results).To(BeEmpty())
+
+					By("simulate successful NCP reconcile", func() {
+						netInterface := &ncpv1alpha1.VirtualNetworkInterface{
+							ObjectMeta: metav1.ObjectMeta{
+								Name:      network.NCPCRName(vm.Name, networkName, interfaceName, true),
+								Namespace: vm.Namespace,
+							},
+						}
+						Expect(ctx.Client.Get(ctx, client.ObjectKeyFromObject(netInterface), netInterface)).To(Succeed())
+						Expect(netInterface.Spec.VirtualNetwork).To(Equal(networkName))
+
+						netInterface.Status.InterfaceID = interfaceID
+						netInterface.Status.MacAddress = macAddress
+						netInterface.Status.ProviderStatus = &ncpv1alpha1.VirtualNetworkInterfaceProviderStatus{
+							NsxLogicalSwitchID: builder.NsxTLogicalSwitchUUID,
+						}
+						netInterface.Status.IPAddresses = []ncpv1alpha1.VirtualNetworkInterfaceIP{
+							{
+								IP:         "192.168.1.110",
+								Gateway:    "192.168.1.1",
+								SubnetMask: "255.255.255.0",
+							},
+							{
+								IP:         "fd1a:6c85:79fe:7c98:0000:0000:0000:000f",
+								Gateway:    "fd1a:6c85:79fe:7c98:0000:0000:0000:0001",
+								SubnetMask: "ffff:ffff:ffff:ff00:0000:0000:0000:0000",
+							},
+						}
+						netInterface.Status.Conditions = []ncpv1alpha1.VirtualNetworkCondition{
+							{
+								Type:   "Ready",
+								Status: "True",
+							},
+						}
+						Expect(ctx.Client.Status().Update(ctx, netInterface)).To(Succeed())
+					})
+
+					results, err = network.CreateAndWaitForNetworkInterfaces(
+						vmCtx,
+						ctx.Client,
+						ctx.VCClient.Client,
+						ctx.Finder,
+						nil,
+						interfaceSpecs)
+					Expect(err).ToNot(HaveOccurred())
+
+					Expect(results.Results).To(HaveLen(1))
+					result := results.Results[0]
+					Expect(result.MacAddress).To(Equal(macAddress))
+					Expect(result.ExternalID).To(Equal(interfaceID))
+					Expect(result.NetworkID).To(Equal(builder.NsxTLogicalSwitchUUID))
+					Expect(result.Name).To(Equal(interfaceName))
+
+					Expect(result.IPConfigs).To(HaveLen(2))
+					ipConfig := result.IPConfigs[0]
+					Expect(ipConfig.IPCIDR).To(Equal("192.168.1.110/24"))
+					Expect(ipConfig.IsIPv4).To(BeTrue())
+					Expect(ipConfig.Gateway).To(Equal("192.168.1.1"))
+					ipConfig = result.IPConfigs[1]
+					Expect(ipConfig.IPCIDR).To(Equal("fd1a:6c85:79fe:7c98::f/56"))
+					Expect(ipConfig.IsIPv4).To(BeFalse())
+					Expect(ipConfig.Gateway).To(Equal("fd1a:6c85:79fe:7c98:0000:0000:0000:0001"))
+
+					// Without the ClusterMoRef on the first call this will be nil for NSXT.
+					Expect(result.Backing).To(BeNil())
+
+					clusterMoRef := ctx.GetSingleClusterCompute().Reference()
+					results, err = network.CreateAndWaitForNetworkInterfaces(
+						vmCtx,
+						ctx.Client,
+						ctx.VCClient.Client,
+						ctx.Finder,
+						&clusterMoRef,
+						interfaceSpecs)
+					Expect(err).ToNot(HaveOccurred())
+					Expect(results.Results).To(HaveLen(1))
+					Expect(results.Results[0].Backing).ToNot(BeNil())
+					Expect(results.Results[0].Backing.Reference()).To(Equal(ctx.NetworkRef.Reference()))
+				})
+			})
 		})
 	})
 })

webhooks/virtualmachine/v1alpha2/validation/virtualmachine_validator.go

@@ -335,7 +335,7 @@ func (v validator) validateNetwork(ctx *context.WebhookRequestContext, vm *vmopv
 		if defaultNetworkInterface.Name == "" {
 			defaultNetworkInterface.Name = "eth0"
 		}
-		allErrs = append(allErrs, v.validateNetworkInterfaceSpec(networkPath, defaultNetworkInterface)...)
+		allErrs = append(allErrs, v.validateNetworkInterfaceSpec(networkPath, defaultNetworkInterface, vm.Name)...)
 	}
 
 	if len(networkSpec.Interfaces) > 0 {
@@ -349,7 +349,7 @@ func (v validator) validateNetwork(ctx *context.WebhookRequestContext, vm *vmopv
 		}
 
 		for i, interfaceSpec := range networkSpec.Interfaces {
-			allErrs = append(allErrs, v.validateNetworkInterfaceSpec(p.Index(i), interfaceSpec)...)
+			allErrs = append(allErrs, v.validateNetworkInterfaceSpec(p.Index(i), interfaceSpec, vm.Name)...)
 		}
 	}
 
@@ -358,11 +358,23 @@ func (v validator) validateNetwork(ctx *context.WebhookRequestContext, vm *vmopv
 
 func (v validator) validateNetworkInterfaceSpec(
 	interfacePath *field.Path,
-	interfaceSpec vmopv1.VirtualMachineNetworkInterfaceSpec) field.ErrorList {
+	interfaceSpec vmopv1.VirtualMachineNetworkInterfaceSpec,
+	vmName string) field.ErrorList {
 
 	var allErrs field.ErrorList
+	var networkIfCRName string
+	networkName := interfaceSpec.Network.Name
 
-	// TODO: Ensure valid name once we finalize the naming convention for the network interface CRD.
+	// The networkInterface CR name ("vmName-networkName-interfaceName" or "vmName-interfaceName") needs to be a DNS1123 Label
+	if networkName != "" {
+		networkIfCRName = fmt.Sprintf("%s-%s-%s", vmName, networkName, interfaceSpec.Name)
+	} else {
+		networkIfCRName = fmt.Sprintf("%s-%s", vmName, interfaceSpec.Name)
+	}
+
+	for _, msg := range validation.NameIsDNSLabel(networkIfCRName, false) {
+		allErrs = append(allErrs, field.Invalid(interfacePath.Child("name"), interfaceSpec.Name, msg))
+	}
 
 	var ipv4Addrs, ipv6Addrs []string
 	for i, ipCIDR := range interfaceSpec.Addresses {

webhooks/virtualmachine/v1alpha2/validation/virtualmachine_validator_unit_test.go

@@ -594,6 +594,7 @@ func unitTestsValidateCreate() {
 							HostName: "my-vm",
 							Interfaces: []vmopv1.VirtualMachineNetworkInterfaceSpec{
 								{
+									Name: "eth1",
 									Addresses: []string{
 										"192.168.1.100/24",
 										"2605:a601:a0ba:720:2ce6:776d:8be4:2496/48",
@@ -634,6 +635,7 @@ func unitTestsValidateCreate() {
 							Gateway4:  "192.168.1.1",
 							Interfaces: []vmopv1.VirtualMachineNetworkInterfaceSpec{
 								{
+									Name:  "eth1",
 									DHCP4: true,
 								},
 							},
@@ -644,6 +646,36 @@ func unitTestsValidateCreate() {
 					),
 				},
 			),
+
+			Entry("disallow creating VM with network interfaces resulting in a non-DNS1123 combined network interface CR name/label (`vmName-networkName-interfaceName`)",
+				testParams{
+					setup: func(ctx *unitValidatingWebhookContext) {
+						ctx.vm.Spec.Network = vmopv1.VirtualMachineNetworkSpec{
+							Interfaces: []vmopv1.VirtualMachineNetworkInterfaceSpec{
+								{
+									Name: fmt.Sprintf("%x", make([]byte, validation.DNS1123LabelMaxLength)),
+									Network: common.PartialObjectRef{
+										Name: "dummy-nw",
+									},
+								},
+								{
+									Name: "dummy_If",
+									Network: common.PartialObjectRef{
+										Name: "dummy-nw",
+									},
+								},
+							},
+						}
+					},
+					validate: doValidateWithMsg(
+						fmt.Sprintf(`spec.network.interfaces[0].name: Invalid value: "%x": must be no more than 63 characters`, make([]byte, validation.DNS1123LabelMaxLength)),
+						`spec.network.interfaces[1].name: Invalid value: "dummy_If": a lowercase RFC 1123 label must consist of lower case alphanumeric characters or '-'`,
+						`and must start and end with an alphanumeric character (e.g. 'my-name'`,
+						` or '123-abc'`,
+						`regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?')`,
+					),
+				},
+			),
 		)
 	})
 }