Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Bring your own network #1472

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions api/v1beta1/conditions_const.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ const (
NetworkReadyCondition clusterv1.ConditionType = "NetworkReady"
// NetworkReconcileFailedReason indicates that reconciling the network failed.
NetworkReconcileFailedReason = "NetworkReconcileFailed"
// MultipleSubnetsExistReason indicates that the network has multiple subnets.
MultipleSubnetsExistReason = "MultipleSubnetsExist"
)

const (
Expand Down
157 changes: 151 additions & 6 deletions api/v1beta1/hetznercluster_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,15 @@ limitations under the License.
package v1beta1

import (
"context"
"fmt"
"net"
"reflect"

apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/utils/ptr"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/webhook"
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
Expand All @@ -42,10 +45,22 @@ var regionNetworkZoneMap = map[string]string{
"sin": "ap-southeast",
}

const (
// DefaultCIDRBlock specifies the default CIDR block used by the HCloudNetwork.
DefaultCIDRBlock = "10.0.0.0/16"

// DefaultSubnetCIDRBlock specifies the default subnet CIDR block used by the HCloudNetwork.
DefaultSubnetCIDRBlock = "10.0.0.0/24"

// DefaultNetworkZone specifies the default network zone used by the HCloudNetwork.
DefaultNetworkZone = "eu-central"
)

// SetupWebhookWithManager initializes webhook manager for HetznerCluster.
func (r *HetznerCluster) SetupWebhookWithManager(mgr ctrl.Manager) error {
return ctrl.NewWebhookManagedBy(mgr).
For(r).
WithDefaulter(r).
Complete()
}

Expand All @@ -60,10 +75,35 @@ func (r *HetznerClusterList) SetupWebhookWithManager(mgr ctrl.Manager) error {

//+kubebuilder:webhook:path=/mutate-infrastructure-cluster-x-k8s-io-v1beta1-hetznercluster,mutating=true,failurePolicy=fail,sideEffects=None,groups=infrastructure.cluster.x-k8s.io,resources=hetznerclusters,verbs=create;update,versions=v1beta1,name=mutation.hetznercluster.infrastructure.cluster.x-k8s.io,admissionReviewVersions={v1,v1beta1}

var _ webhook.Defaulter = &HetznerCluster{}
var _ webhook.CustomDefaulter = &HetznerCluster{}

// Default implements webhook.Defaulter so a webhook will be registered for the type.
func (r *HetznerCluster) Default() {}
// Default implements webhook.CustomDefaulter so a webhook will be registered for the type.
func (r *HetznerCluster) Default(_ context.Context, obj runtime.Object) error {
hetznerclusterlog.V(1).Info("default", "name", r.Name)

cluster, ok := obj.(*HetznerCluster)
if !ok {
return apierrors.NewBadRequest(fmt.Sprintf("expected an HetznerCluster but got a %T", obj))
}

if cluster.Spec.HCloudNetwork.Enabled {
johannesfrey marked this conversation as resolved.
Show resolved Hide resolved
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
}

//+kubebuilder:webhook:path=/validate-infrastructure-cluster-x-k8s-io-v1beta1-hetznercluster,mutating=false,failurePolicy=fail,sideEffects=None,groups=infrastructure.cluster.x-k8s.io,resources=hetznerclusters,verbs=create;update,versions=v1beta1,name=validation.hetznercluster.infrastructure.cluster.x-k8s.io,admissionReviewVersions={v1,v1beta1}

Expand Down Expand Up @@ -109,6 +149,55 @@ func (r *HetznerCluster) ValidateCreate() (admission.Warnings, error) {
if err := isNetworkZoneSameForAllRegions(r.Spec.ControlPlaneRegions, nil); err != nil {
allErrs = append(allErrs, err)
}
} else {
// If ID is given check that all other network settings are empty.
if r.Spec.HCloudNetwork.ID != nil {
janiskemper marked this conversation as resolved.
Show resolved Hide resolved
if errs := areCIDRsAndNetworkZoneEmpty(r.Spec.HCloudNetwork); errs != nil {
allErrs = append(allErrs, errs...)
}
} else {
// If no ID is given check the other network settings for valid entries.
if r.Spec.HCloudNetwork.NetworkZone != nil {
janiskemper marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is supposed to happen if this is nil?

I just wanted to say that we should again avoid the nested if statements if possible and that we can do that here.

Currently, we don't handle the case where NetworkZone is nil at all. Shouldn't we though? Isn't that a problem?

Anyway, let's avoid the nested if statement and if you say we should, then let's handle the case where NetworkZone is nil

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we should probably append an error when NetworkZone is nil. But still I'm struggling to see how to really circumvent the nested statements because the method's idea seems to be to aggregate all errors that might occur instead of returning early. An early return would then potentially miss other validation errors that might happen afterwards. But probably I'm just missing a detail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to avoid the large code block for "NetworkZone != nil". However, I think you are right that this is not really possible, as we cannot just return at that point.

Therefore, I would say let's add a handling of "NetworkZone == nil" and otherwise don't change anything. Does that make sense to you?

givenZone := string(*r.Spec.HCloudNetwork.NetworkZone)

var validNetworkZone bool
for _, z := range regionNetworkZoneMap {
if givenZone == z {
validNetworkZone = true
break
}
}
if !validNetworkZone {
allErrs = append(allErrs, field.Invalid(
field.NewPath("spec", "hcloudNetwork", "networkZone"),
r.Spec.HCloudNetwork.NetworkZone,
"wrong network zone. Should be eu-central, us-east, us-west or ap-southeast"),
)
}
}

if r.Spec.HCloudNetwork.CIDRBlock != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again here and below it should never be nil afaik. Should we return errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aligns with the comment above. Do you mean return with an error here? Because in this method we only aggregate and return at the bottom?!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I mean with "returning errors" to "add them". Sorry for being not precise

_, _, err := net.ParseCIDR(*r.Spec.HCloudNetwork.CIDRBlock)
if err != nil {
allErrs = append(allErrs, field.Invalid(
field.NewPath("spec", "hcloudNetwork", "cidrBlock"),
r.Spec.HCloudNetwork.CIDRBlock,
"malformed cidrBlock"),
)
}
}

if r.Spec.HCloudNetwork.SubnetCIDRBlock != nil {
_, _, err := net.ParseCIDR(*r.Spec.HCloudNetwork.SubnetCIDRBlock)
if err != nil {
allErrs = append(allErrs, field.Invalid(
field.NewPath("spec", "hcloudNetwork", "subnetCIDRBlock"),
r.Spec.HCloudNetwork.SubnetCIDRBlock,
"malformed cidrBlock"),
)
}
}
}
}

// Check whether controlPlaneEndpoint is specified if allow empty is not set or false
Expand Down Expand Up @@ -161,13 +250,46 @@ func (r *HetznerCluster) ValidateUpdate(old runtime.Object) (admission.Warnings,
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected an HetznerCluster but got a %T", old))
}

// Network settings are immutable
if !reflect.DeepEqual(oldC.Spec.HCloudNetwork, r.Spec.HCloudNetwork) {
if !reflect.DeepEqual(oldC.Spec.HCloudNetwork.Enabled, r.Spec.HCloudNetwork.Enabled) {
johannesfrey marked this conversation as resolved.
Show resolved Hide resolved
allErrs = append(allErrs,
field.Invalid(field.NewPath("spec", "hcloudNetwork"), r.Spec.HCloudNetwork, "field is immutable"),
field.Invalid(field.NewPath("spec", "hcloudNetwork", "enabled"), r.Spec.HCloudNetwork.Enabled, "field is immutable"),
)
}

if oldC.Spec.HCloudNetwork.Enabled {
johannesfrey marked this conversation as resolved.
Show resolved Hide resolved
// 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.
if oldC.Spec.HCloudNetwork.ID != nil && !reflect.DeepEqual(oldC.Spec.HCloudNetwork.ID, r.Spec.HCloudNetwork.ID) {
janiskemper marked this conversation as resolved.
Show resolved Hide resolved
allErrs = append(allErrs,
field.Invalid(field.NewPath("spec", "hcloudNetwork", "id"), r.Spec.HCloudNetwork.ID, "field is immutable"),
)
}

if r.Spec.HCloudNetwork.ID != nil {
if errs := areCIDRsAndNetworkZoneEmpty(r.Spec.HCloudNetwork); errs != nil {
johannesfrey marked this conversation as resolved.
Show resolved Hide resolved
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.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"),
)
}
}
}

// Check if all regions are in the same network zone if a private network is enabled
if oldC.Spec.HCloudNetwork.Enabled {
var defaultNetworkZone *string
Expand Down Expand Up @@ -225,3 +347,26 @@ func (r *HetznerCluster) ValidateDelete() (admission.Warnings, error) {
hetznerclusterlog.V(1).Info("validate delete", "name", r.Name)
return nil, nil
}

func areCIDRsAndNetworkZoneEmpty(hcloudNetwork HCloudNetworkSpec) field.ErrorList {
var allErrs field.ErrorList
if hcloudNetwork.CIDRBlock != nil {
allErrs = append(allErrs,
field.Invalid(field.NewPath("spec", "hcloudNetwork", "cidrBlock"), hcloudNetwork.CIDRBlock, "field must be empty"),
)
}

if hcloudNetwork.SubnetCIDRBlock != nil {
allErrs = append(allErrs,
field.Invalid(field.NewPath("spec", "hcloudNetwork", "subnetCIDRBlock"), hcloudNetwork.SubnetCIDRBlock, "field must be empty"),
)
}

if hcloudNetwork.NetworkZone != nil {
allErrs = append(allErrs,
field.Invalid(field.NewPath("spec", "hcloudNetwork", "networkZone"), hcloudNetwork.NetworkZone, "field must be empty"),
)
}

return allErrs
}
31 changes: 19 additions & 12 deletions api/v1beta1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -217,32 +217,39 @@ type LoadBalancerTarget struct {

// HCloudNetworkSpec defines the desired state of the HCloud Private Network.
type HCloudNetworkSpec struct {
// ID is the id of the Network to adopt.
// Mutually exclusive with CIDRBlock, SubnetCIDRBlock and NetworkZone.
// +optional
ID *int64 `json:"id,omitempty"`

// Enabled defines whether the network should be enabled or not.
Enabled bool `json:"enabled"`

// CIDRBlock defines the cidrBlock of the HCloud Network. If omitted, default "10.0.0.0/16" will be used.
// +kubebuilder:default="10.0.0.0/16"
// CIDRBlock defines the cidrBlock of the HCloud Network.
// The webhook defaults this to "10.0.0.0/16".
// Mutually exclusive with ID.
// +optional
CIDRBlock string `json:"cidrBlock,omitempty"`
CIDRBlock *string `json:"cidrBlock,omitempty"`

// SubnetCIDRBlock defines the cidrBlock for the subnet of the HCloud Network.
// Defaults to "10.0.0.0/24".
// Mutually exclusive with ID.
// Note: A subnet is required.
// +kubebuilder:default="10.0.0.0/24"
// +optional
SubnetCIDRBlock string `json:"subnetCidrBlock,omitempty"`
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. The default is eu-central.
// +kubebuilder:validation:Enum=eu-central;us-east;us-west;ap-southeast
// +kubebuilder:default=eu-central
// The zones must be one of eu-central, us-east, or us-west.
// Defaults to "eu-central".
johannesfrey marked this conversation as resolved.
Show resolved Hide resolved
// Mutually exclusive with ID.
// +optional
NetworkZone HCloudNetworkZone `json:"networkZone,omitempty"`
NetworkZone *HCloudNetworkZone `json:"networkZone,omitempty"`
}

// NetworkStatus defines the observed state of the HCloud Private Network.
type NetworkStatus struct {
ID int64 `json:"id,omitempty"`
Labels map[string]string `json:"-"`
Labels map[string]string `json:"labels,omitempty"`
AttachedServers []int64 `json:"attachedServers,omitempty"`
}

Expand All @@ -255,10 +262,10 @@ type HCloudNetworkZone string

// IsZero returns true if a private Network is set.
func (s *HCloudNetworkSpec) IsZero() bool {
if s.CIDRBlock != "" {
if s.CIDRBlock != nil {
return false
}
if s.SubnetCIDRBlock != "" {
if s.SubnetCIDRBlock != nil {
return false
}
return true
Expand Down
22 changes: 21 additions & 1 deletion api/v1beta1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -190,29 +190,33 @@ spec:
for Hetzner Cloud. If left empty, no private Network is configured.
properties:
cidrBlock:
default: 10.0.0.0/16
description: CIDRBlock defines the cidrBlock of the HCloud Network.
If omitted, default "10.0.0.0/16" will be used.
description: |-
CIDRBlock defines the cidrBlock of the HCloud Network.
The webhook defaults this to "10.0.0.0/16".
Mutually exclusive with ID.
type: string
enabled:
description: Enabled defines whether the network should be enabled
or not.
type: boolean
id:
description: |-
ID is the id of the Network to adopt.
Mutually exclusive with CIDRBlock, SubnetCIDRBlock and NetworkZone.
format: int64
type: integer
networkZone:
default: eu-central
description: |-
NetworkZone specifies the HCloud network zone of the private network.
The zones must be one of eu-central, us-east, or us-west. The default is eu-central.
enum:
- eu-central
- us-east
- us-west
- ap-southeast
The zones must be one of eu-central, us-east, or us-west.
Defaults to "eu-central".
Mutually exclusive with ID.
type: string
subnetCidrBlock:
default: 10.0.0.0/24
description: |-
SubnetCIDRBlock defines the cidrBlock for the subnet of the HCloud Network.
Defaults to "10.0.0.0/24".
Mutually exclusive with ID.
Note: A subnet is required.
type: string
required:
Expand Down Expand Up @@ -466,6 +470,10 @@ spec:
id:
format: int64
type: integer
labels:
additionalProperties:
type: string
type: object
type: object
ready:
default: false
Expand Down
Loading
Loading