diff --git a/config/crds/vsphereproviderconfig_v1alpha1_vspheremachineproviderconfig.yaml b/config/crds/vsphereproviderconfig_v1alpha1_vspheremachineproviderconfig.yaml index 28f21d7aec76..e0d6009300f1 100644 --- a/config/crds/vsphereproviderconfig_v1alpha1_vspheremachineproviderconfig.yaml +++ b/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 diff --git a/config/crds/vsphereproviderconfig_v1alpha1_vspheremachineproviderstatus.yaml b/config/crds/vsphereproviderconfig_v1alpha1_vspheremachineproviderstatus.yaml index f7e69ad09a48..5594e1c789df 100644 --- a/config/crds/vsphereproviderconfig_v1alpha1_vspheremachineproviderstatus.yaml +++ b/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 diff --git a/pkg/apis/vsphereproviderconfig/v1alpha1/vspheremachineproviderconfig_types.go b/pkg/apis/vsphereproviderconfig/v1alpha1/vspheremachineproviderconfig_types.go index 7446b55c8ad1..d879bd599e84 100644 --- a/pkg/apis/vsphereproviderconfig/v1alpha1/vspheremachineproviderconfig_types.go +++ b/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. diff --git a/pkg/apis/vsphereproviderconfig/v1alpha1/vspheremachineproviderstatus_types.go b/pkg/apis/vsphereproviderconfig/v1alpha1/vspheremachineproviderstatus_types.go index 894e7021b5fd..0af08c363238 100644 --- a/pkg/apis/vsphereproviderconfig/v1alpha1/vspheremachineproviderstatus_types.go +++ b/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"` diff --git a/pkg/cloud/vsphere/services/govmomi/net/net.go b/pkg/cloud/vsphere/services/govmomi/net/net.go index 518707d97a5c..201ec6589a5f 100644 --- a/pkg/cloud/vsphere/services/govmomi/net/net.go +++ b/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 +} diff --git a/pkg/cloud/vsphere/services/govmomi/net/net_test.go b/pkg/cloud/vsphere/services/govmomi/net/net_test.go new file mode 100644 index 000000000000..248a77486390 --- /dev/null +++ b/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") + } + }) + } +} diff --git a/pkg/cloud/vsphere/services/govmomi/update.go b/pkg/cloud/vsphere/services/govmomi/update.go index 065ce6d66053..4e5e13b32ec9 100644 --- a/pkg/cloud/vsphere/services/govmomi/update.go +++ b/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) } } diff --git a/pkg/cloud/vsphere/services/govmomi/util.go b/pkg/cloud/vsphere/services/govmomi/util.go index c2b52c768e57..c793d113f2db 100644 --- a/pkg/cloud/vsphere/services/govmomi/util.go +++ b/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 diff --git a/pkg/cloud/vsphere/services/govmomi/vcenter/clone.go b/pkg/cloud/vsphere/services/govmomi/vcenter/clone.go index 13a9bc31557e..411d1e00e240 100644 --- a/pkg/cloud/vsphere/services/govmomi/vcenter/clone.go +++ b/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, diff --git a/scripts/e2e/bootstrap_job/spec/provider-components.template b/scripts/e2e/bootstrap_job/spec/provider-components.template index 8ef8387af1a9..5610ccc591df 100644 --- a/scripts/e2e/bootstrap_job/spec/provider-components.template +++ b/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