From 314be745ca44179e6c40c7c0ee4640fce8a3a375 Mon Sep 17 00:00:00 2001 From: akutz Date: Tue, 25 Jun 2019 15:45:02 -0500 Subject: [PATCH 1/4] Remove UUID to track network devices; order only This patch removes the UUID used to track the network device specs to network device mappings. Now the order of the devices added is assumed to be the order of the devices retrieved later. This was confirmed via the test cases in the following gists: * https://gist.github.com/dougm/6061dcf7bf44cc21fae185143923f1fa * https://gist.github.com/dougm/6061dcf7bf44cc21fae185143923f1fa/revisions --- ...v1alpha1_vspheremachineproviderconfig.yaml | 5 ---- ...v1alpha1_vspheremachineproviderstatus.yaml | 6 ----- .../vspheremachineproviderconfig_types.go | 6 ----- .../vspheremachineproviderstatus_types.go | 5 ---- pkg/cloud/vsphere/services/govmomi/net/net.go | 6 ----- pkg/cloud/vsphere/services/govmomi/update.go | 23 ++++++++----------- pkg/cloud/vsphere/services/govmomi/util.go | 19 ++++++--------- .../vsphere/services/govmomi/vcenter/clone.go | 13 +++++------ .../spec/provider-components.template | 11 --------- 9 files changed, 22 insertions(+), 72 deletions(-) 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..9eb4eb21dc1c 100644 --- a/pkg/cloud/vsphere/services/govmomi/net/net.go +++ b/pkg/cloud/vsphere/services/govmomi/net/net.go @@ -33,11 +33,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 +75,6 @@ func GetNetworkStatus( nic := dev.GetVirtualEthernetCard() netStatus := NetworkStatus{ MACAddr: nic.MacAddress, - UUID: nic.ExternalId, } if obj.Guest != nil { for _, i := range obj.Guest.Net { diff --git a/pkg/cloud/vsphere/services/govmomi/update.go b/pkg/cloud/vsphere/services/govmomi/update.go index 065ce6d66053..2343213c67cf 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. @@ -108,19 +108,14 @@ func reconcileNetworkStatus(ctx *context.MachineContext, vm *object.VirtualMachi 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..b6de0a055b23 100644 --- a/pkg/cloud/vsphere/services/govmomi/util.go +++ b/pkg/cloud/vsphere/services/govmomi/util.go @@ -95,18 +95,13 @@ 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: s.IPAddrs, + MACAddr: s.MACAddr, + NetworkName: s.NetworkName, + }) } return apiNetStatus, nil } diff --git a/pkg/cloud/vsphere/services/govmomi/vcenter/clone.go b/pkg/cloud/vsphere/services/govmomi/vcenter/clone.go index 13a9bc31557e..1e0cee39d52c 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,16 @@ func getNetworkSpecs( // "types.BaseVirtualEthernetCard" as a "types.BaseVirtualDevice". nic := dev.(types.BaseVirtualEthernetCard).GetVirtualEthernetCard() + if netSpec.MACAddr != "" { + nic.MacAddress = netSpec.MACAddr + 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 From 144f05e35383b03379b892bd674b1c8d912c8faf Mon Sep 17 00:00:00 2001 From: akutz Date: Tue, 25 Jun 2019 16:24:44 -0500 Subject: [PATCH 2/4] Ignore link-local & loopback addresses This patch removes any IP addresses from the list returned from a VM if the addresses are loopback, link local unicast, link local multicast, invalid, or unspecified. --- pkg/cloud/vsphere/services/govmomi/net/net.go | 23 ++++++ .../vsphere/services/govmomi/net/net_test.go | 79 +++++++++++++++++++ pkg/cloud/vsphere/services/govmomi/util.go | 17 +++- 3 files changed, 118 insertions(+), 1 deletion(-) create mode 100644 pkg/cloud/vsphere/services/govmomi/net/net_test.go diff --git a/pkg/cloud/vsphere/services/govmomi/net/net.go b/pkg/cloud/vsphere/services/govmomi/net/net.go index 9eb4eb21dc1c..1d8ad9a725da 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" @@ -91,3 +92,25 @@ func GetNetworkStatus( return allNetStatus, nil } + +// IsExternalIPAddr returns a nil error if the provided IP address is valid and +// can be used externally to the VM. +func IsExternalIPAddr(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..3fec7ca18e42 --- /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 TestIsExternalIPAddr(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.IsExternalIPAddr(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/util.go b/pkg/cloud/vsphere/services/govmomi/util.go index b6de0a055b23..384aa43bdef6 100644 --- a/pkg/cloud/vsphere/services/govmomi/util.go +++ b/pkg/cloud/vsphere/services/govmomi/util.go @@ -98,7 +98,7 @@ func getNetworkStatus(ctx *context.MachineContext) ([]v1alpha1.NetworkStatus, er for _, s := range allNetStatus { apiNetStatus = append(apiNetStatus, v1alpha1.NetworkStatus{ Connected: s.Connected, - IPAddrs: s.IPAddrs, + IPAddrs: sanitizeIPAddrs(ctx, s.IPAddrs), MACAddr: s.MACAddr, NetworkName: s.NetworkName, }) @@ -106,6 +106,21 @@ func getNetworkStatus(ctx *context.MachineContext) ([]v1alpha1.NetworkStatus, er return apiNetStatus, nil } +func sanitizeIPAddrs(ctx *context.MachineContext, ipAddrs []string) []string { + if len(ipAddrs) == 0 { + return nil + } + newIPAddrs := []string{} + for _, i := range ipAddrs { + if err := net.IsExternalIPAddr(i); err != nil { + ctx.Logger.V(8).Info("ignoring IP address", "reason", err) + } else { + newIPAddrs = append(newIPAddrs, i) + } + } + return newIPAddrs +} + func getExistingMetadata(ctx *context.MachineContext) (string, error) { var ( obj mo.VirtualMachine From bb1f36ccb447041153205210d643a83eb79c2c47 Mon Sep 17 00:00:00 2001 From: akutz Date: Tue, 25 Jun 2019 17:15:00 -0500 Subject: [PATCH 3/4] Addressing PR feedback * Renamed IsExternalIPAddr to ErrOnLocalOnlyIPAddr * Linked to the Address type values for a VM's network device * Renamed a non-index range variable from "i" to "addr" --- pkg/cloud/vsphere/services/govmomi/net/net.go | 6 +++--- pkg/cloud/vsphere/services/govmomi/net/net_test.go | 4 ++-- pkg/cloud/vsphere/services/govmomi/util.go | 6 +++--- pkg/cloud/vsphere/services/govmomi/vcenter/clone.go | 2 ++ 4 files changed, 10 insertions(+), 8 deletions(-) diff --git a/pkg/cloud/vsphere/services/govmomi/net/net.go b/pkg/cloud/vsphere/services/govmomi/net/net.go index 1d8ad9a725da..201ec6589a5f 100644 --- a/pkg/cloud/vsphere/services/govmomi/net/net.go +++ b/pkg/cloud/vsphere/services/govmomi/net/net.go @@ -93,9 +93,9 @@ func GetNetworkStatus( return allNetStatus, nil } -// IsExternalIPAddr returns a nil error if the provided IP address is valid and -// can be used externally to the VM. -func IsExternalIPAddr(addr string) error { +// 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 { diff --git a/pkg/cloud/vsphere/services/govmomi/net/net_test.go b/pkg/cloud/vsphere/services/govmomi/net/net_test.go index 3fec7ca18e42..248a77486390 100644 --- a/pkg/cloud/vsphere/services/govmomi/net/net_test.go +++ b/pkg/cloud/vsphere/services/govmomi/net/net_test.go @@ -22,7 +22,7 @@ import ( "sigs.k8s.io/cluster-api-provider-vsphere/pkg/cloud/vsphere/services/govmomi/net" ) -func TestIsExternalIPAddr(t *testing.T) { +func TestErrOnLocalOnlyIPAddr(t *testing.T) { testCases := []struct { name string ipAddr string @@ -66,7 +66,7 @@ func TestIsExternalIPAddr(t *testing.T) { } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - if err := net.IsExternalIPAddr(tc.ipAddr); err != nil { + if err := net.ErrOnLocalOnlyIPAddr(tc.ipAddr); err != nil { t.Log(err) if !tc.expectErr { t.Fatal(err) diff --git a/pkg/cloud/vsphere/services/govmomi/util.go b/pkg/cloud/vsphere/services/govmomi/util.go index 384aa43bdef6..c793d113f2db 100644 --- a/pkg/cloud/vsphere/services/govmomi/util.go +++ b/pkg/cloud/vsphere/services/govmomi/util.go @@ -111,11 +111,11 @@ func sanitizeIPAddrs(ctx *context.MachineContext, ipAddrs []string) []string { return nil } newIPAddrs := []string{} - for _, i := range ipAddrs { - if err := net.IsExternalIPAddr(i); err != nil { + 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, i) + newIPAddrs = append(newIPAddrs, addr) } } return newIPAddrs diff --git a/pkg/cloud/vsphere/services/govmomi/vcenter/clone.go b/pkg/cloud/vsphere/services/govmomi/vcenter/clone.go index 1e0cee39d52c..411d1e00e240 100644 --- a/pkg/cloud/vsphere/services/govmomi/vcenter/clone.go +++ b/pkg/cloud/vsphere/services/govmomi/vcenter/clone.go @@ -188,6 +188,8 @@ func getNetworkSpecs( 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) } From f4d82affc497e58ae6a00cd78c16bf54bb05b432 Mon Sep 17 00:00:00 2001 From: akutz Date: Tue, 25 Jun 2019 18:52:53 -0500 Subject: [PATCH 4/4] Fix regression where net status not added This patch fixes a regression where the network status was not added to the machine, and therefore the machine was never in the ready state. --- pkg/cloud/vsphere/services/govmomi/update.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/cloud/vsphere/services/govmomi/update.go b/pkg/cloud/vsphere/services/govmomi/update.go index 2343213c67cf..4e5e13b32ec9 100644 --- a/pkg/cloud/vsphere/services/govmomi/update.go +++ b/pkg/cloud/vsphere/services/govmomi/update.go @@ -107,6 +107,7 @@ func reconcileNetwork(ctx *context.MachineContext, vm *object.VirtualMachine) er if expNetCount != actNetCount { return errors.Errorf("invalid network count for %q: exp=%d act=%d", ctx, expNetCount, actNetCount) } + 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.