Address reviews

ionos-cloud · Jan 23, 2024 · 2ca1686 · 2ca1686
1 parent bd8eb4f
commit 2ca1686
Show file tree

Hide file tree

Showing 16 changed files with 102 additions and 82 deletions.
diff --git a/api/v1alpha1/helpers.go b/api/v1alpha1/helpers.go
@@ -1,5 +1,5 @@
 /*
-Copyright 2023 IONOS Cloud.
+Copyright 2024 IONOS Cloud.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -22,7 +22,7 @@ import (
 )
 
 // SetInClusterIPPoolRef will set the reference to the provided InClusterIPPool.
-// If nil was provided, the status field will be cleared.
+// If nil was provided or object is empty, the status field will be cleared.
 func (c *ProxmoxCluster) SetInClusterIPPoolRef(pool metav1.Object) {
 	if pool == nil || pool.GetName() == "" {
 		c.Status.InClusterIPPoolRef = nil
@@ -62,10 +62,6 @@ func (c *ProxmoxCluster) AddNodeLocation(loc NodeLocation, isControlPlane bool)
 func (c *ProxmoxCluster) RemoveNodeLocation(machineName string, isControlPlane bool) {
 	nodeLocations := c.Status.NodeLocations
 
-	if nodeLocations == nil {
-		return
-	}
-
 	if !c.HasMachine(machineName, isControlPlane) {
 		return
 	}
@@ -119,7 +115,7 @@ func (c *ProxmoxCluster) UpdateNodeLocation(machineName, node string, isControlP
 	return false
 }
 
-// HasMachine returns if true if a machine was found on any node.
+// HasMachine returns true if a machine was found on any node.
 func (c *ProxmoxCluster) HasMachine(machineName string, isControlPlane bool) bool {
 	return c.GetNode(machineName, isControlPlane) != ""
 }
@@ -155,3 +151,17 @@ func (c *ProxmoxCluster) addNodeLocation(loc NodeLocation, isControlPlane bool)
 
 	c.Status.NodeLocations.Workers = append(c.Status.NodeLocations.Workers, loc)
 }
+
+// DHCPEnabled returns whether DHCP is enabled.
+func (c ClusterNetworkConfig) DHCPEnabled() bool {
+	switch {
+	case (c.IPv6Config != nil && c.IPv6Config.DHCP) && (c.IPv4Config != nil && c.IPv4Config.DHCP):
+		return true
+	case (c.IPv6Config != nil && c.IPv6Config.DHCP) && c.IPv4Config == nil:
+		return true
+	case (c.IPv4Config != nil && c.IPv4Config.DHCP) && c.IPv6Config == nil:
+		return true
+	default:
+		return false
+	}
+}
diff --git a/api/v1alpha1/proxmoxcluster_types.go b/api/v1alpha1/proxmoxcluster_types.go
@@ -74,6 +74,7 @@ type ClusterNetworkConfig struct {
 type IPConfig struct {
 	// Addresses is a list of IP addresses that can be assigned. This set of
 	// addresses can be non-contiguous.
+	// mutually exclusive with DHCP.
 	// +kubebuilder:validation:MinItems=1
 	Addresses []string `json:"addresses,omitempty"`
 
@@ -86,8 +87,10 @@ type IPConfig struct {
 	// +optional
 	Gateway string `json:"gateway,omitempty"`
 
-	// DHCP indicates if DHCP should be used to assign IP addresses.
+	// DHCP indicates that if DHCP should be used to assign IP addresses.
+	// mutually exclusive with Addresses.
 	// +optional
+	// +kubebuilder:default=false
 	DHCP bool `json:"dhcp,omitempty"`
 }
 

diff --git a/api/v1alpha1/proxmoxcluster_types_test.go b/api/v1alpha1/proxmoxcluster_types_test.go
@@ -212,3 +212,23 @@ func TestSetInClusterIPPoolRef(t *testing.T) {
 	cl.SetInClusterIPPoolRef(pool)
 	require.Equal(t, cl.Status.InClusterIPPoolRef[0].Name, pool.GetName())
 }
+
+func TestClusterNetworkConfig_DHCPEnabled(t *testing.T) {
+	cl := defaultCluster()
+	require.False(t, cl.Spec.ClusterNetworkConfig.DHCPEnabled())
+
+	cl.Spec.ClusterNetworkConfig.IPv4Config.DHCP = true
+	require.True(t, cl.Spec.ClusterNetworkConfig.DHCPEnabled())
+
+	cl.Spec.ClusterNetworkConfig.IPv4Config.DHCP = true
+	cl.Spec.ClusterNetworkConfig.IPv6Config = &IPConfig{DHCP: true}
+	require.True(t, cl.Spec.ClusterNetworkConfig.DHCPEnabled())
+
+	cl.Spec.ClusterNetworkConfig.IPv4Config = nil
+	cl.Spec.ClusterNetworkConfig.IPv6Config = &IPConfig{DHCP: true}
+	require.True(t, cl.Spec.ClusterNetworkConfig.DHCPEnabled())
+
+	cl.Spec.ClusterNetworkConfig.IPv4Config = &IPConfig{DHCP: true}
+	cl.Spec.ClusterNetworkConfig.IPv6Config = nil
+	require.True(t, cl.Spec.ClusterNetworkConfig.DHCPEnabled())
+}
diff --git a/api/v1alpha1/proxmoxmachine_types.go b/api/v1alpha1/proxmoxmachine_types.go
@@ -224,21 +224,21 @@ type NetworkDevice struct {
 	// +kubebuilder:validation:Maximum=65520
 	MTU *uint16 `json:"mtu,omitempty"`
 
-	// DHCP4 indicates if DHCP should be used to assign IPv4 addresses.
-	// DHCP4 enforces cluster.spec.ipv4Config to use DHCP.
+	// DHCP4 indicates that if DHCP should be used to assign IPv4 addresses.
+	// DHCP4 enforce device to use DHCP despite the config set in cluster.spec.ipv4Config.
 	// +optional
 	DHCP4 bool `json:"dhcp4,omitempty"`
 
-	// DHCP6 indicates if DHCP should be used to assign IPv6 addresses.
-	// DHCP6 enforces cluster.spec.ipv6Config to use DHCP.
+	// DHCP6 indicates that if DHCP should be used to assign IPv6 addresses.
+	// DHCP6 enforce device to use DHCP despite the config set in cluster.spec.ipv6Config.
 	// +optional
 	DHCP6 bool `json:"dhcp6,omitempty"`
 }
 
 // AdditionalNetworkDevice the definition of a Proxmox network device.
 // +kubebuilder:validation:XValidation:rule="(self.ipv4PoolRef != null || self.ipv6PoolRef != null || self.dhcp4 || self.dhcp6)",message="at least dhcp and/or one pool reference must be set, either ipv4PoolRef or ipv6PoolRef"
 type AdditionalNetworkDevice struct {
-	*NetworkDevice `json:",inline"`
+	NetworkDevice `json:",inline"`
 
 	// Name is the network device name.
 	// must be unique within the virtual machine and different from the primary device 'net0'.

diff --git a/api/v1alpha1/proxmoxmachine_types_test.go b/api/v1alpha1/proxmoxmachine_types_test.go
@@ -109,7 +109,7 @@ var _ = Describe("ProxmoxMachine Test", func() {
 					Bridge: "vmbr0",
 				},
 				AdditionalDevices: []AdditionalNetworkDevice{{
-					NetworkDevice: &NetworkDevice{},
+					NetworkDevice: NetworkDevice{},
 					Name:          "net0",
 					IPv4PoolRef: &corev1.TypedLocalObjectReference{
 						APIGroup: ptr.To("ipam.cluster.x-k8s.io"),
@@ -127,7 +127,7 @@ var _ = Describe("ProxmoxMachine Test", func() {
 			dm := defaultMachine()
 			dm.Spec.Network = &NetworkSpec{
 				AdditionalDevices: []AdditionalNetworkDevice{{
-					NetworkDevice: &NetworkDevice{},
+					NetworkDevice: NetworkDevice{},
 					Name:          "net1",
 					IPv4PoolRef: &corev1.TypedLocalObjectReference{
 						APIGroup: ptr.To("apps"),
@@ -143,7 +143,7 @@ var _ = Describe("ProxmoxMachine Test", func() {
 			dm := defaultMachine()
 			dm.Spec.Network = &NetworkSpec{
 				AdditionalDevices: []AdditionalNetworkDevice{{
-					NetworkDevice: &NetworkDevice{},
+					NetworkDevice: NetworkDevice{},
 					Name:          "net1",
 					IPv4PoolRef: &corev1.TypedLocalObjectReference{
 						APIGroup: ptr.To("ipam.cluster.x-k8s.io"),
@@ -160,7 +160,7 @@ var _ = Describe("ProxmoxMachine Test", func() {
 			dm := defaultMachine()
 			dm.Spec.Network = &NetworkSpec{
 				AdditionalDevices: []AdditionalNetworkDevice{{
-					NetworkDevice: &NetworkDevice{},
+					NetworkDevice: NetworkDevice{},
 					Name:          "net1",
 					IPv6PoolRef: &corev1.TypedLocalObjectReference{
 						APIGroup: ptr.To("apps"),
@@ -176,7 +176,7 @@ var _ = Describe("ProxmoxMachine Test", func() {
 			dm := defaultMachine()
 			dm.Spec.Network = &NetworkSpec{
 				AdditionalDevices: []AdditionalNetworkDevice{{
-					NetworkDevice: &NetworkDevice{},
+					NetworkDevice: NetworkDevice{},
 					Name:          "net1",
 					IPv6PoolRef: &corev1.TypedLocalObjectReference{
 						APIGroup: ptr.To("ipam.cluster.x-k8s.io"),
@@ -193,7 +193,7 @@ var _ = Describe("ProxmoxMachine Test", func() {
 			dm := defaultMachine()
 			dm.Spec.Network = &NetworkSpec{
 				AdditionalDevices: []AdditionalNetworkDevice{{
-					NetworkDevice: &NetworkDevice{},
+					NetworkDevice: NetworkDevice{},
 					Name:          "net1",
 				},
 				},

diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go
diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_proxmoxclusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_proxmoxclusters.yaml
@@ -86,14 +86,16 @@ spec:
                 properties:
                   addresses:
                     description: Addresses is a list of IP addresses that can be assigned.
-                      This set of addresses can be non-contiguous.
+                      This set of addresses can be non-contiguous. mutually exclusive
+                      with DHCP.
                     items:
                       type: string
                     minItems: 1
                     type: array
                   dhcp:
-                    description: DHCP indicates if DHCP should be used to assign IP
-                      addresses.
+                    default: false
+                    description: DHCP indicates that if DHCP should be used to assign
+                      IP addresses. mutually exclusive with Addresses.
                     type: boolean
                   gateway:
                     description: Gateway
@@ -110,14 +112,16 @@ spec:
                 properties:
                   addresses:
                     description: Addresses is a list of IP addresses that can be assigned.
-                      This set of addresses can be non-contiguous.
+                      This set of addresses can be non-contiguous. mutually exclusive
+                      with DHCP.
                     items:
                       type: string
                     minItems: 1
                     type: array
                   dhcp:
-                    description: DHCP indicates if DHCP should be used to assign IP
-                      addresses.
+                    default: false
+                    description: DHCP indicates that if DHCP should be used to assign
+                      IP addresses. mutually exclusive with Addresses.
                     type: boolean
                   gateway:
                     description: Gateway

diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_proxmoxmachines.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_proxmoxmachines.yaml
@@ -129,14 +129,14 @@ spec:
                           minLength: 1
                           type: string
                         dhcp4:
-                          description: DHCP4 indicates if DHCP should be used to assign
-                            IPv4 addresses. DHCP4 enforces cluster.spec.ipv4Config
-                            to use DHCP.
+                          description: DHCP4 indicates that if DHCP should be used
+                            to assign IPv4 addresses. DHCP4 enforce device to use
+                            DHCP despite the config set in cluster.spec.ipv4Config.
                           type: boolean
                         dhcp6:
-                          description: DHCP6 indicates if DHCP should be used to assign
-                            IPv6 addresses. DHCP6 enforces cluster.spec.ipv6Config
-                            to use DHCP.
+                          description: DHCP6 indicates that if DHCP should be used
+                            to assign IPv6 addresses. DHCP6 enforce device to use
+                            DHCP despite the config set in cluster.spec.ipv6Config.
                           type: boolean
                         dnsServers:
                           description: DNSServers contains information about nameservers
@@ -254,14 +254,14 @@ spec:
                         minLength: 1
                         type: string
                       dhcp4:
-                        description: DHCP4 indicates if DHCP should be used to assign
-                          IPv4 addresses. DHCP4 enforces cluster.spec.ipv4Config to
-                          use DHCP.
+                        description: DHCP4 indicates that if DHCP should be used to
+                          assign IPv4 addresses. DHCP4 enforce device to use DHCP
+                          despite the config set in cluster.spec.ipv4Config.
                         type: boolean
                       dhcp6:
-                        description: DHCP6 indicates if DHCP should be used to assign
-                          IPv6 addresses. DHCP6 enforces cluster.spec.ipv6Config to
-                          use DHCP.
+                        description: DHCP6 indicates that if DHCP should be used to
+                          assign IPv6 addresses. DHCP6 enforce device to use DHCP
+                          despite the config set in cluster.spec.ipv6Config.
                         type: boolean
                       model:
                         default: virtio

diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_proxmoxmachinetemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_proxmoxmachinetemplates.yaml
@@ -136,14 +136,14 @@ spec:
                                   minLength: 1
                                   type: string
                                 dhcp4:
-                                  description: DHCP4 indicates if DHCP should be used
-                                    to assign IPv4 addresses. DHCP4 enforces cluster.spec.ipv4Config
-                                    to use DHCP.
+                                  description: DHCP4 indicates that if DHCP should
+                                    be used to assign IPv4 addresses. DHCP4 enforce
+                                    device to use DHCP despite the config set in cluster.spec.ipv4Config.
                                   type: boolean
                                 dhcp6:
-                                  description: DHCP6 indicates if DHCP should be used
-                                    to assign IPv6 addresses. DHCP6 enforces cluster.spec.ipv6Config
-                                    to use DHCP.
+                                  description: DHCP6 indicates that if DHCP should
+                                    be used to assign IPv6 addresses. DHCP6 enforce
+                                    device to use DHCP despite the config set in cluster.spec.ipv6Config.
                                   type: boolean
                                 dnsServers:
                                   description: DNSServers contains information about
@@ -273,14 +273,14 @@ spec:
                                 minLength: 1
                                 type: string
                               dhcp4:
-                                description: DHCP4 indicates if DHCP should be used
-                                  to assign IPv4 addresses. DHCP4 enforces cluster.spec.ipv4Config
-                                  to use DHCP.
+                                description: DHCP4 indicates that if DHCP should be
+                                  used to assign IPv4 addresses. DHCP4 enforce device
+                                  to use DHCP despite the config set in cluster.spec.ipv4Config.
                                 type: boolean
                               dhcp6:
-                                description: DHCP6 indicates if DHCP should be used
-                                  to assign IPv6 addresses. DHCP6 enforces cluster.spec.ipv6Config
-                                  to use DHCP.
+                                description: DHCP6 indicates that if DHCP should be
+                                  used to assign IPv6 addresses. DHCP6 enforce device
+                                  to use DHCP despite the config set in cluster.spec.ipv6Config.
                                 type: boolean
                               model:
                                 default: virtio

diff --git a/internal/controller/proxmoxcluster_controller.go b/internal/controller/proxmoxcluster_controller.go
@@ -168,7 +168,7 @@ func (r *ProxmoxClusterReconciler) reconcileNormal(ctx context.Context, clusterS
 	// If the ProxmoxCluster doesn't have our finalizer, add it.
 	ctrlutil.AddFinalizer(clusterScope.ProxmoxCluster, infrav1alpha1.ClusterFinalizer)
 
-	if !dhcpEnabled(clusterScope.ProxmoxCluster) {
+	if !clusterScope.ProxmoxCluster.Spec.ClusterNetworkConfig.DHCPEnabled() {
 		res, err := r.reconcileIPAM(ctx, clusterScope)
 		if err != nil {
 			return ctrl.Result{}, err
@@ -244,16 +244,3 @@ func (r *ProxmoxClusterReconciler) SetupWithManager(ctx context.Context, mgr ctr
 			builder.WithPredicates(predicates.ClusterUnpaused(ctrl.LoggerFrom(ctx)))).
 		Complete(r)
 }
-
-func dhcpEnabled(cluster *infrav1alpha1.ProxmoxCluster) bool {
-	switch {
-	case (cluster.Spec.IPv6Config != nil && cluster.Spec.IPv6Config.DHCP) && (cluster.Spec.IPv4Config != nil && cluster.Spec.IPv4Config.DHCP):
-		return true
-	case (cluster.Spec.IPv6Config != nil && cluster.Spec.IPv6Config.DHCP) && cluster.Spec.IPv4Config == nil:
-		return true
-	case (cluster.Spec.IPv4Config != nil && cluster.Spec.IPv4Config.DHCP) && cluster.Spec.IPv6Config == nil:
-		return true
-	default:
-		return false
-	}
-}
diff --git a/internal/service/vmservice/bootstrap.go b/internal/service/vmservice/bootstrap.go
@@ -260,7 +260,7 @@ func getAdditionalNetworkDevices(ctx context.Context, machineScope *scope.Machin
 			device := fmt.Sprintf("%s-%s", nic.Name, infrav1alpha1.DefaultSuffix)
 			conf, err := getNetworkConfigDataForDevice(ctx,
 				machineScope,
-				nic.NetworkDevice,
+				&nic.NetworkDevice,
 				device,
 				infrav1alpha1.IPV4Format)
 			if err != nil {
@@ -277,7 +277,7 @@ func getAdditionalNetworkDevices(ctx context.Context, machineScope *scope.Machin
 			device := fmt.Sprintf("%s-%s", nic.Name, suffix)
 			conf, err := getNetworkConfigDataForDevice(ctx,
 				machineScope,
-				nic.NetworkDevice,
+				&nic.NetworkDevice,
 				device,
 				infrav1alpha1.IPV6Format)
 			if err != nil {

diff --git a/internal/service/vmservice/bootstrap_test.go b/internal/service/vmservice/bootstrap_test.go
@@ -82,7 +82,7 @@ func TestReconcileBootstrapData_UpdateStatus(t *testing.T) {
 	machineScope.ProxmoxMachine.Spec.Network = &infrav1.NetworkSpec{
 		AdditionalDevices: []infrav1.AdditionalNetworkDevice{
 			{
-				NetworkDevice: &infrav1.NetworkDevice{Bridge: "vmbr1", Model: ptr.To("virtio")},
+				NetworkDevice: infrav1.NetworkDevice{Bridge: "vmbr1", Model: ptr.To("virtio")},
 				Name:          "net1",
 				DNSServers:    []string{"1.2.3.4"},
 			},
@@ -173,7 +173,7 @@ func TestReconcileBootstrapData_DualStack_AdditionalDevices(t *testing.T) {
 	machineScope.ProxmoxMachine.Spec.Network = &infrav1.NetworkSpec{
 		AdditionalDevices: []infrav1.AdditionalNetworkDevice{
 			{
-				NetworkDevice: &infrav1.NetworkDevice{Bridge: "vmbr1", Model: ptr.To("virtio")},
+				NetworkDevice: infrav1.NetworkDevice{Bridge: "vmbr1", Model: ptr.To("virtio")},
 				Name:          "net1",
 				DNSServers:    []string{"1.2.3.4"},
 				IPv6PoolRef: &corev1.TypedLocalObjectReference{