From 09cd72faf876acbba6196e31466ff25a23bc41d8 Mon Sep 17 00:00:00 2001 From: Ricardo Katz Date: Fri, 30 Jun 2023 12:19:22 -0300 Subject: [PATCH 01/10] Implement failure domain network configurations --- apis/v1alpha3/topology_conversion.go | 32 ++++++++++ apis/v1alpha4/topology_conversion.go | 32 ++++++++++ apis/v1beta1/vspherefailuredomain_types.go | 60 +++++++++++++++++++ apis/v1beta1/vspherefailuredomain_webhook.go | 11 ++++ pkg/services/vimmachine.go | 61 +++++++++++++++++--- 5 files changed, 189 insertions(+), 7 deletions(-) create mode 100644 apis/v1alpha3/topology_conversion.go create mode 100644 apis/v1alpha4/topology_conversion.go diff --git a/apis/v1alpha3/topology_conversion.go b/apis/v1alpha3/topology_conversion.go new file mode 100644 index 0000000000..6de83082fd --- /dev/null +++ b/apis/v1alpha3/topology_conversion.go @@ -0,0 +1,32 @@ +/* +Copyright 2023 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 v1alpha3 + +import ( + conversion "k8s.io/apimachinery/pkg/conversion" + v1beta1 "sigs.k8s.io/cluster-api-provider-vsphere/apis/v1beta1" +) + +func Convert_v1beta1_Topology_To_v1alpha3_Topology(in *v1beta1.Topology, out *Topology, s conversion.Scope) error { + if len(in.NetworkConfigs) > 0 { + networks := make([]string, len(in.NetworkConfigs)) + for i := range in.NetworkConfigs { + networks[i] = in.NetworkConfigs[i].NetworkName + } + } + return nil +} diff --git a/apis/v1alpha4/topology_conversion.go b/apis/v1alpha4/topology_conversion.go new file mode 100644 index 0000000000..3662480c41 --- /dev/null +++ b/apis/v1alpha4/topology_conversion.go @@ -0,0 +1,32 @@ +/* +Copyright 2023 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 v1alpha4 + +import ( + conversion "k8s.io/apimachinery/pkg/conversion" + v1beta1 "sigs.k8s.io/cluster-api-provider-vsphere/apis/v1beta1" +) + +func Convert_v1beta1_Topology_To_v1alpha4_Topology(in *v1beta1.Topology, out *Topology, s conversion.Scope) error { + if len(in.NetworkConfigs) > 0 { + networks := make([]string, len(in.NetworkConfigs)) + for i := range in.NetworkConfigs { + networks[i] = in.NetworkConfigs[i].NetworkName + } + } + return nil +} diff --git a/apis/v1beta1/vspherefailuredomain_types.go b/apis/v1beta1/vspherefailuredomain_types.go index 4151fe3926..8a586a7d16 100644 --- a/apis/v1beta1/vspherefailuredomain_types.go +++ b/apis/v1beta1/vspherefailuredomain_types.go @@ -18,6 +18,7 @@ limitations under the License. package v1beta1 import ( + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -71,9 +72,14 @@ type Topology struct { Hosts *FailureDomainHosts `json:"hosts,omitempty"` // Networks is the list of networks within this failure domain + // TODO (@rkatz): Deprecate in favor of NetworkConfigs? // +optional Networks []string `json:"networks,omitempty"` + // NetworkConfigs is a list with new network configurations within this failure domain + // + optional + NetworkConfigs []FailureDomainNetwork `json:"networkConfigs,omitempty"` + // Datastore is the name or inventory path of the datastore in which the // virtual machine is created/located. // +optional @@ -88,6 +94,60 @@ type FailureDomainHosts struct { HostGroupName string `json:"hostGroupName"` } +// FailureDomainNetwork defines a network configuration that should be used when consuming +// this failure domain. +// @rkatz - To be discussed with team, should we just embed NetworkDeviceSpec? +type FailureDomainNetwork struct { + // NetworkName is the network name for this machine's VM. + NetworkName string `json:"name,omitempty"` + + // @rkatz - The reason NetworkDeviceSpec is not being copied here, is because DHCP4 and DHCP6 are not pointers there + // This means that, in a case like "I want DHCP4 to be enabled on my template" and "I want DHCP4 to be disabled on this failure domain" + // we cannot verify if DHCP4 was unset (nil, we don't care about it on failure domain) vs set to false (I want to FORCE it to be false) + // DHCP4 is a flag that indicates whether or not to use DHCP for IPv4 + // +optional + DHCP4 *bool `json:"dhcp4,omitempty"` + + // DHCP6 indicates whether or not to use DHCP for IPv6 + // +optional + DHCP6 *bool `json:"dhcp6,omitempty"` + + // Nameservers is a list of IPv4 and/or IPv6 addresses used as DNS + // nameservers. + // Please note that Linux allows only three nameservers (https://linux.die.net/man/5/resolv.conf). + // +optional + Nameservers []string `json:"nameservers,omitempty"` + + // SearchDomains is a list of search domains used when resolving IP + // addresses with DNS. + // +optional + SearchDomains []string `json:"searchDomains,omitempty"` + + // DHCP4Overrides allows for the control over several DHCP behaviors. + // Overrides will only be applied when the corresponding DHCP flag is set. + // Only configured values will be sent, omitted values will default to + // distribution defaults. + // Dependent on support in the network stack for your distribution. + // For more information see the netplan reference (https://netplan.io/reference#dhcp-overrides) + // +optional + DHCP4Overrides *DHCPOverrides `json:"dhcp4Overrides,omitempty"` + + // DHCP6Overrides allows for the control over several DHCP behaviors. + // Overrides will only be applied when the corresponding DHCP flag is set. + // Only configured values will be sent, omitted values will default to + // distribution defaults. + // Dependent on support in the network stack for your distribution. + // For more information see the netplan reference (https://netplan.io/reference#dhcp-overrides) + // +optional + DHCP6Overrides *DHCPOverrides `json:"dhcp6Overrides,omitempty"` + + // AddressesFromPools is a list of IPAddressPools that should be assigned + // to IPAddressClaims. The machine's cloud-init metadata will be populated + // with IPAddresses fulfilled by an IPAM provider. + // +optional + AddressesFromPools []corev1.TypedLocalObjectReference `json:"addressesFromPools,omitempty"` +} + // +kubebuilder:object:root=true // +kubebuilder:storageversion // +kubebuilder:resource:path=vspherefailuredomains,scope=Cluster,categories=cluster-api diff --git a/apis/v1beta1/vspherefailuredomain_webhook.go b/apis/v1beta1/vspherefailuredomain_webhook.go index 5812aa5bcc..2f3e81fa73 100644 --- a/apis/v1beta1/vspherefailuredomain_webhook.go +++ b/apis/v1beta1/vspherefailuredomain_webhook.go @@ -90,4 +90,15 @@ func (r *VSphereFailureDomain) Default() { if r.Spec.Region.AutoConfigure == nil { r.Spec.Region.AutoConfigure = pointer.Bool(false) } + + // Converts the old Networks field to NetworkConfigs + // TODO (@rkatz) - Can this be harmful for Gitops and other users that check spec and re-apply? Probably it will generate + // a difference between "what is expected" and "what we have" as there's going to be the additional "NetworkConfigs" field + // + if len(r.Spec.Topology.NetworkConfigs) == 0 && len(r.Spec.Topology.Networks) > 0 { + r.Spec.Topology.NetworkConfigs = make([]FailureDomainNetwork, len(r.Spec.Topology.Networks)) + for i := range r.Spec.Topology.Networks { + r.Spec.Topology.NetworkConfigs[i].NetworkName = r.Spec.Topology.Networks[i] + } + } } diff --git a/pkg/services/vimmachine.go b/pkg/services/vimmachine.go index 93679cee51..7fb5e0b19e 100644 --- a/pkg/services/vimmachine.go +++ b/pkg/services/vimmachine.go @@ -496,8 +496,17 @@ func (v *VimMachineService) generateOverrideFunc(ctx *context.VIMMachineContext) if vsphereFailureDomain.Spec.Topology.Datastore != "" { vm.Spec.Datastore = vsphereFailureDomain.Spec.Topology.Datastore } - if len(vsphereFailureDomain.Spec.Topology.Networks) > 0 { - vm.Spec.Network.Devices = overrideNetworkDeviceSpecs(vm.Spec.Network.Devices, vsphereFailureDomain.Spec.Topology.Networks) + + // Some old cases may still use Networks instead of NetworkConfigs, so we should use the old fields + if len(vsphereFailureDomain.Spec.Topology.NetworkConfigs) == 0 && len(vsphereFailureDomain.Spec.Topology.Networks) > 0 { + vsphereFailureDomain.Spec.Topology.NetworkConfigs = make([]infrav1.FailureDomainNetwork, len(vsphereFailureDomain.Spec.Topology.Networks)) + for i := range vsphereFailureDomain.Spec.Topology.Networks { + vsphereFailureDomain.Spec.Topology.NetworkConfigs[i].NetworkName = vsphereFailureDomain.Spec.Topology.Networks[i] + } + } + + if len(vsphereFailureDomain.Spec.Topology.NetworkConfigs) > 0 { + vm.Spec.Network.Devices = overrideNetworkDeviceSpecs(vm.Spec.Network.Devices, vsphereFailureDomain.Spec.Topology.NetworkConfigs) } } return overrideWithFailureDomainFunc, true @@ -507,7 +516,7 @@ func (v *VimMachineService) generateOverrideFunc(ctx *context.VIMMachineContext) // The substitution is done based on the order in which the network devices have been defined. // // In case there are more network definitions than the number of network devices specified, the definitions are appended to the list. -func overrideNetworkDeviceSpecs(deviceSpecs []infrav1.NetworkDeviceSpec, networks []string) []infrav1.NetworkDeviceSpec { +func overrideNetworkDeviceSpecs(deviceSpecs []infrav1.NetworkDeviceSpec, networks []infrav1.FailureDomainNetwork) []infrav1.NetworkDeviceSpec { index, length := 0, len(networks) devices := make([]infrav1.NetworkDeviceSpec, 0, integer.IntMax(length, len(deviceSpecs))) @@ -516,16 +525,54 @@ func overrideNetworkDeviceSpecs(deviceSpecs []infrav1.NetworkDeviceSpec, network vmNetworkDeviceSpec := deviceSpecs[i] if i < length { index++ - vmNetworkDeviceSpec.NetworkName = networks[i] + mergeFailureDomainNetSpecToNetworkDeviceSpec(networks[i], &vmNetworkDeviceSpec) } devices = append(devices, vmNetworkDeviceSpec) } // append the remaining network definitions to the VM spec for ; index < length; index++ { - devices = append(devices, infrav1.NetworkDeviceSpec{ - NetworkName: networks[index], - }) + newNetwork := infrav1.NetworkDeviceSpec{} + mergeFailureDomainNetSpecToNetworkDeviceSpec(networks[index], &newNetwork) + devices = append(devices, newNetwork) } return devices } + +func mergeFailureDomainNetSpecToNetworkDeviceSpec(fd infrav1.FailureDomainNetwork, vmNetworkDeviceSpec *infrav1.NetworkDeviceSpec) { + // We don't convert if destination is null + if vmNetworkDeviceSpec == nil { + return + } + if fd.NetworkName != "" { + vmNetworkDeviceSpec.NetworkName = fd.NetworkName + } + if fd.DHCP4 != nil { + vmNetworkDeviceSpec.DHCP4 = *fd.DHCP4 + } + if fd.DHCP6 != nil { + vmNetworkDeviceSpec.DHCP6 = *fd.DHCP6 + } + if len(fd.Nameservers) > 0 { + vmNetworkDeviceSpec.Nameservers = make([]string, len(fd.Nameservers)) + copy(vmNetworkDeviceSpec.Nameservers, fd.Nameservers) + } + + if len(fd.SearchDomains) > 0 { + vmNetworkDeviceSpec.SearchDomains = make([]string, len(fd.SearchDomains)) + copy(vmNetworkDeviceSpec.SearchDomains, fd.SearchDomains) + } + + if fd.DHCP4Overrides != nil { + vmNetworkDeviceSpec.DHCP4Overrides = fd.DHCP4Overrides.DeepCopy() + } + + if fd.DHCP6Overrides != nil { + vmNetworkDeviceSpec.DHCP6Overrides = fd.DHCP6Overrides.DeepCopy() + } + + if len(fd.AddressesFromPools) > 0 { + vmNetworkDeviceSpec.AddressesFromPools = make([]corev1.TypedLocalObjectReference, len(fd.AddressesFromPools)) + copy(vmNetworkDeviceSpec.AddressesFromPools, fd.AddressesFromPools) + } +} From 25c3e14a85d77dfe12a1f60dedd4bee87e907269 Mon Sep 17 00:00:00 2001 From: Ricardo Katz Date: Fri, 30 Jun 2023 15:57:35 -0300 Subject: [PATCH 02/10] Add tests for failure domain network config --- .../vspherefailuredomain_webhook_test.go | 120 +++++++++++++- pkg/services/vimmachine_test.go | 148 +++++++++++++++++- 2 files changed, 261 insertions(+), 7 deletions(-) diff --git a/apis/v1beta1/vspherefailuredomain_webhook_test.go b/apis/v1beta1/vspherefailuredomain_webhook_test.go index df9457d613..93a8102613 100644 --- a/apis/v1beta1/vspherefailuredomain_webhook_test.go +++ b/apis/v1beta1/vspherefailuredomain_webhook_test.go @@ -23,16 +23,124 @@ import ( "k8s.io/utils/pointer" ) -//nolint +// nolint func TestVsphereFailureDomain_Default(t *testing.T) { + tests := []struct { + name string + spec VSphereFailureDomainSpec + expectedSpec VSphereFailureDomainSpec + }{ + { + name: "when autoconfigure is not set", + spec: VSphereFailureDomainSpec{ + Zone: FailureDomain{ + AutoConfigure: nil, + }, + Region: FailureDomain{ + AutoConfigure: nil, + }, + }, + expectedSpec: VSphereFailureDomainSpec{ + Zone: FailureDomain{ + AutoConfigure: pointer.Bool(false), + }, + Region: FailureDomain{ + AutoConfigure: pointer.Bool(false), + }, + }, + }, + { + name: "when autoconfigure is set just on one field", + spec: VSphereFailureDomainSpec{ + Region: FailureDomain{ + AutoConfigure: pointer.Bool(true), + }, + }, + expectedSpec: VSphereFailureDomainSpec{ + Zone: FailureDomain{ + AutoConfigure: pointer.Bool(false), + }, + Region: FailureDomain{ + AutoConfigure: pointer.Bool(true), + }, + }, + }, + { + name: "when networkconfigs is null and network field exists", + spec: VSphereFailureDomainSpec{ + Topology: Topology{ + Networks: []string{"network-a", "network-b"}, + }, + }, + expectedSpec: VSphereFailureDomainSpec{ + Zone: FailureDomain{ + AutoConfigure: pointer.Bool(false), + }, + Region: FailureDomain{ + AutoConfigure: pointer.Bool(false), + }, + Topology: Topology{ + Networks: []string{"network-a", "network-b"}, + NetworkConfigs: []FailureDomainNetwork{ + { + NetworkName: "network-a", + }, + { + NetworkName: "network-b", + }, + }, + }, + }, + }, + { + name: "when networkconfigs is not null and network field exists", + spec: VSphereFailureDomainSpec{ + Topology: Topology{ + Networks: []string{"network-a", "network-b"}, + NetworkConfigs: []FailureDomainNetwork{ + { + NetworkName: "network-c", + }, + { + NetworkName: "network-d", + }, + }, + }, + }, + expectedSpec: VSphereFailureDomainSpec{ + Zone: FailureDomain{ + AutoConfigure: pointer.Bool(false), + }, + Region: FailureDomain{ + AutoConfigure: pointer.Bool(false), + }, + Topology: Topology{ + Networks: []string{"network-a", "network-b"}, + NetworkConfigs: []FailureDomainNetwork{ + { + NetworkName: "network-c", + }, + { + NetworkName: "network-d", + }, + }, + }, + }, + }, + } + g := NewWithT(t) - m := &VSphereFailureDomain{ - Spec: VSphereFailureDomainSpec{}, + + for _, test := range tests { + m := &VSphereFailureDomain{ + Spec: test.spec, + } + m.Default() + g.Expect(m.Spec).To(Equal(test.expectedSpec)) } - m.Default() - g.Expect(*m.Spec.Zone.AutoConfigure).To(BeFalse()) - g.Expect(*m.Spec.Region.AutoConfigure).To(BeFalse()) + //g.Expect(*m.Spec.Zone.AutoConfigure).To(BeFalse()) + //g.Expect(*m.Spec.Region.AutoConfigure).To(BeFalse()) } func TestVSphereFailureDomain_ValidateCreate(t *testing.T) { diff --git a/pkg/services/vimmachine_test.go b/pkg/services/vimmachine_test.go index 7f4abf3f22..499d496477 100644 --- a/pkg/services/vimmachine_test.go +++ b/pkg/services/vimmachine_test.go @@ -59,6 +59,33 @@ var _ = Describe("VimMachineService_GenerateOverrideFunc", func() { }, } } + + failureDomainWithNetConfig := func(suffix string, addOldNetwork bool) *infrav1.VSphereFailureDomain { + var networks []string + if addOldNetwork { + networks = append(networks, fmt.Sprintf("nw-%s", suffix), "another-nw") + } + return &infrav1.VSphereFailureDomain{ + ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("fd-%s", suffix)}, + Spec: infrav1.VSphereFailureDomainSpec{ + Topology: infrav1.Topology{ + Datacenter: fmt.Sprintf("dc-%s", suffix), + Datastore: fmt.Sprintf("ds-%s", suffix), + Networks: networks, + NetworkConfigs: []infrav1.FailureDomainNetwork{ + { + NetworkName: fmt.Sprintf("newnw-%s", suffix), + DHCP4: pointer.Bool(false), + }, + { + NetworkName: "another-new-nw", + Nameservers: []string{"10.10.10.10", "10.10.20.20"}, + }, + }, + }, + }, + } + } var ( controllerCtx *context.ControllerContext machineCtx *context.VIMMachineContext @@ -66,7 +93,8 @@ var _ = Describe("VimMachineService_GenerateOverrideFunc", func() { ) BeforeEach(func() { - controllerCtx = fake.NewControllerContext(fake.NewControllerManagerContext(deplZone("one"), deplZone("two"), failureDomain("one"), failureDomain("two"))) + controllerCtx = fake.NewControllerContext(fake.NewControllerManagerContext(deplZone("one"), deplZone("two"), deplZone("three"), deplZone("four"), + failureDomain("one"), failureDomain("two"), failureDomainWithNetConfig("three", false), failureDomainWithNetConfig("four", true))) machineCtx = fake.NewMachineContext(fake.NewClusterContext(controllerCtx)) vimMachineService = &VimMachineService{} }) @@ -183,7 +211,125 @@ var _ = Describe("VimMachineService_GenerateOverrideFunc", func() { Expect(devices[2].NetworkName).To(Equal("baz")) }) }) + + Context("with only network config specified in the topology", func() { + BeforeEach(func() { + machineCtx.Machine.Spec.FailureDomain = pointer.String("zone-three") + }) + It("overrides the n/w configs from the networks list of the topology", func() { + By("For equal number of networks") + vm := &infrav1.VSphereVM{ + Spec: infrav1.VSphereVMSpec{ + VirtualMachineCloneSpec: infrav1.VirtualMachineCloneSpec{ + Network: infrav1.NetworkSpec{Devices: []infrav1.NetworkDeviceSpec{{NetworkName: "foo", DHCP4: true}, {NetworkName: "bar", DHCP6: true, Nameservers: []string{"10.50.50.10"}}}}, + }, + }, + } + + overrideFunc, ok := vimMachineService.generateOverrideFunc(machineCtx) + Expect(ok).To(BeTrue()) + + overrideFunc(vm) + + devices := vm.Spec.Network.Devices + Expect(devices).To(HaveLen(2)) + Expect(devices[0].NetworkName).To(Equal("newnw-three")) + Expect(devices[0].DHCP4).To(BeFalse()) + Expect(devices[0].DHCP6).To(BeFalse()) + + Expect(devices[1].NetworkName).To(Equal("another-new-nw")) + Expect(devices[1].DHCP4).To(BeFalse()) + Expect(devices[1].DHCP6).To(BeTrue()) + Expect(devices[1].Nameservers).To(HaveLen(2)) + Expect(devices[1].Nameservers).To(Equal([]string{"10.10.10.10", "10.10.20.20"})) + + }) + + It("appends the n/w names present in the networks list of the topology", func() { + By("With number of devices in VMSpec < number of networks in the placement constraint") + vm := &infrav1.VSphereVM{ + Spec: infrav1.VSphereVMSpec{ + VirtualMachineCloneSpec: infrav1.VirtualMachineCloneSpec{ + Network: infrav1.NetworkSpec{Devices: []infrav1.NetworkDeviceSpec{{NetworkName: "foo", DHCP4: false}}}, + }, + }, + } + + overrideFunc, ok := vimMachineService.generateOverrideFunc(machineCtx) + Expect(ok).To(BeTrue()) + + overrideFunc(vm) + + devices := vm.Spec.Network.Devices + Expect(devices).To(HaveLen(2)) + Expect(devices[0].NetworkName).To(Equal("newnw-three")) + Expect(devices[0].DHCP4).To(BeFalse()) + Expect(devices[0].DHCP6).To(BeFalse()) + Expect(devices[1].NetworkName).To(Equal("another-new-nw")) + }) + + It("only overrides the n/w names present in the networks list of the topology", func() { + By("With number of devices in VMSpec > number of networks in the placement constraint") + vm := &infrav1.VSphereVM{ + Spec: infrav1.VSphereVMSpec{ + VirtualMachineCloneSpec: infrav1.VirtualMachineCloneSpec{ + Network: infrav1.NetworkSpec{Devices: []infrav1.NetworkDeviceSpec{{NetworkName: "foo", DHCP4: true}, {NetworkName: "bar", DHCP6: true}, {NetworkName: "baz", DHCP6: false}}}, + }, + }, + } + + overrideFunc, ok := vimMachineService.generateOverrideFunc(machineCtx) + Expect(ok).To(BeTrue()) + + overrideFunc(vm) + + devices := vm.Spec.Network.Devices + Expect(devices).To(HaveLen(3)) + Expect(devices[0].NetworkName).To(Equal("newnw-three")) + Expect(devices[0].DHCP4).To(BeFalse()) + + Expect(devices[1].NetworkName).To(Equal("another-new-nw")) + Expect(devices[1].DHCP6).To(BeTrue()) + + Expect(devices[2].NetworkName).To(Equal("baz")) + }) + }) + + Context("with network config and networks specified in the topology", func() { + BeforeEach(func() { + machineCtx.Machine.Spec.FailureDomain = pointer.String("zone-four") + }) + It("overrides the n/w configs using the networkconfig and discarding networks", func() { + By("For equal number of networks") + vm := &infrav1.VSphereVM{ + Spec: infrav1.VSphereVMSpec{ + VirtualMachineCloneSpec: infrav1.VirtualMachineCloneSpec{ + Network: infrav1.NetworkSpec{Devices: []infrav1.NetworkDeviceSpec{{NetworkName: "foo", DHCP4: true}, {NetworkName: "bar", DHCP6: true, Nameservers: []string{"10.50.50.10"}}}}, + }, + }, + } + + overrideFunc, ok := vimMachineService.generateOverrideFunc(machineCtx) + Expect(ok).To(BeTrue()) + + overrideFunc(vm) + + devices := vm.Spec.Network.Devices + Expect(devices).To(HaveLen(2)) + Expect(devices[0].NetworkName).To(Equal("newnw-four")) + Expect(devices[0].DHCP4).To(BeFalse()) + Expect(devices[0].DHCP6).To(BeFalse()) + + Expect(devices[1].NetworkName).To(Equal("another-new-nw")) + Expect(devices[1].DHCP4).To(BeFalse()) + Expect(devices[1].DHCP6).To(BeTrue()) + Expect(devices[1].Nameservers).To(HaveLen(2)) + Expect(devices[1].Nameservers).To(Equal([]string{"10.10.10.10", "10.10.20.20"})) + + }) + }) }) + }) var _ = Describe("VimMachineService_GetHostInfo", func() { From 08aecdefb2185231391a5b438f449530af5ba629 Mon Sep 17 00:00:00 2001 From: Ricardo Katz Date: Fri, 30 Jun 2023 14:09:07 -0300 Subject: [PATCH 03/10] Add generated files --- apis/v1alpha3/zz_generated.conversion.go | 40 ++-- apis/v1alpha4/zz_generated.conversion.go | 40 ++-- apis/v1beta1/zz_generated.deepcopy.go | 59 ++++++ ...ture.cluster.x-k8s.io_vsphereclusters.yaml | 1 - ...luster.x-k8s.io_vspherefailuredomains.yaml | 180 +++++++++++++++++- ...ture.cluster.x-k8s.io_vspheremachines.yaml | 2 +- ...ster.x-k8s.io_vspheremachinetemplates.yaml | 2 +- ...structure.cluster.x-k8s.io_vspherevms.yaml | 4 +- 8 files changed, 297 insertions(+), 31 deletions(-) diff --git a/apis/v1alpha3/zz_generated.conversion.go b/apis/v1alpha3/zz_generated.conversion.go index 87e728ee2a..0fc9b665f1 100644 --- a/apis/v1alpha3/zz_generated.conversion.go +++ b/apis/v1alpha3/zz_generated.conversion.go @@ -150,11 +150,6 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } - if err := s.AddGeneratedConversionFunc((*v1beta1.Topology)(nil), (*Topology)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1beta1_Topology_To_v1alpha3_Topology(a.(*v1beta1.Topology), b.(*Topology), scope) - }); err != nil { - return err - } if err := s.AddGeneratedConversionFunc((*VSphereCluster)(nil), (*v1beta1.VSphereCluster)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1alpha3_VSphereCluster_To_v1beta1_VSphereCluster(a.(*VSphereCluster), b.(*v1beta1.VSphereCluster), scope) }); err != nil { @@ -450,6 +445,11 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } + if err := s.AddConversionFunc((*v1beta1.Topology)(nil), (*Topology)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1beta1_Topology_To_v1alpha3_Topology(a.(*v1beta1.Topology), b.(*Topology), scope) + }); err != nil { + return err + } if err := s.AddConversionFunc((*v1beta1.VSphereClusterSpec)(nil), (*VSphereClusterSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1beta1_VSphereClusterSpec_To_v1alpha3_VSphereClusterSpec(a.(*v1beta1.VSphereClusterSpec), b.(*VSphereClusterSpec), scope) }); err != nil { @@ -784,15 +784,11 @@ func autoConvert_v1beta1_Topology_To_v1alpha3_Topology(in *v1beta1.Topology, out out.ComputeCluster = (*string)(unsafe.Pointer(in.ComputeCluster)) out.Hosts = (*FailureDomainHosts)(unsafe.Pointer(in.Hosts)) out.Networks = *(*[]string)(unsafe.Pointer(&in.Networks)) + // WARNING: in.NetworkConfigs requires manual conversion: does not exist in peer-type out.Datastore = in.Datastore return nil } -// Convert_v1beta1_Topology_To_v1alpha3_Topology is an autogenerated conversion function. -func Convert_v1beta1_Topology_To_v1alpha3_Topology(in *v1beta1.Topology, out *Topology, s conversion.Scope) error { - return autoConvert_v1beta1_Topology_To_v1alpha3_Topology(in, out, s) -} - func autoConvert_v1alpha3_VSphereCluster_To_v1beta1_VSphereCluster(in *VSphereCluster, out *v1beta1.VSphereCluster, s conversion.Scope) error { out.ObjectMeta = in.ObjectMeta if err := Convert_v1alpha3_VSphereClusterSpec_To_v1beta1_VSphereClusterSpec(&in.Spec, &out.Spec, s); err != nil { @@ -1144,7 +1140,17 @@ func Convert_v1beta1_VSphereFailureDomain_To_v1alpha3_VSphereFailureDomain(in *v func autoConvert_v1alpha3_VSphereFailureDomainList_To_v1beta1_VSphereFailureDomainList(in *VSphereFailureDomainList, out *v1beta1.VSphereFailureDomainList, s conversion.Scope) error { out.ListMeta = in.ListMeta - out.Items = *(*[]v1beta1.VSphereFailureDomain)(unsafe.Pointer(&in.Items)) + if in.Items != nil { + in, out := &in.Items, &out.Items + *out = make([]v1beta1.VSphereFailureDomain, len(*in)) + for i := range *in { + if err := Convert_v1alpha3_VSphereFailureDomain_To_v1beta1_VSphereFailureDomain(&(*in)[i], &(*out)[i], s); err != nil { + return err + } + } + } else { + out.Items = nil + } return nil } @@ -1155,7 +1161,17 @@ func Convert_v1alpha3_VSphereFailureDomainList_To_v1beta1_VSphereFailureDomainLi func autoConvert_v1beta1_VSphereFailureDomainList_To_v1alpha3_VSphereFailureDomainList(in *v1beta1.VSphereFailureDomainList, out *VSphereFailureDomainList, s conversion.Scope) error { out.ListMeta = in.ListMeta - out.Items = *(*[]VSphereFailureDomain)(unsafe.Pointer(&in.Items)) + if in.Items != nil { + in, out := &in.Items, &out.Items + *out = make([]VSphereFailureDomain, len(*in)) + for i := range *in { + if err := Convert_v1beta1_VSphereFailureDomain_To_v1alpha3_VSphereFailureDomain(&(*in)[i], &(*out)[i], s); err != nil { + return err + } + } + } else { + out.Items = nil + } return nil } diff --git a/apis/v1alpha4/zz_generated.conversion.go b/apis/v1alpha4/zz_generated.conversion.go index a401c23933..10311c5df4 100644 --- a/apis/v1alpha4/zz_generated.conversion.go +++ b/apis/v1alpha4/zz_generated.conversion.go @@ -150,11 +150,6 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } - if err := s.AddGeneratedConversionFunc((*v1beta1.Topology)(nil), (*Topology)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1beta1_Topology_To_v1alpha4_Topology(a.(*v1beta1.Topology), b.(*Topology), scope) - }); err != nil { - return err - } if err := s.AddGeneratedConversionFunc((*VSphereCluster)(nil), (*v1beta1.VSphereCluster)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1alpha4_VSphereCluster_To_v1beta1_VSphereCluster(a.(*VSphereCluster), b.(*v1beta1.VSphereCluster), scope) }); err != nil { @@ -490,6 +485,11 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } + if err := s.AddConversionFunc((*v1beta1.Topology)(nil), (*Topology)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1beta1_Topology_To_v1alpha4_Topology(a.(*v1beta1.Topology), b.(*Topology), scope) + }); err != nil { + return err + } if err := s.AddConversionFunc((*v1beta1.VSphereClusterSpec)(nil), (*VSphereClusterSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1beta1_VSphereClusterSpec_To_v1alpha4_VSphereClusterSpec(a.(*v1beta1.VSphereClusterSpec), b.(*VSphereClusterSpec), scope) }); err != nil { @@ -824,15 +824,11 @@ func autoConvert_v1beta1_Topology_To_v1alpha4_Topology(in *v1beta1.Topology, out out.ComputeCluster = (*string)(unsafe.Pointer(in.ComputeCluster)) out.Hosts = (*FailureDomainHosts)(unsafe.Pointer(in.Hosts)) out.Networks = *(*[]string)(unsafe.Pointer(&in.Networks)) + // WARNING: in.NetworkConfigs requires manual conversion: does not exist in peer-type out.Datastore = in.Datastore return nil } -// Convert_v1beta1_Topology_To_v1alpha4_Topology is an autogenerated conversion function. -func Convert_v1beta1_Topology_To_v1alpha4_Topology(in *v1beta1.Topology, out *Topology, s conversion.Scope) error { - return autoConvert_v1beta1_Topology_To_v1alpha4_Topology(in, out, s) -} - func autoConvert_v1alpha4_VSphereCluster_To_v1beta1_VSphereCluster(in *VSphereCluster, out *v1beta1.VSphereCluster, s conversion.Scope) error { out.ObjectMeta = in.ObjectMeta if err := Convert_v1alpha4_VSphereClusterSpec_To_v1beta1_VSphereClusterSpec(&in.Spec, &out.Spec, s); err != nil { @@ -1302,7 +1298,17 @@ func Convert_v1beta1_VSphereFailureDomain_To_v1alpha4_VSphereFailureDomain(in *v func autoConvert_v1alpha4_VSphereFailureDomainList_To_v1beta1_VSphereFailureDomainList(in *VSphereFailureDomainList, out *v1beta1.VSphereFailureDomainList, s conversion.Scope) error { out.ListMeta = in.ListMeta - out.Items = *(*[]v1beta1.VSphereFailureDomain)(unsafe.Pointer(&in.Items)) + if in.Items != nil { + in, out := &in.Items, &out.Items + *out = make([]v1beta1.VSphereFailureDomain, len(*in)) + for i := range *in { + if err := Convert_v1alpha4_VSphereFailureDomain_To_v1beta1_VSphereFailureDomain(&(*in)[i], &(*out)[i], s); err != nil { + return err + } + } + } else { + out.Items = nil + } return nil } @@ -1313,7 +1319,17 @@ func Convert_v1alpha4_VSphereFailureDomainList_To_v1beta1_VSphereFailureDomainLi func autoConvert_v1beta1_VSphereFailureDomainList_To_v1alpha4_VSphereFailureDomainList(in *v1beta1.VSphereFailureDomainList, out *VSphereFailureDomainList, s conversion.Scope) error { out.ListMeta = in.ListMeta - out.Items = *(*[]VSphereFailureDomain)(unsafe.Pointer(&in.Items)) + if in.Items != nil { + in, out := &in.Items, &out.Items + *out = make([]VSphereFailureDomain, len(*in)) + for i := range *in { + if err := Convert_v1beta1_VSphereFailureDomain_To_v1alpha4_VSphereFailureDomain(&(*in)[i], &(*out)[i], s); err != nil { + return err + } + } + } else { + out.Items = nil + } return nil } diff --git a/apis/v1beta1/zz_generated.deepcopy.go b/apis/v1beta1/zz_generated.deepcopy.go index 0b1928995f..b775fa84b1 100644 --- a/apis/v1beta1/zz_generated.deepcopy.go +++ b/apis/v1beta1/zz_generated.deepcopy.go @@ -170,6 +170,58 @@ func (in *FailureDomainHosts) DeepCopy() *FailureDomainHosts { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *FailureDomainNetwork) DeepCopyInto(out *FailureDomainNetwork) { + *out = *in + if in.DHCP4 != nil { + in, out := &in.DHCP4, &out.DHCP4 + *out = new(bool) + **out = **in + } + if in.DHCP6 != nil { + in, out := &in.DHCP6, &out.DHCP6 + *out = new(bool) + **out = **in + } + if in.Nameservers != nil { + in, out := &in.Nameservers, &out.Nameservers + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.SearchDomains != nil { + in, out := &in.SearchDomains, &out.SearchDomains + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.DHCP4Overrides != nil { + in, out := &in.DHCP4Overrides, &out.DHCP4Overrides + *out = new(DHCPOverrides) + (*in).DeepCopyInto(*out) + } + if in.DHCP6Overrides != nil { + in, out := &in.DHCP6Overrides, &out.DHCP6Overrides + *out = new(DHCPOverrides) + (*in).DeepCopyInto(*out) + } + if in.AddressesFromPools != nil { + in, out := &in.AddressesFromPools, &out.AddressesFromPools + *out = make([]v1.TypedLocalObjectReference, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new FailureDomainNetwork. +func (in *FailureDomainNetwork) DeepCopy() *FailureDomainNetwork { + if in == nil { + return nil + } + out := new(FailureDomainNetwork) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Network) DeepCopyInto(out *Network) { *out = *in @@ -392,6 +444,13 @@ func (in *Topology) DeepCopyInto(out *Topology) { *out = make([]string, len(*in)) copy(*out, *in) } + if in.NetworkConfigs != nil { + in, out := &in.NetworkConfigs, &out.NetworkConfigs + *out = make([]FailureDomainNetwork, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Topology. diff --git a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vsphereclusters.yaml b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vsphereclusters.yaml index 8d2d9546cf..0f8160c319 100644 --- a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vsphereclusters.yaml +++ b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vsphereclusters.yaml @@ -321,7 +321,6 @@ spec: description: 'UID of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#uids' type: string type: object - x-kubernetes-map-type: atomic server: description: Server is the address of the vSphere endpoint. type: string diff --git a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherefailuredomains.yaml b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherefailuredomains.yaml index 4e894a5c2b..66ec216d63 100644 --- a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherefailuredomains.yaml +++ b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherefailuredomains.yaml @@ -334,9 +334,185 @@ spec: - hostGroupName - vmGroupName type: object + networkConfigs: + description: NetworkConfigs is a list with new network configurations + within this failure domain + items: + description: FailureDomainNetwork defines a network configuration + that should be used when consuming this failure domain. @rkatz + - To be discussed with team, should we just embed NetworkDeviceSpec? + properties: + addressesFromPools: + description: AddressesFromPools is a list of IPAddressPools + that should be assigned to IPAddressClaims. The machine's + cloud-init metadata will be populated with IPAddresses + fulfilled by an IPAM provider. + items: + description: TypedLocalObjectReference contains enough + information to let you locate the typed referenced object + inside the same namespace. + properties: + apiGroup: + description: APIGroup is the group for the resource + being referenced. If APIGroup is not specified, + the specified Kind must be in the core API group. + For any other third-party types, APIGroup is required. + type: string + kind: + description: Kind is the type of resource being referenced + type: string + name: + description: Name is the name of resource being referenced + type: string + required: + - kind + - name + type: object + x-kubernetes-map-type: atomic + type: array + dhcp4: + description: '@rkatz - The reason NetworkDeviceSpec is not + being copied here, is because DHCP4 and DHCP6 are not + pointers there This means that, in a case like "I want + DHCP4 to be enabled on my template" and "I want DHCP4 + to be disabled on this failure domain" we cannot verify + if DHCP4 was unset (nil, we don''t care about it on failure + domain) vs set to false (I want to FORCE it to be false) + DHCP4 is a flag that indicates whether or not to use DHCP + for IPv4' + type: boolean + dhcp4Overrides: + description: DHCP4Overrides allows for the control over + several DHCP behaviors. Overrides will only be applied + when the corresponding DHCP flag is set. Only configured + values will be sent, omitted values will default to distribution + defaults. Dependent on support in the network stack for + your distribution. For more information see the netplan + reference (https://netplan.io/reference#dhcp-overrides) + properties: + hostname: + description: Hostname is the name which will be sent + to the DHCP server instead of the machine's hostname. + type: string + routeMetric: + description: RouteMetric is used to prioritize routes + for devices. A lower metric for an interface will + have a higher priority. + type: integer + sendHostname: + description: SendHostname when `true`, the hostname + of the machine will be sent to the DHCP server. + type: boolean + useDNS: + description: UseDNS when `true`, the DNS servers in + the DHCP server will be used and take precedence. + type: boolean + useDomains: + description: UseDomains can take the values `true`, + `false`, or `route`. When `true`, the domain name + from the DHCP server will be used as the DNS search + domain for this device. When `route`, the domain name + from the DHCP response will be used for routing DNS + only, not for searching. + type: string + useHostname: + description: UseHostname when `true`, the hostname from + the DHCP server will be set as the transient hostname + of the machine. + type: boolean + useMTU: + description: UseMTU when `true`, the MTU from the DHCP + server will be set as the MTU of the device. + type: boolean + useNTP: + description: UseNTP when `true`, the NTP servers from + the DHCP server will be used by systemd-timesyncd + and take precedence. + type: boolean + useRoutes: + description: UseRoutes when `true`, the routes from + the DHCP server will be installed in the routing table. + type: string + type: object + dhcp6: + description: DHCP6 indicates whether or not to use DHCP + for IPv6 + type: boolean + dhcp6Overrides: + description: DHCP6Overrides allows for the control over + several DHCP behaviors. Overrides will only be applied + when the corresponding DHCP flag is set. Only configured + values will be sent, omitted values will default to distribution + defaults. Dependent on support in the network stack for + your distribution. For more information see the netplan + reference (https://netplan.io/reference#dhcp-overrides) + properties: + hostname: + description: Hostname is the name which will be sent + to the DHCP server instead of the machine's hostname. + type: string + routeMetric: + description: RouteMetric is used to prioritize routes + for devices. A lower metric for an interface will + have a higher priority. + type: integer + sendHostname: + description: SendHostname when `true`, the hostname + of the machine will be sent to the DHCP server. + type: boolean + useDNS: + description: UseDNS when `true`, the DNS servers in + the DHCP server will be used and take precedence. + type: boolean + useDomains: + description: UseDomains can take the values `true`, + `false`, or `route`. When `true`, the domain name + from the DHCP server will be used as the DNS search + domain for this device. When `route`, the domain name + from the DHCP response will be used for routing DNS + only, not for searching. + type: string + useHostname: + description: UseHostname when `true`, the hostname from + the DHCP server will be set as the transient hostname + of the machine. + type: boolean + useMTU: + description: UseMTU when `true`, the MTU from the DHCP + server will be set as the MTU of the device. + type: boolean + useNTP: + description: UseNTP when `true`, the NTP servers from + the DHCP server will be used by systemd-timesyncd + and take precedence. + type: boolean + useRoutes: + description: UseRoutes when `true`, the routes from + the DHCP server will be installed in the routing table. + type: string + type: object + name: + description: NetworkName is the network name for this machine's + VM. + type: string + nameservers: + description: Nameservers is a list of IPv4 and/or IPv6 addresses + used as DNS nameservers. Please note that Linux allows + only three nameservers (https://linux.die.net/man/5/resolv.conf). + items: + type: string + type: array + searchDomains: + description: SearchDomains is a list of search domains used + when resolving IP addresses with DNS. + items: + type: string + type: array + type: object + type: array networks: - description: Networks is the list of networks within this failure - domain + description: 'Networks is the list of networks within this failure + domain TODO (@rkatz): Deprecate in favor of NetworkConfigs?' items: type: string type: array diff --git a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachines.yaml b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachines.yaml index 919a85312c..27e72fe0c8 100644 --- a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachines.yaml +++ b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachines.yaml @@ -943,10 +943,10 @@ spec: description: Name is the name of resource being referenced type: string required: - - apiGroup - kind - name type: object + x-kubernetes-map-type: atomic type: array deviceName: description: DeviceName may be used to explicitly assign diff --git a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml index 2172061e52..f4b06af3bb 100644 --- a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml +++ b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml @@ -832,10 +832,10 @@ spec: being referenced type: string required: - - apiGroup - kind - name type: object + x-kubernetes-map-type: atomic type: array deviceName: description: DeviceName may be used to explicitly diff --git a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherevms.yaml b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherevms.yaml index ed8ec68c54..2fb38a4d7f 100644 --- a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherevms.yaml +++ b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherevms.yaml @@ -80,7 +80,6 @@ spec: description: 'UID of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#uids' type: string type: object - x-kubernetes-map-type: atomic cloneMode: description: CloneMode specifies the type of clone operation. The LinkedClone mode is only support for templates that have at least @@ -909,6 +908,7 @@ spec: description: 'UID of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#uids' type: string type: object + x-kubernetes-map-type: atomic cloneMode: description: CloneMode specifies the type of clone operation. The LinkedClone mode is only support for templates that have at least @@ -990,10 +990,10 @@ spec: description: Name is the name of resource being referenced type: string required: - - apiGroup - kind - name type: object + x-kubernetes-map-type: atomic type: array deviceName: description: DeviceName may be used to explicitly assign From 405077c7cee609b1b477a7223c746eeb6fdc4443 Mon Sep 17 00:00:00 2001 From: Ricardo Katz Date: Mon, 3 Jul 2023 10:15:55 -0300 Subject: [PATCH 04/10] Do not change the original fd object Co-authored-by: Christian Schlotter --- pkg/services/vimmachine.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/pkg/services/vimmachine.go b/pkg/services/vimmachine.go index 7fb5e0b19e..8b2b4c641d 100644 --- a/pkg/services/vimmachine.go +++ b/pkg/services/vimmachine.go @@ -497,16 +497,18 @@ func (v *VimMachineService) generateOverrideFunc(ctx *context.VIMMachineContext) vm.Spec.Datastore = vsphereFailureDomain.Spec.Topology.Datastore } - // Some old cases may still use Networks instead of NetworkConfigs, so we should use the old fields - if len(vsphereFailureDomain.Spec.Topology.NetworkConfigs) == 0 && len(vsphereFailureDomain.Spec.Topology.Networks) > 0 { - vsphereFailureDomain.Spec.Topology.NetworkConfigs = make([]infrav1.FailureDomainNetwork, len(vsphereFailureDomain.Spec.Topology.Networks)) + networkConfigs := vsphereFailureDomain.Spec.Topology.NetworkConfigs + // Convert the deprecated vsphereFailureDomain.Spec.Topology to networkConfigs if necessary. + if len(networkConfigs) == 0 && len(vsphereFailureDomain.Spec.Topology.Networks) > 0 { + networkConfigs = make([]infrav1.FailureDomainNetwork, len(vsphereFailureDomain.Spec.Topology.Networks)) for i := range vsphereFailureDomain.Spec.Topology.Networks { - vsphereFailureDomain.Spec.Topology.NetworkConfigs[i].NetworkName = vsphereFailureDomain.Spec.Topology.Networks[i] + networkConfigs[i].NetworkName = vsphereFailureDomain.Spec.Topology.Networks[i] } + } - if len(vsphereFailureDomain.Spec.Topology.NetworkConfigs) > 0 { - vm.Spec.Network.Devices = overrideNetworkDeviceSpecs(vm.Spec.Network.Devices, vsphereFailureDomain.Spec.Topology.NetworkConfigs) + if len(networkConfigs) > 0 { + vm.Spec.Network.Devices = overrideNetworkDeviceSpecs(vm.Spec.Network.Devices, networkConfigs) } } return overrideWithFailureDomainFunc, true From 10a9ab48e6909e89cff3e92d9fdd35cc577fc02a Mon Sep 17 00:00:00 2001 From: Ricardo Katz Date: Mon, 3 Jul 2023 10:45:48 -0300 Subject: [PATCH 05/10] Validate topology network creation --- apis/v1beta1/vspherefailuredomain_webhook.go | 15 +- .../vspherefailuredomain_webhook_test.go | 150 ++++++++++-------- ...ture.cluster.x-k8s.io_vsphereclusters.yaml | 1 + ...luster.x-k8s.io_vspherefailuredomains.yaml | 2 +- ...ture.cluster.x-k8s.io_vspheremachines.yaml | 2 +- ...ster.x-k8s.io_vspheremachinetemplates.yaml | 2 +- ...structure.cluster.x-k8s.io_vspherevms.yaml | 4 +- 7 files changed, 93 insertions(+), 83 deletions(-) diff --git a/apis/v1beta1/vspherefailuredomain_webhook.go b/apis/v1beta1/vspherefailuredomain_webhook.go index 2f3e81fa73..31f7f18fda 100644 --- a/apis/v1beta1/vspherefailuredomain_webhook.go +++ b/apis/v1beta1/vspherefailuredomain_webhook.go @@ -64,6 +64,10 @@ func (r *VSphereFailureDomain) ValidateCreate() error { allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "Topology", "ComputeCluster"), fmt.Sprintf("cannot be nil if zone's Failure Domain type is %s", r.Spec.Zone.Type))) } + if len(r.Spec.Topology.NetworkConfigs) != 0 && len(r.Spec.Topology.Networks) != 0 { + allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "Topology", "Networks"), "cannot be set if spec.Topology.NetworkConfigs is not empty")) + } + return aggregateObjErrors(r.GroupVersionKind().GroupKind(), r.Name, allErrs) } @@ -90,15 +94,4 @@ func (r *VSphereFailureDomain) Default() { if r.Spec.Region.AutoConfigure == nil { r.Spec.Region.AutoConfigure = pointer.Bool(false) } - - // Converts the old Networks field to NetworkConfigs - // TODO (@rkatz) - Can this be harmful for Gitops and other users that check spec and re-apply? Probably it will generate - // a difference between "what is expected" and "what we have" as there's going to be the additional "NetworkConfigs" field - // - if len(r.Spec.Topology.NetworkConfigs) == 0 && len(r.Spec.Topology.Networks) > 0 { - r.Spec.Topology.NetworkConfigs = make([]FailureDomainNetwork, len(r.Spec.Topology.Networks)) - for i := range r.Spec.Topology.Networks { - r.Spec.Topology.NetworkConfigs[i].NetworkName = r.Spec.Topology.Networks[i] - } - } } diff --git a/apis/v1beta1/vspherefailuredomain_webhook_test.go b/apis/v1beta1/vspherefailuredomain_webhook_test.go index 93a8102613..8be5fdc36c 100644 --- a/apis/v1beta1/vspherefailuredomain_webhook_test.go +++ b/apis/v1beta1/vspherefailuredomain_webhook_test.go @@ -65,68 +65,6 @@ func TestVsphereFailureDomain_Default(t *testing.T) { }, }, }, - { - name: "when networkconfigs is null and network field exists", - spec: VSphereFailureDomainSpec{ - Topology: Topology{ - Networks: []string{"network-a", "network-b"}, - }, - }, - expectedSpec: VSphereFailureDomainSpec{ - Zone: FailureDomain{ - AutoConfigure: pointer.Bool(false), - }, - Region: FailureDomain{ - AutoConfigure: pointer.Bool(false), - }, - Topology: Topology{ - Networks: []string{"network-a", "network-b"}, - NetworkConfigs: []FailureDomainNetwork{ - { - NetworkName: "network-a", - }, - { - NetworkName: "network-b", - }, - }, - }, - }, - }, - { - name: "when networkconfigs is not null and network field exists", - spec: VSphereFailureDomainSpec{ - Topology: Topology{ - Networks: []string{"network-a", "network-b"}, - NetworkConfigs: []FailureDomainNetwork{ - { - NetworkName: "network-c", - }, - { - NetworkName: "network-d", - }, - }, - }, - }, - expectedSpec: VSphereFailureDomainSpec{ - Zone: FailureDomain{ - AutoConfigure: pointer.Bool(false), - }, - Region: FailureDomain{ - AutoConfigure: pointer.Bool(false), - }, - Topology: Topology{ - Networks: []string{"network-a", "network-b"}, - NetworkConfigs: []FailureDomainNetwork{ - { - NetworkName: "network-c", - }, - { - NetworkName: "network-d", - }, - }, - }, - }, - }, } g := NewWithT(t) @@ -138,9 +76,6 @@ func TestVsphereFailureDomain_Default(t *testing.T) { m.Default() g.Expect(m.Spec).To(Equal(test.expectedSpec)) } - - //g.Expect(*m.Spec.Zone.AutoConfigure).To(BeFalse()) - //g.Expect(*m.Spec.Region.AutoConfigure).To(BeFalse()) } func TestVSphereFailureDomain_ValidateCreate(t *testing.T) { @@ -148,7 +83,7 @@ func TestVSphereFailureDomain_ValidateCreate(t *testing.T) { tests := []struct { name string - errExpected *bool + errExpected bool failureDomain VSphereFailureDomain }{ { @@ -161,6 +96,7 @@ func TestVSphereFailureDomain_ValidateCreate(t *testing.T) { AutoConfigure: pointer.Bool(true), }, }}, + errExpected: true, }, { name: "hostGroup failureDomain set but compute Cluster is empty", @@ -174,6 +110,7 @@ func TestVSphereFailureDomain_ValidateCreate(t *testing.T) { }, }, }}, + errExpected: true, }, { name: "type of zone failure domain is Hostgroup but topology's hostgroup is not set", @@ -194,6 +131,7 @@ func TestVSphereFailureDomain_ValidateCreate(t *testing.T) { Hosts: nil, }, }}, + errExpected: true, }, { name: "type of region failure domain is Compute Cluster but topology is not set", @@ -217,6 +155,7 @@ func TestVSphereFailureDomain_ValidateCreate(t *testing.T) { }, }, }}, + errExpected: true, }, { name: "type of zone failure domain is Compute Cluster but topology is not set", @@ -236,6 +175,83 @@ func TestVSphereFailureDomain_ValidateCreate(t *testing.T) { ComputeCluster: nil, }, }}, + errExpected: true, + }, + { + name: "valid failure domain configuration should not cause an error", + failureDomain: VSphereFailureDomain{Spec: VSphereFailureDomainSpec{ + Region: FailureDomain{ + Name: "foo", + Type: DatacenterFailureDomain, + TagCategory: "k8s-bar", + }, + Zone: FailureDomain{ + Name: "foo", + Type: ComputeClusterFailureDomain, + TagCategory: "k8s-bar", + }, + Topology: Topology{ + Datacenter: "/blah", + ComputeCluster: pointer.String("Cluster1"), + Networks: []string{"network-a", "network-b"}, + }, + }}, + errExpected: false, + }, + { + name: "valid failure domain configuration with new NetworkConfig should not cause an error", + failureDomain: VSphereFailureDomain{Spec: VSphereFailureDomainSpec{ + Region: FailureDomain{ + Name: "foo", + Type: DatacenterFailureDomain, + TagCategory: "k8s-bar", + }, + Zone: FailureDomain{ + Name: "foo", + Type: ComputeClusterFailureDomain, + TagCategory: "k8s-bar", + }, + Topology: Topology{ + Datacenter: "/blah", + ComputeCluster: pointer.String("Cluster1"), + NetworkConfigs: []FailureDomainNetwork{ + { + NetworkName: "network-a", + DHCP4: pointer.Bool(true), + DHCP6: pointer.Bool(false), + }, + }, + }, + }}, + errExpected: false, + }, + { + name: "failure domain configuration with new NetworkConfig and networks should cause an error", + failureDomain: VSphereFailureDomain{Spec: VSphereFailureDomainSpec{ + Region: FailureDomain{ + Name: "foo", + Type: DatacenterFailureDomain, + TagCategory: "k8s-bar", + }, + Zone: FailureDomain{ + Name: "foo", + Type: ComputeClusterFailureDomain, + TagCategory: "k8s-bar", + }, + Topology: Topology{ + Datacenter: "/blah", + ComputeCluster: pointer.String("Cluster1"), + Networks: []string{"network-a", "network-b"}, + NetworkConfigs: []FailureDomainNetwork{ + { + NetworkName: "network-a", + DHCP4: pointer.Bool(true), + DHCP6: pointer.Bool(false), + }, + }, + }, + }}, + errExpected: true, }, } @@ -244,7 +260,7 @@ func TestVSphereFailureDomain_ValidateCreate(t *testing.T) { tt := tt t.Run(tt.name, func(t *testing.T) { err := tt.failureDomain.ValidateCreate() - if tt.errExpected == nil || !*tt.errExpected { + if tt.errExpected { g.Expect(err).To(HaveOccurred()) } else { g.Expect(err).NotTo(HaveOccurred()) diff --git a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vsphereclusters.yaml b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vsphereclusters.yaml index 0f8160c319..8d2d9546cf 100644 --- a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vsphereclusters.yaml +++ b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vsphereclusters.yaml @@ -321,6 +321,7 @@ spec: description: 'UID of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#uids' type: string type: object + x-kubernetes-map-type: atomic server: description: Server is the address of the vSphere endpoint. type: string diff --git a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherefailuredomains.yaml b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherefailuredomains.yaml index 66ec216d63..ec37adac6c 100644 --- a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherefailuredomains.yaml +++ b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherefailuredomains.yaml @@ -365,10 +365,10 @@ spec: description: Name is the name of resource being referenced type: string required: + - apiGroup - kind - name type: object - x-kubernetes-map-type: atomic type: array dhcp4: description: '@rkatz - The reason NetworkDeviceSpec is not diff --git a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachines.yaml b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachines.yaml index 27e72fe0c8..919a85312c 100644 --- a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachines.yaml +++ b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachines.yaml @@ -943,10 +943,10 @@ spec: description: Name is the name of resource being referenced type: string required: + - apiGroup - kind - name type: object - x-kubernetes-map-type: atomic type: array deviceName: description: DeviceName may be used to explicitly assign diff --git a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml index f4b06af3bb..2172061e52 100644 --- a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml +++ b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml @@ -832,10 +832,10 @@ spec: being referenced type: string required: + - apiGroup - kind - name type: object - x-kubernetes-map-type: atomic type: array deviceName: description: DeviceName may be used to explicitly diff --git a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherevms.yaml b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherevms.yaml index 2fb38a4d7f..ed8ec68c54 100644 --- a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherevms.yaml +++ b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherevms.yaml @@ -80,6 +80,7 @@ spec: description: 'UID of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#uids' type: string type: object + x-kubernetes-map-type: atomic cloneMode: description: CloneMode specifies the type of clone operation. The LinkedClone mode is only support for templates that have at least @@ -908,7 +909,6 @@ spec: description: 'UID of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#uids' type: string type: object - x-kubernetes-map-type: atomic cloneMode: description: CloneMode specifies the type of clone operation. The LinkedClone mode is only support for templates that have at least @@ -990,10 +990,10 @@ spec: description: Name is the name of resource being referenced type: string required: + - apiGroup - kind - name type: object - x-kubernetes-map-type: atomic type: array deviceName: description: DeviceName may be used to explicitly assign From c09ec63d62c5397d4d289c1546075a4b9062fb14 Mon Sep 17 00:00:00 2001 From: Ricardo Katz Date: Mon, 3 Jul 2023 16:03:07 -0300 Subject: [PATCH 06/10] Rename network config API and regenerate --- apis/v1alpha3/topology_conversion.go | 8 +-- apis/v1alpha3/zz_generated.conversion.go | 2 +- apis/v1alpha4/topology_conversion.go | 8 +-- apis/v1alpha4/zz_generated.conversion.go | 2 +- apis/v1beta1/vspherefailuredomain_types.go | 19 ++---- apis/v1beta1/vspherefailuredomain_webhook.go | 2 +- .../vspherefailuredomain_webhook_test.go | 4 +- apis/v1beta1/zz_generated.deepcopy.go | 64 +++++++++---------- ...luster.x-k8s.io_vspherefailuredomains.yaml | 28 +++----- pkg/services/vimmachine.go | 8 +-- pkg/services/vimmachine_test.go | 2 +- 11 files changed, 67 insertions(+), 80 deletions(-) diff --git a/apis/v1alpha3/topology_conversion.go b/apis/v1alpha3/topology_conversion.go index 6de83082fd..6560e2cc37 100644 --- a/apis/v1alpha3/topology_conversion.go +++ b/apis/v1alpha3/topology_conversion.go @@ -22,10 +22,10 @@ import ( ) func Convert_v1beta1_Topology_To_v1alpha3_Topology(in *v1beta1.Topology, out *Topology, s conversion.Scope) error { - if len(in.NetworkConfigs) > 0 { - networks := make([]string, len(in.NetworkConfigs)) - for i := range in.NetworkConfigs { - networks[i] = in.NetworkConfigs[i].NetworkName + if len(in.NetworkConfigurations) > 0 { + networks := make([]string, len(in.NetworkConfigurations)) + for i := range in.NetworkConfigurations { + networks[i] = in.NetworkConfigurations[i].NetworkName } } return nil diff --git a/apis/v1alpha3/zz_generated.conversion.go b/apis/v1alpha3/zz_generated.conversion.go index 0fc9b665f1..fa4ec96357 100644 --- a/apis/v1alpha3/zz_generated.conversion.go +++ b/apis/v1alpha3/zz_generated.conversion.go @@ -784,7 +784,7 @@ func autoConvert_v1beta1_Topology_To_v1alpha3_Topology(in *v1beta1.Topology, out out.ComputeCluster = (*string)(unsafe.Pointer(in.ComputeCluster)) out.Hosts = (*FailureDomainHosts)(unsafe.Pointer(in.Hosts)) out.Networks = *(*[]string)(unsafe.Pointer(&in.Networks)) - // WARNING: in.NetworkConfigs requires manual conversion: does not exist in peer-type + // WARNING: in.NetworkConfigurations requires manual conversion: does not exist in peer-type out.Datastore = in.Datastore return nil } diff --git a/apis/v1alpha4/topology_conversion.go b/apis/v1alpha4/topology_conversion.go index 3662480c41..6244e57bcc 100644 --- a/apis/v1alpha4/topology_conversion.go +++ b/apis/v1alpha4/topology_conversion.go @@ -22,10 +22,10 @@ import ( ) func Convert_v1beta1_Topology_To_v1alpha4_Topology(in *v1beta1.Topology, out *Topology, s conversion.Scope) error { - if len(in.NetworkConfigs) > 0 { - networks := make([]string, len(in.NetworkConfigs)) - for i := range in.NetworkConfigs { - networks[i] = in.NetworkConfigs[i].NetworkName + if len(in.NetworkConfigurations) > 0 { + networks := make([]string, len(in.NetworkConfigurations)) + for i := range in.NetworkConfigurations { + networks[i] = in.NetworkConfigurations[i].NetworkName } } return nil diff --git a/apis/v1alpha4/zz_generated.conversion.go b/apis/v1alpha4/zz_generated.conversion.go index 10311c5df4..b1f72ac734 100644 --- a/apis/v1alpha4/zz_generated.conversion.go +++ b/apis/v1alpha4/zz_generated.conversion.go @@ -824,7 +824,7 @@ func autoConvert_v1beta1_Topology_To_v1alpha4_Topology(in *v1beta1.Topology, out out.ComputeCluster = (*string)(unsafe.Pointer(in.ComputeCluster)) out.Hosts = (*FailureDomainHosts)(unsafe.Pointer(in.Hosts)) out.Networks = *(*[]string)(unsafe.Pointer(&in.Networks)) - // WARNING: in.NetworkConfigs requires manual conversion: does not exist in peer-type + // WARNING: in.NetworkConfigurations requires manual conversion: does not exist in peer-type out.Datastore = in.Datastore return nil } diff --git a/apis/v1beta1/vspherefailuredomain_types.go b/apis/v1beta1/vspherefailuredomain_types.go index 8a586a7d16..697e4c4dcb 100644 --- a/apis/v1beta1/vspherefailuredomain_types.go +++ b/apis/v1beta1/vspherefailuredomain_types.go @@ -72,13 +72,12 @@ type Topology struct { Hosts *FailureDomainHosts `json:"hosts,omitempty"` // Networks is the list of networks within this failure domain - // TODO (@rkatz): Deprecate in favor of NetworkConfigs? // +optional Networks []string `json:"networks,omitempty"` - // NetworkConfigs is a list with new network configurations within this failure domain - // + optional - NetworkConfigs []FailureDomainNetwork `json:"networkConfigs,omitempty"` + // NetworkConfigurations is a list with new network configurations within this failure domain + // +optional + NetworkConfigurations []NetworkConfiguration `json:"networkConfigs,omitempty"` // Datastore is the name or inventory path of the datastore in which the // virtual machine is created/located. @@ -94,21 +93,17 @@ type FailureDomainHosts struct { HostGroupName string `json:"hostGroupName"` } -// FailureDomainNetwork defines a network configuration that should be used when consuming -// this failure domain. -// @rkatz - To be discussed with team, should we just embed NetworkDeviceSpec? -type FailureDomainNetwork struct { +// NetworkConfiguration defines a network configuration that should be used when consuming +// a failure domain. +type NetworkConfiguration struct { // NetworkName is the network name for this machine's VM. NetworkName string `json:"name,omitempty"` - // @rkatz - The reason NetworkDeviceSpec is not being copied here, is because DHCP4 and DHCP6 are not pointers there - // This means that, in a case like "I want DHCP4 to be enabled on my template" and "I want DHCP4 to be disabled on this failure domain" - // we cannot verify if DHCP4 was unset (nil, we don't care about it on failure domain) vs set to false (I want to FORCE it to be false) // DHCP4 is a flag that indicates whether or not to use DHCP for IPv4 // +optional DHCP4 *bool `json:"dhcp4,omitempty"` - // DHCP6 indicates whether or not to use DHCP for IPv6 + // DHCP6 is a flag that indicates whether or not to use DHCP for IPv6 // +optional DHCP6 *bool `json:"dhcp6,omitempty"` diff --git a/apis/v1beta1/vspherefailuredomain_webhook.go b/apis/v1beta1/vspherefailuredomain_webhook.go index 31f7f18fda..20d75818ff 100644 --- a/apis/v1beta1/vspherefailuredomain_webhook.go +++ b/apis/v1beta1/vspherefailuredomain_webhook.go @@ -64,7 +64,7 @@ func (r *VSphereFailureDomain) ValidateCreate() error { allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "Topology", "ComputeCluster"), fmt.Sprintf("cannot be nil if zone's Failure Domain type is %s", r.Spec.Zone.Type))) } - if len(r.Spec.Topology.NetworkConfigs) != 0 && len(r.Spec.Topology.Networks) != 0 { + if len(r.Spec.Topology.NetworkConfigurations) != 0 && len(r.Spec.Topology.Networks) != 0 { allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "Topology", "Networks"), "cannot be set if spec.Topology.NetworkConfigs is not empty")) } diff --git a/apis/v1beta1/vspherefailuredomain_webhook_test.go b/apis/v1beta1/vspherefailuredomain_webhook_test.go index 8be5fdc36c..8661a71ac9 100644 --- a/apis/v1beta1/vspherefailuredomain_webhook_test.go +++ b/apis/v1beta1/vspherefailuredomain_webhook_test.go @@ -214,7 +214,7 @@ func TestVSphereFailureDomain_ValidateCreate(t *testing.T) { Topology: Topology{ Datacenter: "/blah", ComputeCluster: pointer.String("Cluster1"), - NetworkConfigs: []FailureDomainNetwork{ + NetworkConfigurations: []NetworkConfiguration{ { NetworkName: "network-a", DHCP4: pointer.Bool(true), @@ -242,7 +242,7 @@ func TestVSphereFailureDomain_ValidateCreate(t *testing.T) { Datacenter: "/blah", ComputeCluster: pointer.String("Cluster1"), Networks: []string{"network-a", "network-b"}, - NetworkConfigs: []FailureDomainNetwork{ + NetworkConfigurations: []NetworkConfiguration{ { NetworkName: "network-a", DHCP4: pointer.Bool(true), diff --git a/apis/v1beta1/zz_generated.deepcopy.go b/apis/v1beta1/zz_generated.deepcopy.go index b775fa84b1..a545521e98 100644 --- a/apis/v1beta1/zz_generated.deepcopy.go +++ b/apis/v1beta1/zz_generated.deepcopy.go @@ -171,7 +171,32 @@ func (in *FailureDomainHosts) DeepCopy() *FailureDomainHosts { } // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *FailureDomainNetwork) DeepCopyInto(out *FailureDomainNetwork) { +func (in *Network) DeepCopyInto(out *Network) { + *out = *in + if in.DHCP4 != nil { + in, out := &in.DHCP4, &out.DHCP4 + *out = new(bool) + **out = **in + } + if in.DHCP6 != nil { + in, out := &in.DHCP6, &out.DHCP6 + *out = new(bool) + **out = **in + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Network. +func (in *Network) DeepCopy() *Network { + if in == nil { + return nil + } + out := new(Network) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *NetworkConfiguration) DeepCopyInto(out *NetworkConfiguration) { *out = *in if in.DHCP4 != nil { in, out := &in.DHCP4, &out.DHCP4 @@ -212,37 +237,12 @@ func (in *FailureDomainNetwork) DeepCopyInto(out *FailureDomainNetwork) { } } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new FailureDomainNetwork. -func (in *FailureDomainNetwork) DeepCopy() *FailureDomainNetwork { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new NetworkConfiguration. +func (in *NetworkConfiguration) DeepCopy() *NetworkConfiguration { if in == nil { return nil } - out := new(FailureDomainNetwork) - in.DeepCopyInto(out) - return out -} - -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *Network) DeepCopyInto(out *Network) { - *out = *in - if in.DHCP4 != nil { - in, out := &in.DHCP4, &out.DHCP4 - *out = new(bool) - **out = **in - } - if in.DHCP6 != nil { - in, out := &in.DHCP6, &out.DHCP6 - *out = new(bool) - **out = **in - } -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Network. -func (in *Network) DeepCopy() *Network { - if in == nil { - return nil - } - out := new(Network) + out := new(NetworkConfiguration) in.DeepCopyInto(out) return out } @@ -444,9 +444,9 @@ func (in *Topology) DeepCopyInto(out *Topology) { *out = make([]string, len(*in)) copy(*out, *in) } - if in.NetworkConfigs != nil { - in, out := &in.NetworkConfigs, &out.NetworkConfigs - *out = make([]FailureDomainNetwork, len(*in)) + if in.NetworkConfigurations != nil { + in, out := &in.NetworkConfigurations, &out.NetworkConfigurations + *out = make([]NetworkConfiguration, len(*in)) for i := range *in { (*in)[i].DeepCopyInto(&(*out)[i]) } diff --git a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherefailuredomains.yaml b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherefailuredomains.yaml index ec37adac6c..79730b55ce 100644 --- a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherefailuredomains.yaml +++ b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherefailuredomains.yaml @@ -335,12 +335,11 @@ spec: - vmGroupName type: object networkConfigs: - description: NetworkConfigs is a list with new network configurations - within this failure domain + description: NetworkConfigurations is a list with new network + configurations within this failure domain items: - description: FailureDomainNetwork defines a network configuration - that should be used when consuming this failure domain. @rkatz - - To be discussed with team, should we just embed NetworkDeviceSpec? + description: NetworkConfiguration defines a network configuration + that should be used when consuming a failure domain. properties: addressesFromPools: description: AddressesFromPools is a list of IPAddressPools @@ -371,15 +370,8 @@ spec: type: object type: array dhcp4: - description: '@rkatz - The reason NetworkDeviceSpec is not - being copied here, is because DHCP4 and DHCP6 are not - pointers there This means that, in a case like "I want - DHCP4 to be enabled on my template" and "I want DHCP4 - to be disabled on this failure domain" we cannot verify - if DHCP4 was unset (nil, we don''t care about it on failure - domain) vs set to false (I want to FORCE it to be false) - DHCP4 is a flag that indicates whether or not to use DHCP - for IPv4' + description: DHCP4 is a flag that indicates whether or not + to use DHCP for IPv4 type: boolean dhcp4Overrides: description: DHCP4Overrides allows for the control over @@ -435,8 +427,8 @@ spec: type: string type: object dhcp6: - description: DHCP6 indicates whether or not to use DHCP - for IPv6 + description: DHCP6 is a flag that indicates whether or not + to use DHCP for IPv6 type: boolean dhcp6Overrides: description: DHCP6Overrides allows for the control over @@ -511,8 +503,8 @@ spec: type: object type: array networks: - description: 'Networks is the list of networks within this failure - domain TODO (@rkatz): Deprecate in favor of NetworkConfigs?' + description: Networks is the list of networks within this failure + domain items: type: string type: array diff --git a/pkg/services/vimmachine.go b/pkg/services/vimmachine.go index 8b2b4c641d..4f3e7ed2da 100644 --- a/pkg/services/vimmachine.go +++ b/pkg/services/vimmachine.go @@ -497,10 +497,10 @@ func (v *VimMachineService) generateOverrideFunc(ctx *context.VIMMachineContext) vm.Spec.Datastore = vsphereFailureDomain.Spec.Topology.Datastore } - networkConfigs := vsphereFailureDomain.Spec.Topology.NetworkConfigs + networkConfigs := vsphereFailureDomain.Spec.Topology.NetworkConfigurations // Convert the deprecated vsphereFailureDomain.Spec.Topology to networkConfigs if necessary. if len(networkConfigs) == 0 && len(vsphereFailureDomain.Spec.Topology.Networks) > 0 { - networkConfigs = make([]infrav1.FailureDomainNetwork, len(vsphereFailureDomain.Spec.Topology.Networks)) + networkConfigs = make([]infrav1.NetworkConfiguration, len(vsphereFailureDomain.Spec.Topology.Networks)) for i := range vsphereFailureDomain.Spec.Topology.Networks { networkConfigs[i].NetworkName = vsphereFailureDomain.Spec.Topology.Networks[i] } @@ -518,7 +518,7 @@ func (v *VimMachineService) generateOverrideFunc(ctx *context.VIMMachineContext) // The substitution is done based on the order in which the network devices have been defined. // // In case there are more network definitions than the number of network devices specified, the definitions are appended to the list. -func overrideNetworkDeviceSpecs(deviceSpecs []infrav1.NetworkDeviceSpec, networks []infrav1.FailureDomainNetwork) []infrav1.NetworkDeviceSpec { +func overrideNetworkDeviceSpecs(deviceSpecs []infrav1.NetworkDeviceSpec, networks []infrav1.NetworkConfiguration) []infrav1.NetworkDeviceSpec { index, length := 0, len(networks) devices := make([]infrav1.NetworkDeviceSpec, 0, integer.IntMax(length, len(deviceSpecs))) @@ -541,7 +541,7 @@ func overrideNetworkDeviceSpecs(deviceSpecs []infrav1.NetworkDeviceSpec, network return devices } -func mergeFailureDomainNetSpecToNetworkDeviceSpec(fd infrav1.FailureDomainNetwork, vmNetworkDeviceSpec *infrav1.NetworkDeviceSpec) { +func mergeFailureDomainNetSpecToNetworkDeviceSpec(fd infrav1.NetworkConfiguration, vmNetworkDeviceSpec *infrav1.NetworkDeviceSpec) { // We don't convert if destination is null if vmNetworkDeviceSpec == nil { return diff --git a/pkg/services/vimmachine_test.go b/pkg/services/vimmachine_test.go index 499d496477..d04fc8ad4c 100644 --- a/pkg/services/vimmachine_test.go +++ b/pkg/services/vimmachine_test.go @@ -72,7 +72,7 @@ var _ = Describe("VimMachineService_GenerateOverrideFunc", func() { Datacenter: fmt.Sprintf("dc-%s", suffix), Datastore: fmt.Sprintf("ds-%s", suffix), Networks: networks, - NetworkConfigs: []infrav1.FailureDomainNetwork{ + NetworkConfigurations: []infrav1.NetworkConfiguration{ { NetworkName: fmt.Sprintf("newnw-%s", suffix), DHCP4: pointer.Bool(false), From 93d304ed7ae904440d64362c659497f63eb40e11 Mon Sep 17 00:00:00 2001 From: Ricardo Katz Date: Mon, 3 Jul 2023 16:36:04 -0300 Subject: [PATCH 07/10] Verify also networkConfig portgroup name --- controllers/vspheredeploymentzone_controller_domain.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/controllers/vspheredeploymentzone_controller_domain.go b/controllers/vspheredeploymentzone_controller_domain.go index 5437825b63..44a53044c2 100644 --- a/controllers/vspheredeploymentzone_controller_domain.go +++ b/controllers/vspheredeploymentzone_controller_domain.go @@ -84,6 +84,16 @@ func (r vsphereDeploymentZoneReconciler) reconcileTopology(ctx *context.VSphereD } } + for _, networkConfig := range topology.NetworkConfigurations { + if networkConfig.NetworkName == "" { + continue + } + if _, err := ctx.AuthSession.Finder.Network(ctx, networkConfig.NetworkName); err != nil { + conditions.MarkFalse(ctx.VSphereDeploymentZone, infrav1.VSphereFailureDomainValidatedCondition, infrav1.NetworkNotFoundReason, clusterv1.ConditionSeverityError, "network %s is misconfigured", networkConfig.NetworkName) + return errors.Wrapf(err, "unable to find network %s", networkConfig.NetworkName) + } + } + if hostPlacementInfo := topology.Hosts; hostPlacementInfo != nil { rule, err := cluster.VerifyAffinityRule(ctx, *topology.ComputeCluster, hostPlacementInfo.HostGroupName, hostPlacementInfo.VMGroupName) switch { From 6ede1056866a947048e85a6cb4c1174a0b456aed Mon Sep 17 00:00:00 2001 From: Ricardo Katz Date: Mon, 3 Jul 2023 18:27:54 -0300 Subject: [PATCH 08/10] Add tests for failure domains on controllers --- ...ture.cluster.x-k8s.io_vsphereclusters.yaml | 1 - ...luster.x-k8s.io_vspherefailuredomains.yaml | 2 +- ...ture.cluster.x-k8s.io_vspheremachines.yaml | 2 +- ...ster.x-k8s.io_vspheremachinetemplates.yaml | 2 +- ...structure.cluster.x-k8s.io_vspherevms.yaml | 4 +- .../vspheredeploymentzone_controller_test.go | 139 ++++++++++++++++++ 6 files changed, 144 insertions(+), 6 deletions(-) diff --git a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vsphereclusters.yaml b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vsphereclusters.yaml index 8d2d9546cf..0f8160c319 100644 --- a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vsphereclusters.yaml +++ b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vsphereclusters.yaml @@ -321,7 +321,6 @@ spec: description: 'UID of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#uids' type: string type: object - x-kubernetes-map-type: atomic server: description: Server is the address of the vSphere endpoint. type: string diff --git a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherefailuredomains.yaml b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherefailuredomains.yaml index 79730b55ce..081b2dad59 100644 --- a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherefailuredomains.yaml +++ b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherefailuredomains.yaml @@ -364,10 +364,10 @@ spec: description: Name is the name of resource being referenced type: string required: - - apiGroup - kind - name type: object + x-kubernetes-map-type: atomic type: array dhcp4: description: DHCP4 is a flag that indicates whether or not diff --git a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachines.yaml b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachines.yaml index 919a85312c..27e72fe0c8 100644 --- a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachines.yaml +++ b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachines.yaml @@ -943,10 +943,10 @@ spec: description: Name is the name of resource being referenced type: string required: - - apiGroup - kind - name type: object + x-kubernetes-map-type: atomic type: array deviceName: description: DeviceName may be used to explicitly assign diff --git a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml index 2172061e52..f4b06af3bb 100644 --- a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml +++ b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml @@ -832,10 +832,10 @@ spec: being referenced type: string required: - - apiGroup - kind - name type: object + x-kubernetes-map-type: atomic type: array deviceName: description: DeviceName may be used to explicitly diff --git a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherevms.yaml b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherevms.yaml index ed8ec68c54..2fb38a4d7f 100644 --- a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherevms.yaml +++ b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherevms.yaml @@ -80,7 +80,6 @@ spec: description: 'UID of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#uids' type: string type: object - x-kubernetes-map-type: atomic cloneMode: description: CloneMode specifies the type of clone operation. The LinkedClone mode is only support for templates that have at least @@ -909,6 +908,7 @@ spec: description: 'UID of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#uids' type: string type: object + x-kubernetes-map-type: atomic cloneMode: description: CloneMode specifies the type of clone operation. The LinkedClone mode is only support for templates that have at least @@ -990,10 +990,10 @@ spec: description: Name is the name of resource being referenced type: string required: - - apiGroup - kind - name type: object + x-kubernetes-map-type: atomic type: array deviceName: description: DeviceName may be used to explicitly assign diff --git a/controllers/vspheredeploymentzone_controller_test.go b/controllers/vspheredeploymentzone_controller_test.go index baf7ed1918..26a4616b05 100644 --- a/controllers/vspheredeploymentzone_controller_test.go +++ b/controllers/vspheredeploymentzone_controller_test.go @@ -408,6 +408,145 @@ func TestVSphereDeploymentZone_Reconcile(t *testing.T) { g.Expect(ownerRefs[0].Kind).To(Equal("VSphereDeploymentZone")) }) + t.Run("should fail to reconcile a deployment zone with wrong networkconfig", func(t *testing.T) { + g := NewWithT(t) + + vsphereFailureDomain := &infrav1.VSphereFailureDomain{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "blah-fd-", + }, + Spec: infrav1.VSphereFailureDomainSpec{ + Region: infrav1.FailureDomain{ + Name: "k8s-region-west", + Type: infrav1.DatacenterFailureDomain, + TagCategory: "k8s-region", + AutoConfigure: pointer.Bool(false), + }, + Zone: infrav1.FailureDomain{ + Name: "k8s-zone-west-1", + Type: infrav1.ComputeClusterFailureDomain, + TagCategory: "k8s-zone", + AutoConfigure: pointer.Bool(false), + }, + Topology: infrav1.Topology{ + Datacenter: "DC0", + ComputeCluster: pointer.String("DC0_C0"), + Datastore: "LocalDS_0", + NetworkConfigurations: []infrav1.NetworkConfiguration{ + { + NetworkName: "VM Networkxpto", + DHCP4: pointer.Bool(true), + }, + }, + }, + }, + } + g.Expect(testEnv.Create(ctx, vsphereFailureDomain)).To(Succeed()) + + vsphereDeploymentZone := &infrav1.VSphereDeploymentZone{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "blah-", + }, + Spec: infrav1.VSphereDeploymentZoneSpec{ + Server: simr.ServerURL().Host, + FailureDomain: vsphereFailureDomain.Name, + ControlPlane: pointer.Bool(true), + PlacementConstraint: infrav1.PlacementConstraint{ + ResourcePool: "DC0_C0_RP1", + Folder: "/", + }, + }} + g.Expect(testEnv.Create(ctx, vsphereDeploymentZone)).To(Succeed()) + + deploymentZoneKey := client.ObjectKey{Name: vsphereDeploymentZone.Name} + g.Eventually(func() bool { + if err := testEnv.Get(ctx, deploymentZoneKey, vsphereDeploymentZone); err != nil { + return false + } + return conditions.IsFalse(vsphereDeploymentZone, infrav1.VSphereFailureDomainValidatedCondition) && + conditions.GetReason(vsphereDeploymentZone, infrav1.VSphereFailureDomainValidatedCondition) == infrav1.NetworkNotFoundReason + }, timeout).Should(BeTrue()) + + }) + + t.Run("should create a deployment zone & failure domain with networkconfig", func(t *testing.T) { + g := NewWithT(t) + + vsphereFailureDomain := &infrav1.VSphereFailureDomain{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "blah-fd-", + }, + Spec: infrav1.VSphereFailureDomainSpec{ + Region: infrav1.FailureDomain{ + Name: "k8s-region-west", + Type: infrav1.DatacenterFailureDomain, + TagCategory: "k8s-region", + AutoConfigure: pointer.Bool(false), + }, + Zone: infrav1.FailureDomain{ + Name: "k8s-zone-west-1", + Type: infrav1.ComputeClusterFailureDomain, + TagCategory: "k8s-zone", + AutoConfigure: pointer.Bool(false), + }, + Topology: infrav1.Topology{ + Datacenter: "DC0", + ComputeCluster: pointer.String("DC0_C0"), + Datastore: "LocalDS_0", + NetworkConfigurations: []infrav1.NetworkConfiguration{ + { + NetworkName: "VM Network", + DHCP4: pointer.Bool(true), + }, + }, + }, + }, + } + g.Expect(testEnv.Create(ctx, vsphereFailureDomain)).To(Succeed()) + + vsphereDeploymentZone := &infrav1.VSphereDeploymentZone{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "blah-", + }, + Spec: infrav1.VSphereDeploymentZoneSpec{ + Server: simr.ServerURL().Host, + FailureDomain: vsphereFailureDomain.Name, + ControlPlane: pointer.Bool(true), + PlacementConstraint: infrav1.PlacementConstraint{ + ResourcePool: "DC0_C0_RP1", + Folder: "/", + }, + }} + g.Expect(testEnv.Create(ctx, vsphereDeploymentZone)).To(Succeed()) + + defer func(do ...client.Object) { + g.Expect(testEnv.Cleanup(ctx, do...)).To(Succeed()) + }(vsphereDeploymentZone, vsphereFailureDomain) + + g.Eventually(func() bool { + if err := testEnv.Get(ctx, client.ObjectKeyFromObject(vsphereDeploymentZone), vsphereDeploymentZone); err != nil { + return false + } + return len(vsphereDeploymentZone.Finalizers) > 0 + }, timeout).Should(BeTrue()) + + g.Eventually(func() bool { + if err := testEnv.Get(ctx, client.ObjectKeyFromObject(vsphereDeploymentZone), vsphereDeploymentZone); err != nil { + return false + } + return conditions.IsTrue(vsphereDeploymentZone, infrav1.VCenterAvailableCondition) && + conditions.IsTrue(vsphereDeploymentZone, infrav1.PlacementConstraintMetCondition) && + conditions.IsTrue(vsphereDeploymentZone, infrav1.VSphereFailureDomainValidatedCondition) + }, timeout).Should(BeTrue()) + + By("sets the owner ref on the vsphereFailureDomain object") + g.Expect(testEnv.Get(ctx, client.ObjectKeyFromObject(vsphereFailureDomain), vsphereFailureDomain)).To(Succeed()) + ownerRefs := vsphereFailureDomain.GetOwnerReferences() + g.Expect(ownerRefs).To(HaveLen(1)) + g.Expect(ownerRefs[0].Name).To(Equal(vsphereDeploymentZone.Name)) + g.Expect(ownerRefs[0].Kind).To(Equal("VSphereDeploymentZone")) + }) + By("deleting deployment zone") t.Run("it should delete associated failure domain", func(t *testing.T) { g := NewWithT(t) From 3b311f53cae0cb917d503964e59be1c239de4bf3 Mon Sep 17 00:00:00 2001 From: Ricardo Katz Date: Mon, 3 Jul 2023 18:41:47 -0300 Subject: [PATCH 09/10] Fix linter errors --- apis/v1beta1/vspherefailuredomain_webhook_test.go | 1 - controllers/vspheredeploymentzone_controller_test.go | 1 - pkg/services/vimmachine.go | 1 - 3 files changed, 3 deletions(-) diff --git a/apis/v1beta1/vspherefailuredomain_webhook_test.go b/apis/v1beta1/vspherefailuredomain_webhook_test.go index 8661a71ac9..0d0d210f9f 100644 --- a/apis/v1beta1/vspherefailuredomain_webhook_test.go +++ b/apis/v1beta1/vspherefailuredomain_webhook_test.go @@ -23,7 +23,6 @@ import ( "k8s.io/utils/pointer" ) -// nolint func TestVsphereFailureDomain_Default(t *testing.T) { tests := []struct { name string diff --git a/controllers/vspheredeploymentzone_controller_test.go b/controllers/vspheredeploymentzone_controller_test.go index 26a4616b05..22564052b2 100644 --- a/controllers/vspheredeploymentzone_controller_test.go +++ b/controllers/vspheredeploymentzone_controller_test.go @@ -466,7 +466,6 @@ func TestVSphereDeploymentZone_Reconcile(t *testing.T) { return conditions.IsFalse(vsphereDeploymentZone, infrav1.VSphereFailureDomainValidatedCondition) && conditions.GetReason(vsphereDeploymentZone, infrav1.VSphereFailureDomainValidatedCondition) == infrav1.NetworkNotFoundReason }, timeout).Should(BeTrue()) - }) t.Run("should create a deployment zone & failure domain with networkconfig", func(t *testing.T) { diff --git a/pkg/services/vimmachine.go b/pkg/services/vimmachine.go index 4f3e7ed2da..92b65284d2 100644 --- a/pkg/services/vimmachine.go +++ b/pkg/services/vimmachine.go @@ -504,7 +504,6 @@ func (v *VimMachineService) generateOverrideFunc(ctx *context.VIMMachineContext) for i := range vsphereFailureDomain.Spec.Topology.Networks { networkConfigs[i].NetworkName = vsphereFailureDomain.Spec.Topology.Networks[i] } - } if len(networkConfigs) > 0 { From 4c1451a77fe65dbd7f94991bea758c25795c2ab3 Mon Sep 17 00:00:00 2001 From: Ricardo Katz Date: Tue, 4 Jul 2023 09:57:01 -0300 Subject: [PATCH 10/10] Improve webhook error message Co-authored-by: Christian Schlotter --- apis/v1beta1/vspherefailuredomain_webhook.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apis/v1beta1/vspherefailuredomain_webhook.go b/apis/v1beta1/vspherefailuredomain_webhook.go index 20d75818ff..de6f30d58f 100644 --- a/apis/v1beta1/vspherefailuredomain_webhook.go +++ b/apis/v1beta1/vspherefailuredomain_webhook.go @@ -65,7 +65,7 @@ func (r *VSphereFailureDomain) ValidateCreate() error { } if len(r.Spec.Topology.NetworkConfigurations) != 0 && len(r.Spec.Topology.Networks) != 0 { - allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "Topology", "Networks"), "cannot be set if spec.Topology.NetworkConfigs is not empty")) + allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "Topology", "Networks"), "cannot be set if spec.Topology.NetworkConfigs is already set")) } return aggregateObjErrors(r.GroupVersionKind().GroupKind(), r.Name, allErrs)