Merge pull request kubernetes-sigs#356 from akutz/feature/network-dev…

…ice-matching-by-order

Net devices by order, remove invalid IP addrs

config/crds/vsphereproviderconfig_v1alpha1_vspheremachineproviderconfig.yaml

@@ -391,11 +391,6 @@ spec:
                         items:
                           type: string
                         type: array
-                      uuid:
-                        description: UUID is used to relate this network spec to the
-                          network device created from this spec. This value is set
-                          at runtime and should not be assigned manually.
-                        type: string
                     required:
                     - networkName
                     type: object

config/crds/vsphereproviderconfig_v1alpha1_vspheremachineproviderstatus.yaml

@@ -46,13 +46,7 @@ spec:
               networkName:
                 description: NetworkName is the name of the network.
                 type: string
-              uuid:
-                description: UUID is stored as the ExternalID field on a network device
-                  and uniquely identifies the device as one that was created from
-                  a known network spec.
-                type: string
             required:
-            - uuid
             - macAddr
             type: object
           type: array

pkg/apis/vsphereproviderconfig/v1alpha1/vspheremachineproviderconfig_types.go

@@ -98,12 +98,6 @@ type NetworkDeviceSpec struct {
 	// will be connected.
 	NetworkName string `json:"networkName"`
 
-	// UUID is used to relate this network spec to the network device created
-	// from this spec. This value is set at runtime and should not be assigned
-	// manually.
-	// +optional
-	UUID string `json:"uuid,omitempty"`
-
 	// DHCP4 is a flag that indicates whether or not to use DHCP for IPv4
 	// on this device.
 	// If true then IPAddrs should not contain any IPv4 addresses.

pkg/apis/vsphereproviderconfig/v1alpha1/vspheremachineproviderstatus_types.go

@@ -45,11 +45,6 @@ type NetworkStatus struct {
 	// connected to the VM.
 	Connected bool `json:"connected,omitempty"`
 
-	// UUID is stored as the ExternalID field on a network device and uniquely
-	// identifies the device as one that was created from a known network
-	// spec.
-	UUID string `json:"uuid"`
-
 	// IPAddrs is one or more IP addresses reported by vm-tools.
 	// +optional
 	IPAddrs []string `json:"ipAddrs,omitempty"`

pkg/cloud/vsphere/services/govmomi/net/net.go

@@ -18,6 +18,7 @@ package net
 
 import (
 	"context"
+	"net"
 	"strings"
 
 	"github.com/pkg/errors"
@@ -33,11 +34,6 @@ type NetworkStatus struct {
 	// connected to the VM.
 	Connected bool `json:"connected,omitempty"`
 
-	// UUID is stored as the ExternalID field on a network device and uniquely
-	// identifies the device as one that was created from a known network
-	// spec.
-	UUID string `json:"uuid"`
-
 	// IPAddrs is one or more IP addresses reported by vm-tools.
 	// +optional
 	IPAddrs []string `json:"ipAddrs,omitempty"`
@@ -80,7 +76,6 @@ func GetNetworkStatus(
 			nic := dev.GetVirtualEthernetCard()
 			netStatus := NetworkStatus{
 				MACAddr: nic.MacAddress,
-				UUID:    nic.ExternalId,
 			}
 			if obj.Guest != nil {
 				for _, i := range obj.Guest.Net {
@@ -97,3 +92,25 @@ func GetNetworkStatus(
 
 	return allNetStatus, nil
 }
+
+// ErrOnLocalOnlyIPAddr returns an error if the provided IP address is
+// accessible only on the VM's guest OS.
+func ErrOnLocalOnlyIPAddr(addr string) error {
+	var reason string
+	a := net.ParseIP(addr)
+	if a == nil {
+		reason = "invalid"
+	} else if a.IsUnspecified() {
+		reason = "unspecified"
+	} else if a.IsLinkLocalMulticast() {
+		reason = "link-local-mutlicast"
+	} else if a.IsLinkLocalUnicast() {
+		reason = "link-local-unicast"
+	} else if a.IsLoopback() {
+		reason = "loopback"
+	}
+	if reason != "" {
+		return errors.Errorf("failed to validate ip addr=%v: %s", addr, reason)
+	}
+	return nil
+}

pkg/cloud/vsphere/services/govmomi/net/net_test.go

@@ -0,0 +1,79 @@
+/*
+Copyright 2019 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package net_test
+
+import (
+	"testing"
+
+	"sigs.k8s.io/cluster-api-provider-vsphere/pkg/cloud/vsphere/services/govmomi/net"
+)
+
+func TestErrOnLocalOnlyIPAddr(t *testing.T) {
+	testCases := []struct {
+		name      string
+		ipAddr    string
+		expectErr bool
+	}{
+		{
+			name:      "valid-ipv4",
+			ipAddr:    "192.168.2.33",
+			expectErr: false,
+		},
+		{
+			name:      "valid-ipv6",
+			ipAddr:    "1200:0000:AB00:1234:0000:2552:7777:1313",
+			expectErr: false,
+		},
+		{
+			name:      "localhost",
+			ipAddr:    "127.0.0.1",
+			expectErr: true,
+		},
+		{
+			name:      "link-local-unicast-ipv4",
+			ipAddr:    "169.254.2.3",
+			expectErr: true,
+		},
+		{
+			name:      "link-local-unicast-ipv6",
+			ipAddr:    "fe80::250:56ff:feb0:345d",
+			expectErr: true,
+		},
+		{
+			name:      "link-local-multicast-ipv4",
+			ipAddr:    "224.0.0.252",
+			expectErr: true,
+		},
+		{
+			name:      "link-local-multicast-ipv6",
+			ipAddr:    "FF02:0:0:0:0:0:1:3",
+			expectErr: true,
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			if err := net.ErrOnLocalOnlyIPAddr(tc.ipAddr); err != nil {
+				t.Log(err)
+				if !tc.expectErr {
+					t.Fatal(err)
+				}
+			} else if tc.expectErr {
+				t.Fatal("expected error did not occur")
+			}
+		})
+	}
+}

pkg/cloud/vsphere/services/govmomi/update.go

@@ -46,7 +46,7 @@ func Update(ctx *context.MachineContext) error {
 		return errors.Errorf("vm is supposed to exist %q", ctx)
 	}
 
-	if err := reconcileNetworkStatus(ctx, vm); err != nil {
+	if err := reconcileNetwork(ctx, vm); err != nil {
 		return err
 	}
 
@@ -94,8 +94,8 @@ func reconcileMetadata(ctx *context.MachineContext, vm *object.VirtualMachine) e
 	return &clustererror.RequeueAfterError{RequeueAfter: constants.DefaultRequeue}
 }
 
-// reconcileNetworkStatus updates the machine's network status from the VM
-func reconcileNetworkStatus(ctx *context.MachineContext, vm *object.VirtualMachine) error {
+// reconcileNetwork updates the machine's network spec and status
+func reconcileNetwork(ctx *context.MachineContext, vm *object.VirtualMachine) error {
 
 	// Validate the number of reported networks match the number of configured
 	// networks.
@@ -107,20 +107,16 @@ func reconcileNetworkStatus(ctx *context.MachineContext, vm *object.VirtualMachi
 	if expNetCount != actNetCount {
 		return errors.Errorf("invalid network count for %q: exp=%d act=%d", ctx, expNetCount, actNetCount)
 	}
-
-	// Update the machine's network status.
-	ctx.Logger.V(4).Info("updating network status")
 	ctx.MachineStatus.Network = allNetStatus
 
 	// Update the MAC addresses in the machine's network config as well. This
 	// is required in order to generate the metadata.
-	for _, netStatus := range allNetStatus {
-		for i := range ctx.MachineConfig.MachineSpec.Network.Devices {
-			dev := &ctx.MachineConfig.MachineSpec.Network.Devices[i]
-			if netStatus.UUID == dev.UUID && netStatus.MACAddr != dev.MACAddr {
-				ctx.Logger.V(4).Info("updating MAC address for device", "device-uuid", dev.UUID, "network-name", dev.NetworkName, "old-mac-addr", dev.MACAddr, "new-mac-addr", netStatus.MACAddr)
-				dev.MACAddr = netStatus.MACAddr
-			}
+	for i := range ctx.MachineConfig.MachineSpec.Network.Devices {
+		devSpec := &ctx.MachineConfig.MachineSpec.Network.Devices[i]
+		oldMac, newMac := devSpec.MACAddr, allNetStatus[i].MACAddr
+		if oldMac != newMac {
+			devSpec.MACAddr = newMac
+			ctx.Logger.V(6).Info("updating MAC address for device", "network-name", devSpec.NetworkName, "old-mac-addr", oldMac, "new-mac-addr", newMac)
 		}
 	}
 

pkg/cloud/vsphere/services/govmomi/util.go

@@ -95,22 +95,32 @@ func getNetworkStatus(ctx *context.MachineContext) ([]v1alpha1.NetworkStatus, er
 	}
 	ctx.Logger.V(6).Info("got allNetStatus", "status", allNetStatus)
 	apiNetStatus := []v1alpha1.NetworkStatus{}
-	for _, netDevSpec := range ctx.MachineConfig.MachineSpec.Network.Devices {
-		for _, netStatus := range allNetStatus {
-			if netDevSpec.UUID == netStatus.UUID {
-				apiNetStatus = append(apiNetStatus, v1alpha1.NetworkStatus{
-					Connected:   netStatus.Connected,
-					UUID:        netStatus.UUID,
-					IPAddrs:     netStatus.IPAddrs,
-					MACAddr:     netStatus.MACAddr,
-					NetworkName: netStatus.NetworkName,
-				})
-			}
-		}
+	for _, s := range allNetStatus {
+		apiNetStatus = append(apiNetStatus, v1alpha1.NetworkStatus{
+			Connected:   s.Connected,
+			IPAddrs:     sanitizeIPAddrs(ctx, s.IPAddrs),
+			MACAddr:     s.MACAddr,
+			NetworkName: s.NetworkName,
+		})
 	}
 	return apiNetStatus, nil
 }
 
+func sanitizeIPAddrs(ctx *context.MachineContext, ipAddrs []string) []string {
+	if len(ipAddrs) == 0 {
+		return nil
+	}
+	newIPAddrs := []string{}
+	for _, addr := range ipAddrs {
+		if err := net.ErrOnLocalOnlyIPAddr(addr); err != nil {
+			ctx.Logger.V(8).Info("ignoring IP address", "reason", err)
+		} else {
+			newIPAddrs = append(newIPAddrs, addr)
+		}
+	}
+	return newIPAddrs
+}
+
 func getExistingMetadata(ctx *context.MachineContext) (string, error) {
 	var (
 		obj mo.VirtualMachine

pkg/cloud/vsphere/services/govmomi/vcenter/clone.go

@@ -17,7 +17,6 @@ limitations under the License.
 package vcenter
 
 import (
-	"github.com/google/uuid"
 	"github.com/pkg/errors"
 	"github.com/vmware/govmomi/object"
 	"github.com/vmware/govmomi/vim25/types"
@@ -187,16 +186,18 @@ func getNetworkSpecs(
 		// "types.BaseVirtualEthernetCard" as a "types.BaseVirtualDevice".
 		nic := dev.(types.BaseVirtualEthernetCard).GetVirtualEthernetCard()
 
+		if netSpec.MACAddr != "" {
+			nic.MacAddress = netSpec.MACAddr
+			// Please see https://www.vmware.com/support/developer/converter-sdk/conv60_apireference/vim.vm.device.VirtualEthernetCard.html#addressType
+			// for the valid values for this field.
+			nic.AddressType = "Manual"
+			ctx.Logger.V(6).Info("configured manual mac address", "mac-addr", nic.MacAddress)
+		}
+
 		// Assign a temporary device key to ensure that a unique one will be
 		// generated when the device is created.
 		nic.Key = key
 
-		// Create the device with a unique ID that is also stored on the machine's
-		// network device spec. This allows deterministic lookup of MAC/Device
-		// later in the workflow, once the VM is created.
-		netSpec.UUID = uuid.New().String()
-		nic.ExternalId = netSpec.UUID
-
 		deviceSpecs = append(deviceSpecs, &types.VirtualDeviceConfigSpec{
 			Device:    dev,
 			Operation: types.VirtualDeviceConfigSpecOperationAdd,

scripts/e2e/bootstrap_job/spec/provider-components.template

@@ -841,11 +841,6 @@ spec:
                         items:
                           type: string
                         type: array
-                      uuid:
-                        description: UUID is used to relate this network spec to the
-                          network device created from this spec. This value is set
-                          at runtime and should not be assigned manually.
-                        type: string
                     required:
                     - networkName
                     type: object
@@ -960,13 +955,7 @@ spec:
               networkName:
                 description: NetworkName is the name of the network.
                 type: string
-              uuid:
-                description: UUID is stored as DeviceInfo.Label on a network device
-                  and uniquely identifies the device as one that was created from
-                  a known network spec.
-                type: string
             required:
-            - uuid
             - macAddr
             type: object
           type: array