From 028f798cf5a8c3dff81b46479afa90fc2fd1aa2c Mon Sep 17 00:00:00 2001 From: Johannes Frey Date: Wed, 13 Nov 2024 18:07:01 +0100 Subject: [PATCH] Review suggestions --- api/v1beta1/conditions_const.go | 2 ++ api/v1beta1/types.go | 2 +- ...ture.cluster.x-k8s.io_hetznerclusters.yaml | 2 +- ...ster.x-k8s.io_hetznerclustertemplates.yaml | 2 +- controllers/hetznercluster_controller.go | 12 ++----- pkg/services/hcloud/network/network.go | 32 ++++++++++++++----- 6 files changed, 32 insertions(+), 20 deletions(-) diff --git a/api/v1beta1/conditions_const.go b/api/v1beta1/conditions_const.go index 57c9d9d2d..2bdcad674 100644 --- a/api/v1beta1/conditions_const.go +++ b/api/v1beta1/conditions_const.go @@ -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 ( diff --git a/api/v1beta1/types.go b/api/v1beta1/types.go index 0b0c84918..3097466d6 100644 --- a/api/v1beta1/types.go +++ b/api/v1beta1/types.go @@ -226,7 +226,7 @@ type HCloudNetworkSpec struct { Enabled bool `json:"enabled"` // CIDRBlock defines the cidrBlock of the HCloud Network. - // Defaults to "10.0.0.0/16". + // The webhook defaults this to "10.0.0.0/16". // Mutually exclusive with ID. // +optional CIDRBlock *string `json:"cidrBlock,omitempty"` diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_hetznerclusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_hetznerclusters.yaml index 6d5d3b8e9..f8105619a 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_hetznerclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_hetznerclusters.yaml @@ -192,7 +192,7 @@ spec: cidrBlock: description: |- CIDRBlock defines the cidrBlock of the HCloud Network. - Defaults to "10.0.0.0/16". + The webhook defaults this to "10.0.0.0/16". Mutually exclusive with ID. type: string enabled: diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_hetznerclustertemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_hetznerclustertemplates.yaml index 0c0e1434d..3a69a0f0b 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_hetznerclustertemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_hetznerclustertemplates.yaml @@ -223,7 +223,7 @@ spec: cidrBlock: description: |- CIDRBlock defines the cidrBlock of the HCloud Network. - Defaults to "10.0.0.0/16". + The webhook defaults this to "10.0.0.0/16". Mutually exclusive with ID. type: string enabled: diff --git a/controllers/hetznercluster_controller.go b/controllers/hetznercluster_controller.go index 517ba1ee0..4e33e055f 100644 --- a/controllers/hetznercluster_controller.go +++ b/controllers/hetznercluster_controller.go @@ -346,15 +346,9 @@ func (r *HetznerClusterReconciler) reconcileDelete(ctx context.Context, clusterS return reconcile.Result{}, fmt.Errorf("failed to delete load balancers for HetznerCluster %s/%s: %w", hetznerCluster.Namespace, hetznerCluster.Name, err) } - // delete the network only if it is owned by us - if hetznerCluster.Status.Network != nil { - if hetznerCluster.Status.Network.Labels[hetznerCluster.ClusterTagKey()] == string(infrav1.ResourceLifecycleOwned) { - if err := network.NewService(clusterScope).Delete(ctx); err != nil { - return reconcile.Result{}, fmt.Errorf("failed to delete network for HetznerCluster %s/%s: %w", hetznerCluster.Namespace, hetznerCluster.Name, err) - } - } else { - clusterScope.V(1).Info("network is not owned by us", "id", hetznerCluster.Status.Network.ID, "labels", hetznerCluster.Status.Network.Labels) - } + // delete the network + if err := network.NewService(clusterScope).Delete(ctx); err != nil { + return reconcile.Result{}, fmt.Errorf("failed to delete network for HetznerCluster %s/%s: %w", hetznerCluster.Namespace, hetznerCluster.Name, err) } // delete the placement groups diff --git a/pkg/services/hcloud/network/network.go b/pkg/services/hcloud/network/network.go index d69aca4c2..24df1ffc2 100644 --- a/pkg/services/hcloud/network/network.go +++ b/pkg/services/hcloud/network/network.go @@ -81,6 +81,17 @@ func (s *Service) Reconcile(ctx context.Context) (err error) { } } + if len(network.Subnets) > 1 { + conditions.MarkFalse( + s.scope.HetznerCluster, + infrav1.NetworkReadyCondition, + infrav1.MultipleSubnetsExistReason, + clusterv1.ConditionSeverityWarning, + "multiple subnets not allowed", + ) + return nil + } + conditions.MarkTrue(s.scope.HetznerCluster, infrav1.NetworkReadyCondition) s.scope.HetznerCluster.Status.Network = statusFromHCloudNetwork(network) @@ -137,25 +148,33 @@ func (s *Service) createOpts() (hcloud.NetworkCreateOpts, error) { // Delete implements deletion of the network. func (s *Service) Delete(ctx context.Context) error { - if s.scope.HetznerCluster.Status.Network == nil { + hetznerCluster := s.scope.HetznerCluster + + if hetznerCluster.Status.Network == nil { // nothing to delete return nil } - id := s.scope.HetznerCluster.Status.Network.ID + // only delete the network if it is owned by us + if hetznerCluster.Status.Network.Labels[hetznerCluster.ClusterTagKey()] != string(infrav1.ResourceLifecycleOwned) { + s.scope.V(1).Info("network is not owned by us", "id", hetznerCluster.Status.Network.ID, "labels", hetznerCluster.Status.Network.Labels) + return nil + } + + id := hetznerCluster.Status.Network.ID if err := s.scope.HCloudClient.DeleteNetwork(ctx, &hcloud.Network{ID: id}); err != nil { - hcloudutil.HandleRateLimitExceeded(s.scope.HetznerCluster, err, "DeleteNetwork") + hcloudutil.HandleRateLimitExceeded(hetznerCluster, err, "DeleteNetwork") // if resource has been deleted already then do nothing if hcloud.IsError(err, hcloud.ErrorCodeNotFound) { s.scope.V(1).Info("deleting network failed - not found", "id", id) return nil } - record.Warnf(s.scope.HetznerCluster, "NetworkDeleteFailed", "Failed to delete network with ID %v", id) + record.Warnf(hetznerCluster, "NetworkDeleteFailed", "Failed to delete network with ID %v", id) return fmt.Errorf("failed to delete network: %w", err) } - record.Eventf(s.scope.HetznerCluster, "NetworkDeleted", "Deleted network with ID %v", id) + record.Eventf(hetznerCluster, "NetworkDeleted", "Deleted network with ID %v", id) return nil } @@ -170,9 +189,6 @@ func (s *Service) findNetwork(ctx context.Context) (*hcloud.Network, error) { } if network != nil { - if len(network.Subnets) > 1 { - return nil, fmt.Errorf("multiple subnets not allowed") - } s.scope.V(1).Info("found network", "id", network.ID, "name", network.Name, "labels", network.Labels) return network, nil }