Make svclb as simple as possible

Signed-off-by: manuelbuil <mbuil@suse.com>

.github/workflows/e2e.yaml

@@ -38,7 +38,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        etest: [startup, s3, btrfs, externalip, privateregistry, embeddedmirror, wasm]
+        etest: [startup, s3, btrfs, externalip, privateregistry, embeddedmirror, wasm, svcpoliciesandfirewall]
       max-parallel: 3
     steps:
       - name: "Checkout"

docs/adrs/remove-svclb-klipperlb.md

@@ -1,4 +1,4 @@
-# Remove svclb daemonset
+# Remove svclb klipper-lb
 
 Date: 2024-09-26
 
@@ -77,10 +77,22 @@ KUBE-SVC-CVG3OEGEH7H5P3HQ  all  --  0.0.0.0/0            0.0.0.0/0
 
 As a consequence, the traffic never gets into the svclb daemonset pod. This can be additionally demonstrated by running a tcpdump on the svclb daemonset pod and no traffic will appear. This can also be demonstrated by tracing the iptables flow, where we will see how traffic is following the described path.
 
-Therefore, if we replace the logic to find the node IPs of the serviceLB controller by something which does not require the svclb daemonset, we could get rid of that daemonset since traffic is never reaching it. That replacement should be easy because in the end a daemonset means all nodes, so we could basically query kube-api to provide the IPs of all nodes.
+In Kubernetes 1.29, a [new related kube-proxy feature](https://kubernetes.io/docs/concepts/services-networking/service/#load-balancer-ip-mode) was added and it became beta in 1.30. This feature allows the load-balancer controller to specify if kube-proxy should include that `KUBE-EXT` rule or not. The selection is controlled by a new field called ipMode that has two values: * VIP (default): Keeps the current behaviour
+* Proxy: Removes the `KUBE-EXT` rule and hence Kube-proxy does not react to changes in the external-ip field
 
-*/ ON-GOING IMPLEMENTATION */
 
+Therefore, if our k3s load-balancer controller sets ipMode=Proxy, the traffic would finally get into the svclb daemon
+
+
+## Solutions
+
+One quick solution would be to use ipMode=Proxy when setting the status of the LoadBalancer services. However, it must be noted that klipper-lb is using iptables to do exactly what kube-proxy is doing, so there is no real benefit. We could include some extra features such as `X-Forwarded-For` to preserve the source IP, which would give a good reason to forward traffic to the svclb daemonset. Nonetheless, to achive that, we would need to replace klipper-lb with a proper load-balancer, which is out of the scope of this ADR. Note as well, that svclb is not working as expected when using `externalTrafficPolicy: Local`: it allows access to the service via a node IP even if there is no instance of a service pod running on that node
+
+Another solution is to get rid of the daemonset completely. However, that solution will not detect if there is a process already using the port in a node (by another k8s service or by a node server) because kube-proxy does not check this. Unfortunately, this solution will obscure the error to the user and make debugging more difficult.
+
+One simpler solution would be to replace klipper-lb with a tiny image that includes the binary `sleep`. Additionally, we would remove all the extra permissions required to run klipper-lb (e.g. NET_ADMIN). In this case, we would use the daemonset to reserve the port by HostPort and if a node is already using that port, that node's IP will not be included as external-IP. In such a case, the daemonset pod running on that node will clearly warn the user that it can't run because the port is already reserved, hence making it easy to debug.
+
+This commit includes the last solution by setting busybox as the new image for the daemonset instead of klipper-lb.
 
 ## Decision
 
@@ -89,10 +101,8 @@ Therefore, if we replace the logic to find the node IPs of the serviceLB control
 ## Consequences
 
 ### Positives
-* Less resource consumption as we won't need one daemonset per LoadBalancer type of service
 * One fewer repo to maintain (klipper-lb)
 * Easier to understand flow of traffic
 
 ### Negatives
-* Possible confusion for users that have been using this feature for a long time ("Where is the daemonset?") or users relying on that daemonset for their automation
+* None that I can think of
-* In today's solution, if two LoadBalancer type services are using the same port, it is rather easy to notice that things don't work because the second daemonset will not deploy, as the port is already being used by the first daemonset. Kube-proxy does not check if two services are using the same port and it will create both rules without any error. The service that gets its rules higher in the chain is the one that will be reached when querying $nodeIP:$port. Perhaps we could add some logic in the controller that warns users about a duplication of the pair ip&port

pkg/cloudprovider/servicelb.go

@@ -2,12 +2,12 @@ package cloudprovider
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"sort"
-	"strconv"
 	"strings"
 	"time"
-	"encoding/json"
+
 	"sigs.k8s.io/yaml"
 
 	"github.com/k3s-io/k3s/pkg/util"
@@ -43,6 +43,7 @@ var (
 	daemonsetNodeLabel     = "svccontroller." + version.Program + ".cattle.io/enablelb"
 	daemonsetNodePoolLabel = "svccontroller." + version.Program + ".cattle.io/lbpool"
 	nodeSelectorLabel      = "svccontroller." + version.Program + ".cattle.io/nodeselector"
+	extTrafficPolicyLabel  = "svccontroller." + version.Program + ".cattle.io/exttrafficpolicy"
 	priorityAnnotation     = "svccontroller." + version.Program + ".cattle.io/priorityclassname"
 	tolerationsAnnotation  = "svccontroller." + version.Program + ".cattle.io/tolerations"
 	controllerName         = names.ServiceLBController
@@ -55,7 +56,7 @@ const (
 )
 
 var (
-	DefaultLBImage = "rancher/klipper-lb:v0.4.9"
+	DefaultLBImage = "rancher/mirrored-library-busybox:1.36.1"
 )
 
 func (k *k3s) Register(ctx context.Context,
@@ -435,27 +436,8 @@ func (k *k3s) newDaemonSet(svc *core.Service) (*apps.DaemonSet, error) {
 	oneInt := intstr.FromInt(1)
 	priorityClassName := k.getPriorityClassName(svc)
 	localTraffic := servicehelper.RequestsOnlyLocalTraffic(svc)
-	sourceRangesSet, err := servicehelper.GetLoadBalancerSourceRanges(svc)
-	if err != nil {
-		return nil, err
-	}
-	sourceRanges := strings.Join(sourceRangesSet.StringSlice(), ",")
 	securityContext := &core.PodSecurityContext{}
 
-	for _, ipFamily := range svc.Spec.IPFamilies {
-		switch ipFamily {
-		case core.IPv4Protocol:
-			securityContext.Sysctls = append(securityContext.Sysctls, core.Sysctl{Name: "net.ipv4.ip_forward", Value: "1"})
-		case core.IPv6Protocol:
-			securityContext.Sysctls = append(securityContext.Sysctls, core.Sysctl{Name: "net.ipv6.conf.all.forwarding", Value: "1"})
-			if sourceRanges == "0.0.0.0/0" {
-				// The upstream default load-balancer source range only includes IPv4, even if the service is IPv6-only or dual-stack.
-				// If using the default range, and IPv6 is enabled, also allow IPv6.
-				sourceRanges += ",::/0"
-			}
-		}
-	}
-
 	ds := &apps.DaemonSet{
 		ObjectMeta: meta.ObjectMeta{
 			Name:      name,
@@ -464,6 +446,7 @@ func (k *k3s) newDaemonSet(svc *core.Service) (*apps.DaemonSet, error) {
 				nodeSelectorLabel:     "false",
 				svcNameLabel:          svc.Name,
 				svcNamespaceLabel:     svc.Namespace,
+				extTrafficPolicyLabel: "Cluster",
 			},
 		},
 		TypeMeta: meta.TypeMeta{
@@ -522,6 +505,7 @@ func (k *k3s) newDaemonSet(svc *core.Service) (*apps.DaemonSet, error) {
 			Name:            portName,
 			Image:           k.LBImage,
 			ImagePullPolicy: core.PullIfNotPresent,
+			Command:         []string{"sleep", "inf"},
 			Ports: []core.ContainerPort{
 				{
 					Name:          portName,
@@ -530,57 +514,7 @@ func (k *k3s) newDaemonSet(svc *core.Service) (*apps.DaemonSet, error) {
 					Protocol:      port.Protocol,
 				},
 			},
-			Env: []core.EnvVar{
-				{
-					Name:  "SRC_PORT",
-					Value: strconv.Itoa(int(port.Port)),
-				},
-				{
-					Name:  "SRC_RANGES",
-					Value: sourceRanges,
-				},
-				{
-					Name:  "DEST_PROTO",
-					Value: string(port.Protocol),
-				},
-			},
-			SecurityContext: &core.SecurityContext{
-				Capabilities: &core.Capabilities{
-					Add: []core.Capability{
-						"NET_ADMIN",
-					},
-				},
-			},
-		}
-
-		if localTraffic {
-			container.Env = append(container.Env,
-				core.EnvVar{
-					Name:  "DEST_PORT",
-					Value: strconv.Itoa(int(port.NodePort)),
-				},
-				core.EnvVar{
-					Name: "DEST_IPS",
-					ValueFrom: &core.EnvVarSource{
-						FieldRef: &core.ObjectFieldSelector{
-							FieldPath: getHostIPsFieldPath(),
-						},
-					},
-				},
-			)
-		} else {
-			container.Env = append(container.Env,
-				core.EnvVar{
-					Name:  "DEST_PORT",
-					Value: strconv.Itoa(int(port.Port)),
-				},
-				core.EnvVar{
-					Name:  "DEST_IPS",
-					Value: strings.Join(svc.Spec.ClusterIPs, ","),
-				},
-			)
 		}
-
 		ds.Spec.Template.Spec.Containers = append(ds.Spec.Template.Spec.Containers, container)
 	}
 
@@ -608,6 +542,11 @@ func (k *k3s) newDaemonSet(svc *core.Service) (*apps.DaemonSet, error) {
 	}
 	ds.Spec.Template.Spec.Tolerations = append(ds.Spec.Template.Spec.Tolerations, tolerations...)
 
+	// Change the label to force the DaemonSet to update and call onPodChange if the ExternalTrafficPolicy changes
+	if localTraffic {
+		ds.Spec.Template.Labels[extTrafficPolicyLabel] = "Local"
+	}
+
 	return ds, nil
 }
 

scripts/airgap/image-list.txt

@@ -1,5 +1,4 @@
 docker.io/rancher/klipper-helm:v0.9.3-build20241008
-docker.io/rancher/klipper-lb:v0.4.9
 docker.io/rancher/local-path-provisioner:v0.0.30
 docker.io/rancher/mirrored-coredns-coredns:1.11.3
 docker.io/rancher/mirrored-library-busybox:1.36.1

updatecli/updatecli.d/klipper-lb.yaml

@@ -1,71 +0,0 @@
----
-name: "Bump Klipper LB version"
-scms:
-  k3s:
-    kind: "github"
-    spec:
-      user: "{{ .github.user }}"
-      email: "{{ .github.email }}"
-      username: "{{ .github.username }}"
-      token: "{{ requiredEnv .github.token }}"
-      owner: "{{ .k3s.org }}"
-      repository: "{{ .k3s.repo }}"
-      branch: "{{ .k3s.branch }}"
-      commitmessage:
-        title: "Bump Klipper LB version"
-  klipper-lb:
-    kind: "github"
-    spec:
-      user: "{{ .github.user }}"
-      email: "{{ .github.email }}"
-      username: "{{ .github.username }}"
-      token: "{{ requiredEnv .github.token }}"
-      owner: "{{ .k3s.org }}"
-      repository: "{{ .klipper_lb.repo }}"
-      branch: "{{ .klipper_lb.branch }}"
-
-actions:
-  github:
-    title: "Bump Klipper LB version"
-    kind: "github/pullrequest"
-    scmid: "k3s"
-    spec:
-      automerge: false
-      mergemethod: "squash"
-      usetitleforautomerge: true
-      parent: false
-      labels: 
-        - "dependencies"
-
-sources:
-  klipper-lb:
-    name: "Get Klipper LB latest release version"
-    kind: "githubrelease"
-    spec:
-      owner: "{{ .klipper_lb.org }}"
-      repository: "{{ .klipper_lb.repo }}"
-      branch: "{{ .klipper_lb.branch }}"
-      token: "{{ requiredEnv .github.token }}"
-      versionfilter:
-        kind: "latest"
-
-conditions:
-  klipper-lb:
-    name: "Check rancher/klipper-lb image version in DockerHub"
-    kind: "dockerimage"
-    sourceid: "klipper-lb"
-    spec:
-      image: "rancher/klipper-lb"
-
-targets:
-  klipper-lb:
-    name: "Update rancher/klipper-lb image versions"
-    kind: "file"
-    scmid: "k3s"
-    sourceid: "klipper-lb"
-    spec:
-      files:
-        - "pkg/cloudprovider/servicelb.go"
-        - "scripts/airgap/image-list.txt"
-      matchpattern: 'rancher/klipper-lb:v\d+\.\d+\.\d+(-\w+)?'
-      replacepattern: 'rancher/klipper-lb:{{ source "klipper-lb" }}'

updatecli/values.yaml

@@ -11,10 +11,6 @@ klipper_helm:
   org: "k3s-io"
   repo: "klipper-helm"
   branch: "master"
-klipper_lb:
-  org: "k3s-io"
-  repo: "klipper-lb"
-  branch: "master"
 local_path_provisioner:
   org: "rancher"
   repo: "local-path-provisioner"