Skip to content

Commit d6ecf9e

Browse files
mcbenjemaamcbenjemaa
committed
Address reviews
1 parent 401b91e commit d6ecf9e

16 files changed

+103
-83
lines changed

‎api/v1alpha1/helpers.go‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
Copyright 2023 IONOS Cloud.
2+
Copyright 2024 IONOS Cloud.
33
44
Licensed under the Apache License, Version 2.0 (the "License");
55
you may not use this file except in compliance with the License.
@@ -22,7 +22,7 @@ import (
2222
)
2323

2424
// SetInClusterIPPoolRef will set the reference to the provided InClusterIPPool.
25-
// If nil was provided, the status field will be cleared.
25+
// If nil was provided or object is empty, the status field will be cleared.
2626
func (c *ProxmoxCluster) SetInClusterIPPoolRef(pool metav1.Object) {
2727
if pool == nil || pool.GetName() == "" {
2828
c.Status.InClusterIPPoolRef = nil
@@ -62,10 +62,6 @@ func (c *ProxmoxCluster) AddNodeLocation(loc NodeLocation, isControlPlane bool)
6262
func (c *ProxmoxCluster) RemoveNodeLocation(machineName string, isControlPlane bool) {
6363
nodeLocations := c.Status.NodeLocations
6464

65-
if nodeLocations == nil {
66-
return
67-
}
68-
6965
if !c.HasMachine(machineName, isControlPlane) {
7066
return
7167
}
@@ -119,7 +115,7 @@ func (c *ProxmoxCluster) UpdateNodeLocation(machineName, node string, isControlP
119115
return false
120116
}
121117

122-
// HasMachine returns if true if a machine was found on any node.
118+
// HasMachine returns true if a machine was found on any node.
123119
func (c *ProxmoxCluster) HasMachine(machineName string, isControlPlane bool) bool {
124120
return c.GetNode(machineName, isControlPlane) != ""
125121
}
@@ -155,3 +151,17 @@ func (c *ProxmoxCluster) addNodeLocation(loc NodeLocation, isControlPlane bool)
155151

156152
c.Status.NodeLocations.Workers = append(c.Status.NodeLocations.Workers, loc)
157153
}
154+
155+
// DHCPEnabled returns whether DHCP is enabled.
156+
func (c ClusterNetworkConfig) DHCPEnabled() bool {
157+
switch {
158+
case (c.IPv6Config != nil && c.IPv6Config.DHCP) && (c.IPv4Config != nil && c.IPv4Config.DHCP):
159+
return true
160+
case (c.IPv6Config != nil && c.IPv6Config.DHCP) && c.IPv4Config == nil:
161+
return true
162+
case (c.IPv4Config != nil && c.IPv4Config.DHCP) && c.IPv6Config == nil:
163+
return true
164+
default:
165+
return false
166+
}
167+
}

‎api/v1alpha1/proxmoxcluster_types.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ type ClusterNetworkConfig struct {
7474
type IPConfig struct {
7575
// Addresses is a list of IP addresses that can be assigned. This set of
7676
// addresses can be non-contiguous.
77+
// mutually exclusive with DHCP.
7778
// +kubebuilder:validation:MinItems=1
7879
Addresses []string `json:"addresses,omitempty"`
7980

@@ -86,8 +87,10 @@ type IPConfig struct {
8687
// +optional
8788
Gateway string `json:"gateway,omitempty"`
8889

89-
// DHCP indicates if DHCP should be used to assign IP addresses.
90+
// DHCP indicates that if DHCP should be used to assign IP addresses.
91+
// mutually exclusive with Addresses.
9092
// +optional
93+
// +kubebuilder:default=false
9194
DHCP bool `json:"dhcp,omitempty"`
9295
}
9396

‎api/v1alpha1/proxmoxcluster_types_test.go‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,3 +212,23 @@ func TestSetInClusterIPPoolRef(t *testing.T) {
212212
cl.SetInClusterIPPoolRef(pool)
213213
require.Equal(t, cl.Status.InClusterIPPoolRef[0].Name, pool.GetName())
214214
}
215+
216+
func TestClusterNetworkConfig_DHCPEnabled(t *testing.T) {
217+
cl := defaultCluster()
218+
require.False(t, cl.Spec.ClusterNetworkConfig.DHCPEnabled())
219+
220+
cl.Spec.ClusterNetworkConfig.IPv4Config.DHCP = true
221+
require.True(t, cl.Spec.ClusterNetworkConfig.DHCPEnabled())
222+
223+
cl.Spec.ClusterNetworkConfig.IPv4Config.DHCP = true
224+
cl.Spec.ClusterNetworkConfig.IPv6Config = &IPConfig{DHCP: true}
225+
require.True(t, cl.Spec.ClusterNetworkConfig.DHCPEnabled())
226+
227+
cl.Spec.ClusterNetworkConfig.IPv4Config = nil
228+
cl.Spec.ClusterNetworkConfig.IPv6Config = &IPConfig{DHCP: true}
229+
require.True(t, cl.Spec.ClusterNetworkConfig.DHCPEnabled())
230+
231+
cl.Spec.ClusterNetworkConfig.IPv4Config = &IPConfig{DHCP: true}
232+
cl.Spec.ClusterNetworkConfig.IPv6Config = nil
233+
require.True(t, cl.Spec.ClusterNetworkConfig.DHCPEnabled())
234+
}

‎api/v1alpha1/proxmoxmachine_types.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -224,21 +224,21 @@ type NetworkDevice struct {
224224
// +kubebuilder:validation:Maximum=65520
225225
MTU *uint16 `json:"mtu,omitempty"`
226226

227-
// DHCP4 indicates if DHCP should be used to assign IPv4 addresses.
228-
// DHCP4 enforces cluster.spec.ipv4Config to use DHCP.
227+
// DHCP4 indicates that if DHCP should be used to assign IPv4 addresses.
228+
// DHCP4 enforce device to use DHCP despite the config set in cluster.spec.ipv4Config.
229229
// +optional
230230
DHCP4 bool `json:"dhcp4,omitempty"`
231231

232-
// DHCP6 indicates if DHCP should be used to assign IPv6 addresses.
233-
// DHCP6 enforces cluster.spec.ipv6Config to use DHCP.
232+
// DHCP6 indicates that if DHCP should be used to assign IPv6 addresses.
233+
// DHCP6 enforce device to use DHCP despite the config set in cluster.spec.ipv6Config.
234234
// +optional
235235
DHCP6 bool `json:"dhcp6,omitempty"`
236236
}
237237

238238
// AdditionalNetworkDevice the definition of a Proxmox network device.
239239
// +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"
240240
type AdditionalNetworkDevice struct {
241-
*NetworkDevice `json:",inline"`
241+
NetworkDevice `json:",inline"`
242242

243243
// Name is the network device name.
244244
// must be unique within the virtual machine and different from the primary device 'net0'.

‎api/v1alpha1/proxmoxmachine_types_test.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ var _ = Describe("ProxmoxMachine Test", func() {
109109
Bridge: "vmbr0",
110110
},
111111
AdditionalDevices: []AdditionalNetworkDevice{{
112-
NetworkDevice: &NetworkDevice{},
112+
NetworkDevice: NetworkDevice{},
113113
Name: "net0",
114114
IPv4PoolRef: &corev1.TypedLocalObjectReference{
115115
APIGroup: ptr.To("ipam.cluster.x-k8s.io"),
@@ -127,7 +127,7 @@ var _ = Describe("ProxmoxMachine Test", func() {
127127
dm := defaultMachine()
128128
dm.Spec.Network = &NetworkSpec{
129129
AdditionalDevices: []AdditionalNetworkDevice{{
130-
NetworkDevice: &NetworkDevice{},
130+
NetworkDevice: NetworkDevice{},
131131
Name: "net1",
132132
IPv4PoolRef: &corev1.TypedLocalObjectReference{
133133
APIGroup: ptr.To("apps"),
@@ -143,7 +143,7 @@ var _ = Describe("ProxmoxMachine Test", func() {
143143
dm := defaultMachine()
144144
dm.Spec.Network = &NetworkSpec{
145145
AdditionalDevices: []AdditionalNetworkDevice{{
146-
NetworkDevice: &NetworkDevice{},
146+
NetworkDevice: NetworkDevice{},
147147
Name: "net1",
148148
IPv4PoolRef: &corev1.TypedLocalObjectReference{
149149
APIGroup: ptr.To("ipam.cluster.x-k8s.io"),
@@ -160,7 +160,7 @@ var _ = Describe("ProxmoxMachine Test", func() {
160160
dm := defaultMachine()
161161
dm.Spec.Network = &NetworkSpec{
162162
AdditionalDevices: []AdditionalNetworkDevice{{
163-
NetworkDevice: &NetworkDevice{},
163+
NetworkDevice: NetworkDevice{},
164164
Name: "net1",
165165
IPv6PoolRef: &corev1.TypedLocalObjectReference{
166166
APIGroup: ptr.To("apps"),
@@ -176,7 +176,7 @@ var _ = Describe("ProxmoxMachine Test", func() {
176176
dm := defaultMachine()
177177
dm.Spec.Network = &NetworkSpec{
178178
AdditionalDevices: []AdditionalNetworkDevice{{
179-
NetworkDevice: &NetworkDevice{},
179+
NetworkDevice: NetworkDevice{},
180180
Name: "net1",
181181
IPv6PoolRef: &corev1.TypedLocalObjectReference{
182182
APIGroup: ptr.To("ipam.cluster.x-k8s.io"),
@@ -193,7 +193,7 @@ var _ = Describe("ProxmoxMachine Test", func() {
193193
dm := defaultMachine()
194194
dm.Spec.Network = &NetworkSpec{
195195
AdditionalDevices: []AdditionalNetworkDevice{{
196-
NetworkDevice: &NetworkDevice{},
196+
NetworkDevice: NetworkDevice{},
197197
Name: "net1",
198198
},
199199
},

‎api/v1alpha1/zz_generated.deepcopy.go‎

Lines changed: 2 additions & 6 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎config/crd/bases/infrastructure.cluster.x-k8s.io_proxmoxclusters.yaml‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -86,14 +86,16 @@ spec:
8686
properties:
8787
addresses:
8888
description: Addresses is a list of IP addresses that can be assigned.
89-
This set of addresses can be non-contiguous.
89+
This set of addresses can be non-contiguous. mutually exclusive
90+
with DHCP.
9091
items:
9192
type: string
9293
minItems: 1
9394
type: array
9495
dhcp:
95-
description: DHCP indicates if DHCP should be used to assign IP
96-
addresses.
96+
default: false
97+
description: DHCP indicates that if DHCP should be used to assign
98+
IP addresses. mutually exclusive with Addresses.
9799
type: boolean
98100
gateway:
99101
description: Gateway
@@ -110,14 +112,16 @@ spec:
110112
properties:
111113
addresses:
112114
description: Addresses is a list of IP addresses that can be assigned.
113-
This set of addresses can be non-contiguous.
115+
This set of addresses can be non-contiguous. mutually exclusive
116+
with DHCP.
114117
items:
115118
type: string
116119
minItems: 1
117120
type: array
118121
dhcp:
119-
description: DHCP indicates if DHCP should be used to assign IP
120-
addresses.
122+
default: false
123+
description: DHCP indicates that if DHCP should be used to assign
124+
IP addresses. mutually exclusive with Addresses.
121125
type: boolean
122126
gateway:
123127
description: Gateway

‎config/crd/bases/infrastructure.cluster.x-k8s.io_proxmoxmachines.yaml‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -129,14 +129,14 @@ spec:
129129
minLength: 1
130130
type: string
131131
dhcp4:
132-
description: DHCP4 indicates if DHCP should be used to assign
133-
IPv4 addresses. DHCP4 enforces cluster.spec.ipv4Config
134-
to use DHCP.
132+
description: DHCP4 indicates that if DHCP should be used
133+
to assign IPv4 addresses. DHCP4 enforce device to use
134+
DHCP despite the config set in cluster.spec.ipv4Config.
135135
type: boolean
136136
dhcp6:
137-
description: DHCP6 indicates if DHCP should be used to assign
138-
IPv6 addresses. DHCP6 enforces cluster.spec.ipv6Config
139-
to use DHCP.
137+
description: DHCP6 indicates that if DHCP should be used
138+
to assign IPv6 addresses. DHCP6 enforce device to use
139+
DHCP despite the config set in cluster.spec.ipv6Config.
140140
type: boolean
141141
dnsServers:
142142
description: DNSServers contains information about nameservers
@@ -254,14 +254,14 @@ spec:
254254
minLength: 1
255255
type: string
256256
dhcp4:
257-
description: DHCP4 indicates if DHCP should be used to assign
258-
IPv4 addresses. DHCP4 enforces cluster.spec.ipv4Config to
259-
use DHCP.
257+
description: DHCP4 indicates that if DHCP should be used to
258+
assign IPv4 addresses. DHCP4 enforce device to use DHCP
259+
despite the config set in cluster.spec.ipv4Config.
260260
type: boolean
261261
dhcp6:
262-
description: DHCP6 indicates if DHCP should be used to assign
263-
IPv6 addresses. DHCP6 enforces cluster.spec.ipv6Config to
264-
use DHCP.
262+
description: DHCP6 indicates that if DHCP should be used to
263+
assign IPv6 addresses. DHCP6 enforce device to use DHCP
264+
despite the config set in cluster.spec.ipv6Config.
265265
type: boolean
266266
model:
267267
default: virtio

‎config/crd/bases/infrastructure.cluster.x-k8s.io_proxmoxmachinetemplates.yaml‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -136,14 +136,14 @@ spec:
136136
minLength: 1
137137
type: string
138138
dhcp4:
139-
description: DHCP4 indicates if DHCP should be used
140-
to assign IPv4 addresses. DHCP4 enforces cluster.spec.ipv4Config
141-
to use DHCP.
139+
description: DHCP4 indicates that if DHCP should
140+
be used to assign IPv4 addresses. DHCP4 enforce
141+
device to use DHCP despite the config set in cluster.spec.ipv4Config.
142142
type: boolean
143143
dhcp6:
144-
description: DHCP6 indicates if DHCP should be used
145-
to assign IPv6 addresses. DHCP6 enforces cluster.spec.ipv6Config
146-
to use DHCP.
144+
description: DHCP6 indicates that if DHCP should
145+
be used to assign IPv6 addresses. DHCP6 enforce
146+
device to use DHCP despite the config set in cluster.spec.ipv6Config.
147147
type: boolean
148148
dnsServers:
149149
description: DNSServers contains information about
@@ -273,14 +273,14 @@ spec:
273273
minLength: 1
274274
type: string
275275
dhcp4:
276-
description: DHCP4 indicates if DHCP should be used
277-
to assign IPv4 addresses. DHCP4 enforces cluster.spec.ipv4Config
278-
to use DHCP.
276+
description: DHCP4 indicates that if DHCP should be
277+
used to assign IPv4 addresses. DHCP4 enforce device
278+
to use DHCP despite the config set in cluster.spec.ipv4Config.
279279
type: boolean
280280
dhcp6:
281-
description: DHCP6 indicates if DHCP should be used
282-
to assign IPv6 addresses. DHCP6 enforces cluster.spec.ipv6Config
283-
to use DHCP.
281+
description: DHCP6 indicates that if DHCP should be
282+
used to assign IPv6 addresses. DHCP6 enforce device
283+
to use DHCP despite the config set in cluster.spec.ipv6Config.
284284
type: boolean
285285
model:
286286
default: virtio

‎internal/controller/proxmoxcluster_controller.go‎

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ func (r *ProxmoxClusterReconciler) reconcileNormal(ctx context.Context, clusterS
168168
// If the ProxmoxCluster doesn't have our finalizer, add it.
169169
ctrlutil.AddFinalizer(clusterScope.ProxmoxCluster, infrav1alpha1.ClusterFinalizer)
170170

171-
if !dhcpEnabled(clusterScope.ProxmoxCluster) {
171+
if !clusterScope.ProxmoxCluster.Spec.ClusterNetworkConfig.DHCPEnabled() {
172172
res, err := r.reconcileIPAM(ctx, clusterScope)
173173
if err != nil {
174174
return ctrl.Result{}, err
@@ -244,16 +244,3 @@ func (r *ProxmoxClusterReconciler) SetupWithManager(ctx context.Context, mgr ctr
244244
builder.WithPredicates(predicates.ClusterUnpaused(ctrl.LoggerFrom(ctx)))).
245245
Complete(r)
246246
}
247-
248-
func dhcpEnabled(cluster *infrav1alpha1.ProxmoxCluster) bool {
249-
switch {
250-
case (cluster.Spec.IPv6Config != nil && cluster.Spec.IPv6Config.DHCP) && (cluster.Spec.IPv4Config != nil && cluster.Spec.IPv4Config.DHCP):
251-
return true
252-
case (cluster.Spec.IPv6Config != nil && cluster.Spec.IPv6Config.DHCP) && cluster.Spec.IPv4Config == nil:
253-
return true
254-
case (cluster.Spec.IPv4Config != nil && cluster.Spec.IPv4Config.DHCP) && cluster.Spec.IPv6Config == nil:
255-
return true
256-
default:
257-
return false
258-
}
259-
}

0 commit comments

Comments
 (0)