Simplify svclb daemonset

[manuelbuil]

Proposed Changes

Simplify svclb by replacing the klipper-lb image with a simpler one and remove extra permissions, etc required by the daemonset to be able to run iptables and forward traffic

TODO: Replace klipper-lb image in the image-list by the final image which would be used (right now bci-busybox is used as example)

Types of Changes

New feature

Verification

Create a service of type LoadBalancer and verify that:
1 - It gets and externalIP
2 - The service is reachable via that IP

Testing

Linked Issues

User-Facing Change

Further Comments

[brandond]

If nothing else, the daemonset currently handles port conflicts by ensuring that the LB pods are not scheduled to nodes where the port is currently in use, which in turn ensures that the loadbalancer does not advertise those node IPs. We would need to do the same via some other mechanism if we remove use of the daemonset.

I'm also curious if this works with kube-proxy replacements from other CNIs.

[manuelbuil]

If nothing else, the daemonset currently handles port conflicts by ensuring that the LB pods are not scheduled to nodes where the port is currently in use, which in turn ensures that the loadbalancer does not advertise those node IPs. We would need to do the same via some other mechanism if we remove use of the daemonset.

Thanks Brad for the feedback. That's a good point, kube-proxy will not complain if the port is already used by other k8s service or by the node. I wonder if we could leave the daemonset with a dummy image (e.g. busybox) just as a mechanism to prevent port duplication. The alternatives to that seem all pretty complicated.

I'm also curious if this works with kube-proxy replacements from other CNIs.

Yes, Cilium can take care of creating the infrastructure for loadbalancer services.

[brandond]

Don't use the latest tag, and this needs to come from something in the rancher/mirrored- images alongside everything else.

[manuelbuil]

Just curious, is this a requirement from CNCF?

[brandond]

no, but not using latest is a best practice (and required for airgap), and moving off docker hub to registry.suse.com would be a bigger deal as existing users may expect all of our images to be under the docker.io/rancher namespace, and we'd want to coordinate that change much further out.

[brandond]

I'd like to see tests that ensure that this does not change current behavior with regards to internalTrafficPolicy, externalTrafficPolicy, and lb source ranges.

[manuelbuil]

I'd like to see tests that ensure that this does not change current behavior with regards to internalTrafficPolicy, externalTrafficPolicy, and lb source ranges.

#10972

[brandond]

As we discussed in standup, I'm also curious if having ServiceLB set VIP: Proxy in the LoadBalancer status field, so that traffic actually hits the pod rules, allows the source address through as desired. If so, then the issues folks have been reporting are due to kube-proxy and we might consider keeping the svclb iptables rules around.

[brandond]

I'm curious why this is necessary. This would appear to duplicate the OnChange handler in the core Kubernetes cloud-controller code base, which calls EnsureLoadBalancer or UpdateLoadBalancer when the Service object changes. Is it not calling those handlers on the right events to see these changes?

[manuelbuil]

Imagine I have a cluster with two nodes and an HTTP server in one of the nodes. That server can be accessed by a service of type LoadBalancer. It'll have two externalIPs: node1-IP and node2-IP.

Now, I change the field externalTrafficPolicy from Cluster to Local. That should reduce the number of externalIPs to only 1.

When I do that, I see that EnsureLoadBalancer gets called, which redeploys the daemonset. In the current code, the daemonset manifest gets updated depending on the externalTrafficPolicy value. However, in my PR, that part of the manifest is removed.

To update the service from 2 externalIPs to 1, the function updateStatus must be called: https://github.com/k3s-io/k3s/blob/master/pkg/cloudprovider/servicelb.go#L231. In the current code, it gets called via the onChangePod function because the svclb daemonset changed. However, in my PR, this is not happening. That's why I need a function to add the "item" to the workqueue, e.g. k.workqueue.Add(eps.Namespace + "/" + serviceName), so that updateStatus gets called.

An alternative to this would be to add a "dummy" label/annotation in the svclb daemonset to force onChangePod function to get called when externalTrafficPolicy changes. Do you know if adding a new watcher on services is expensive in terms of resource consumption?

[brandond]

It's not super expensive but it does mean that both the built-in CCM and Wrangler are now both watching Services. Adding a dummy annotation so that the behavior of the daemonset is retained might be more efficient than adding another watch?

[manuelbuil]

I'll add the label to the ds. That way, this new alternative, is more similar to what we are currently doing

[codecov]

Codecov Report

Attention: Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.

Project coverage is 43.72%. Comparing base (ed14f7f) to head (709a0d7).
Report is 19 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/cloudprovider/servicelb.go	71.42%	1 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (ed14f7f) and HEAD (709a0d7). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (ed14f7f)	HEAD (709a0d7)
e2etests	7	6

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10954      +/-   ##
==========================================
- Coverage   49.64%   43.72%   -5.92%     
==========================================
  Files         178      178              
  Lines       14801    14762      -39     
==========================================
- Hits         7348     6455     -893     
- Misses       6105     7099     +994     
+ Partials     1348     1208     -140     

Flag	Coverage Δ
e2etests	35.59% <71.42%> (-10.38%)	⬇️
inttests	36.64% <71.42%> (+16.92%)	⬆️
unittests	13.57% <0.00%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.