From 4be6c9976504d6553c31a19138b4d4f383ab6314 Mon Sep 17 00:00:00 2001 From: Maximilian Geberl <48486938+dergeberl@users.noreply.github.com> Date: Wed, 19 Jul 2023 13:42:19 +0200 Subject: [PATCH 1/3] Add possibility to use multiple loadbalancerClasses and use service.spec.loadBalancerClass --- api/v1beta1/loadbalancer_types.go | 1 + cmd/yawol-cloud-controller/main.go | 32 ++++++- .../targetcontroller/service_controller.go | 15 +-- .../service_controller_test.go | 95 +++++++++++++++++-- .../targetcontroller/suite_test.go | 3 +- internal/helper/const.go | 1 + internal/helper/service.go | 23 +++++ 7 files changed, 145 insertions(+), 25 deletions(-) diff --git a/api/v1beta1/loadbalancer_types.go b/api/v1beta1/loadbalancer_types.go index c0eb9e94..162314dd 100644 --- a/api/v1beta1/loadbalancer_types.go +++ b/api/v1beta1/loadbalancer_types.go @@ -32,6 +32,7 @@ const ( // ServiceDebugSSHKey set an sshkey ServiceDebugSSHKey = "yawol.stackit.cloud/debugsshkey" // ServiceClassName for filtering services in cloud-controller + // Deprecated: use .spec.loadBalancerClass instead ServiceClassName = "yawol.stackit.cloud/className" // ServiceReplicas for setting loadbalancer replicas in cloud-controller ServiceReplicas = "yawol.stackit.cloud/replicas" diff --git a/cmd/yawol-cloud-controller/main.go b/cmd/yawol-cloud-controller/main.go index b8dc8112..a2c0dfd6 100644 --- a/cmd/yawol-cloud-controller/main.go +++ b/cmd/yawol-cloud-controller/main.go @@ -5,6 +5,7 @@ import ( "flag" "os" "strconv" + "strings" "time" // Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.) @@ -18,6 +19,7 @@ import ( yawolv1beta1 "github.com/stackitcloud/yawol/api/v1beta1" "github.com/stackitcloud/yawol/controllers/yawol-cloud-controller/controlcontroller" "github.com/stackitcloud/yawol/controllers/yawol-cloud-controller/targetcontroller" + "github.com/stackitcloud/yawol/internal/helper" "go.uber.org/zap/zapcore" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" @@ -33,6 +35,8 @@ var ( setupLog = ctrl.Log.WithName("setup") ) +type loadbalancerClassNames []string + const ( // Namespace in for LoadBalancer CRs EnvClusterNamespace = "CLUSTER_NAMESPACE" @@ -72,7 +76,8 @@ func main() { var targetEnableLeaderElection bool var targetKubeconfig string var controlKubeconfig string - var className string + var classNames loadbalancerClassNames + var emptyClassName bool // settings for leases var leasesDurationInt int var leasesRenewDeadlineInt int @@ -94,9 +99,12 @@ func main() { "K8s credentials for watching the Service resources.") flag.StringVar(&controlKubeconfig, "control-kubeconfig", "", "K8s credentials for deploying the LoadBalancer resources.") - flag.StringVar(&className, "classname", "", - "Only listen to Services with the given className. "+ - "Default is empty and listen to all services with out className annotation") + flag.Var(&classNames, "classname", + "Only listen to Services with the given className. Can be set multiple times. "+ + "Always listen to "+helper.DefaultLoadbalancerClass+"."+ + "See also --empty-classname.") + flag.BoolVar(&emptyClassName, "empty-classname", true, + "Listen to services without a loadBalancerClass. Default is true.") flag.IntVar(&leasesDurationInt, "leases-duration", 60, "Is the time in seconds a non-leader will wait until forcing to acquire leadership.") flag.IntVar(&leasesRenewDeadlineInt, "leases-renew-deadline", 50, @@ -113,6 +121,11 @@ func main() { opts.BindFlags(flag.CommandLine) flag.Parse() + classNames = append(classNames, helper.DefaultLoadbalancerClass) + if emptyClassName { + classNames = append(classNames, "") + } + leasesDuration = time.Duration(leasesDurationInt) * time.Second leasesRenewDeadline = time.Duration(leasesRenewDeadlineInt) * time.Second leasesRetryPeriod = time.Duration(leasesRetryPeriodInt) * time.Second @@ -178,7 +191,7 @@ func main() { Log: ctrl.Log.WithName("controller").WithName("Service"), Scheme: targetMgr.GetScheme(), Recorder: targetMgr.GetEventRecorderFor("yawol-cloud-controller"), - ClassName: className, + ClassNames: classNames, }).SetupWithManager(targetMgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "Service") os.Exit(1) @@ -352,3 +365,12 @@ func getInfrastructureDefaultsFromEnvOrDie() targetcontroller.InfrastructureDefa InternalLB: pointer.Bool(internalLb), } } + +func (i *loadbalancerClassNames) String() string { + return strings.Join(*i, ",") +} + +func (i *loadbalancerClassNames) Set(value string) error { + *i = append(*i, value) + return nil +} diff --git a/controllers/yawol-cloud-controller/targetcontroller/service_controller.go b/controllers/yawol-cloud-controller/targetcontroller/service_controller.go index 05f22471..801e33e3 100644 --- a/controllers/yawol-cloud-controller/targetcontroller/service_controller.go +++ b/controllers/yawol-cloud-controller/targetcontroller/service_controller.go @@ -41,7 +41,7 @@ type ServiceReconciler struct { Log logr.Logger Scheme *runtime.Scheme Recorder record.EventRecorder - ClassName string + ClassNames []string } // +kubebuilder:rbac:groups=core,resources=services,verbs=get;list;watch @@ -57,14 +57,9 @@ func (r *ServiceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct return ctrl.Result{}, client.IgnoreNotFound(err) } - className, ok := svc.Annotations[yawolv1beta1.ServiceClassName] - if !ok { - className = "" - } - infraDefaults := GetMergedInfrastructureDetails(r.InfrastructureDefaults, svc) - if className != r.ClassName { + if !helper.CheckLoadBalancerClasses(svc, r.ClassNames) { r.Log.WithValues("service", req.NamespacedName).Info("service and controller classname does not match") if err := r.ControlClient.Get(ctx, types.NamespacedName{ Namespace: *infraDefaults.Namespace, @@ -560,11 +555,7 @@ func (r *ServiceReconciler) deletionRoutine( } if apierrors.IsNotFound(err) { - className, ok := svc.Annotations[yawolv1beta1.ServiceClassName] - if !ok { - className = "" - } - if r.ClassName != className { + if !helper.CheckLoadBalancerClasses(svc, r.ClassNames) { return ctrl.Result{}, nil } diff --git a/controllers/yawol-cloud-controller/targetcontroller/service_controller_test.go b/controllers/yawol-cloud-controller/targetcontroller/service_controller_test.go index 27f20410..89507961 100644 --- a/controllers/yawol-cloud-controller/targetcontroller/service_controller_test.go +++ b/controllers/yawol-cloud-controller/targetcontroller/service_controller_test.go @@ -472,11 +472,11 @@ var _ = Describe("Check loadbalancer reconcile", Serial, Ordered, func() { }, time.Second*5, time.Millisecond*500).Should(Succeed()) }) - It("create service with wrong className", func() { + It("create service with wrong className in annotation", func() { By("create service") service := v1.Service{ ObjectMeta: metav1.ObjectMeta{ - Name: "service-test8", + Name: "class-name-service-test1", Namespace: "default", Annotations: map[string]string{ yawolv1beta1.ServiceClassName: "foo", @@ -498,7 +498,7 @@ var _ = Describe("Check loadbalancer reconcile", Serial, Ordered, func() { By("check for LB creation") Consistently(func() error { - err := k8sClient.Get(ctx, types.NamespacedName{Name: "default--service-test8", Namespace: "default"}, &lb) + err := k8sClient.Get(ctx, types.NamespacedName{Name: "default--class-name-service-test1", Namespace: "default"}, &lb) if err != nil { return client.IgnoreNotFound(err) } @@ -506,11 +506,11 @@ var _ = Describe("Check loadbalancer reconcile", Serial, Ordered, func() { }, time.Second*5, time.Millisecond*500).Should(Succeed()) }) - It("create service with correct classname", func() { + It("create service with correct classname in annotation", func() { By("create service") service := v1.Service{ ObjectMeta: metav1.ObjectMeta{ - Name: "service-test15", + Name: "class-name-service-test2", Namespace: "default", Annotations: map[string]string{ yawolv1beta1.ServiceClassName: "", @@ -533,7 +533,7 @@ var _ = Describe("Check loadbalancer reconcile", Serial, Ordered, func() { By("check creation of LB") Eventually(func() error { err := k8sClient.Get(ctx, types.NamespacedName{ - Name: "default--service-test15", + Name: "default--class-name-service-test2", Namespace: "default", }, &lb) return err @@ -547,7 +547,88 @@ var _ = Describe("Check loadbalancer reconcile", Serial, Ordered, func() { return err } for _, event := range eventList.Items { - if event.InvolvedObject.Name == "service-test15" && + if event.InvolvedObject.Name == "class-name-service-test2" && + event.InvolvedObject.Kind == "Service" && + strings.Contains(event.Message, "LoadBalancer is in creation") { + return nil + } + } + return helper.ErrNoEventFound + }, time.Second*5, time.Millisecond*500).Should(Succeed()) + }) + + It("create service with wrong className in spec", func() { + By("create service") + service := v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "class-name-service-test3", + Namespace: "default", + }, + Spec: v1.ServiceSpec{ + LoadBalancerClass: pointer.String("foo"), + Ports: []v1.ServicePort{ + { + Name: "port1", + Protocol: v1.ProtocolTCP, + Port: 65030, + TargetPort: intstr.IntOrString{IntVal: 12345}, + NodePort: 30133, + }, + }, + Type: "LoadBalancer", + }} + Expect(k8sClient.Create(ctx, &service)).Should(Succeed()) + + By("check for LB creation") + Consistently(func() error { + err := k8sClient.Get(ctx, types.NamespacedName{Name: "default--class-name-service-test1", Namespace: "default"}, &lb) + if err != nil { + return client.IgnoreNotFound(err) + } + return helper.ErrInvalidClassname + }, time.Second*5, time.Millisecond*500).Should(Succeed()) + }) + + It("create service with correct classname in spec", func() { + By("create service") + service := v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "class-name-service-test4", + Namespace: "default", + }, + Spec: v1.ServiceSpec{ + LoadBalancerClass: pointer.String(helper.DefaultLoadbalancerClass), + Ports: []v1.ServicePort{ + { + Name: "port1", + Protocol: v1.ProtocolTCP, + Port: 12345, + TargetPort: intstr.IntOrString{IntVal: 12345}, + NodePort: 30335, + }, + }, + Type: "LoadBalancer", + }} + Expect(k8sClient.Create(ctx, &service)).Should(Succeed()) + + By("check creation of LB") + Eventually(func() error { + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: "default--class-name-service-test4", + Namespace: "default", + }, &lb) + return err + }, time.Second*5, time.Millisecond*500).Should(Succeed()) + + By("Check Event for creation") + Eventually(func() error { + eventList := v1.EventList{} + err := k8sClient.List(ctx, &eventList) + if err != nil { + return err + } + for _, event := range eventList.Items { + if event.InvolvedObject.Name == "class-name-service-test4" && event.InvolvedObject.Kind == "Service" && strings.Contains(event.Message, "LoadBalancer is in creation") { return nil diff --git a/controllers/yawol-cloud-controller/targetcontroller/suite_test.go b/controllers/yawol-cloud-controller/targetcontroller/suite_test.go index ce4457a6..f84e6f14 100644 --- a/controllers/yawol-cloud-controller/targetcontroller/suite_test.go +++ b/controllers/yawol-cloud-controller/targetcontroller/suite_test.go @@ -17,6 +17,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log/zap" yawolv1beta1 "github.com/stackitcloud/yawol/api/v1beta1" + "github.com/stackitcloud/yawol/internal/helper" // +kubebuilder:scaffold:imports ) @@ -101,7 +102,7 @@ var _ = BeforeSuite(func() { Log: ctrl.Log.WithName("controllers").WithName("Service"), Scheme: k8sManager.GetScheme(), Recorder: k8sManager.GetEventRecorderFor("Loadbalancer"), - ClassName: "", + ClassNames: []string{"", helper.DefaultLoadbalancerClass}, }).SetupWithManager(k8sManager) Expect(err).ToNot(HaveOccurred()) diff --git a/internal/helper/const.go b/internal/helper/const.go index 709d7179..7645217d 100644 --- a/internal/helper/const.go +++ b/internal/helper/const.go @@ -12,4 +12,5 @@ const ( LoadBalancerKind = "LoadBalancer" VRRPInstanceName = "ENVOY" HasKeepalivedMaster = "HasKeepalivedMaster" + DefaultLoadbalancerClass = "stackit.cloud/yawol" ) diff --git a/internal/helper/service.go b/internal/helper/service.go index 12aa2403..77bf5042 100644 --- a/internal/helper/service.go +++ b/internal/helper/service.go @@ -164,6 +164,29 @@ func GetLoadBalancerNameFromService(service *coreV1.Service) string { return service.Namespace + "--" + service.Name } +func getLoadBalancerClass(service *coreV1.Service) string { + if className, ok := service.Annotations[yawolv1beta1.ServiceClassName]; ok { + return className + } + + if service.Spec.LoadBalancerClass != nil { + return *service.Spec.LoadBalancerClass + } + + return "" +} + +func CheckLoadBalancerClasses(service *coreV1.Service, validClasses []string) bool { + serviceClassName := getLoadBalancerClass(service) + + for _, validClass := range validClasses { + if validClass == serviceClassName { + return true + } + } + return false +} + // ValidateService checks if the service is valid func ValidateService(svc *coreV1.Service) error { for _, port := range svc.Spec.Ports { From af9d1adb9175763483d6ab04b410a38c9aeaf946 Mon Sep 17 00:00:00 2001 From: Maximilian Geberl <48486938+dergeberl@users.noreply.github.com> Date: Wed, 19 Jul 2023 13:42:53 +0200 Subject: [PATCH 2/3] update docs and dev setup --- README.md | 1 + docs/development.md | 1 - example-setup/yawol-cloud-controller/service.yaml | 1 + run-ycc.sh | 2 +- 4 files changed, 3 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 9b0ea417..76630d7b 100644 --- a/README.md +++ b/README.md @@ -252,6 +252,7 @@ metadata: # Reference the name of the SSH key provided to OpenStack for debugging . yawol.stackit.cloud/debugsshkey: "OS-keyName" # Allows filtering services in cloud-controller. + # Deprecated: Use service.spec.loadBalancerClass instead. yawol.stackit.cloud/className: "test" # Specify the number of LoadBalancer machines to deploy (default 1). yawol.stackit.cloud/replicas: "3" diff --git a/docs/development.md b/docs/development.md index 0570c6bf..6d0d2bcd 100644 --- a/docs/development.md +++ b/docs/development.md @@ -104,7 +104,6 @@ The controllers are using the default kubeconfig ($KUBECONFIG, InCluster or # or kubectl create deployment --image nginx:latest nginx --replicas 1 kubectl expose deployment --port 80 --type LoadBalancer nginx --name loadbalancer - kubectl annotate service loadbalancer yawol.stackit.cloud/className=test # annotation needs to match the value of the `classname` flag from `run-ycc.sh` ``` 2. Check if the yawol-cloud-controller created a new `LoadBalancer` object diff --git a/example-setup/yawol-cloud-controller/service.yaml b/example-setup/yawol-cloud-controller/service.yaml index 3349df90..e8756df4 100644 --- a/example-setup/yawol-cloud-controller/service.yaml +++ b/example-setup/yawol-cloud-controller/service.yaml @@ -15,6 +15,7 @@ metadata: # yawol.stackit.cloud/tcpProxyProtocol: "false" # yawol.stackit.cloud/tcpProxyProtocolPortsFilter: "" spec: + loadBalancerClass: "test" type: LoadBalancer selector: app: nginx diff --git a/run-ycc.sh b/run-ycc.sh index b2f57959..67c19783 100755 --- a/run-ycc.sh +++ b/run-ycc.sh @@ -7,4 +7,4 @@ #export FLAVOR_ID="osAyv1W3z2TU5D6h" # m1.amphora #export IMAGE_ID="332c90c3-3141-4413-9ef9-f70a472cedb6" # yawol-alpine-v0.5.0 -go run ./cmd/yawol-cloud-controller/main.go --classname test --target-kubeconfig=inClusterConfig --control-kubeconfig=inClusterConfig +go run ./cmd/yawol-cloud-controller/main.go --classname test --empty-classname=false --target-kubeconfig=inClusterConfig --control-kubeconfig=inClusterConfig From b789815e183b2407f707db2e45edfeea5aeace68 Mon Sep 17 00:00:00 2001 From: Maximilian Geberl <48486938+dergeberl@users.noreply.github.com> Date: Thu, 20 Jul 2023 13:37:33 +0200 Subject: [PATCH 3/3] add code suggestions from PR review --- cmd/yawol-cloud-controller/main.go | 8 +-- .../service_controller_test.go | 48 +++++++++++++++++ internal/helper/service.go | 12 ++--- internal/helper/service_test.go | 53 +++++++++++++++++++ 4 files changed, 109 insertions(+), 12 deletions(-) create mode 100644 internal/helper/service_test.go diff --git a/cmd/yawol-cloud-controller/main.go b/cmd/yawol-cloud-controller/main.go index a2c0dfd6..c261344a 100644 --- a/cmd/yawol-cloud-controller/main.go +++ b/cmd/yawol-cloud-controller/main.go @@ -101,8 +101,8 @@ func main() { "K8s credentials for deploying the LoadBalancer resources.") flag.Var(&classNames, "classname", "Only listen to Services with the given className. Can be set multiple times. "+ - "Always listen to "+helper.DefaultLoadbalancerClass+"."+ - "See also --empty-classname.") + "If no classname is set it will defaults to "+helper.DefaultLoadbalancerClass+" "+ + "and services without class. See also --empty-classname.") flag.BoolVar(&emptyClassName, "empty-classname", true, "Listen to services without a loadBalancerClass. Default is true.") flag.IntVar(&leasesDurationInt, "leases-duration", 60, @@ -121,7 +121,9 @@ func main() { opts.BindFlags(flag.CommandLine) flag.Parse() - classNames = append(classNames, helper.DefaultLoadbalancerClass) + if len(classNames) == 0 { + classNames = append(classNames, helper.DefaultLoadbalancerClass) + } if emptyClassName { classNames = append(classNames, "") } diff --git a/controllers/yawol-cloud-controller/targetcontroller/service_controller_test.go b/controllers/yawol-cloud-controller/targetcontroller/service_controller_test.go index 89507961..2cec7d03 100644 --- a/controllers/yawol-cloud-controller/targetcontroller/service_controller_test.go +++ b/controllers/yawol-cloud-controller/targetcontroller/service_controller_test.go @@ -638,6 +638,54 @@ var _ = Describe("Check loadbalancer reconcile", Serial, Ordered, func() { }, time.Second*5, time.Millisecond*500).Should(Succeed()) }) + It("create service without classname", func() { + By("create service") + service := v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "class-name-service-test5", + Namespace: "default", + }, + Spec: v1.ServiceSpec{ + Ports: []v1.ServicePort{ + { + Name: "port1", + Protocol: v1.ProtocolTCP, + Port: 12345, + TargetPort: intstr.IntOrString{IntVal: 12345}, + NodePort: 30360, + }, + }, + Type: "LoadBalancer", + }} + Expect(k8sClient.Create(ctx, &service)).Should(Succeed()) + + By("check creation of LB") + Eventually(func() error { + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: "default--class-name-service-test5", + Namespace: "default", + }, &lb) + return err + }, time.Second*5, time.Millisecond*500).Should(Succeed()) + + By("Check Event for creation") + Eventually(func() error { + eventList := v1.EventList{} + err := k8sClient.List(ctx, &eventList) + if err != nil { + return err + } + for _, event := range eventList.Items { + if event.InvolvedObject.Name == "class-name-service-test5" && + event.InvolvedObject.Kind == "Service" && + strings.Contains(event.Message, "LoadBalancer is in creation") { + return nil + } + } + return helper.ErrNoEventFound + }, time.Second*5, time.Millisecond*500).Should(Succeed()) + }) + It("create service with existingFloatingIP", func() { By("create service") service := v1.Service{ diff --git a/internal/helper/service.go b/internal/helper/service.go index 77bf5042..95571cc4 100644 --- a/internal/helper/service.go +++ b/internal/helper/service.go @@ -13,6 +13,7 @@ import ( coreV1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -165,7 +166,7 @@ func GetLoadBalancerNameFromService(service *coreV1.Service) string { } func getLoadBalancerClass(service *coreV1.Service) string { - if className, ok := service.Annotations[yawolv1beta1.ServiceClassName]; ok { + if className := service.Annotations[yawolv1beta1.ServiceClassName]; className != "" { return className } @@ -177,14 +178,7 @@ func getLoadBalancerClass(service *coreV1.Service) string { } func CheckLoadBalancerClasses(service *coreV1.Service, validClasses []string) bool { - serviceClassName := getLoadBalancerClass(service) - - for _, validClass := range validClasses { - if validClass == serviceClassName { - return true - } - } - return false + return sets.New(validClasses...).Has(getLoadBalancerClass(service)) } // ValidateService checks if the service is valid diff --git a/internal/helper/service_test.go b/internal/helper/service_test.go new file mode 100644 index 00000000..be03cb49 --- /dev/null +++ b/internal/helper/service_test.go @@ -0,0 +1,53 @@ +package helper + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/pointer" + + yawolv1beta1 "github.com/stackitcloud/yawol/api/v1beta1" +) + +var _ = Describe("loadbalancerClasses", Serial, Ordered, func() { + svc := corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "default", + }, + Spec: corev1.ServiceSpec{}, + } + + It("should return correct loadbalancerClass from annotation", func() { + s := svc.DeepCopy() + s.Annotations = map[string]string{ + yawolv1beta1.ServiceClassName: "foo", + } + expected := "foo" + Expect(getLoadBalancerClass(s)).To(Equal(expected)) + }) + + It("should return correct loadbalancerClass from spec", func() { + s := svc.DeepCopy() + s.Spec.LoadBalancerClass = pointer.String("bar") + expected := "bar" + Expect(getLoadBalancerClass(s)).To(Equal(expected)) + }) + + It("should return correct loadbalancerClass from annotation if also set in spec", func() { + s := svc.DeepCopy() + s.Annotations = map[string]string{ + yawolv1beta1.ServiceClassName: "foo", + } + s.Spec.LoadBalancerClass = pointer.String("bar") + expected := "foo" + Expect(getLoadBalancerClass(s)).To(Equal(expected)) + }) + + It("should return empty string if no class is set", func() { + s := svc.DeepCopy() + expected := "" + Expect(getLoadBalancerClass(s)).To(Equal(expected)) + }) +})