Skip to content

Commit

Permalink
Review suggestions 2
Browse files Browse the repository at this point in the history
  • Loading branch information
@johannesfrey
johannesfrey committed Nov 21, 2024
1 parent 028f798 commit 7b36136
Show file tree
Hide file tree
Showing 5 changed files with 51 additions and 44 deletions.
80 changes: 44 additions & 36 deletions api/v1beta1/hetznercluster_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,20 +86,22 @@ func (r *HetznerCluster) Default(_ context.Context, obj runtime.Object) error {
return apierrors.NewBadRequest(fmt.Sprintf("expected an HetznerCluster but got a %T", obj))
}

if cluster.Spec.HCloudNetwork.Enabled {
if cluster.Spec.HCloudNetwork.ID != nil {
return nil
}
if !cluster.Spec.HCloudNetwork.Enabled {
return nil
}

if cluster.Spec.HCloudNetwork.CIDRBlock == nil {
cluster.Spec.HCloudNetwork.CIDRBlock = ptr.To(DefaultCIDRBlock)
}
if cluster.Spec.HCloudNetwork.SubnetCIDRBlock == nil {
cluster.Spec.HCloudNetwork.SubnetCIDRBlock = ptr.To(DefaultSubnetCIDRBlock)
}
if cluster.Spec.HCloudNetwork.NetworkZone == nil {
cluster.Spec.HCloudNetwork.NetworkZone = ptr.To[HCloudNetworkZone](DefaultNetworkZone)
}
if cluster.Spec.HCloudNetwork.ID != nil {
return nil
}

if cluster.Spec.HCloudNetwork.CIDRBlock == nil {
cluster.Spec.HCloudNetwork.CIDRBlock = ptr.To(DefaultCIDRBlock)
}
if cluster.Spec.HCloudNetwork.SubnetCIDRBlock == nil {
cluster.Spec.HCloudNetwork.SubnetCIDRBlock = ptr.To(DefaultSubnetCIDRBlock)
}
if cluster.Spec.HCloudNetwork.NetworkZone == nil {
cluster.Spec.HCloudNetwork.NetworkZone = ptr.To[HCloudNetworkZone](DefaultNetworkZone)
}

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

if !reflect.DeepEqual(oldC.Spec.HCloudNetwork.Enabled, r.Spec.HCloudNetwork.Enabled) {
if oldC.Spec.HCloudNetwork.Enabled != r.Spec.HCloudNetwork.Enabled {
allErrs = append(allErrs,
field.Invalid(field.NewPath("spec", "hcloudNetwork", "enabled"), r.Spec.HCloudNetwork.Enabled, "field is immutable"),
)
}

if !oldC.Spec.HCloudNetwork.Enabled {
// If the network is disabled check that all other network related fields are empty.
if r.Spec.HCloudNetwork.ID != nil {
allErrs = append(allErrs,
field.Invalid(field.NewPath("spec", "hcloudNetwork", "id"), oldC.Spec.HCloudNetwork.ID, "field must be empty"),
)
}
if errs := areCIDRsAndNetworkZoneEmpty(r.Spec.HCloudNetwork); errs != nil {
allErrs = append(allErrs, errs...)
}
}

if oldC.Spec.HCloudNetwork.Enabled {
// Only allow updating the network ID when it was not set previously. This makes it possible to e.g. adopt the
// network that was created initially by CAPH.
Expand All @@ -265,28 +279,22 @@ func (r *HetznerCluster) ValidateUpdate(old runtime.Object) (admission.Warnings,
)
}

if r.Spec.HCloudNetwork.ID != nil {
if errs := areCIDRsAndNetworkZoneEmpty(r.Spec.HCloudNetwork); errs != nil {
allErrs = append(allErrs, errs...)
}
} else {
if !reflect.DeepEqual(oldC.Spec.HCloudNetwork.CIDRBlock, r.Spec.HCloudNetwork.CIDRBlock) {
allErrs = append(allErrs,
field.Invalid(field.NewPath("spec", "hcloudNetwork", "cidrBlock"), r.Spec.HCloudNetwork.CIDRBlock, "field is immutable"),
)
}
if !reflect.DeepEqual(oldC.Spec.HCloudNetwork.CIDRBlock, r.Spec.HCloudNetwork.CIDRBlock) {
allErrs = append(allErrs,
field.Invalid(field.NewPath("spec", "hcloudNetwork", "cidrBlock"), r.Spec.HCloudNetwork.CIDRBlock, "field is immutable"),
)
}

if !reflect.DeepEqual(oldC.Spec.HCloudNetwork.SubnetCIDRBlock, r.Spec.HCloudNetwork.SubnetCIDRBlock) {
allErrs = append(allErrs,
field.Invalid(field.NewPath("spec", "hcloudNetwork", "subnetCIDRBlock"), r.Spec.HCloudNetwork.SubnetCIDRBlock, "field is immutable"),
)
}
if !reflect.DeepEqual(oldC.Spec.HCloudNetwork.SubnetCIDRBlock, r.Spec.HCloudNetwork.SubnetCIDRBlock) {
allErrs = append(allErrs,
field.Invalid(field.NewPath("spec", "hcloudNetwork", "subnetCIDRBlock"), r.Spec.HCloudNetwork.SubnetCIDRBlock, "field is immutable"),
)
}

if !reflect.DeepEqual(oldC.Spec.HCloudNetwork.NetworkZone, r.Spec.HCloudNetwork.NetworkZone) {
allErrs = append(allErrs,
field.Invalid(field.NewPath("spec", "hcloudNetwork", "networkZone"), r.Spec.HCloudNetwork.NetworkZone, "field is immutable"),
)
}
if !reflect.DeepEqual(oldC.Spec.HCloudNetwork.NetworkZone, r.Spec.HCloudNetwork.NetworkZone) {
allErrs = append(allErrs,
field.Invalid(field.NewPath("spec", "hcloudNetwork", "networkZone"), r.Spec.HCloudNetwork.NetworkZone, "field is immutable"),
)
}
}

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

// Load balancer enabled/disabled is immutable
if !reflect.DeepEqual(oldC.Spec.ControlPlaneLoadBalancer.Enabled, r.Spec.ControlPlaneLoadBalancer.Enabled) {
if oldC.Spec.ControlPlaneLoadBalancer.Enabled != r.Spec.ControlPlaneLoadBalancer.Enabled {
allErrs = append(allErrs,
field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "enabled"), r.Spec.ControlPlaneLoadBalancer.Enabled, "field is immutable"),
)
}

// Load balancer region and port are immutable
if !reflect.DeepEqual(oldC.Spec.ControlPlaneLoadBalancer.Port, r.Spec.ControlPlaneLoadBalancer.Port) {
if oldC.Spec.ControlPlaneLoadBalancer.Port != r.Spec.ControlPlaneLoadBalancer.Port {
allErrs = append(allErrs,
field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "port"), r.Spec.ControlPlaneLoadBalancer.Port, "field is immutable"),
)
Expand Down
4 changes: 2 additions & 2 deletions api/v1beta1/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -232,15 +232,15 @@ type HCloudNetworkSpec struct {
CIDRBlock *string `json:"cidrBlock,omitempty"`

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

// NetworkZone specifies the HCloud network zone of the private network.
// The zones must be one of eu-central, us-east, or us-west.
// Defaults to "eu-central".
// The webhook defaults this to "eu-central".
// Mutually exclusive with ID.
// +optional
NetworkZone *HCloudNetworkZone `json:"networkZone,omitempty"`
Expand Down
4 changes: 2 additions & 2 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_hetznerclusters.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,13 +209,13 @@ spec:
description: |-
NetworkZone specifies the HCloud network zone of the private network.
The zones must be one of eu-central, us-east, or us-west.
Defaults to "eu-central".
The webhook defaults this to "eu-central".
Mutually exclusive with ID.
type: string
subnetCidrBlock:
description: |-
SubnetCIDRBlock defines the cidrBlock for the subnet of the HCloud Network.
Defaults to "10.0.0.0/24".
The webhook defaults this to "10.0.0.0/24".
Mutually exclusive with ID.
Note: A subnet is required.
type: string
Expand Down
4 changes: 2 additions & 2 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_hetznerclustertemplates.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -240,13 +240,13 @@ spec:
description: |-
NetworkZone specifies the HCloud network zone of the private network.
The zones must be one of eu-central, us-east, or us-west.
Defaults to "eu-central".
The webhook defaults this to "eu-central".
Mutually exclusive with ID.
type: string
subnetCidrBlock:
description: |-
SubnetCIDRBlock defines the cidrBlock for the subnet of the HCloud Network.
Defaults to "10.0.0.0/24".
The webhook defaults this to "10.0.0.0/24".
Mutually exclusive with ID.
Note: A subnet is required.
type: string
Expand Down
3 changes: 1 addition & 2 deletions pkg/services/hcloud/network/network.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ package network

import (
"context"
"errors"
"fmt"
"net"
"slices"
Expand Down Expand Up @@ -119,7 +118,7 @@ func (s *Service) createOpts() (hcloud.NetworkCreateOpts, error) {
spec := s.scope.HetznerCluster.Spec.HCloudNetwork

if spec.CIDRBlock == nil || spec.SubnetCIDRBlock == nil || spec.NetworkZone == nil {
return hcloud.NetworkCreateOpts{}, errors.New("nil CIDRs or NetworkZone given")
return hcloud.NetworkCreateOpts{}, fmt.Errorf("nil CIDRs or NetworkZone given")
}

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

0 comments on commit 7b36136

Please sign in to comment.