From b40f37726b6f4259a889de0c30b8d7245f08c394 Mon Sep 17 00:00:00 2001 From: Hans Rakers Date: Mon, 12 Aug 2024 16:24:01 +0200 Subject: [PATCH] feat: Allow firewall rules to be defined for the API server load balancer also allows load balancing additional ports to the API server --- .golangci.yml | 3 + .../cloudstackisolatednetwork_conversion.go | 17 + .../cloudstackmachinetemplate_conversion.go | 8 +- api/v1beta1/conversion.go | 5 +- api/v1beta1/conversion_test.go | 6 + api/v1beta1/v1beta1_suite_test.go | 29 ++ api/v1beta1/zz_generated.conversion.go | 33 +- api/v1beta2/cloudstackcluster_conversion.go | 24 ++ .../cloudstackisolatednetwork_conversion.go | 16 + .../cloudstackmachinetemplate_conversion.go | 3 + api/v1beta2/zz_generated.conversion.go | 64 ++-- api/v1beta3/cloudstackcluster_types.go | 5 + .../cloudstackisolatednetwork_types.go | 13 +- api/v1beta3/types.go | 56 ++++ api/v1beta3/zz_generated.deepcopy.go | 67 +++- ...e.cluster.x-k8s.io_cloudstackclusters.yaml | 33 ++ ...r.x-k8s.io_cloudstackisolatednetworks.yaml | 25 +- controllers/cloudstackmachine_controller.go | 2 +- pkg/cloud/isolated_network.go | 301 +++++++++++++++--- pkg/cloud/isolated_network_test.go | 280 ++++++++++++++-- pkg/utils/strings/strings.go | 54 ++++ pkg/utils/strings/strings_test.go | 123 +++++++ test/dummies/v1beta3/vars.go | 22 +- 23 files changed, 1031 insertions(+), 158 deletions(-) create mode 100644 api/v1beta1/v1beta1_suite_test.go create mode 100644 api/v1beta3/types.go create mode 100644 pkg/utils/strings/strings.go create mode 100644 pkg/utils/strings/strings_test.go diff --git a/.golangci.yml b/.golangci.yml index fc2e86d7..1e747287 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -52,3 +52,6 @@ issues: - path: _test\.go linters: - gosec + - text: "SA1019: .+LBRuleID is deprecated" + linters: + - staticcheck \ No newline at end of file diff --git a/api/v1beta1/cloudstackisolatednetwork_conversion.go b/api/v1beta1/cloudstackisolatednetwork_conversion.go index 4fe3e577..37a64b49 100644 --- a/api/v1beta1/cloudstackisolatednetwork_conversion.go +++ b/api/v1beta1/cloudstackisolatednetwork_conversion.go @@ -19,6 +19,7 @@ package v1beta1 import ( machineryconversion "k8s.io/apimachinery/pkg/conversion" "sigs.k8s.io/cluster-api-provider-cloudstack/api/v1beta3" + infrav1 "sigs.k8s.io/cluster-api-provider-cloudstack/api/v1beta3" utilconversion "sigs.k8s.io/cluster-api/util/conversion" "sigs.k8s.io/controller-runtime/pkg/conversion" ) @@ -53,3 +54,19 @@ func (dst *CloudStackIsolatedNetwork) ConvertFrom(srcRaw conversion.Hub) error { func Convert_v1beta3_CloudStackIsolatedNetworkSpec_To_v1beta1_CloudStackIsolatedNetworkSpec(in *v1beta3.CloudStackIsolatedNetworkSpec, out *CloudStackIsolatedNetworkSpec, s machineryconversion.Scope) error { // nolint return autoConvert_v1beta3_CloudStackIsolatedNetworkSpec_To_v1beta1_CloudStackIsolatedNetworkSpec(in, out, s) } + +func Convert_v1beta1_CloudStackIsolatedNetworkStatus_To_v1beta3_CloudStackIsolatedNetworkStatus(in *CloudStackIsolatedNetworkStatus, out *v1beta3.CloudStackIsolatedNetworkStatus, s machineryconversion.Scope) error { + out.PublicIPID = in.PublicIPID + out.LBRuleID = in.LBRuleID + out.APIServerLoadBalancer = &infrav1.LoadBalancer{} + out.LoadBalancerRuleIDs = []string{in.LBRuleID} + out.Ready = in.Ready + return nil +} + +func Convert_v1beta3_CloudStackIsolatedNetworkStatus_To_v1beta1_CloudStackIsolatedNetworkStatus(in *v1beta3.CloudStackIsolatedNetworkStatus, out *CloudStackIsolatedNetworkStatus, s machineryconversion.Scope) error { + out.PublicIPID = in.PublicIPID + out.LBRuleID = in.LBRuleID + out.Ready = in.Ready + return nil +} diff --git a/api/v1beta1/cloudstackmachinetemplate_conversion.go b/api/v1beta1/cloudstackmachinetemplate_conversion.go index 93c3cd33..bd672f87 100644 --- a/api/v1beta1/cloudstackmachinetemplate_conversion.go +++ b/api/v1beta1/cloudstackmachinetemplate_conversion.go @@ -17,13 +17,12 @@ limitations under the License. package v1beta1 import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" machineryconversion "k8s.io/apimachinery/pkg/conversion" "sigs.k8s.io/cluster-api-provider-cloudstack/api/v1beta3" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" utilconversion "sigs.k8s.io/cluster-api/util/conversion" "sigs.k8s.io/controller-runtime/pkg/conversion" - - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) func (src *CloudStackMachineTemplate) ConvertTo(dstRaw conversion.Hub) error { // nolint @@ -43,6 +42,9 @@ func (src *CloudStackMachineTemplate) ConvertTo(dstRaw conversion.Hub) error { / if restored.Spec.Template.Spec.UncompressedUserData != nil { dst.Spec.Template.Spec.UncompressedUserData = restored.Spec.Template.Spec.UncompressedUserData } + + dst.Spec.Template.ObjectMeta = restored.Spec.Template.ObjectMeta + return nil } diff --git a/api/v1beta1/conversion.go b/api/v1beta1/conversion.go index 59b8839f..fa11a414 100644 --- a/api/v1beta1/conversion.go +++ b/api/v1beta1/conversion.go @@ -18,8 +18,7 @@ package v1beta1 import ( "context" - "fmt" - + "errors" corev1 "k8s.io/api/core/v1" machineryconversion "k8s.io/apimachinery/pkg/conversion" infrav1 "sigs.k8s.io/cluster-api-provider-cloudstack/api/v1beta3" @@ -51,7 +50,7 @@ func Convert_v1beta1_CloudStackCluster_To_v1beta3_CloudStackCluster(in *CloudSta //nolint:golint,revive,stylecheck func Convert_v1beta3_CloudStackCluster_To_v1beta1_CloudStackCluster(in *infrav1.CloudStackCluster, out *CloudStackCluster, _ machineryconversion.Scope) error { if len(in.Spec.FailureDomains) < 1 { - return fmt.Errorf("infrav1 to v1beta1 conversion not supported when < 1 failure domain is provided. Input CloudStackCluster spec %s", in.Spec) + return errors.New("infrav1 to v1beta1 conversion not supported when < 1 failure domain is provided") } out.ObjectMeta = in.ObjectMeta out.Spec = CloudStackClusterSpec{ diff --git a/api/v1beta1/conversion_test.go b/api/v1beta1/conversion_test.go index 32ffcd37..4da5fbe6 100644 --- a/api/v1beta1/conversion_test.go +++ b/api/v1beta1/conversion_test.go @@ -21,6 +21,7 @@ import ( . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/pointer" v1beta1 "sigs.k8s.io/cluster-api-provider-cloudstack/api/v1beta1" "sigs.k8s.io/cluster-api-provider-cloudstack/api/v1beta3" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" @@ -103,6 +104,11 @@ var _ = Describe("Conversion", func() { Host: "endpoint1", Port: 443, }, + APIServerLoadBalancer: &v1beta3.APIServerLoadBalancer{ + Enabled: pointer.Bool(true), + AdditionalPorts: []int{}, + AllowedCIDRs: []string{}, + }, }, Status: v1beta3.CloudStackClusterStatus{}, } diff --git a/api/v1beta1/v1beta1_suite_test.go b/api/v1beta1/v1beta1_suite_test.go new file mode 100644 index 00000000..4c49498e --- /dev/null +++ b/api/v1beta1/v1beta1_suite_test.go @@ -0,0 +1,29 @@ +/* +Copyright 2024 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1beta1_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestV1beta1(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "V1beta1 Suite") +} diff --git a/api/v1beta1/zz_generated.conversion.go b/api/v1beta1/zz_generated.conversion.go index 851a2286..f0b6e099 100644 --- a/api/v1beta1/zz_generated.conversion.go +++ b/api/v1beta1/zz_generated.conversion.go @@ -98,16 +98,6 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } - if err := s.AddGeneratedConversionFunc((*CloudStackIsolatedNetworkStatus)(nil), (*v1beta3.CloudStackIsolatedNetworkStatus)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1beta1_CloudStackIsolatedNetworkStatus_To_v1beta3_CloudStackIsolatedNetworkStatus(a.(*CloudStackIsolatedNetworkStatus), b.(*v1beta3.CloudStackIsolatedNetworkStatus), scope) - }); err != nil { - return err - } - if err := s.AddGeneratedConversionFunc((*v1beta3.CloudStackIsolatedNetworkStatus)(nil), (*CloudStackIsolatedNetworkStatus)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1beta3_CloudStackIsolatedNetworkStatus_To_v1beta1_CloudStackIsolatedNetworkStatus(a.(*v1beta3.CloudStackIsolatedNetworkStatus), b.(*CloudStackIsolatedNetworkStatus), scope) - }); err != nil { - return err - } if err := s.AddGeneratedConversionFunc((*CloudStackMachine)(nil), (*v1beta3.CloudStackMachine)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1beta1_CloudStackMachine_To_v1beta3_CloudStackMachine(a.(*CloudStackMachine), b.(*v1beta3.CloudStackMachine), scope) }); err != nil { @@ -248,6 +238,11 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } + if err := s.AddConversionFunc((*CloudStackIsolatedNetworkStatus)(nil), (*v1beta3.CloudStackIsolatedNetworkStatus)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1beta1_CloudStackIsolatedNetworkStatus_To_v1beta3_CloudStackIsolatedNetworkStatus(a.(*CloudStackIsolatedNetworkStatus), b.(*v1beta3.CloudStackIsolatedNetworkStatus), scope) + }); err != nil { + return err + } if err := s.AddConversionFunc((*CloudStackMachineSpec)(nil), (*v1beta3.CloudStackMachineSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1beta1_CloudStackMachineSpec_To_v1beta3_CloudStackMachineSpec(a.(*CloudStackMachineSpec), b.(*v1beta3.CloudStackMachineSpec), scope) }); err != nil { @@ -278,6 +273,11 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } + if err := s.AddConversionFunc((*v1beta3.CloudStackIsolatedNetworkStatus)(nil), (*CloudStackIsolatedNetworkStatus)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1beta3_CloudStackIsolatedNetworkStatus_To_v1beta1_CloudStackIsolatedNetworkStatus(a.(*v1beta3.CloudStackIsolatedNetworkStatus), b.(*CloudStackIsolatedNetworkStatus), scope) + }); err != nil { + return err + } if err := s.AddConversionFunc((*v1beta3.CloudStackMachineSpec)(nil), (*CloudStackMachineSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1beta3_CloudStackMachineSpec_To_v1beta1_CloudStackMachineSpec(a.(*v1beta3.CloudStackMachineSpec), b.(*CloudStackMachineSpec), scope) }); err != nil { @@ -517,23 +517,16 @@ func autoConvert_v1beta1_CloudStackIsolatedNetworkStatus_To_v1beta3_CloudStackIs return nil } -// Convert_v1beta1_CloudStackIsolatedNetworkStatus_To_v1beta3_CloudStackIsolatedNetworkStatus is an autogenerated conversion function. -func Convert_v1beta1_CloudStackIsolatedNetworkStatus_To_v1beta3_CloudStackIsolatedNetworkStatus(in *CloudStackIsolatedNetworkStatus, out *v1beta3.CloudStackIsolatedNetworkStatus, s conversion.Scope) error { - return autoConvert_v1beta1_CloudStackIsolatedNetworkStatus_To_v1beta3_CloudStackIsolatedNetworkStatus(in, out, s) -} - func autoConvert_v1beta3_CloudStackIsolatedNetworkStatus_To_v1beta1_CloudStackIsolatedNetworkStatus(in *v1beta3.CloudStackIsolatedNetworkStatus, out *CloudStackIsolatedNetworkStatus, s conversion.Scope) error { + // WARNING: in.CIDR requires manual conversion: does not exist in peer-type out.PublicIPID = in.PublicIPID out.LBRuleID = in.LBRuleID + // WARNING: in.LoadBalancerRuleIDs requires manual conversion: does not exist in peer-type + // WARNING: in.APIServerLoadBalancer requires manual conversion: does not exist in peer-type out.Ready = in.Ready return nil } -// Convert_v1beta3_CloudStackIsolatedNetworkStatus_To_v1beta1_CloudStackIsolatedNetworkStatus is an autogenerated conversion function. -func Convert_v1beta3_CloudStackIsolatedNetworkStatus_To_v1beta1_CloudStackIsolatedNetworkStatus(in *v1beta3.CloudStackIsolatedNetworkStatus, out *CloudStackIsolatedNetworkStatus, s conversion.Scope) error { - return autoConvert_v1beta3_CloudStackIsolatedNetworkStatus_To_v1beta1_CloudStackIsolatedNetworkStatus(in, out, s) -} - func autoConvert_v1beta1_CloudStackMachine_To_v1beta3_CloudStackMachine(in *CloudStackMachine, out *v1beta3.CloudStackMachine, s conversion.Scope) error { out.ObjectMeta = in.ObjectMeta if err := Convert_v1beta1_CloudStackMachineSpec_To_v1beta3_CloudStackMachineSpec(&in.Spec, &out.Spec, s); err != nil { diff --git a/api/v1beta2/cloudstackcluster_conversion.go b/api/v1beta2/cloudstackcluster_conversion.go index 31fd3c20..ed47000e 100644 --- a/api/v1beta2/cloudstackcluster_conversion.go +++ b/api/v1beta2/cloudstackcluster_conversion.go @@ -17,6 +17,8 @@ limitations under the License. package v1beta2 import ( + machineryconversion "k8s.io/apimachinery/pkg/conversion" + "k8s.io/utils/pointer" "sigs.k8s.io/cluster-api-provider-cloudstack/api/v1beta3" "sigs.k8s.io/controller-runtime/pkg/conversion" ) @@ -30,3 +32,25 @@ func (dst *CloudStackCluster) ConvertFrom(srcRaw conversion.Hub) error { // noli src := srcRaw.(*v1beta3.CloudStackCluster) return Convert_v1beta3_CloudStackCluster_To_v1beta2_CloudStackCluster(src, dst, nil) } + +func Convert_v1beta3_CloudStackClusterSpec_To_v1beta2_CloudStackClusterSpec(in *v1beta3.CloudStackClusterSpec, out *CloudStackClusterSpec, s machineryconversion.Scope) error { // nolint + err := autoConvert_v1beta3_CloudStackClusterSpec_To_v1beta2_CloudStackClusterSpec(in, out, s) + if err != nil { + return err + } + + return nil +} + +func Convert_v1beta2_CloudStackClusterSpec_To_v1beta3_CloudStackClusterSpec(in *CloudStackClusterSpec, out *v1beta3.CloudStackClusterSpec, s machineryconversion.Scope) error { // nolint + err := autoConvert_v1beta2_CloudStackClusterSpec_To_v1beta3_CloudStackClusterSpec(in, out, s) + if err != nil { + return err + } + + out.APIServerLoadBalancer = &v1beta3.APIServerLoadBalancer{ + Enabled: pointer.Bool(true), + } + + return nil +} diff --git a/api/v1beta2/cloudstackisolatednetwork_conversion.go b/api/v1beta2/cloudstackisolatednetwork_conversion.go index 44e824ff..51cccc8d 100644 --- a/api/v1beta2/cloudstackisolatednetwork_conversion.go +++ b/api/v1beta2/cloudstackisolatednetwork_conversion.go @@ -17,6 +17,7 @@ limitations under the License. package v1beta2 import ( + machineryconversion "k8s.io/apimachinery/pkg/conversion" "sigs.k8s.io/cluster-api-provider-cloudstack/api/v1beta3" "sigs.k8s.io/controller-runtime/pkg/conversion" ) @@ -30,3 +31,18 @@ func (dst *CloudStackIsolatedNetwork) ConvertFrom(srcRaw conversion.Hub) error { src := srcRaw.(*v1beta3.CloudStackIsolatedNetwork) return Convert_v1beta3_CloudStackIsolatedNetwork_To_v1beta2_CloudStackIsolatedNetwork(src, dst, nil) } + +func Convert_v1beta2_CloudStackIsolatedNetworkStatus_To_v1beta3_CloudStackIsolatedNetworkStatus(in *CloudStackIsolatedNetworkStatus, out *v1beta3.CloudStackIsolatedNetworkStatus, s machineryconversion.Scope) error { + out.PublicIPID = in.PublicIPID + out.LBRuleID = in.LBRuleID + out.LoadBalancerRuleIDs = []string{in.LBRuleID} + out.Ready = in.Ready + return nil +} + +func Convert_v1beta3_CloudStackIsolatedNetworkStatus_To_v1beta2_CloudStackIsolatedNetworkStatus(in *v1beta3.CloudStackIsolatedNetworkStatus, out *CloudStackIsolatedNetworkStatus, s machineryconversion.Scope) error { + out.PublicIPID = in.PublicIPID + out.LBRuleID = in.LBRuleID + out.Ready = in.Ready + return nil +} diff --git a/api/v1beta2/cloudstackmachinetemplate_conversion.go b/api/v1beta2/cloudstackmachinetemplate_conversion.go index 85964140..7621936b 100644 --- a/api/v1beta2/cloudstackmachinetemplate_conversion.go +++ b/api/v1beta2/cloudstackmachinetemplate_conversion.go @@ -42,6 +42,9 @@ func (src *CloudStackMachineTemplate) ConvertTo(dstRaw conversion.Hub) error { / if restored.Spec.Template.Spec.UncompressedUserData != nil { dst.Spec.Template.Spec.UncompressedUserData = restored.Spec.Template.Spec.UncompressedUserData } + + dst.Spec.Template.ObjectMeta = restored.Spec.Template.ObjectMeta + return nil } diff --git a/api/v1beta2/zz_generated.conversion.go b/api/v1beta2/zz_generated.conversion.go index 41dd8333..94eac74e 100644 --- a/api/v1beta2/zz_generated.conversion.go +++ b/api/v1beta2/zz_generated.conversion.go @@ -98,16 +98,6 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } - if err := s.AddGeneratedConversionFunc((*CloudStackClusterSpec)(nil), (*v1beta3.CloudStackClusterSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1beta2_CloudStackClusterSpec_To_v1beta3_CloudStackClusterSpec(a.(*CloudStackClusterSpec), b.(*v1beta3.CloudStackClusterSpec), scope) - }); err != nil { - return err - } - if err := s.AddGeneratedConversionFunc((*v1beta3.CloudStackClusterSpec)(nil), (*CloudStackClusterSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1beta3_CloudStackClusterSpec_To_v1beta2_CloudStackClusterSpec(a.(*v1beta3.CloudStackClusterSpec), b.(*CloudStackClusterSpec), scope) - }); err != nil { - return err - } if err := s.AddGeneratedConversionFunc((*CloudStackClusterStatus)(nil), (*v1beta3.CloudStackClusterStatus)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1beta2_CloudStackClusterStatus_To_v1beta3_CloudStackClusterStatus(a.(*CloudStackClusterStatus), b.(*v1beta3.CloudStackClusterStatus), scope) }); err != nil { @@ -183,16 +173,6 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } - if err := s.AddGeneratedConversionFunc((*CloudStackIsolatedNetworkStatus)(nil), (*v1beta3.CloudStackIsolatedNetworkStatus)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1beta2_CloudStackIsolatedNetworkStatus_To_v1beta3_CloudStackIsolatedNetworkStatus(a.(*CloudStackIsolatedNetworkStatus), b.(*v1beta3.CloudStackIsolatedNetworkStatus), scope) - }); err != nil { - return err - } - if err := s.AddGeneratedConversionFunc((*v1beta3.CloudStackIsolatedNetworkStatus)(nil), (*CloudStackIsolatedNetworkStatus)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1beta3_CloudStackIsolatedNetworkStatus_To_v1beta2_CloudStackIsolatedNetworkStatus(a.(*v1beta3.CloudStackIsolatedNetworkStatus), b.(*CloudStackIsolatedNetworkStatus), scope) - }); err != nil { - return err - } if err := s.AddGeneratedConversionFunc((*CloudStackMachine)(nil), (*v1beta3.CloudStackMachine)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1beta2_CloudStackMachine_To_v1beta3_CloudStackMachine(a.(*CloudStackMachine), b.(*v1beta3.CloudStackMachine), scope) }); err != nil { @@ -338,6 +318,16 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } + if err := s.AddConversionFunc((*CloudStackClusterSpec)(nil), (*v1beta3.CloudStackClusterSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1beta2_CloudStackClusterSpec_To_v1beta3_CloudStackClusterSpec(a.(*CloudStackClusterSpec), b.(*v1beta3.CloudStackClusterSpec), scope) + }); err != nil { + return err + } + if err := s.AddConversionFunc((*CloudStackIsolatedNetworkStatus)(nil), (*v1beta3.CloudStackIsolatedNetworkStatus)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1beta2_CloudStackIsolatedNetworkStatus_To_v1beta3_CloudStackIsolatedNetworkStatus(a.(*CloudStackIsolatedNetworkStatus), b.(*v1beta3.CloudStackIsolatedNetworkStatus), scope) + }); err != nil { + return err + } if err := s.AddConversionFunc((*CloudStackMachineSpec)(nil), (*v1beta3.CloudStackMachineSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1beta2_CloudStackMachineSpec_To_v1beta3_CloudStackMachineSpec(a.(*CloudStackMachineSpec), b.(*v1beta3.CloudStackMachineSpec), scope) }); err != nil { @@ -348,11 +338,21 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } + if err := s.AddConversionFunc((*v1beta3.CloudStackClusterSpec)(nil), (*CloudStackClusterSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1beta3_CloudStackClusterSpec_To_v1beta2_CloudStackClusterSpec(a.(*v1beta3.CloudStackClusterSpec), b.(*CloudStackClusterSpec), scope) + }); err != nil { + return err + } if err := s.AddConversionFunc((*v1beta3.CloudStackIsolatedNetworkSpec)(nil), (*CloudStackIsolatedNetworkSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1beta3_CloudStackIsolatedNetworkSpec_To_v1beta2_CloudStackIsolatedNetworkSpec(a.(*v1beta3.CloudStackIsolatedNetworkSpec), b.(*CloudStackIsolatedNetworkSpec), scope) }); err != nil { return err } + if err := s.AddConversionFunc((*v1beta3.CloudStackIsolatedNetworkStatus)(nil), (*CloudStackIsolatedNetworkStatus)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1beta3_CloudStackIsolatedNetworkStatus_To_v1beta2_CloudStackIsolatedNetworkStatus(a.(*v1beta3.CloudStackIsolatedNetworkStatus), b.(*CloudStackIsolatedNetworkStatus), scope) + }); err != nil { + return err + } if err := s.AddConversionFunc((*v1beta3.CloudStackMachineSpec)(nil), (*CloudStackMachineSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1beta3_CloudStackMachineSpec_To_v1beta2_CloudStackMachineSpec(a.(*v1beta3.CloudStackMachineSpec), b.(*CloudStackMachineSpec), scope) }); err != nil { @@ -561,11 +561,6 @@ func autoConvert_v1beta2_CloudStackClusterSpec_To_v1beta3_CloudStackClusterSpec( return nil } -// Convert_v1beta2_CloudStackClusterSpec_To_v1beta3_CloudStackClusterSpec is an autogenerated conversion function. -func Convert_v1beta2_CloudStackClusterSpec_To_v1beta3_CloudStackClusterSpec(in *CloudStackClusterSpec, out *v1beta3.CloudStackClusterSpec, s conversion.Scope) error { - return autoConvert_v1beta2_CloudStackClusterSpec_To_v1beta3_CloudStackClusterSpec(in, out, s) -} - func autoConvert_v1beta3_CloudStackClusterSpec_To_v1beta2_CloudStackClusterSpec(in *v1beta3.CloudStackClusterSpec, out *CloudStackClusterSpec, s conversion.Scope) error { if in.FailureDomains != nil { in, out := &in.FailureDomains, &out.FailureDomains @@ -579,14 +574,10 @@ func autoConvert_v1beta3_CloudStackClusterSpec_To_v1beta2_CloudStackClusterSpec( out.FailureDomains = nil } out.ControlPlaneEndpoint = in.ControlPlaneEndpoint + // WARNING: in.APIServerLoadBalancer requires manual conversion: does not exist in peer-type return nil } -// Convert_v1beta3_CloudStackClusterSpec_To_v1beta2_CloudStackClusterSpec is an autogenerated conversion function. -func Convert_v1beta3_CloudStackClusterSpec_To_v1beta2_CloudStackClusterSpec(in *v1beta3.CloudStackClusterSpec, out *CloudStackClusterSpec, s conversion.Scope) error { - return autoConvert_v1beta3_CloudStackClusterSpec_To_v1beta2_CloudStackClusterSpec(in, out, s) -} - func autoConvert_v1beta2_CloudStackClusterStatus_To_v1beta3_CloudStackClusterStatus(in *CloudStackClusterStatus, out *v1beta3.CloudStackClusterStatus, s conversion.Scope) error { out.FailureDomains = *(*v1beta1.FailureDomains)(unsafe.Pointer(&in.FailureDomains)) out.Ready = in.Ready @@ -838,23 +829,16 @@ func autoConvert_v1beta2_CloudStackIsolatedNetworkStatus_To_v1beta3_CloudStackIs return nil } -// Convert_v1beta2_CloudStackIsolatedNetworkStatus_To_v1beta3_CloudStackIsolatedNetworkStatus is an autogenerated conversion function. -func Convert_v1beta2_CloudStackIsolatedNetworkStatus_To_v1beta3_CloudStackIsolatedNetworkStatus(in *CloudStackIsolatedNetworkStatus, out *v1beta3.CloudStackIsolatedNetworkStatus, s conversion.Scope) error { - return autoConvert_v1beta2_CloudStackIsolatedNetworkStatus_To_v1beta3_CloudStackIsolatedNetworkStatus(in, out, s) -} - func autoConvert_v1beta3_CloudStackIsolatedNetworkStatus_To_v1beta2_CloudStackIsolatedNetworkStatus(in *v1beta3.CloudStackIsolatedNetworkStatus, out *CloudStackIsolatedNetworkStatus, s conversion.Scope) error { + // WARNING: in.CIDR requires manual conversion: does not exist in peer-type out.PublicIPID = in.PublicIPID out.LBRuleID = in.LBRuleID + // WARNING: in.LoadBalancerRuleIDs requires manual conversion: does not exist in peer-type + // WARNING: in.APIServerLoadBalancer requires manual conversion: does not exist in peer-type out.Ready = in.Ready return nil } -// Convert_v1beta3_CloudStackIsolatedNetworkStatus_To_v1beta2_CloudStackIsolatedNetworkStatus is an autogenerated conversion function. -func Convert_v1beta3_CloudStackIsolatedNetworkStatus_To_v1beta2_CloudStackIsolatedNetworkStatus(in *v1beta3.CloudStackIsolatedNetworkStatus, out *CloudStackIsolatedNetworkStatus, s conversion.Scope) error { - return autoConvert_v1beta3_CloudStackIsolatedNetworkStatus_To_v1beta2_CloudStackIsolatedNetworkStatus(in, out, s) -} - func autoConvert_v1beta2_CloudStackMachine_To_v1beta3_CloudStackMachine(in *CloudStackMachine, out *v1beta3.CloudStackMachine, s conversion.Scope) error { out.ObjectMeta = in.ObjectMeta if err := Convert_v1beta2_CloudStackMachineSpec_To_v1beta3_CloudStackMachineSpec(&in.Spec, &out.Spec, s); err != nil { diff --git a/api/v1beta3/cloudstackcluster_types.go b/api/v1beta3/cloudstackcluster_types.go index 309caefd..837dd0eb 100644 --- a/api/v1beta3/cloudstackcluster_types.go +++ b/api/v1beta3/cloudstackcluster_types.go @@ -34,6 +34,11 @@ type CloudStackClusterSpec struct { // The kubernetes control plane endpoint. ControlPlaneEndpoint clusterv1.APIEndpoint `json:"controlPlaneEndpoint"` + + // APIServerLoadBalancer configures the optional LoadBalancer for the APIServer. + // If not specified, no load balancer will be created for the API server. + //+optional + APIServerLoadBalancer *APIServerLoadBalancer `json:"apiServerLoadBalancer,omitempty"` } // The status of the CloudStackCluster object. diff --git a/api/v1beta3/cloudstackisolatednetwork_types.go b/api/v1beta3/cloudstackisolatednetwork_types.go index 1e5db27d..42da0db9 100644 --- a/api/v1beta3/cloudstackisolatednetwork_types.go +++ b/api/v1beta3/cloudstackisolatednetwork_types.go @@ -47,12 +47,23 @@ type CloudStackIsolatedNetworkSpec struct { // CloudStackIsolatedNetworkStatus defines the observed state of CloudStackIsolatedNetwork type CloudStackIsolatedNetworkStatus struct { + // The CIDR of the assigned subnet. + CIDR string `json:"cidr,omitempty"` + // The CS public IP ID to use for the k8s endpoint. PublicIPID string `json:"publicIPID,omitempty"` - // The ID of the lb rule used to assign VMs to the lb. + // Deprecated: The ID of the lb rule used to assign VMs to the lb. + // No longer used, see LoadBalancerRuleIDs. Will be removed in next API version. LBRuleID string `json:"loadBalancerRuleID,omitempty"` + // The IDs of the lb rule used to assign VMs to the lb. + LoadBalancerRuleIDs []string `json:"loadBalancerRuleIDs,omitempty"` + + // APIServerLoadBalancer describes the api server load balancer if one exists + //+optional + APIServerLoadBalancer *LoadBalancer `json:"apiServerLoadBalancer,omitempty"` + // Ready indicates the readiness of this provider resource. //+optional Ready bool `json:"ready"` diff --git a/api/v1beta3/types.go b/api/v1beta3/types.go new file mode 100644 index 00000000..564c62e0 --- /dev/null +++ b/api/v1beta3/types.go @@ -0,0 +1,56 @@ +/* +Copyright 2024 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1beta3 + +// LoadBalancer represents basic information about the associated OpenStack LoadBalancer. +type LoadBalancer struct { + IP string `json:"ip"` + //+optional + AllowedCIDRs []string `json:"allowedCIDRs,omitempty"` +} + +type APIServerLoadBalancer struct { + // Enabled defines whether a load balancer should be created. This value + // defaults to true if an APIServerLoadBalancer is given. + // + // There is no reason to set this to false. To disable creation of the + // API server loadbalancer, omit the APIServerLoadBalancer field in the + // cluster spec instead. + // + //+kubebuilder:validation:Required + //+kubebuilder:default:=true + Enabled *bool `json:"enabled"` + + // AdditionalPorts adds additional tcp ports to the load balancer. + //+optional + //+listType=set + AdditionalPorts []int `json:"additionalPorts,omitempty"` + + // AllowedCIDRs restrict access to all API-Server listeners to the given address CIDRs. + //+optional + //+listType=set + AllowedCIDRs []string `json:"allowedCIDRs,omitempty"` +} + +func (s *APIServerLoadBalancer) IsZero() bool { + return s == nil || ((s.Enabled == nil || !*s.Enabled) && len(s.AdditionalPorts) == 0 && len(s.AllowedCIDRs) == 0) +} + +func (s *APIServerLoadBalancer) IsEnabled() bool { + // The CRD default value for Enabled is true, so if the field is nil, it should be considered as true. + return s != nil && (s.Enabled == nil || *s.Enabled) +} diff --git a/api/v1beta3/zz_generated.deepcopy.go b/api/v1beta3/zz_generated.deepcopy.go index 66a2e486..92f5538c 100644 --- a/api/v1beta3/zz_generated.deepcopy.go +++ b/api/v1beta3/zz_generated.deepcopy.go @@ -26,6 +26,36 @@ import ( "sigs.k8s.io/cluster-api/api/v1beta1" ) +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *APIServerLoadBalancer) DeepCopyInto(out *APIServerLoadBalancer) { + *out = *in + if in.Enabled != nil { + in, out := &in.Enabled, &out.Enabled + *out = new(bool) + **out = **in + } + if in.AdditionalPorts != nil { + in, out := &in.AdditionalPorts, &out.AdditionalPorts + *out = make([]int, len(*in)) + copy(*out, *in) + } + if in.AllowedCIDRs != nil { + in, out := &in.AllowedCIDRs, &out.AllowedCIDRs + *out = make([]string, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new APIServerLoadBalancer. +func (in *APIServerLoadBalancer) DeepCopy() *APIServerLoadBalancer { + if in == nil { + return nil + } + out := new(APIServerLoadBalancer) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *CloudStackAffinityGroup) DeepCopyInto(out *CloudStackAffinityGroup) { *out = *in @@ -183,6 +213,11 @@ func (in *CloudStackClusterSpec) DeepCopyInto(out *CloudStackClusterSpec) { copy(*out, *in) } out.ControlPlaneEndpoint = in.ControlPlaneEndpoint + if in.APIServerLoadBalancer != nil { + in, out := &in.APIServerLoadBalancer, &out.APIServerLoadBalancer + *out = new(APIServerLoadBalancer) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CloudStackClusterSpec. @@ -314,7 +349,7 @@ func (in *CloudStackIsolatedNetwork) DeepCopyInto(out *CloudStackIsolatedNetwork out.TypeMeta = in.TypeMeta in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) out.Spec = in.Spec - out.Status = in.Status + in.Status.DeepCopyInto(&out.Status) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CloudStackIsolatedNetwork. @@ -386,6 +421,16 @@ func (in *CloudStackIsolatedNetworkSpec) DeepCopy() *CloudStackIsolatedNetworkSp // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *CloudStackIsolatedNetworkStatus) DeepCopyInto(out *CloudStackIsolatedNetworkStatus) { *out = *in + if in.LoadBalancerRuleIDs != nil { + in, out := &in.LoadBalancerRuleIDs, &out.LoadBalancerRuleIDs + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.APIServerLoadBalancer != nil { + in, out := &in.APIServerLoadBalancer, &out.APIServerLoadBalancer + *out = new(LoadBalancer) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CloudStackIsolatedNetworkStatus. @@ -769,6 +814,26 @@ func (in *CloudStackZoneSpec) DeepCopy() *CloudStackZoneSpec { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *LoadBalancer) DeepCopyInto(out *LoadBalancer) { + *out = *in + if in.AllowedCIDRs != nil { + in, out := &in.AllowedCIDRs, &out.AllowedCIDRs + *out = make([]string, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LoadBalancer. +func (in *LoadBalancer) DeepCopy() *LoadBalancer { + if in == nil { + return nil + } + out := new(LoadBalancer) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Network) DeepCopyInto(out *Network) { *out = *in diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_cloudstackclusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_cloudstackclusters.yaml index 26ef8b09..1351c750 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_cloudstackclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_cloudstackclusters.yaml @@ -353,6 +353,39 @@ spec: spec: description: CloudStackClusterSpec defines the desired state of CloudStackCluster. properties: + apiServerLoadBalancer: + description: |- + APIServerLoadBalancer configures the optional LoadBalancer for the APIServer. + If not specified, no load balancer will be created for the API server. + properties: + additionalPorts: + description: AdditionalPorts adds additional tcp ports to the + load balancer. + items: + type: integer + type: array + x-kubernetes-list-type: set + allowedCIDRs: + description: AllowedCIDRs restrict access to all API-Server listeners + to the given address CIDRs. + items: + type: string + type: array + x-kubernetes-list-type: set + enabled: + default: true + description: |- + Enabled defines whether a load balancer should be created. This value + defaults to true if an APIServerLoadBalancer is given. + + + There is no reason to set this to false. To disable creation of the + API server loadbalancer, omit the APIServerLoadBalancer field in the + cluster spec instead. + type: boolean + required: + - enabled + type: object controlPlaneEndpoint: description: The kubernetes control plane endpoint. properties: diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_cloudstackisolatednetworks.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_cloudstackisolatednetworks.yaml index 7fa68b52..5cc35af8 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_cloudstackisolatednetworks.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_cloudstackisolatednetworks.yaml @@ -224,9 +224,32 @@ spec: description: CloudStackIsolatedNetworkStatus defines the observed state of CloudStackIsolatedNetwork properties: + apiServerLoadBalancer: + description: APIServerLoadBalancer describes the api server load balancer + if one exists + properties: + allowedCIDRs: + items: + type: string + type: array + ip: + type: string + required: + - ip + type: object + cidr: + description: The CIDR of the assigned subnet. + type: string loadBalancerRuleID: - description: The ID of the lb rule used to assign VMs to the lb. + description: |- + Deprecated: The ID of the lb rule used to assign VMs to the lb. + No longer used, see LoadBalancerRuleIDs. Will be removed in next API version. type: string + loadBalancerRuleIDs: + description: The IDs of the lb rule used to assign VMs to the lb. + items: + type: string + type: array publicIPID: description: The CS public IP ID to use for the k8s endpoint. type: string diff --git a/controllers/cloudstackmachine_controller.go b/controllers/cloudstackmachine_controller.go index 01617476..ab882a63 100644 --- a/controllers/cloudstackmachine_controller.go +++ b/controllers/cloudstackmachine_controller.go @@ -293,7 +293,7 @@ func (r *CloudStackMachineReconciliationRunner) AddToLBIfNeeded() (retRes ctrl.R if r.IsoNet.Spec.Name == "" { return r.RequeueWithMessage("Could not get required Isolated Network for VM, requeueing.") } - err := r.CSUser.AssignVMToLoadBalancerRule(r.IsoNet, *r.ReconciliationSubject.Spec.InstanceID) + err := r.CSUser.AssignVMToLoadBalancerRules(r.IsoNet, *r.ReconciliationSubject.Spec.InstanceID) if err != nil { return ctrl.Result{}, err } diff --git a/pkg/cloud/isolated_network.go b/pkg/cloud/isolated_network.go index cdbce5c5..17a96945 100644 --- a/pkg/cloud/isolated_network.go +++ b/pkg/cloud/isolated_network.go @@ -17,26 +17,33 @@ limitations under the License. package cloud import ( + "fmt" + "slices" "strconv" "strings" "github.com/apache/cloudstack-go/v2/cloudstack" "github.com/hashicorp/go-multierror" "github.com/pkg/errors" + utilsnet "k8s.io/utils/net" infrav1 "sigs.k8s.io/cluster-api-provider-cloudstack/api/v1beta3" + capcstrings "sigs.k8s.io/cluster-api-provider-cloudstack/pkg/utils/strings" "sigs.k8s.io/cluster-api/util/annotations" + "sigs.k8s.io/cluster-api/util/record" ) type IsoNetworkIface interface { GetOrCreateIsolatedNetwork(*infrav1.CloudStackFailureDomain, *infrav1.CloudStackIsolatedNetwork, *infrav1.CloudStackCluster) error AssociatePublicIPAddress(*infrav1.CloudStackFailureDomain, *infrav1.CloudStackIsolatedNetwork, *infrav1.CloudStackCluster) error - GetOrCreateLoadBalancerRule(*infrav1.CloudStackIsolatedNetwork, *infrav1.CloudStackCluster) error CreateEgressFirewallRules(*infrav1.CloudStackIsolatedNetwork) error GetPublicIP(*infrav1.CloudStackFailureDomain, *infrav1.CloudStackCluster) (*cloudstack.PublicIpAddress, error) - ResolveLoadBalancerRuleDetails(*infrav1.CloudStackIsolatedNetwork) error + GetLoadBalancerRules(isoNet *infrav1.CloudStackIsolatedNetwork) ([]*cloudstack.LoadBalancerRule, error) + ReconcileLoadBalancerRules(isoNet *infrav1.CloudStackIsolatedNetwork, csCluster *infrav1.CloudStackCluster) error + GetFirewallRules(isoNet *infrav1.CloudStackIsolatedNetwork) ([]*cloudstack.FirewallRule, error) + ReconcileFirewallRules(isoNet *infrav1.CloudStackIsolatedNetwork, csCluster *infrav1.CloudStackCluster) error - AssignVMToLoadBalancerRule(isoNet *infrav1.CloudStackIsolatedNetwork, instanceID string) error + AssignVMToLoadBalancerRules(isoNet *infrav1.CloudStackIsolatedNetwork, instanceID string) error DeleteNetwork(infrav1.Network) error DisposeIsoNetResources(*infrav1.CloudStackIsolatedNetwork, *infrav1.CloudStackCluster) error } @@ -73,6 +80,10 @@ func (c *client) AssociatePublicIPAddress( csCluster.Spec.ControlPlaneEndpoint.Host = publicAddress.Ipaddress } isoNet.Status.PublicIPID = publicAddress.Id + if isoNet.Status.APIServerLoadBalancer == nil { + isoNet.Status.APIServerLoadBalancer = &infrav1.LoadBalancer{} + } + isoNet.Status.APIServerLoadBalancer.IP = publicAddress.Ipaddress // Check if the address is already associated with the network. if publicAddress.Associatednetworkid == isoNet.Spec.ID { @@ -117,6 +128,7 @@ func (c *client) CreateIsolatedNetwork(fd *infrav1.CloudStackFailureDomain, isoN return errors.Wrapf(err, "creating network with name %s", isoNet.Spec.Name) } isoNet.Spec.ID = resp.Id + isoNet.Status.CIDR = resp.Cidr return c.AddCreatedByCAPCTag(ResourceTypeNetwork, isoNet.Spec.ID) } @@ -188,6 +200,7 @@ func (c *client) GetIsolatedNetwork(isoNet *infrav1.CloudStackIsolatedNetwork) ( "expected 1 Network with name %s, but got %d", isoNet.Name, count)) } else { // Got netID from the network's name. isoNet.Spec.ID = netDetails.Id + isoNet.Status.CIDR = netDetails.Cidr return nil } @@ -198,58 +211,226 @@ func (c *client) GetIsolatedNetwork(isoNet *infrav1.CloudStackIsolatedNetwork) ( } else if count != 1 { return multierror.Append(retErr, errors.Errorf("expected 1 Network with UUID %s, but got %d", isoNet.Spec.ID, count)) } - isoNet.Name = netDetails.Name + isoNet.Spec.Name = netDetails.Name + isoNet.Status.CIDR = netDetails.Cidr return nil } -// ResolveLoadBalancerRuleDetails resolves the details of a load balancer rule by PublicIPID and Port. -func (c *client) ResolveLoadBalancerRuleDetails( - isoNet *infrav1.CloudStackIsolatedNetwork, -) error { +// GetLoadBalancerRules fetches the current loadbalancer rules for the isolated network. +func (c *client) GetLoadBalancerRules(isoNet *infrav1.CloudStackIsolatedNetwork) ([]*cloudstack.LoadBalancerRule, error) { p := c.cs.LoadBalancer.NewListLoadBalancerRulesParams() p.SetPublicipid(isoNet.Status.PublicIPID) loadBalancerRules, err := c.cs.LoadBalancer.ListLoadBalancerRules(p) if err != nil { c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err) - return errors.Wrap(err, "listing load balancer rules") + return nil, errors.Wrap(err, "listing load balancer rules") + } + + return loadBalancerRules.LoadBalancerRules, nil +} + +func (c *client) ReconcileLoadBalancerRules(isoNet *infrav1.CloudStackIsolatedNetwork, csCluster *infrav1.CloudStackCluster) error { + lbr, err := c.GetLoadBalancerRules(isoNet) + if err != nil { + c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err) + return errors.Wrap(err, "retrieving load balancer rules") } - for _, rule := range loadBalancerRules.LoadBalancerRules { - if rule.Publicport == strconv.Itoa(int(isoNet.Spec.ControlPlaneEndpoint.Port)) { - isoNet.Status.LBRuleID = rule.Id - return nil + ports := []int{int(csCluster.Spec.ControlPlaneEndpoint.Port)} + if len(csCluster.Spec.APIServerLoadBalancer.AdditionalPorts) > 0 { + ports = append(ports, csCluster.Spec.APIServerLoadBalancer.AdditionalPorts...) + } + + lbRuleIDs := make([]string, 0) + var found bool + for _, port := range ports { + var ruleID string + found = false + // Check if lb rule for port already exists + for _, rule := range lbr { + ruleID = rule.Id + if rule.Publicport == strconv.Itoa(port) { + found = true + lbRuleIDs = append(lbRuleIDs, ruleID) + } + } + // If not found, create the lb rule for port + if !found { + ruleID, err = c.CreateLoadBalancerRule(isoNet, port) + if err != nil { + return errors.Wrap(err, "creating load balancer rule") + } + lbRuleIDs = append(lbRuleIDs, ruleID) + } + + // For backwards compatibility. + if port == int(csCluster.Spec.ControlPlaneEndpoint.Port) { + isoNet.Status.LBRuleID = ruleID } } - return errors.New("no load balancer rule found") -} -// GetOrCreateLoadBalancerRule Create a load balancer rule that can be assigned to instances. -func (c *client) GetOrCreateLoadBalancerRule( - isoNet *infrav1.CloudStackIsolatedNetwork, - csCluster *infrav1.CloudStackCluster, -) (retErr error) { - // Check if rule exists. - if err := c.ResolveLoadBalancerRuleDetails(isoNet); err == nil || - !strings.Contains(strings.ToLower(err.Error()), "no load balancer rule found") { - return errors.Wrap(err, "resolving load balancer rule details") + if len(lbRuleIDs) > 1 { + capcstrings.Canonicalize(lbRuleIDs) } + isoNet.Status.LoadBalancerRuleIDs = lbRuleIDs + + return nil +} + +// CreateLoadBalancerRule configures the loadbalancer to accept traffic to a certain IP:port. +// +// Note that due to the lack of a cidrlist parameter in UpdateLoadbalancerRule, we can't use +// loadbalancer ACLs to implement the allowedCIDR functionality, and are forced to use firewall +// rules instead. See https://github.com/apache/cloudstack/issues/8382 for details. +func (c *client) CreateLoadBalancerRule(isoNet *infrav1.CloudStackIsolatedNetwork, port int) (string, error) { + name := fmt.Sprintf("K8s_API_%d", port) p := c.cs.LoadBalancer.NewCreateLoadBalancerRuleParams( - "roundrobin", "Kubernetes_API_Server", K8sDefaultAPIPort, K8sDefaultAPIPort) - p.SetPublicport(int(csCluster.Spec.ControlPlaneEndpoint.Port)) + "roundrobin", name, port, port) + p.SetPublicport(port) p.SetNetworkid(isoNet.Spec.ID) p.SetPublicipid(isoNet.Status.PublicIPID) p.SetProtocol(NetworkProtocolTCP) + // Do not open the firewall to the world, we'll manage that ourselves (unfortunately). + p.SetOpenfirewall(false) resp, err := c.cs.LoadBalancer.CreateLoadBalancerRule(p) if err != nil { c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err) + + return "", err + } + + return resp.Id, nil +} + +// GetFirewallRules fetches the current firewall rules for the isolated network. +func (c *client) GetFirewallRules(isoNet *infrav1.CloudStackIsolatedNetwork) ([]*cloudstack.FirewallRule, error) { + p := c.cs.Firewall.NewListFirewallRulesParams() + p.SetIpaddressid(isoNet.Status.PublicIPID) + p.SetNetworkid(isoNet.Spec.ID) + fwRules, err := c.cs.Firewall.ListFirewallRules(p) + if err != nil { + c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err) + return nil, errors.Wrap(err, "listing firewall rules") + } + + return fwRules.FirewallRules, nil +} + +func (c *client) ReconcileFirewallRules(isoNet *infrav1.CloudStackIsolatedNetwork, csCluster *infrav1.CloudStackCluster) error { + fwr, err := c.GetFirewallRules(isoNet) + if err != nil { + c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err) + return errors.Wrap(err, "retrieving load balancer rules") + } + + ports := []int{int(csCluster.Spec.ControlPlaneEndpoint.Port)} + if len(csCluster.Spec.APIServerLoadBalancer.AdditionalPorts) > 0 { + ports = append(ports, csCluster.Spec.APIServerLoadBalancer.AdditionalPorts...) + } + + // A note on the implementation here: + // Due to the lack of a `cidrlist` parameter in UpdateFirewallRule, we have to manage + // firewall rules for every item in the list of allowed CIDRs. + // See https://github.com/apache/cloudstack/issues/8382 + allowedCIDRS := getCanonicalAllowedCIDRs(isoNet, csCluster) + for _, port := range ports { + foundCIDRs := make([]string, 0) + // Check if fw rule for port already exists + for _, rule := range fwr { + if rule.Startport == port && rule.Endport == port { + // If the port matches and the rule CIDR is not in allowedCIDRs, delete + if !slices.Contains(allowedCIDRS, rule.Cidrlist) { + success, err := c.DeleteFirewallRule(rule.Id) + if err != nil || !success { + return errors.Wrap(err, "deleting firewall rule") + } + + continue + } + foundCIDRs = append(foundCIDRs, rule.Cidrlist) + } + } + + _, createCIDRs := capcstrings.SliceDiff(foundCIDRs, allowedCIDRS) + for _, cidr := range createCIDRs { + // create fw rule + if err := c.CreateFirewallRule(isoNet, port, cidr); err != nil { + return errors.Wrap(err, "creating firewall rule") + } + } + } + // Update the list of allowed CIDRs in the status + isoNet.Status.APIServerLoadBalancer.AllowedCIDRs = allowedCIDRS + + return nil +} + +// CreateFirewallRule creates a firewall rule to allow traffic from a certain CIDR to a port on our public IP. +func (c *client) CreateFirewallRule(isoNet *infrav1.CloudStackIsolatedNetwork, port int, cidr string) error { + cidrList := []string{cidr} + p := c.cs.Firewall.NewCreateFirewallRuleParams(isoNet.Status.PublicIPID, NetworkProtocolTCP) + p.SetStartport(port) + p.SetEndport(port) + p.SetCidrlist(cidrList) + _, err := c.cs.Firewall.CreateFirewallRule(p) + if err != nil { + c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err) + return err } - isoNet.Status.LBRuleID = resp.Id + return nil } +// DeleteFirewallRule deletes a firewall rule. +func (c *client) DeleteFirewallRule(id string) (bool, error) { + p := c.cs.Firewall.NewDeleteFirewallRuleParams(id) + resp, err := c.cs.Firewall.DeleteFirewallRule(p) + if err != nil { + c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err) + + return false, err + } + + return resp.Success, nil +} + +// getCanonicalAllowedCIDRs gets a filtered list of CIDRs which should be allowed to access the API server loadbalancer. +// Invalid CIDRs are filtered from the list and emil a warning event. +// It returns a canonical representation that can be directly compared with other canonicalized lists. +func getCanonicalAllowedCIDRs(isoNet *infrav1.CloudStackIsolatedNetwork, csCluster *infrav1.CloudStackCluster) []string { + allowedCIDRs := []string{} + + if csCluster.Spec.APIServerLoadBalancer != nil && len(csCluster.Spec.APIServerLoadBalancer.AllowedCIDRs) > 0 { + allowedCIDRs = append(allowedCIDRs, csCluster.Spec.APIServerLoadBalancer.AllowedCIDRs...) + + if isoNet.Status.CIDR != "" { + allowedCIDRs = append(allowedCIDRs, isoNet.Status.CIDR) + } + } else { + // If there are no specific CIDRs defined to allow traffic from, default to allow all. + allowedCIDRs = append(allowedCIDRs, "0.0.0.0/0") + } + + // Filter invalid CIDRs and convert any IPs into CIDRs. + validCIDRs := []string{} + for _, v := range allowedCIDRs { + switch { + case utilsnet.IsIPv4String(v): + validCIDRs = append(validCIDRs, v+"/32") + case utilsnet.IsIPv4CIDRString(v): + validCIDRs = append(validCIDRs, v) + default: + record.Warnf(csCluster, "FailedIPAddressValidation", "%s is not a valid IPv4 nor CIDR address and will not get applied to firewall rules", v) + } + } + + // Canonicalize by sorting and removing duplicates. + return capcstrings.Canonicalize(validCIDRs) +} + // GetOrCreateIsolatedNetwork fetches or builds out the necessary structures for isolated network use. func (c *client) GetOrCreateIsolatedNetwork( fd *infrav1.CloudStackFailureDomain, @@ -273,11 +454,6 @@ func (c *client) GetOrCreateIsolatedNetwork( } if !annotations.IsExternallyManaged(csCluster) { - // Associate Public IP with CloudStackIsolatedNetwork - if err := c.AssociatePublicIPAddress(fd, isoNet, csCluster); err != nil { - return errors.Wrapf(err, "associating public IP address to csCluster") - } - // Check/set ControlPlaneEndpoint port. // Prefer csCluster ControlPlaneEndpoint port. Use isonet port if CP missing. Set to default if both missing. if csCluster.Spec.ControlPlaneEndpoint.Port != 0 { @@ -289,9 +465,20 @@ func (c *client) GetOrCreateIsolatedNetwork( isoNet.Spec.ControlPlaneEndpoint.Port = 6443 } - // Setup a load balancing rule to map VMs to Public IP. - if err := c.GetOrCreateLoadBalancerRule(isoNet, csCluster); err != nil { - return errors.Wrap(err, "getting or creating load balancing rule") + // Set up a load balancing rules to map VM ports to Public IP ports. + if csCluster.Spec.APIServerLoadBalancer.IsEnabled() { + // Associate Public IP with CloudStackIsolatedNetwork + if err := c.AssociatePublicIPAddress(fd, isoNet, csCluster); err != nil { + return errors.Wrapf(err, "associating public IP address to csCluster") + } + + if err := c.ReconcileLoadBalancerRules(isoNet, csCluster); err != nil { + return errors.Wrap(err, "reconciling load balancing rules") + } + + if err := c.ReconcileFirewallRules(isoNet, csCluster); err != nil { + return errors.Wrap(err, "reconciling firewall rules") + } } } @@ -299,28 +486,36 @@ func (c *client) GetOrCreateIsolatedNetwork( return errors.Wrap(c.CreateEgressFirewallRules(isoNet), "opening the isolated network's egress firewall") } -// AssignVMToLoadBalancerRule assigns a VM instance to a load balancing rule (specifying lb membership). -func (c *client) AssignVMToLoadBalancerRule(isoNet *infrav1.CloudStackIsolatedNetwork, instanceID string) (retErr error) { +// AssignVMToLoadBalancerRules assigns a VM instance to load balancing rules (specifying lb membership). +func (c *client) AssignVMToLoadBalancerRules(isoNet *infrav1.CloudStackIsolatedNetwork, instanceID string) error { + var found bool + for _, lbRuleID := range isoNet.Status.LoadBalancerRuleIDs { + // Check that the instance isn't already in LB rotation. + found = false + lbRuleInstances, err := c.cs.LoadBalancer.ListLoadBalancerRuleInstances( + c.cs.LoadBalancer.NewListLoadBalancerRuleInstancesParams(lbRuleID)) + if err != nil { + c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err) + return err + } + for _, instance := range lbRuleInstances.LoadBalancerRuleInstances { + if instance.Id == instanceID { // Already assigned to load balancer.. + found = true + } + } - // Check that the instance isn't already in LB rotation. - lbRuleInstances, retErr := c.cs.LoadBalancer.ListLoadBalancerRuleInstances( - c.cs.LoadBalancer.NewListLoadBalancerRuleInstancesParams(isoNet.Status.LBRuleID)) - if retErr != nil { - c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(retErr) - return retErr - } - for _, instance := range lbRuleInstances.LoadBalancerRuleInstances { - if instance.Id == instanceID { // Already assigned to load balancer.. - return nil + if !found { + // Assign to Load Balancer. + p := c.cs.LoadBalancer.NewAssignToLoadBalancerRuleParams(lbRuleID) + p.SetVirtualmachineids([]string{instanceID}) + if _, err = c.cs.LoadBalancer.AssignToLoadBalancerRule(p); err != nil { + c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err) + return err + } } } - // Assign to Load Balancer. - p := c.cs.LoadBalancer.NewAssignToLoadBalancerRuleParams(isoNet.Status.LBRuleID) - p.SetVirtualmachineids([]string{instanceID}) - _, retErr = c.cs.LoadBalancer.AssignToLoadBalancerRule(p) - c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(retErr) - return retErr + return nil } // DeleteNetwork deletes an isolated network. diff --git a/pkg/cloud/isolated_network_test.go b/pkg/cloud/isolated_network_test.go index 2fe3cb8e..05520122 100644 --- a/pkg/cloud/isolated_network_test.go +++ b/pkg/cloud/isolated_network_test.go @@ -121,6 +121,17 @@ var _ = Describe("Network", func() { &csapi.ListLoadBalancerRulesResponse{LoadBalancerRules: []*csapi.LoadBalancerRule{ {Publicport: strconv.Itoa(int(dummies.EndPointPort)), Id: dummies.LBRuleID}}}, nil) + fs.EXPECT().NewListFirewallRulesParams().Return(&csapi.ListFirewallRulesParams{}) + fs.EXPECT().ListFirewallRules(gomock.Any()).Return( + &csapi.ListFirewallRulesResponse{FirewallRules: []*csapi.FirewallRule{ + { + Cidrlist: "0.0.0.0/0", + Startport: int(dummies.EndPointPort), + Endport: int(dummies.EndPointPort), + Id: dummies.FWRuleID, + }, + }}, nil) + Ω(client.GetOrCreateIsolatedNetwork(dummies.CSFailureDomain1, dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) }) @@ -277,27 +288,31 @@ var _ = Describe("Network", func() { }) }) - Context("The specific load balancer rule does exist", func() { + Context("The specific load balancer rule exists", func() { It("resolves the rule's ID", func() { lbs.EXPECT().NewListLoadBalancerRulesParams().Return(&csapi.ListLoadBalancerRulesParams{}) lbs.EXPECT().ListLoadBalancerRules(gomock.Any()).Return( &csapi.ListLoadBalancerRulesResponse{LoadBalancerRules: []*csapi.LoadBalancerRule{ {Publicport: strconv.Itoa(int(dummies.EndPointPort)), Id: dummies.LBRuleID}}}, nil) - dummies.CSISONet1.Status.LBRuleID = "" - Ω(client.ResolveLoadBalancerRuleDetails(dummies.CSISONet1)).Should(Succeed()) - Ω(dummies.CSISONet1.Status.LBRuleID).Should(Equal(dummies.LBRuleID)) + dummies.CSISONet1.Status.LoadBalancerRuleIDs = []string{} + Ω(client.ReconcileLoadBalancerRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) + Ω(dummies.CSISONet1.Status.LoadBalancerRuleIDs).Should(Equal(dummies.LoadBalancerRuleIDs)) }) - It("Failed to resolve LB rule details", func() { + It("when API loadbalancer additional ports are defined, resolves all rule IDs", func() { + dummies.CSCluster.Spec.APIServerLoadBalancer.AdditionalPorts = append(dummies.CSCluster.Spec.APIServerLoadBalancer.AdditionalPorts, 456) lbs.EXPECT().NewListLoadBalancerRulesParams().Return(&csapi.ListLoadBalancerRulesParams{}) lbs.EXPECT().ListLoadBalancerRules(gomock.Any()).Return( &csapi.ListLoadBalancerRulesResponse{LoadBalancerRules: []*csapi.LoadBalancerRule{ - {Publicport: "differentPublicPort", Id: dummies.LBRuleID}}}, nil) - - dummies.CSISONet1.Status.LBRuleID = "" - Ω(client.ResolveLoadBalancerRuleDetails(dummies.CSISONet1).Error()). - Should(Equal("no load balancer rule found")) + {Publicport: strconv.Itoa(int(dummies.EndPointPort)), Id: dummies.LBRuleID}, + {Publicport: strconv.Itoa(456), Id: "FakeLBRuleID2"}, + }}, nil) + + dummies.CSISONet1.Status.LoadBalancerRuleIDs = []string{} + Ω(client.ReconcileLoadBalancerRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) + dummies.LoadBalancerRuleIDs = []string{dummies.LBRuleID, "FakeLBRuleID2"} + Ω(dummies.CSISONet1.Status.LoadBalancerRuleIDs).Should(Equal(dummies.LoadBalancerRuleIDs)) }) It("Failed to list LB rules", func() { @@ -305,9 +320,7 @@ var _ = Describe("Network", func() { lbs.EXPECT().ListLoadBalancerRules(gomock.Any()).Return( nil, fakeError) - dummies.CSISONet1.Status.LBRuleID = "" - Ω(client.ResolveLoadBalancerRuleDetails(dummies.CSISONet1).Error()). - Should(ContainSubstring("listing load balancer rules")) + Ω(client.GetLoadBalancerRules(dummies.CSISONet1)).Error().Should(MatchError(ContainSubstring("listing load balancer rules"))) }) It("doesn't create a new load balancer rule on create", func() { @@ -317,42 +330,42 @@ var _ = Describe("Network", func() { LoadBalancerRules: []*csapi.LoadBalancerRule{ {Publicport: strconv.Itoa(int(dummies.EndPointPort)), Id: dummies.LBRuleID}}}, nil) - Ω(client.GetOrCreateLoadBalancerRule(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) - Ω(dummies.CSISONet1.Status.LBRuleID).Should(Equal(dummies.LBRuleID)) + Ω(client.ReconcileLoadBalancerRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) + Ω(dummies.CSISONet1.Status.LoadBalancerRuleIDs).Should(Equal(dummies.LoadBalancerRuleIDs)) }) }) Context("Assign VM to Load Balancer rule", func() { It("Associates VM to LB rule", func() { - dummies.CSISONet1.Status.LBRuleID = "lbruleid" + dummies.CSISONet1.Status.LoadBalancerRuleIDs = []string{"lbruleid"} lbip := &csapi.ListLoadBalancerRuleInstancesParams{} albp := &csapi.AssignToLoadBalancerRuleParams{} - lbs.EXPECT().NewListLoadBalancerRuleInstancesParams(dummies.CSISONet1.Status.LBRuleID). + lbs.EXPECT().NewListLoadBalancerRuleInstancesParams(dummies.CSISONet1.Status.LoadBalancerRuleIDs[0]). Return(lbip) lbs.EXPECT().ListLoadBalancerRuleInstances(lbip).Return(&csapi.ListLoadBalancerRuleInstancesResponse{}, nil) - lbs.EXPECT().NewAssignToLoadBalancerRuleParams(dummies.CSISONet1.Status.LBRuleID).Return(albp) + lbs.EXPECT().NewAssignToLoadBalancerRuleParams(dummies.CSISONet1.Status.LoadBalancerRuleIDs[0]).Return(albp) lbs.EXPECT().AssignToLoadBalancerRule(albp).Return(&csapi.AssignToLoadBalancerRuleResponse{}, nil) - Ω(client.AssignVMToLoadBalancerRule(dummies.CSISONet1, *dummies.CSMachine1.Spec.InstanceID)).Should(Succeed()) + Ω(client.AssignVMToLoadBalancerRules(dummies.CSISONet1, *dummies.CSMachine1.Spec.InstanceID)).Should(Succeed()) }) It("Associating VM to LB rule fails", func() { - dummies.CSISONet1.Status.LBRuleID = "lbruleid" + dummies.CSISONet1.Status.LoadBalancerRuleIDs = []string{"lbruleid"} lbip := &csapi.ListLoadBalancerRuleInstancesParams{} albp := &csapi.AssignToLoadBalancerRuleParams{} - lbs.EXPECT().NewListLoadBalancerRuleInstancesParams(dummies.CSISONet1.Status.LBRuleID). + lbs.EXPECT().NewListLoadBalancerRuleInstancesParams(dummies.CSISONet1.Status.LoadBalancerRuleIDs[0]). Return(lbip) lbs.EXPECT().ListLoadBalancerRuleInstances(lbip).Return(&csapi.ListLoadBalancerRuleInstancesResponse{}, nil) - lbs.EXPECT().NewAssignToLoadBalancerRuleParams(dummies.CSISONet1.Status.LBRuleID).Return(albp) + lbs.EXPECT().NewAssignToLoadBalancerRuleParams(dummies.CSISONet1.Status.LoadBalancerRuleIDs[0]).Return(albp) lbs.EXPECT().AssignToLoadBalancerRule(albp).Return(nil, fakeError) - Ω(client.AssignVMToLoadBalancerRule(dummies.CSISONet1, *dummies.CSMachine1.Spec.InstanceID)).ShouldNot(Succeed()) + Ω(client.AssignVMToLoadBalancerRules(dummies.CSISONet1, *dummies.CSMachine1.Spec.InstanceID)).ShouldNot(Succeed()) }) It("LB Rule already assigned to VM", func() { - dummies.CSISONet1.Status.LBRuleID = "lbruleid" + dummies.CSISONet1.Status.LoadBalancerRuleIDs = []string{"lbruleid"} lbip := &csapi.ListLoadBalancerRuleInstancesParams{} - lbs.EXPECT().NewListLoadBalancerRuleInstancesParams(dummies.CSISONet1.Status.LBRuleID). + lbs.EXPECT().NewListLoadBalancerRuleInstancesParams(dummies.CSISONet1.Status.LoadBalancerRuleIDs[0]). Return(lbip) lbs.EXPECT().ListLoadBalancerRuleInstances(lbip).Return(&csapi.ListLoadBalancerRuleInstancesResponse{ Count: 1, @@ -361,12 +374,12 @@ var _ = Describe("Network", func() { }}, }, nil) - Ω(client.AssignVMToLoadBalancerRule(dummies.CSISONet1, *dummies.CSMachine1.Spec.InstanceID)).Should(Succeed()) + Ω(client.AssignVMToLoadBalancerRules(dummies.CSISONet1, *dummies.CSMachine1.Spec.InstanceID)).Should(Succeed()) }) }) Context("load balancer rule does not exist", func() { - It("calls cloudstack to create a new load balancer rule.", func() { + It("calls CloudStack to create a new load balancer rule", func() { lbs.EXPECT().NewListLoadBalancerRulesParams().Return(&csapi.ListLoadBalancerRulesParams{}) lbs.EXPECT().ListLoadBalancerRules(gomock.Any()). Return(&csapi.ListLoadBalancerRulesResponse{ @@ -376,15 +389,35 @@ var _ = Describe("Network", func() { lbs.EXPECT().CreateLoadBalancerRule(gomock.Any()). Return(&csapi.CreateLoadBalancerRuleResponse{Id: "2ndLBRuleID"}, nil) - Ω(client.GetOrCreateLoadBalancerRule(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) - Ω(dummies.CSISONet1.Status.LBRuleID).Should(Equal("2ndLBRuleID")) + Ω(client.ReconcileLoadBalancerRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) + loadBalancerRuleIDs := []string{"2ndLBRuleID"} + Ω(dummies.CSISONet1.Status.LoadBalancerRuleIDs).Should(Equal(loadBalancerRuleIDs)) + }) + + It("when API load balancer additional ports are defined, creates additional rules", func() { + dummies.CSCluster.Spec.APIServerLoadBalancer.AdditionalPorts = append(dummies.CSCluster.Spec.APIServerLoadBalancer.AdditionalPorts, 456) + lbs.EXPECT().NewListLoadBalancerRulesParams().Return(&csapi.ListLoadBalancerRulesParams{}) + lbs.EXPECT().ListLoadBalancerRules(gomock.Any()).Return( + &csapi.ListLoadBalancerRulesResponse{LoadBalancerRules: []*csapi.LoadBalancerRule{ + {Publicport: strconv.Itoa(int(dummies.EndPointPort)), Id: dummies.LBRuleID}, + }}, nil) + + lbs.EXPECT().NewCreateLoadBalancerRuleParams(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(&csapi.CreateLoadBalancerRuleParams{}) + lbs.EXPECT().CreateLoadBalancerRule(gomock.Any()). + Return(&csapi.CreateLoadBalancerRuleResponse{Id: "2ndLBRuleID"}, nil) + + dummies.CSISONet1.Status.LoadBalancerRuleIDs = []string{dummies.LBRuleID} + Ω(client.ReconcileLoadBalancerRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) + dummies.LoadBalancerRuleIDs = []string{"2ndLBRuleID", dummies.LBRuleID} + Ω(dummies.CSISONet1.Status.LoadBalancerRuleIDs).Should(Equal(dummies.LoadBalancerRuleIDs)) }) It("Fails to resolve load balancer rule details", func() { lbs.EXPECT().NewListLoadBalancerRulesParams().Return(&csapi.ListLoadBalancerRulesParams{}) lbs.EXPECT().ListLoadBalancerRules(gomock.Any()). Return(nil, fakeError) - err := client.GetOrCreateLoadBalancerRule(dummies.CSISONet1, dummies.CSCluster) + err := client.ReconcileLoadBalancerRules(dummies.CSISONet1, dummies.CSCluster) Ω(err).ShouldNot(Succeed()) Ω(err.Error()).Should(ContainSubstring(errorMessage)) }) @@ -398,10 +431,193 @@ var _ = Describe("Network", func() { Return(&csapi.CreateLoadBalancerRuleParams{}) lbs.EXPECT().CreateLoadBalancerRule(gomock.Any()). Return(nil, fakeError) - err := client.GetOrCreateLoadBalancerRule(dummies.CSISONet1, dummies.CSCluster) + err := client.ReconcileLoadBalancerRules(dummies.CSISONet1, dummies.CSCluster) Ω(err).ShouldNot(Succeed()) - Ω(err.Error()).Should(Equal(errorMessage)) + Ω(err.Error()).Should(ContainSubstring(errorMessage)) + }) + }) + + Context("The specific firewall rule exists", func() { + It("does not call create or delete firewall rule", func() { + fs.EXPECT().NewListFirewallRulesParams().Return(&csapi.ListFirewallRulesParams{}) + fs.EXPECT().ListFirewallRules(gomock.Any()).Return( + &csapi.ListFirewallRulesResponse{FirewallRules: []*csapi.FirewallRule{ + { + Cidrlist: "0.0.0.0/0", + Startport: int(dummies.EndPointPort), + Endport: int(dummies.EndPointPort), + Id: dummies.FWRuleID, + }, + }}, nil) + + fs.EXPECT().CreateFirewallRule(gomock.Any()).Times(0) + fs.EXPECT().DeleteFirewallRule(gomock.Any()).Times(0) + + Ω(client.ReconcileFirewallRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) + }) + + It("calls delete firewall rule when there is a rule with a cidr not in allowed cidr list", func() { + fs.EXPECT().NewListFirewallRulesParams().Return(&csapi.ListFirewallRulesParams{}) + fs.EXPECT().ListFirewallRules(gomock.Any()).Return( + &csapi.ListFirewallRulesResponse{FirewallRules: []*csapi.FirewallRule{ + { + Cidrlist: "0.0.0.0/0", + Startport: int(dummies.EndPointPort), + Endport: int(dummies.EndPointPort), + Id: dummies.FWRuleID, + }, + { + Cidrlist: "192.168.1.0/24", + Startport: int(dummies.EndPointPort), + Endport: int(dummies.EndPointPort), + Id: "FakeFWRuleID2", + }, + }}, nil) + + fs.EXPECT().NewDeleteFirewallRuleParams("FakeFWRuleID2").DoAndReturn(func(ruleid string) *csapi.DeleteFirewallRuleParams { + p := &csapi.DeleteFirewallRuleParams{} + p.SetId(ruleid) + return p + }) + fs.EXPECT().DeleteFirewallRule(gomock.Any()).Return(&csapi.DeleteFirewallRuleResponse{Success: true}, nil).Times(1) + fs.EXPECT().NewCreateFirewallRuleParams(gomock.Any(), gomock.Any()).Times(0) + fs.EXPECT().CreateFirewallRule(gomock.Any()).Times(0) + + Ω(client.ReconcileFirewallRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) + }) + }) + + Context("The specific firewall rule does not exist", func() { + It("calls create firewall rule, does not call delete firewall rule", func() { + dummies.CSISONet1.Status.PublicIPID = dummies.PublicIPID + fs.EXPECT().NewListFirewallRulesParams().Return(&csapi.ListFirewallRulesParams{}) + fs.EXPECT().ListFirewallRules(gomock.Any()).Return( + &csapi.ListFirewallRulesResponse{FirewallRules: []*csapi.FirewallRule{}}, nil) + + fs.EXPECT().NewCreateFirewallRuleParams(gomock.Any(), gomock.Any()).DoAndReturn(func(publicipid, proto string) *csapi.CreateFirewallRuleParams { + p := &csapi.CreateFirewallRuleParams{} + p.SetIpaddressid(publicipid) + p.SetStartport(int(dummies.EndPointPort)) + p.SetEndport(int(dummies.EndPointPort)) + p.SetProtocol(proto) + return p + }).Times(1) + fs.EXPECT().CreateFirewallRule(gomock.Any()).Return(&csapi.CreateFirewallRuleResponse{}, nil).Times(1) + fs.EXPECT().DeleteFirewallRule(gomock.Any()).Times(0) + + Ω(client.ReconcileFirewallRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) + }) + + It("calls create and delete firewall rule when there is a rule with a cidr not in allowed cidr list", func() { + dummies.CSISONet1.Status.PublicIPID = dummies.PublicIPID + fs.EXPECT().NewListFirewallRulesParams().Return(&csapi.ListFirewallRulesParams{}) + fs.EXPECT().ListFirewallRules(gomock.Any()).Return( + &csapi.ListFirewallRulesResponse{FirewallRules: []*csapi.FirewallRule{ + { + Cidrlist: "192.168.1.0/24", + Startport: int(dummies.EndPointPort), + Endport: int(dummies.EndPointPort), + Id: "FakeFWRuleID2", + }, + }}, nil) + + fs.EXPECT().NewDeleteFirewallRuleParams("FakeFWRuleID2").DoAndReturn(func(ruleid string) *csapi.DeleteFirewallRuleParams { + p := &csapi.DeleteFirewallRuleParams{} + p.SetId(ruleid) + return p + }).Times(1) + fs.EXPECT().DeleteFirewallRule(gomock.Any()).Return(&csapi.DeleteFirewallRuleResponse{Success: true}, nil).Times(1) + fs.EXPECT().NewCreateFirewallRuleParams(gomock.Any(), gomock.Any()).DoAndReturn(func(publicipid, proto string) *csapi.CreateFirewallRuleParams { + p := &csapi.CreateFirewallRuleParams{} + p.SetIpaddressid(publicipid) + p.SetStartport(int(dummies.EndPointPort)) + p.SetEndport(int(dummies.EndPointPort)) + p.SetProtocol(proto) + return p + }).Times(1) + fs.EXPECT().CreateFirewallRule(gomock.Any()).Return(&csapi.CreateFirewallRuleResponse{}, nil).Times(1) + + Ω(client.ReconcileFirewallRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) + }) + + It("calls create firewall rule 2 times with additional port, does not call delete firewall rule", func() { + dummies.CSCluster.Spec.APIServerLoadBalancer.AdditionalPorts = append(dummies.CSCluster.Spec.APIServerLoadBalancer.AdditionalPorts, 456) + dummies.CSISONet1.Status.PublicIPID = dummies.PublicIPID + fs.EXPECT().NewListFirewallRulesParams().Return(&csapi.ListFirewallRulesParams{}) + fs.EXPECT().ListFirewallRules(gomock.Any()).Return( + &csapi.ListFirewallRulesResponse{FirewallRules: []*csapi.FirewallRule{}}, nil) + + gomock.InOrder( + fs.EXPECT().NewCreateFirewallRuleParams(gomock.Any(), gomock.Any()).DoAndReturn(func(publicipid, proto string) *csapi.CreateFirewallRuleParams { + p := &csapi.CreateFirewallRuleParams{} + p.SetIpaddressid(publicipid) + p.SetStartport(int(dummies.EndPointPort)) + p.SetEndport(int(dummies.EndPointPort)) + p.SetProtocol(proto) + return p + }).Times(1), + fs.EXPECT().CreateFirewallRule(gomock.Any()).Return(&csapi.CreateFirewallRuleResponse{}, nil).Times(1), + fs.EXPECT().NewCreateFirewallRuleParams(gomock.Any(), gomock.Any()).DoAndReturn(func(publicipid, proto string) *csapi.CreateFirewallRuleParams { + p := &csapi.CreateFirewallRuleParams{} + p.SetIpaddressid(publicipid) + p.SetStartport(456) + p.SetEndport(456) + p.SetProtocol(proto) + return p + }).Times(1), + fs.EXPECT().CreateFirewallRule(gomock.Any()).Return(&csapi.CreateFirewallRuleResponse{}, nil).Times(1), + ) + + fs.EXPECT().DeleteFirewallRule(gomock.Any()).Times(0) + + Ω(client.ReconcileFirewallRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) + }) + + It("with a list of allowed CIDRs, calls create firewall rule for each of them, and the isonet CIDR, does not call delete firewall rule", func() { + dummies.CSCluster.Spec.APIServerLoadBalancer.AllowedCIDRs = append(dummies.CSCluster.Spec.APIServerLoadBalancer.AllowedCIDRs, + "192.168.1.0/24", + "192.168.2.0/24") + dummies.CSISONet1.Status.PublicIPID = dummies.PublicIPID + dummies.CSISONet1.Status.CIDR = "10.1.0.0/24" + fs.EXPECT().NewListFirewallRulesParams().Return(&csapi.ListFirewallRulesParams{}) + fs.EXPECT().ListFirewallRules(gomock.Any()).Return( + &csapi.ListFirewallRulesResponse{FirewallRules: []*csapi.FirewallRule{}}, nil) + + gomock.InOrder( + fs.EXPECT().NewCreateFirewallRuleParams(gomock.Any(), gomock.Any()).DoAndReturn(func(publicipid, proto string) *csapi.CreateFirewallRuleParams { + p := &csapi.CreateFirewallRuleParams{} + p.SetIpaddressid(publicipid) + p.SetCidrlist([]string{"10.1.0.0/24"}) + p.SetStartport(int(dummies.EndPointPort)) + p.SetEndport(int(dummies.EndPointPort)) + p.SetProtocol(proto) + return p + }).Times(1), + fs.EXPECT().CreateFirewallRule(gomock.Any()).Return(&csapi.CreateFirewallRuleResponse{}, nil).Times(1), + fs.EXPECT().NewCreateFirewallRuleParams(gomock.Any(), gomock.Any()).DoAndReturn(func(publicipid, proto string) *csapi.CreateFirewallRuleParams { + p := &csapi.CreateFirewallRuleParams{} + p.SetIpaddressid(publicipid) + p.SetCidrlist([]string{"192.168.1.0/24"}) + p.SetStartport(int(dummies.EndPointPort)) + p.SetEndport(int(dummies.EndPointPort)) + p.SetProtocol(proto) + return p + }).Times(1), + fs.EXPECT().CreateFirewallRule(gomock.Any()).Return(&csapi.CreateFirewallRuleResponse{}, nil).Times(1), + fs.EXPECT().NewCreateFirewallRuleParams(gomock.Any(), gomock.Any()).DoAndReturn(func(publicipid, proto string) *csapi.CreateFirewallRuleParams { + p := &csapi.CreateFirewallRuleParams{} + p.SetIpaddressid(publicipid) + p.SetCidrlist([]string{"192.168.2.0/24"}) + p.SetStartport(int(dummies.EndPointPort)) + p.SetEndport(int(dummies.EndPointPort)) + p.SetProtocol(proto) + return p + }).Times(1), + fs.EXPECT().CreateFirewallRule(gomock.Any()).Return(&csapi.CreateFirewallRuleResponse{}, nil).Times(1), + ) + fs.EXPECT().DeleteFirewallRule(gomock.Any()).Times(0) + Ω(client.ReconcileFirewallRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) }) }) diff --git a/pkg/utils/strings/strings.go b/pkg/utils/strings/strings.go new file mode 100644 index 00000000..91c36cdd --- /dev/null +++ b/pkg/utils/strings/strings.go @@ -0,0 +1,54 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package strings + +import ( + "cmp" + "slices" +) + +func Canonicalize[S ~[]E, E cmp.Ordered](s S) S { + slices.Sort(s) + return slices.Compact(s) +} + +// SliceDiff determines the differences between two slices, returning the items in list1 that are not present in list2, +// and the items in list2 that are not present in list1. +func SliceDiff[T ~[]E, E comparable](list1 T, list2 T) (ret1 T, ret2 T) { + m1 := map[E]struct{}{} + m2 := map[E]struct{}{} + for _, v := range list1 { + m1[v] = struct{}{} + } + for _, v := range list2 { + m2[v] = struct{}{} + } + + ret1 = make(T, 0) + ret2 = make(T, 0) + for _, v := range list1 { + if _, ok := m2[v]; !ok { + ret1 = append(ret1, v) + } + } + for _, v := range list2 { + if _, ok := m1[v]; !ok { + ret2 = append(ret2, v) + } + } + return ret1, ret2 +} diff --git a/pkg/utils/strings/strings_test.go b/pkg/utils/strings/strings_test.go new file mode 100644 index 00000000..9a6bb6df --- /dev/null +++ b/pkg/utils/strings/strings_test.go @@ -0,0 +1,123 @@ +/* +Copyright 2024 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package strings + +import ( + "slices" + "testing" +) + +func TestCanonicalize(t *testing.T) { + tests := []struct { + name string + value []string + want []string + }{ + { + name: "Empty list", + value: []string{}, + want: []string{}, + }, + { + name: "Identity", + value: []string{"a", "b", "c"}, + want: []string{"a", "b", "c"}, + }, + { + name: "Out of order", + value: []string{"c", "b", "a"}, + want: []string{"a", "b", "c"}, + }, + { + name: "Duplicate elements", + value: []string{"c", "b", "a", "c"}, + want: []string{"a", "b", "c"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := Canonicalize(tt.value) + if !slices.Equal(got, tt.want) { + t.Errorf("CompareLists() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestSliceDiff(t *testing.T) { + tests := []struct { + name string + value1, value2 []string + want1, want2 []string + }{ + { + name: "Empty list", + value1: []string{}, + value2: []string{}, + want1: []string{}, + want2: []string{}, + }, + { + name: "Same", + value1: []string{"a", "b", "c"}, + value2: []string{"a", "b", "c"}, + want1: []string{}, + want2: []string{}, + }, + { + name: "one different", + value1: []string{"a", "b", "c"}, + value2: []string{"a", "b", "c", "d"}, + want1: []string{}, + want2: []string{"d"}, + }, + { + name: "other different", + value1: []string{"a", "b", "c", "d"}, + value2: []string{"a", "b", "c"}, + want1: []string{"d"}, + want2: []string{}, + }, + { + name: "both different", + value1: []string{"a", "b", "c", "e"}, + value2: []string{"a", "b", "d", "f"}, + want1: []string{"c", "e"}, + want2: []string{"d", "f"}, + }, + { + name: "Duplicate elements", + value1: []string{"c", "b", "a", "c"}, + value2: []string{"c", "b", "a", "c"}, + want1: []string{}, + want2: []string{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + res1, res2 := SliceDiff(tt.value1, tt.value2) + if !slices.Equal(res1, tt.want1) { + t.Errorf("CompareLists() = %v, want %v", res1, tt.want1) + } + if !slices.Equal(res2, tt.want2) { + t.Errorf("CompareLists() = %v, want %v", res2, tt.want2) + } + }) + } +} diff --git a/test/dummies/v1beta3/vars.go b/test/dummies/v1beta3/vars.go index 2c950123..a98906c3 100644 --- a/test/dummies/v1beta3/vars.go +++ b/test/dummies/v1beta3/vars.go @@ -39,6 +39,7 @@ var ( // Declare exported dummy vars. Zone2 infrav1.CloudStackZoneSpec CSFailureDomain1 *infrav1.CloudStackFailureDomain CSFailureDomain2 *infrav1.CloudStackFailureDomain + APIServerLoadBalancer *infrav1.APIServerLoadBalancer Net1 infrav1.Network Net2 infrav1.Network ISONet1 infrav1.Network @@ -77,7 +78,9 @@ var ( // Declare exported dummy vars. CSClusterTag map[string]string CreatedByCapcKey string CreatedByCapcVal string + FWRuleID string LBRuleID string + LoadBalancerRuleIDs []string PublicIPID string EndPointHost string EndPointPort int32 @@ -114,7 +117,9 @@ func SetDummyVars() { SetDummyBootstrapSecretVar() SetCSMachineOwner() SetDummyOwnerReferences() + FWRuleID = "FakeFWRuleID" LBRuleID = "FakeLBRuleID" + LoadBalancerRuleIDs = []string{"FakeLBRuleID"} } func SetDiskOfferingVars() { @@ -284,6 +289,11 @@ func SetDummyCAPCClusterVars() { Net1 = infrav1.Network{Name: GetYamlVal("CLOUDSTACK_NETWORK_NAME"), Type: cloud.NetworkTypeShared} Net2 = infrav1.Network{Name: "SharedGuestNet2", Type: cloud.NetworkTypeShared, ID: "FakeSharedNetID2"} ISONet1 = infrav1.Network{Name: "isoguestnet1", Type: cloud.NetworkTypeIsolated, ID: "FakeIsolatedNetID1"} + APIServerLoadBalancer = &infrav1.APIServerLoadBalancer{ + Enabled: pointer.Bool(true), + AdditionalPorts: []int{}, + AllowedCIDRs: []string{}, + } CSFailureDomain1 = &infrav1.CloudStackFailureDomain{ TypeMeta: metav1.TypeMeta{ APIVersion: CSApiVersion, @@ -335,8 +345,9 @@ func SetDummyCAPCClusterVars() { Labels: ClusterLabel, }, Spec: infrav1.CloudStackClusterSpec{ - ControlPlaneEndpoint: clusterv1.APIEndpoint{Host: EndPointHost, Port: EndPointPort}, - FailureDomains: []infrav1.CloudStackFailureDomainSpec{CSFailureDomain1.Spec, CSFailureDomain2.Spec}, + ControlPlaneEndpoint: clusterv1.APIEndpoint{Host: EndPointHost, Port: EndPointPort}, + FailureDomains: []infrav1.CloudStackFailureDomainSpec{CSFailureDomain1.Spec, CSFailureDomain2.Spec}, + APIServerLoadBalancer: APIServerLoadBalancer, }, Status: infrav1.CloudStackClusterStatus{}, } @@ -352,7 +363,12 @@ func SetDummyCAPCClusterVars() { Labels: ClusterLabel, }, Spec: infrav1.CloudStackIsolatedNetworkSpec{ - ControlPlaneEndpoint: CSCluster.Spec.ControlPlaneEndpoint}} + ControlPlaneEndpoint: CSCluster.Spec.ControlPlaneEndpoint, + }, + Status: infrav1.CloudStackIsolatedNetworkStatus{ + APIServerLoadBalancer: &infrav1.LoadBalancer{}, + }, + } CSISONet1.Spec.Name = ISONet1.Name CSISONet1.Spec.ID = ISONet1.ID }