From 939a32f63505a482e3dd8cf3a64fcdfec6cbe858 Mon Sep 17 00:00:00 2001 From: Johannes Frey Date: Sat, 31 Aug 2024 13:16:00 +0200 Subject: [PATCH 01/10] Add ID field to HCloudNetwork and change other fields to pointers --- api/v1beta1/types.go | 31 ++++++++++++------- api/v1beta1/zz_generated.deepcopy.go | 22 ++++++++++++- ...ture.cluster.x-k8s.io_hetznerclusters.yaml | 30 +++++++++++------- ...ster.x-k8s.io_hetznerclustertemplates.yaml | 26 +++++++++------- controllers/controllers_suite_test.go | 5 +-- test/helpers/defaults.go | 7 +++-- 6 files changed, 79 insertions(+), 42 deletions(-) diff --git a/api/v1beta1/types.go b/api/v1beta1/types.go index f574a7942..0b0c84918 100644 --- a/api/v1beta1/types.go +++ b/api/v1beta1/types.go @@ -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. + // Defaults 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". + // 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"` } @@ -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 diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index fa34fdc2d..1a81d115b 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -381,6 +381,26 @@ func (in *HCloudMachineTemplateStatus) DeepCopy() *HCloudMachineTemplateStatus { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *HCloudNetworkSpec) DeepCopyInto(out *HCloudNetworkSpec) { *out = *in + if in.ID != nil { + in, out := &in.ID, &out.ID + *out = new(int64) + **out = **in + } + if in.CIDRBlock != nil { + in, out := &in.CIDRBlock, &out.CIDRBlock + *out = new(string) + **out = **in + } + if in.SubnetCIDRBlock != nil { + in, out := &in.SubnetCIDRBlock, &out.SubnetCIDRBlock + *out = new(string) + **out = **in + } + if in.NetworkZone != nil { + in, out := &in.NetworkZone, &out.NetworkZone + *out = new(HCloudNetworkZone) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new HCloudNetworkSpec. @@ -1251,7 +1271,7 @@ func (in *HetznerClusterList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *HetznerClusterSpec) DeepCopyInto(out *HetznerClusterSpec) { *out = *in - out.HCloudNetwork = in.HCloudNetwork + in.HCloudNetwork.DeepCopyInto(&out.HCloudNetwork) if in.ControlPlaneRegions != nil { in, out := &in.ControlPlaneRegions, &out.ControlPlaneRegions *out = make([]Region, len(*in)) 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 cea2fdcea..6d5d3b8e9 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_hetznerclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_hetznerclusters.yaml @@ -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. + Defaults 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: @@ -466,6 +470,10 @@ spec: id: format: int64 type: integer + labels: + additionalProperties: + type: string + type: object type: object ready: default: false 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 6661bd466..0c0e1434d 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_hetznerclustertemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_hetznerclustertemplates.yaml @@ -221,29 +221,33 @@ spec: 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. + Defaults 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: diff --git a/controllers/controllers_suite_test.go b/controllers/controllers_suite_test.go index d59015fcc..139529ee5 100644 --- a/controllers/controllers_suite_test.go +++ b/controllers/controllers_suite_test.go @@ -160,10 +160,7 @@ func getDefaultHetznerClusterSpec() infrav1.HetznerClusterSpec { ControlPlaneEndpoint: &clusterv1.APIEndpoint{}, ControlPlaneRegions: []infrav1.Region{"fsn1"}, HCloudNetwork: infrav1.HCloudNetworkSpec{ - CIDRBlock: "10.0.0.0/16", - Enabled: true, - NetworkZone: "eu-central", - SubnetCIDRBlock: "10.0.0.0/24", + Enabled: true, }, HCloudPlacementGroups: []infrav1.HCloudPlacementGroupSpec{ { diff --git a/test/helpers/defaults.go b/test/helpers/defaults.go index 622a3b8d0..e8ef1936b 100644 --- a/test/helpers/defaults.go +++ b/test/helpers/defaults.go @@ -22,6 +22,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" infrav1 "github.com/syself/cluster-api-provider-hetzner/api/v1beta1" @@ -199,10 +200,10 @@ func GetDefaultHetznerClusterSpec() infrav1.HetznerClusterSpec { ControlPlaneEndpoint: &clusterv1.APIEndpoint{}, ControlPlaneRegions: []infrav1.Region{"fsn1"}, HCloudNetwork: infrav1.HCloudNetworkSpec{ - CIDRBlock: "10.0.0.0/16", + CIDRBlock: ptr.To(infrav1.DefaultCIDRBlock), Enabled: true, - NetworkZone: "eu-central", - SubnetCIDRBlock: "10.0.0.0/24", + NetworkZone: ptr.To[infrav1.HCloudNetworkZone](infrav1.DefaultNetworkZone), + SubnetCIDRBlock: ptr.To(infrav1.DefaultSubnetCIDRBlock), }, HCloudPlacementGroups: []infrav1.HCloudPlacementGroupSpec{ { From a59626f874e28c62d48c3ebc0f7f72acfa890985 Mon Sep 17 00:00:00 2001 From: Johannes Frey Date: Sat, 31 Aug 2024 13:19:44 +0200 Subject: [PATCH 02/10] Add GetNetwork method to hcloud client --- pkg/services/hcloud/client/client.go | 6 ++++ .../hcloud/client/fake/hcloud_client.go | 3 ++ .../hcloud/client/fake/hcloud_client_test.go | 7 +++++ pkg/services/hcloud/client/mocks/Client.go | 30 +++++++++++++++++++ 4 files changed, 46 insertions(+) diff --git a/pkg/services/hcloud/client/client.go b/pkg/services/hcloud/client/client.go index 777d2ce2d..606a4f8ff 100644 --- a/pkg/services/hcloud/client/client.go +++ b/pkg/services/hcloud/client/client.go @@ -69,6 +69,7 @@ type Client interface { RebootServer(context.Context, *hcloud.Server) error CreateNetwork(context.Context, hcloud.NetworkCreateOpts) (*hcloud.Network, error) ListNetworks(context.Context, hcloud.NetworkListOpts) ([]*hcloud.Network, error) + GetNetwork(ctx context.Context, id int64) (*hcloud.Network, error) DeleteNetwork(context.Context, *hcloud.Network) error ListSSHKeys(context.Context, hcloud.SSHKeyListOpts) ([]*hcloud.SSHKey, error) CreatePlacementGroup(context.Context, hcloud.PlacementGroupCreateOpts) (*hcloud.PlacementGroup, error) @@ -300,6 +301,11 @@ func (c *realClient) ListNetworks(ctx context.Context, opts hcloud.NetworkListOp return resp, err } +func (c *realClient) GetNetwork(ctx context.Context, id int64) (*hcloud.Network, error) { + res, _, err := c.client.Network.GetByID(ctx, id) + return res, err +} + func (c *realClient) DeleteNetwork(ctx context.Context, network *hcloud.Network) error { _, err := c.client.Network.Delete(ctx, network) return err diff --git a/pkg/services/hcloud/client/fake/hcloud_client.go b/pkg/services/hcloud/client/fake/hcloud_client.go index 5369babe3..e902a023a 100644 --- a/pkg/services/hcloud/client/fake/hcloud_client.go +++ b/pkg/services/hcloud/client/fake/hcloud_client.go @@ -652,6 +652,9 @@ func (c *cacheHCloudClient) ListNetworks(_ context.Context, opts hcloud.NetworkL return networks, nil } +func (c *cacheHCloudClient) GetNetwork(_ context.Context, id int64) (*hcloud.Network, error) { + return c.networkCache.idMap[id], nil +} func (c *cacheHCloudClient) DeleteNetwork(_ context.Context, network *hcloud.Network) error { if _, found := c.networkCache.idMap[network.ID]; !found { diff --git a/pkg/services/hcloud/client/fake/hcloud_client_test.go b/pkg/services/hcloud/client/fake/hcloud_client_test.go index 919bf96a5..4b0602c9a 100644 --- a/pkg/services/hcloud/client/fake/hcloud_client_test.go +++ b/pkg/services/hcloud/client/fake/hcloud_client_test.go @@ -644,6 +644,13 @@ var _ = Describe("Network", func() { Expect(resp[0].ID).To(Equal(network.ID)) }) + It("gets a network", func() { + resp, err := client.GetNetwork(ctx, 1) + Expect(err).To(Succeed()) + Expect(resp).ToNot(BeNil()) + Expect(resp.ID).To(Equal(network.ID)) + }) + It("deletes a network", func() { Expect(client.DeleteNetwork(ctx, network)).To(Succeed()) resp, err := client.ListNetworks(ctx, listOpts) diff --git a/pkg/services/hcloud/client/mocks/Client.go b/pkg/services/hcloud/client/mocks/Client.go index 4ed39a3fe..8d6966ad9 100644 --- a/pkg/services/hcloud/client/mocks/Client.go +++ b/pkg/services/hcloud/client/mocks/Client.go @@ -407,6 +407,36 @@ func (_m *Client) DeleteTargetServerOfLoadBalancer(_a0 context.Context, _a1 *hcl return r0 } +// GetNetwork provides a mock function with given fields: ctx, id +func (_m *Client) GetNetwork(ctx context.Context, id int64) (*hcloud.Network, error) { + ret := _m.Called(ctx, id) + + if len(ret) == 0 { + panic("no return value specified for GetNetwork") + } + + var r0 *hcloud.Network + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, int64) (*hcloud.Network, error)); ok { + return rf(ctx, id) + } + if rf, ok := ret.Get(0).(func(context.Context, int64) *hcloud.Network); ok { + r0 = rf(ctx, id) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*hcloud.Network) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, int64) error); ok { + r1 = rf(ctx, id) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // GetServer provides a mock function with given fields: _a0, _a1 func (_m *Client) GetServer(_a0 context.Context, _a1 int64) (*hcloud.Server, error) { ret := _m.Called(_a0, _a1) From 5f927d2b4da1a9e78fd3ce4333bd55ec36a42716 Mon Sep 17 00:00:00 2001 From: Johannes Frey Date: Sat, 31 Aug 2024 13:20:11 +0200 Subject: [PATCH 03/10] Adopt existing Network in findNetwork when ID is given --- pkg/services/hcloud/network/network.go | 33 +++++- pkg/services/hcloud/network/network_test.go | 114 +++++++++++++++++++- 2 files changed, 137 insertions(+), 10 deletions(-) diff --git a/pkg/services/hcloud/network/network.go b/pkg/services/hcloud/network/network.go index 3db250f0b..d69aca4c2 100644 --- a/pkg/services/hcloud/network/network.go +++ b/pkg/services/hcloud/network/network.go @@ -19,6 +19,7 @@ package network import ( "context" + "errors" "fmt" "net" "slices" @@ -106,14 +107,18 @@ func (s *Service) createNetwork(ctx context.Context) (*hcloud.Network, error) { func (s *Service) createOpts() (hcloud.NetworkCreateOpts, error) { spec := s.scope.HetznerCluster.Spec.HCloudNetwork - _, network, err := net.ParseCIDR(spec.CIDRBlock) + if spec.CIDRBlock == nil || spec.SubnetCIDRBlock == nil || spec.NetworkZone == nil { + return hcloud.NetworkCreateOpts{}, errors.New("nil CIDRs or NetworkZone given") + } + + _, network, err := net.ParseCIDR(*spec.CIDRBlock) if err != nil { - return hcloud.NetworkCreateOpts{}, fmt.Errorf("invalid network %q: %w", spec.CIDRBlock, err) + return hcloud.NetworkCreateOpts{}, fmt.Errorf("invalid network %q: %w", *spec.CIDRBlock, err) } - _, subnet, err := net.ParseCIDR(spec.SubnetCIDRBlock) + _, subnet, err := net.ParseCIDR(*spec.SubnetCIDRBlock) if err != nil { - return hcloud.NetworkCreateOpts{}, fmt.Errorf("invalid network %q: %w", spec.SubnetCIDRBlock, err) + return hcloud.NetworkCreateOpts{}, fmt.Errorf("invalid network %q: %w", *spec.SubnetCIDRBlock, err) } return hcloud.NetworkCreateOpts{ @@ -123,7 +128,7 @@ func (s *Service) createOpts() (hcloud.NetworkCreateOpts, error) { Subnets: []hcloud.NetworkSubnet{ { IPRange: subnet, - NetworkZone: hcloud.NetworkZone(spec.NetworkZone), + NetworkZone: hcloud.NetworkZone(*spec.NetworkZone), Type: hcloud.NetworkSubnetTypeCloud, }, }, @@ -155,6 +160,24 @@ func (s *Service) Delete(ctx context.Context) error { } func (s *Service) findNetwork(ctx context.Context) (*hcloud.Network, error) { + // if an ID was provided we want to use the existing Network. + id := s.scope.HetznerCluster.Spec.HCloudNetwork.ID + if id != nil { + network, err := s.scope.HCloudClient.GetNetwork(ctx, *id) + if err != nil { + hcloudutil.HandleRateLimitExceeded(s.scope.HetznerCluster, err, "GetNetwork") + return nil, fmt.Errorf("failed to get network %d: %w", *id, err) + } + + 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 + } + } + opts := hcloud.NetworkListOpts{} opts.LabelSelector = utils.LabelsToLabelSelector(s.labels()) diff --git a/pkg/services/hcloud/network/network_test.go b/pkg/services/hcloud/network/network_test.go index 3bfcf61a9..6d85643fd 100644 --- a/pkg/services/hcloud/network/network_test.go +++ b/pkg/services/hcloud/network/network_test.go @@ -17,15 +17,18 @@ limitations under the License. package network import ( + "context" "net" "testing" "github.com/hetznercloud/hcloud-go/v2/hcloud" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "k8s.io/utils/ptr" infrav1 "github.com/syself/cluster-api-provider-hetzner/api/v1beta1" "github.com/syself/cluster-api-provider-hetzner/pkg/scope" + fakeclient "github.com/syself/cluster-api-provider-hetzner/pkg/services/hcloud/client/fake" ) func TestNetwork(t *testing.T) { @@ -39,9 +42,9 @@ var _ = Describe("Test createOpts", func() { BeforeEach(func() { hetznerCluster.Spec.HCloudNetwork = infrav1.HCloudNetworkSpec{ Enabled: true, - CIDRBlock: "10.0.0.0/16", - SubnetCIDRBlock: "10.0.0.0/24", - NetworkZone: "eu-central", + CIDRBlock: ptr.To(infrav1.DefaultCIDRBlock), + SubnetCIDRBlock: ptr.To(infrav1.DefaultSubnetCIDRBlock), + NetworkZone: ptr.To[infrav1.HCloudNetworkZone](infrav1.DefaultNetworkZone), } hetznerCluster.Name = "hetzner-cluster" @@ -73,14 +76,115 @@ var _ = Describe("Test createOpts", func() { }) It("gives an error with wrong CIDRBlock", func() { - hetznerCluster.Spec.HCloudNetwork.CIDRBlock = "invalid-cidr-block" + hetznerCluster.Spec.HCloudNetwork.CIDRBlock = ptr.To("invalid-cidr-block") _, err := service.createOpts() Expect(err).ToNot(BeNil()) }) It("gives an error with wrong SubnetCIDRBlock", func() { - hetznerCluster.Spec.HCloudNetwork.SubnetCIDRBlock = "invalid-cidr-block" + hetznerCluster.Spec.HCloudNetwork.SubnetCIDRBlock = ptr.To("invalid-cidr-block") _, err := service.createOpts() Expect(err).ToNot(BeNil()) }) + + It("gives an error with nil CIDRBlock", func() { + hetznerCluster.Spec.HCloudNetwork.CIDRBlock = nil + _, err := service.createOpts() + Expect(err).ToNot(BeNil()) + }) + + It("gives an error with nil SubnetCIDRBlock", func() { + hetznerCluster.Spec.HCloudNetwork.SubnetCIDRBlock = nil + _, err := service.createOpts() + Expect(err).ToNot(BeNil()) + }) + + It("gives an error with nil NetworkZone", func() { + hetznerCluster.Spec.HCloudNetwork.NetworkZone = nil + _, err := service.createOpts() + Expect(err).ToNot(BeNil()) + }) +}) + +var _ = Describe("Test findNetwork", func() { + var hetznerCluster infrav1.HetznerCluster + var service Service + var network *hcloud.Network + client := fakeclient.NewHCloudClientFactory().NewClient("") + + BeforeEach(func() { + hetznerCluster.Spec.HCloudNetwork = infrav1.HCloudNetworkSpec{ + Enabled: true, + CIDRBlock: ptr.To(infrav1.DefaultCIDRBlock), + SubnetCIDRBlock: ptr.To(infrav1.DefaultSubnetCIDRBlock), + NetworkZone: ptr.To[infrav1.HCloudNetworkZone](infrav1.DefaultNetworkZone), + } + hetznerCluster.Name = "hetzner-cluster" + + service = Service{&scope.ClusterScope{HetznerCluster: &hetznerCluster, HCloudClient: client}} + }) + AfterEach(func() { + err := client.DeleteNetwork(context.Background(), network) + Expect(err).To(Succeed()) + }) + It("Gets the Network if ID is set", func() { + hetznerCluster.Spec.HCloudNetwork.ID = ptr.To(int64(1)) + + var err error + network, err = client.CreateNetwork(context.Background(), hcloud.NetworkCreateOpts{Name: "networkName"}) + Expect(err).To(Succeed()) + res, err := service.findNetwork(context.Background()) + Expect(err).To(BeNil()) + Expect(res).To(Equal(network)) + }) + It("Finds the labeled Network if ID is not set", func() { + var err error + network, err = client.CreateNetwork(context.Background(), hcloud.NetworkCreateOpts{ + Name: "networkName", + Labels: map[string]string{ + hetznerCluster.ClusterTagKey(): string(infrav1.ResourceLifecycleOwned), + }, + }) + Expect(err).To(Succeed()) + res, err := service.findNetwork(context.Background()) + Expect(err).To(BeNil()) + Expect(res).To(Equal(network)) + }) + It("gives an error when there is more than one Network", func() { + var err error + network, err = client.CreateNetwork(context.Background(), hcloud.NetworkCreateOpts{ + Name: "networkName", + Labels: map[string]string{ + hetznerCluster.ClusterTagKey(): string(infrav1.ResourceLifecycleOwned), + }, + }) + Expect(err).To(Succeed()) + network2, err := client.CreateNetwork(context.Background(), hcloud.NetworkCreateOpts{ + Name: "networkName2", + Labels: map[string]string{ + hetznerCluster.ClusterTagKey(): string(infrav1.ResourceLifecycleOwned), + }, + }) + Expect(err).To(Succeed()) + res, err := service.findNetwork(context.Background()) + Expect(res).To(BeNil()) + Expect(err).ToNot(BeNil()) + + err = client.DeleteNetwork(context.Background(), network2) + Expect(err).To(Succeed()) + }) + It("gives an error when there is more than one Subnet", func() { + var err error + network, err = client.CreateNetwork(context.Background(), hcloud.NetworkCreateOpts{ + Name: "networkName", + Labels: map[string]string{ + hetznerCluster.ClusterTagKey(): string(infrav1.ResourceLifecycleOwned), + }, + Subnets: make([]hcloud.NetworkSubnet, 2), + }) + Expect(err).To(Succeed()) + res, err := service.findNetwork(context.Background()) + Expect(res).To(BeNil()) + Expect(err).ToNot(BeNil()) + }) }) From 0baf8c1d0b575f8764771c98ecea79c6d292c312 Mon Sep 17 00:00:00 2001 From: Johannes Frey Date: Sat, 31 Aug 2024 13:20:41 +0200 Subject: [PATCH 04/10] Only delete network if it has our label --- controllers/hetznercluster_controller.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/controllers/hetznercluster_controller.go b/controllers/hetznercluster_controller.go index 4e33e055f..517ba1ee0 100644 --- a/controllers/hetznercluster_controller.go +++ b/controllers/hetznercluster_controller.go @@ -346,9 +346,15 @@ 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 - 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 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 placement groups From ab5ad1dee08031cd35fb7535542809235e91e4c6 Mon Sep 17 00:00:00 2001 From: Johannes Frey Date: Sat, 31 Aug 2024 13:21:05 +0200 Subject: [PATCH 05/10] Add validation and defaulting --- api/v1beta1/hetznercluster_webhook.go | 157 +++++++++++++++++++++++++- 1 file changed, 151 insertions(+), 6 deletions(-) diff --git a/api/v1beta1/hetznercluster_webhook.go b/api/v1beta1/hetznercluster_webhook.go index 72c0746e2..c888ba77f 100644 --- a/api/v1beta1/hetznercluster_webhook.go +++ b/api/v1beta1/hetznercluster_webhook.go @@ -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" @@ -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() } @@ -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 { + 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} @@ -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 { + 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 { + 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 { + _, _, 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 @@ -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) { 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 { + // 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) { + 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 { + 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 @@ -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 +} From 1a69d44f234bb240c0d004d1cd2b4334a8c7f3f3 Mon Sep 17 00:00:00 2001 From: Johannes Frey Date: Sat, 31 Aug 2024 13:21:26 +0200 Subject: [PATCH 06/10] Add tests for existing network --- controllers/hetznercluster_controller_test.go | 269 ++++++++++++++++++ 1 file changed, 269 insertions(+) diff --git a/controllers/hetznercluster_controller_test.go b/controllers/hetznercluster_controller_test.go index 4365ab068..83e3a7687 100644 --- a/controllers/hetznercluster_controller_test.go +++ b/controllers/hetznercluster_controller_test.go @@ -25,6 +25,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/klog/v2" "k8s.io/utils/ptr" @@ -879,6 +880,274 @@ var _ = Describe("Hetzner ClusterReconciler", func() { }, timeout).Should(BeTrue()) }, ) + + Describe("For an existing Network", func() { + It("should attach the existing unlabeled Network by ID and not create a new one", func() { + networkName := utils.GenerateName(nil, "network1-") + network, err := hcloudClient.CreateNetwork(context.Background(), hcloud.NetworkCreateOpts{Name: networkName}) + Expect(err).To(Succeed()) + defer func() { + err := hcloudClient.DeleteNetwork(context.Background(), network) + Expect(err).To(Succeed()) + }() + networksBeforeClusterCreate, err := hcloudClient.ListNetworks(context.Background(), hcloud.NetworkListOpts{}) + Expect(err).To(Succeed()) + + hetznerClusterName := utils.GenerateName(nil, "test1-") + + capiCluster := &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "capi-test1-", + Namespace: namespace, + Finalizers: []string{clusterv1.ClusterFinalizer}, + }, + Spec: clusterv1.ClusterSpec{ + InfrastructureRef: &corev1.ObjectReference{ + APIVersion: infrav1.GroupVersion.String(), + Kind: "HetznerCluster", + Name: hetznerClusterName, + Namespace: namespace, + }, + }, + } + Expect(testEnv.Create(ctx, capiCluster)).To(Succeed()) + defer func() { + Expect(testEnv.Cleanup(ctx, capiCluster)).To(Succeed()) + }() + + instance := &infrav1.HetznerCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: hetznerClusterName, + Namespace: namespace, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "cluster.x-k8s.io/v1beta1", + Kind: "Cluster", + Name: capiCluster.Name, + UID: capiCluster.UID, + }, + }, + }, + Spec: getDefaultHetznerClusterSpec(), + } + // the creation of a HetznerCluster should not lead to the creation of a network when the ID of an + // existing network was given. + instance.Spec.HCloudNetwork.ID = ptr.To(network.ID) + Expect(testEnv.Create(ctx, instance)).To(Succeed()) + defer func() { + Expect(testEnv.Cleanup(ctx, instance)).To(Succeed()) + }() + + key := client.ObjectKey{Namespace: instance.Namespace, Name: instance.Name} + + Eventually(func() bool { + if err := testEnv.Get(ctx, key, instance); err != nil { + return false + } + if isPresentAndTrue(key, instance, infrav1.NetworkReadyCondition) && instance.Status.Network != nil { + return true + } + return false + }, timeout).Should(BeTrue()) + + networksAfterClusterCreate, err := hcloudClient.ListNetworks(ctx, hcloud.NetworkListOpts{}) + Expect(err).To(Succeed()) + + Expect(len(networksAfterClusterCreate)).To(Equal(len(networksBeforeClusterCreate))) + + var found bool + for _, n := range networksAfterClusterCreate { + if n.ID == instance.Status.Network.ID { + found = true + break + } + } + Expect(found).To(Equal(true)) + }) + It("should not delete the existing unlabeled Network when deleting the Cluster", func() { + networkName := utils.GenerateName(nil, "network2-") + network, err := hcloudClient.CreateNetwork(context.Background(), hcloud.NetworkCreateOpts{Name: networkName}) + Expect(err).To(Succeed()) + defer func() { + err := hcloudClient.DeleteNetwork(context.Background(), network) + Expect(err).To(Succeed()) + }() + networksBeforeClusterDelete, err := hcloudClient.ListNetworks(context.Background(), hcloud.NetworkListOpts{}) + Expect(err).To(Succeed()) + + hetznerClusterName := utils.GenerateName(nil, "test1-") + + capiCluster := &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "capi-test1-", + Namespace: namespace, + Finalizers: []string{clusterv1.ClusterFinalizer}, + }, + Spec: clusterv1.ClusterSpec{ + InfrastructureRef: &corev1.ObjectReference{ + APIVersion: infrav1.GroupVersion.String(), + Kind: "HetznerCluster", + Name: hetznerClusterName, + Namespace: namespace, + }, + }, + } + Expect(testEnv.Create(ctx, capiCluster)).To(Succeed()) + + instance := &infrav1.HetznerCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: hetznerClusterName, + Namespace: namespace, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "cluster.x-k8s.io/v1beta1", + Kind: "Cluster", + Name: capiCluster.Name, + UID: capiCluster.UID, + }, + }, + }, + Spec: getDefaultHetznerClusterSpec(), + } + instance.Spec.HCloudNetwork.ID = ptr.To(network.ID) + Expect(testEnv.Create(ctx, instance)).To(Succeed()) + + key := client.ObjectKey{Namespace: instance.Namespace, Name: instance.Name} + + var networkID *int64 + Eventually(func() bool { + if err := testEnv.Get(ctx, key, instance); err != nil { + return false + } + if isPresentAndTrue(key, instance, infrav1.NetworkReadyCondition) && instance.Status.Network != nil { + networkID = &instance.Status.Network.ID + return true + } + return false + }, timeout).Should(BeTrue()) + + Expect(networkID).ToNot(BeNil()) + + // the deletion of a HetznerCluster should not lead to the deletion of an existing network + // when the network misses the `owned` label. + Expect(testEnv.Cleanup(ctx, instance, capiCluster)).To(Succeed()) + + Eventually(func() bool { + if err := testEnv.Get(ctx, client.ObjectKey{Namespace: instance.Namespace, Name: instance.Name}, instance); err != nil && apierrors.IsNotFound(err) { + return true + } else if err != nil { + return false + } + return false + }, timeout).Should(BeTrue()) + + networksAfterClusterDelete, err := hcloudClient.ListNetworks(ctx, hcloud.NetworkListOpts{}) + Expect(err).To(Succeed()) + + Expect(len(networksAfterClusterDelete)).To(Equal(len(networksBeforeClusterDelete))) + + var found bool + for _, n := range networksAfterClusterDelete { + if n.ID == *networkID { + found = true + break + } + } + Expect(found).To(Equal(true)) + }) + It(`should delete the existing "owned" labeled Network when deleting the Cluster`, func() { + hetznerClusterName := utils.GenerateName(nil, "test1-") + + capiCluster := &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "capi-test1-", + Namespace: namespace, + Finalizers: []string{clusterv1.ClusterFinalizer}, + }, + Spec: clusterv1.ClusterSpec{ + InfrastructureRef: &corev1.ObjectReference{ + APIVersion: infrav1.GroupVersion.String(), + Kind: "HetznerCluster", + Name: hetznerClusterName, + Namespace: namespace, + }, + }, + } + Expect(testEnv.Create(ctx, capiCluster)).To(Succeed()) + + instance := &infrav1.HetznerCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: hetznerClusterName, + Namespace: namespace, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "cluster.x-k8s.io/v1beta1", + Kind: "Cluster", + Name: capiCluster.Name, + UID: capiCluster.UID, + }, + }, + }, + Spec: getDefaultHetznerClusterSpec(), + } + + networkName := utils.GenerateName(nil, "network3-") + network, err := hcloudClient.CreateNetwork(context.Background(), hcloud.NetworkCreateOpts{ + Name: networkName, + Labels: map[string]string{instance.ClusterTagKey(): "owned"}, + }) + Expect(err).To(Succeed()) + + networksBeforeClusterDelete, err := hcloudClient.ListNetworks(context.Background(), hcloud.NetworkListOpts{}) + Expect(err).To(Succeed()) + + instance.Spec.HCloudNetwork.ID = ptr.To(network.ID) + Expect(testEnv.Create(ctx, instance)).To(Succeed()) + + key := client.ObjectKey{Namespace: instance.Namespace, Name: instance.Name} + + var networkID *int64 + Eventually(func() bool { + if err := testEnv.Get(ctx, key, instance); err != nil { + return false + } + if isPresentAndTrue(key, instance, infrav1.NetworkReadyCondition) && instance.Status.Network != nil { + networkID = &instance.Status.Network.ID + return true + } + return false + }, timeout).Should(BeTrue()) + + Expect(networkID).ToNot(BeNil()) + + // As the network has the `owned` label, the deletion of a HetznerCluster will also lead to the + // deletion of the network. + Expect(testEnv.Cleanup(ctx, instance, capiCluster)).To(Succeed()) + + Eventually(func() bool { + if err := testEnv.Get(ctx, client.ObjectKey{Namespace: instance.Namespace, Name: instance.Name}, instance); err != nil && apierrors.IsNotFound(err) { + return true + } else if err != nil { + return false + } + return false + }, timeout).Should(BeTrue()) + + networksAfterClusterDelete, err := hcloudClient.ListNetworks(ctx, hcloud.NetworkListOpts{}) + Expect(err).To(Succeed()) + + Expect(len(networksAfterClusterDelete)).To(Equal(len(networksBeforeClusterDelete) - 1)) + + var found bool + for _, n := range networksAfterClusterDelete { + if n.ID == *networkID { + found = true + break + } + } + Expect(found).To(Not(Equal(true))) + }) + }) }) }) }) From 028f798cf5a8c3dff81b46479afa90fc2fd1aa2c Mon Sep 17 00:00:00 2001 From: Johannes Frey Date: Wed, 13 Nov 2024 18:07:01 +0100 Subject: [PATCH 07/10] 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 } From 7b361362cf77c812515b8f434294ba86449d38df Mon Sep 17 00:00:00 2001 From: Johannes Frey Date: Thu, 21 Nov 2024 06:53:33 +0100 Subject: [PATCH 08/10] Review suggestions 2 --- api/v1beta1/hetznercluster_webhook.go | 80 ++++++++++--------- api/v1beta1/types.go | 4 +- ...ture.cluster.x-k8s.io_hetznerclusters.yaml | 4 +- ...ster.x-k8s.io_hetznerclustertemplates.yaml | 4 +- pkg/services/hcloud/network/network.go | 3 +- 5 files changed, 51 insertions(+), 44 deletions(-) diff --git a/api/v1beta1/hetznercluster_webhook.go b/api/v1beta1/hetznercluster_webhook.go index c888ba77f..b95496910 100644 --- a/api/v1beta1/hetznercluster_webhook.go +++ b/api/v1beta1/hetznercluster_webhook.go @@ -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 @@ -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. @@ -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"), + ) } } @@ -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"), ) diff --git a/api/v1beta1/types.go b/api/v1beta1/types.go index 3097466d6..9f76f39dd 100644 --- a/api/v1beta1/types.go +++ b/api/v1beta1/types.go @@ -232,7 +232,7 @@ 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 @@ -240,7 +240,7 @@ type HCloudNetworkSpec struct { // 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"` 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 f8105619a..0fbc3d11a 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_hetznerclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_hetznerclusters.yaml @@ -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 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 3a69a0f0b..3e63c22db 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_hetznerclustertemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_hetznerclustertemplates.yaml @@ -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 diff --git a/pkg/services/hcloud/network/network.go b/pkg/services/hcloud/network/network.go index 24df1ffc2..d97d30e8d 100644 --- a/pkg/services/hcloud/network/network.go +++ b/pkg/services/hcloud/network/network.go @@ -19,7 +19,6 @@ package network import ( "context" - "errors" "fmt" "net" "slices" @@ -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) From eca91ee7a102a4bb995c1e0b716d36012559ebc2 Mon Sep 17 00:00:00 2001 From: Johannes Frey Date: Sun, 24 Nov 2024 11:17:11 +0100 Subject: [PATCH 09/10] Review suggestions 2 --- api/v1beta1/hetznercluster_webhook.go | 26 ++++++++++++++++++++++---- pkg/services/hcloud/network/network.go | 3 ++- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/api/v1beta1/hetznercluster_webhook.go b/api/v1beta1/hetznercluster_webhook.go index b95496910..0649e71df 100644 --- a/api/v1beta1/hetznercluster_webhook.go +++ b/api/v1beta1/hetznercluster_webhook.go @@ -158,8 +158,14 @@ func (r *HetznerCluster) ValidateCreate() (admission.Warnings, error) { allErrs = append(allErrs, errs...) } } else { - // If no ID is given check the other network settings for valid entries. - if r.Spec.HCloudNetwork.NetworkZone != nil { + if r.Spec.HCloudNetwork.NetworkZone == nil { + allErrs = append(allErrs, field.Invalid( + field.NewPath("spec", "hcloudNetwork", "networkZone"), + r.Spec.HCloudNetwork.NetworkZone, + "network zone must not be nil when hcloudNetwork is enabled"), + ) + // If no ID is given check the other network settings for valid entries. + } else { givenZone := string(*r.Spec.HCloudNetwork.NetworkZone) var validNetworkZone bool @@ -178,7 +184,13 @@ func (r *HetznerCluster) ValidateCreate() (admission.Warnings, error) { } } - if r.Spec.HCloudNetwork.CIDRBlock != nil { + if r.Spec.HCloudNetwork.CIDRBlock == nil { + allErrs = append(allErrs, field.Invalid( + field.NewPath("spec", "hcloudNetwork", "cidrBlock"), + r.Spec.HCloudNetwork.NetworkZone, + "cidrBlock must not be nil when hcloudNetwork is enabled"), + ) + } else { _, _, err := net.ParseCIDR(*r.Spec.HCloudNetwork.CIDRBlock) if err != nil { allErrs = append(allErrs, field.Invalid( @@ -189,7 +201,13 @@ func (r *HetznerCluster) ValidateCreate() (admission.Warnings, error) { } } - if r.Spec.HCloudNetwork.SubnetCIDRBlock != nil { + if r.Spec.HCloudNetwork.SubnetCIDRBlock == nil { + allErrs = append(allErrs, field.Invalid( + field.NewPath("spec", "hcloudNetwork", "subnetCIDRBlock"), + r.Spec.HCloudNetwork.SubnetCIDRBlock, + "subnetCIDRBlock must not be nil when hcloudNetwork is enabled"), + ) + } else { _, _, err := net.ParseCIDR(*r.Spec.HCloudNetwork.SubnetCIDRBlock) if err != nil { allErrs = append(allErrs, field.Invalid( diff --git a/pkg/services/hcloud/network/network.go b/pkg/services/hcloud/network/network.go index d97d30e8d..8d98b8b4f 100644 --- a/pkg/services/hcloud/network/network.go +++ b/pkg/services/hcloud/network/network.go @@ -86,8 +86,9 @@ func (s *Service) Reconcile(ctx context.Context) (err error) { infrav1.NetworkReadyCondition, infrav1.MultipleSubnetsExistReason, clusterv1.ConditionSeverityWarning, - "multiple subnets not allowed", + "multiple subnets exist", ) + record.Warnf(s.scope.HetznerCluster, "MultipleSubnetsExist", "Multiple subnets exist") return nil } From 4806de29a375b5727ac7f2bcfff8f5da14a4502a Mon Sep 17 00:00:00 2001 From: Johannes Frey Date: Sun, 24 Nov 2024 20:03:48 +0100 Subject: [PATCH 10/10] Return error when requested network is not found --- controllers/hetznercluster_controller_test.go | 16 ++-------------- pkg/services/hcloud/client/client.go | 6 ++++++ pkg/services/hcloud/client/fake/hcloud_client.go | 7 ++++++- pkg/services/hcloud/network/network.go | 14 ++++++++++++-- 4 files changed, 26 insertions(+), 17 deletions(-) diff --git a/controllers/hetznercluster_controller_test.go b/controllers/hetznercluster_controller_test.go index 83e3a7687..f5f29896a 100644 --- a/controllers/hetznercluster_controller_test.go +++ b/controllers/hetznercluster_controller_test.go @@ -886,10 +886,7 @@ var _ = Describe("Hetzner ClusterReconciler", func() { networkName := utils.GenerateName(nil, "network1-") network, err := hcloudClient.CreateNetwork(context.Background(), hcloud.NetworkCreateOpts{Name: networkName}) Expect(err).To(Succeed()) - defer func() { - err := hcloudClient.DeleteNetwork(context.Background(), network) - Expect(err).To(Succeed()) - }() + networksBeforeClusterCreate, err := hcloudClient.ListNetworks(context.Background(), hcloud.NetworkListOpts{}) Expect(err).To(Succeed()) @@ -911,9 +908,6 @@ var _ = Describe("Hetzner ClusterReconciler", func() { }, } Expect(testEnv.Create(ctx, capiCluster)).To(Succeed()) - defer func() { - Expect(testEnv.Cleanup(ctx, capiCluster)).To(Succeed()) - }() instance := &infrav1.HetznerCluster{ ObjectMeta: metav1.ObjectMeta{ @@ -934,9 +928,6 @@ var _ = Describe("Hetzner ClusterReconciler", func() { // existing network was given. instance.Spec.HCloudNetwork.ID = ptr.To(network.ID) Expect(testEnv.Create(ctx, instance)).To(Succeed()) - defer func() { - Expect(testEnv.Cleanup(ctx, instance)).To(Succeed()) - }() key := client.ObjectKey{Namespace: instance.Namespace, Name: instance.Name} @@ -968,10 +959,7 @@ var _ = Describe("Hetzner ClusterReconciler", func() { networkName := utils.GenerateName(nil, "network2-") network, err := hcloudClient.CreateNetwork(context.Background(), hcloud.NetworkCreateOpts{Name: networkName}) Expect(err).To(Succeed()) - defer func() { - err := hcloudClient.DeleteNetwork(context.Background(), network) - Expect(err).To(Succeed()) - }() + networksBeforeClusterDelete, err := hcloudClient.ListNetworks(context.Background(), hcloud.NetworkListOpts{}) Expect(err).To(Succeed()) diff --git a/pkg/services/hcloud/client/client.go b/pkg/services/hcloud/client/client.go index 606a4f8ff..fc69d7355 100644 --- a/pkg/services/hcloud/client/client.go +++ b/pkg/services/hcloud/client/client.go @@ -38,6 +38,9 @@ const errStringUnauthorized = "(unauthorized)" // ErrUnauthorized means that the API call is unauthorized. var ErrUnauthorized = fmt.Errorf("unauthorized") +// ErrNotFound means that the requested resource cannot be found. +var ErrNotFound = fmt.Errorf("not found") + // Client collects all methods used by the controller in the hcloud cloud API. type Client interface { // Reset resets the local cache. Only implemented in the fake client. @@ -303,6 +306,9 @@ func (c *realClient) ListNetworks(ctx context.Context, opts hcloud.NetworkListOp func (c *realClient) GetNetwork(ctx context.Context, id int64) (*hcloud.Network, error) { res, _, err := c.client.Network.GetByID(ctx, id) + if res == nil { + return nil, fmt.Errorf("%w: id: %d", ErrNotFound, id) + } return res, err } diff --git a/pkg/services/hcloud/client/fake/hcloud_client.go b/pkg/services/hcloud/client/fake/hcloud_client.go index e902a023a..70afb3ae7 100644 --- a/pkg/services/hcloud/client/fake/hcloud_client.go +++ b/pkg/services/hcloud/client/fake/hcloud_client.go @@ -653,7 +653,12 @@ func (c *cacheHCloudClient) ListNetworks(_ context.Context, opts hcloud.NetworkL return networks, nil } func (c *cacheHCloudClient) GetNetwork(_ context.Context, id int64) (*hcloud.Network, error) { - return c.networkCache.idMap[id], nil + n, found := c.networkCache.idMap[id] + if !found { + return nil, fmt.Errorf("%w: id: %d", hcloudclient.ErrNotFound, id) + } + + return n, nil } func (c *cacheHCloudClient) DeleteNetwork(_ context.Context, network *hcloud.Network) error { diff --git a/pkg/services/hcloud/network/network.go b/pkg/services/hcloud/network/network.go index 8d98b8b4f..245fe7e40 100644 --- a/pkg/services/hcloud/network/network.go +++ b/pkg/services/hcloud/network/network.go @@ -178,14 +178,24 @@ func (s *Service) Delete(ctx context.Context) error { return nil } +func (s *Service) findNetworkByID(ctx context.Context, id int64) (*hcloud.Network, error) { + network, err := s.scope.HCloudClient.GetNetwork(ctx, id) + if err != nil { + hcloudutil.HandleRateLimitExceeded(s.scope.HetznerCluster, err, "GetNetwork") + return nil, fmt.Errorf("failed to get network %d: %w", id, err) + } + + return network, nil +} + func (s *Service) findNetwork(ctx context.Context) (*hcloud.Network, error) { // if an ID was provided we want to use the existing Network. id := s.scope.HetznerCluster.Spec.HCloudNetwork.ID if id != nil { - network, err := s.scope.HCloudClient.GetNetwork(ctx, *id) + network, err := s.findNetworkByID(ctx, *id) if err != nil { hcloudutil.HandleRateLimitExceeded(s.scope.HetznerCluster, err, "GetNetwork") - return nil, fmt.Errorf("failed to get network %d: %w", *id, err) + return nil, fmt.Errorf("failed to find network with id %d: %w", *id, err) } if network != nil {