Skip to content

Commit 7b36136

Browse files
johannesfreyjohannesfrey
committed
Review suggestions 2
1 parent 028f798 commit 7b36136

File tree

5 files changed

+51
-44
lines changed

5 files changed

+51
-44
lines changed

‎api/v1beta1/hetznercluster_webhook.go

Lines changed: 44 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -86,20 +86,22 @@ func (r *HetznerCluster) Default(_ context.Context, obj runtime.Object) error {
8686
return apierrors.NewBadRequest(fmt.Sprintf("expected an HetznerCluster but got a %T", obj))
8787
}
8888

89-
if cluster.Spec.HCloudNetwork.Enabled {
90-
if cluster.Spec.HCloudNetwork.ID != nil {
91-
return nil
92-
}
89+
if !cluster.Spec.HCloudNetwork.Enabled {
90+
return nil
91+
}
9392

94-
if cluster.Spec.HCloudNetwork.CIDRBlock == nil {
95-
cluster.Spec.HCloudNetwork.CIDRBlock = ptr.To(DefaultCIDRBlock)
96-
}
97-
if cluster.Spec.HCloudNetwork.SubnetCIDRBlock == nil {
98-
cluster.Spec.HCloudNetwork.SubnetCIDRBlock = ptr.To(DefaultSubnetCIDRBlock)
99-
}
100-
if cluster.Spec.HCloudNetwork.NetworkZone == nil {
101-
cluster.Spec.HCloudNetwork.NetworkZone = ptr.To[HCloudNetworkZone](DefaultNetworkZone)
102-
}
93+
if cluster.Spec.HCloudNetwork.ID != nil {
94+
return nil
95+
}
96+
97+
if cluster.Spec.HCloudNetwork.CIDRBlock == nil {
98+
cluster.Spec.HCloudNetwork.CIDRBlock = ptr.To(DefaultCIDRBlock)
99+
}
100+
if cluster.Spec.HCloudNetwork.SubnetCIDRBlock == nil {
101+
cluster.Spec.HCloudNetwork.SubnetCIDRBlock = ptr.To(DefaultSubnetCIDRBlock)
102+
}
103+
if cluster.Spec.HCloudNetwork.NetworkZone == nil {
104+
cluster.Spec.HCloudNetwork.NetworkZone = ptr.To[HCloudNetworkZone](DefaultNetworkZone)
103105
}
104106

105107
return nil
@@ -250,12 +252,24 @@ func (r *HetznerCluster) ValidateUpdate(old runtime.Object) (admission.Warnings,
250252
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected an HetznerCluster but got a %T", old))
251253
}
252254

253-
if !reflect.DeepEqual(oldC.Spec.HCloudNetwork.Enabled, r.Spec.HCloudNetwork.Enabled) {
255+
if oldC.Spec.HCloudNetwork.Enabled != r.Spec.HCloudNetwork.Enabled {
254256
allErrs = append(allErrs,
255257
field.Invalid(field.NewPath("spec", "hcloudNetwork", "enabled"), r.Spec.HCloudNetwork.Enabled, "field is immutable"),
256258
)
257259
}
258260

261+
if !oldC.Spec.HCloudNetwork.Enabled {
262+
// If the network is disabled check that all other network related fields are empty.
263+
if r.Spec.HCloudNetwork.ID != nil {
264+
allErrs = append(allErrs,
265+
field.Invalid(field.NewPath("spec", "hcloudNetwork", "id"), oldC.Spec.HCloudNetwork.ID, "field must be empty"),
266+
)
267+
}
268+
if errs := areCIDRsAndNetworkZoneEmpty(r.Spec.HCloudNetwork); errs != nil {
269+
allErrs = append(allErrs, errs...)
270+
}
271+
}
272+
259273
if oldC.Spec.HCloudNetwork.Enabled {
260274
// Only allow updating the network ID when it was not set previously. This makes it possible to e.g. adopt the
261275
// network that was created initially by CAPH.
@@ -265,28 +279,22 @@ func (r *HetznerCluster) ValidateUpdate(old runtime.Object) (admission.Warnings,
265279
)
266280
}
267281

268-
if r.Spec.HCloudNetwork.ID != nil {
269-
if errs := areCIDRsAndNetworkZoneEmpty(r.Spec.HCloudNetwork); errs != nil {
270-
allErrs = append(allErrs, errs...)
271-
}
272-
} else {
273-
if !reflect.DeepEqual(oldC.Spec.HCloudNetwork.CIDRBlock, r.Spec.HCloudNetwork.CIDRBlock) {
274-
allErrs = append(allErrs,
275-
field.Invalid(field.NewPath("spec", "hcloudNetwork", "cidrBlock"), r.Spec.HCloudNetwork.CIDRBlock, "field is immutable"),
276-
)
277-
}
282+
if !reflect.DeepEqual(oldC.Spec.HCloudNetwork.CIDRBlock, r.Spec.HCloudNetwork.CIDRBlock) {
283+
allErrs = append(allErrs,
284+
field.Invalid(field.NewPath("spec", "hcloudNetwork", "cidrBlock"), r.Spec.HCloudNetwork.CIDRBlock, "field is immutable"),
285+
)
286+
}
278287

279-
if !reflect.DeepEqual(oldC.Spec.HCloudNetwork.SubnetCIDRBlock, r.Spec.HCloudNetwork.SubnetCIDRBlock) {
280-
allErrs = append(allErrs,
281-
field.Invalid(field.NewPath("spec", "hcloudNetwork", "subnetCIDRBlock"), r.Spec.HCloudNetwork.SubnetCIDRBlock, "field is immutable"),
282-
)
283-
}
288+
if !reflect.DeepEqual(oldC.Spec.HCloudNetwork.SubnetCIDRBlock, r.Spec.HCloudNetwork.SubnetCIDRBlock) {
289+
allErrs = append(allErrs,
290+
field.Invalid(field.NewPath("spec", "hcloudNetwork", "subnetCIDRBlock"), r.Spec.HCloudNetwork.SubnetCIDRBlock, "field is immutable"),
291+
)
292+
}
284293

285-
if !reflect.DeepEqual(oldC.Spec.HCloudNetwork.NetworkZone, r.Spec.HCloudNetwork.NetworkZone) {
286-
allErrs = append(allErrs,
287-
field.Invalid(field.NewPath("spec", "hcloudNetwork", "networkZone"), r.Spec.HCloudNetwork.NetworkZone, "field is immutable"),
288-
)
289-
}
294+
if !reflect.DeepEqual(oldC.Spec.HCloudNetwork.NetworkZone, r.Spec.HCloudNetwork.NetworkZone) {
295+
allErrs = append(allErrs,
296+
field.Invalid(field.NewPath("spec", "hcloudNetwork", "networkZone"), r.Spec.HCloudNetwork.NetworkZone, "field is immutable"),
297+
)
290298
}
291299
}
292300

@@ -304,14 +312,14 @@ func (r *HetznerCluster) ValidateUpdate(old runtime.Object) (admission.Warnings,
304312
}
305313

306314
// Load balancer enabled/disabled is immutable
307-
if !reflect.DeepEqual(oldC.Spec.ControlPlaneLoadBalancer.Enabled, r.Spec.ControlPlaneLoadBalancer.Enabled) {
315+
if oldC.Spec.ControlPlaneLoadBalancer.Enabled != r.Spec.ControlPlaneLoadBalancer.Enabled {
308316
allErrs = append(allErrs,
309317
field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "enabled"), r.Spec.ControlPlaneLoadBalancer.Enabled, "field is immutable"),
310318
)
311319
}
312320

313321
// Load balancer region and port are immutable
314-
if !reflect.DeepEqual(oldC.Spec.ControlPlaneLoadBalancer.Port, r.Spec.ControlPlaneLoadBalancer.Port) {
322+
if oldC.Spec.ControlPlaneLoadBalancer.Port != r.Spec.ControlPlaneLoadBalancer.Port {
315323
allErrs = append(allErrs,
316324
field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "port"), r.Spec.ControlPlaneLoadBalancer.Port, "field is immutable"),
317325
)

‎api/v1beta1/types.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -232,15 +232,15 @@ type HCloudNetworkSpec struct {
232232
CIDRBlock *string `json:"cidrBlock,omitempty"`
233233

234234
// SubnetCIDRBlock defines the cidrBlock for the subnet of the HCloud Network.
235-
// Defaults to "10.0.0.0/24".
235+
// The webhook defaults this to "10.0.0.0/24".
236236
// Mutually exclusive with ID.
237237
// Note: A subnet is required.
238238
// +optional
239239
SubnetCIDRBlock *string `json:"subnetCidrBlock,omitempty"`
240240

241241
// NetworkZone specifies the HCloud network zone of the private network.
242242
// The zones must be one of eu-central, us-east, or us-west.
243-
// Defaults to "eu-central".
243+
// The webhook defaults this to "eu-central".
244244
// Mutually exclusive with ID.
245245
// +optional
246246
NetworkZone *HCloudNetworkZone `json:"networkZone,omitempty"`

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -209,13 +209,13 @@ spec:
209209
description: |-
210210
NetworkZone specifies the HCloud network zone of the private network.
211211
The zones must be one of eu-central, us-east, or us-west.
212-
Defaults to "eu-central".
212+
The webhook defaults this to "eu-central".
213213
Mutually exclusive with ID.
214214
type: string
215215
subnetCidrBlock:
216216
description: |-
217217
SubnetCIDRBlock defines the cidrBlock for the subnet of the HCloud Network.
218-
Defaults to "10.0.0.0/24".
218+
The webhook defaults this to "10.0.0.0/24".
219219
Mutually exclusive with ID.
220220
Note: A subnet is required.
221221
type: string

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -240,13 +240,13 @@ spec:
240240
description: |-
241241
NetworkZone specifies the HCloud network zone of the private network.
242242
The zones must be one of eu-central, us-east, or us-west.
243-
Defaults to "eu-central".
243+
The webhook defaults this to "eu-central".
244244
Mutually exclusive with ID.
245245
type: string
246246
subnetCidrBlock:
247247
description: |-
248248
SubnetCIDRBlock defines the cidrBlock for the subnet of the HCloud Network.
249-
Defaults to "10.0.0.0/24".
249+
The webhook defaults this to "10.0.0.0/24".
250250
Mutually exclusive with ID.
251251
Note: A subnet is required.
252252
type: string

‎pkg/services/hcloud/network/network.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ package network
1919

2020
import (
2121
"context"
22-
"errors"
2322
"fmt"
2423
"net"
2524
"slices"
@@ -119,7 +118,7 @@ func (s *Service) createOpts() (hcloud.NetworkCreateOpts, error) {
119118
spec := s.scope.HetznerCluster.Spec.HCloudNetwork
120119

121120
if spec.CIDRBlock == nil || spec.SubnetCIDRBlock == nil || spec.NetworkZone == nil {
122-
return hcloud.NetworkCreateOpts{}, errors.New("nil CIDRs or NetworkZone given")
121+
return hcloud.NetworkCreateOpts{}, fmt.Errorf("nil CIDRs or NetworkZone given")
123122
}
124123

125124
_, network, err := net.ParseCIDR(*spec.CIDRBlock)

0 commit comments

Comments
 (0)