daemon: Add option --bpf-lb-external-clusterip #15650

joamaki · 2021-04-12T12:50:35Z

Add the option --bpf-lb-external-clusterip to enable
routing to ClusterIP services from outside the cluster.

The loadbalancer routing logic is modified to only look
at the SVC_FLAG_ROUTABLE flag instead of expecting an
additional higher bit in addition to it.

The SVC_FLAG_ROUTABLE is only set for ClusterIP services
when the --bpf-lb-external-clusterip agent flag is set.

Fixes: #14581

joamaki · 2021-04-12T15:44:16Z

test-me-please

joamaki · 2021-04-13T13:53:18Z

test-me-please

joamaki · 2021-04-13T14:40:33Z

test-me-please

joamaki · 2021-04-13T15:22:50Z

Documentation/cmdref/cilium-agent.md

@@ -74,6 +74,7 @@ cilium-agent [flags]
      --enable-bpf-clock-probe                               Enable BPF clock source probing for more efficient tick retrieval
      --enable-bpf-masquerade                                Masquerade packets from endpoints leaving the host with BPF instead of iptables
      --enable-bpf-tproxy                                    Enable BPF-based proxy redirection, if support available
+      --enable-cluster-ip-external-access                    Enable external access to ClusterIP services


What other documentation related changes should be made for this change?

It'll be good to document this flag in the kube-proxy replacement getting started guide.

Thanks. Added documentation.

joamaki · 2021-04-13T15:23:04Z

bpf/lib/lb.h

 static __always_inline bool __lb_svc_is_routable(__u8 flags)
 {
-	return (flags & svc_is_routable_mask()) > SVC_FLAG_ROUTABLE;
+	return (flags & SVC_FLAG_ROUTABLE) != 0;


Is there anything that could break with this change?

Thinking loud about the upgrade path. When cilium-agent starts, services are updated from kube-apiserver before the datapath gets regenerated. So, it means that for some time, the old code will be running with the new service flags. I think this should be fine, as for previously routable services nothing should change.

During the downgrade, we will have the old service flags (= pre your changes) with the new datapath (= with your changes) for awhile. This will allow ClusterIP access from outside. But I guess this is tolerable.

aditighag · 2021-04-13T15:57:49Z

bpf/lib/lb.h

 static __always_inline bool __lb_svc_is_routable(__u8 flags)
 {
-	return (flags & svc_is_routable_mask()) > SVC_FLAG_ROUTABLE;


Shouldn't this be guarded by EnableClusterIPExternalAccess config?

Not sure what you mean by guarded. The flag is disabled by default in which case SVC_FLAG_ROUTABLE is unset for ClusterIP services. Earlier SVC_FLAG_ROUTABLE was set for ClusterIP services, but higher bits were unset and hence this function returned false for ClusterIP services. Now we only need to check for SVC_FLAG_ROUTABLE.

Speaking of which, I can't find where, in your PR, SVC_FLAG_ROUTABLE would be set or unset depending on the value for config.ExternalClusterIP. Shouldn't there be a change to the definition of SVC_FLAG_ROUTABLE in bpf/lib/common.h?

The change to this logic is in the agent in updateMasterService: https://github.com/cilium/cilium/pull/15650/files#diff-8eff0d99dd1ceb7d15ce632811672e7b17a17fb5e984fafa7875d6ad2433b3d8R519. It was already set earlier and in this PR I'm turning it off for ClusterIP services unless the new external access flag is set.

Ok thank you. On my first read, I somehow understood that you would be changing the value for SVC_FLAG_ROUTABLE depending on the value of ExternalClusterIP, but I understand this is not the case - You're just changing the flags.

joamaki · 2021-04-14T14:06:28Z

test-me-please

brb

Nice! Some comments.

pkg/option/config.go

install/kubernetes/cilium/templates/cilium-configmap.yaml

pkg/maps/lbmap/lbmap.go

brb · 2021-04-14T14:33:55Z

pkg/option/config.go

@@ -1963,6 +1971,8 @@ var (

 		k8sEnableLeasesFallbackDiscovery: defaults.K8sEnableLeasesFallbackDiscovery,
 		APIRateLimit:                     make(map[string]string),
+
+		EnableClusterIPExternalAccess: defaults.EnableClusterIPExternalAccess,


I think you don't need this.

Yep it isn't strictly necessary (as it'll default to false if unspecified). I added it since there were bunch of other boolean options that defaulted to false as well and were specified here. I'm fine either way.

brb · 2021-04-14T14:35:02Z

Documentation/gettingstarted/kubeproxy-free.rst

+Outside access to ClusterIP services
+************************************
+
+By default Cilium does not route packets coming from outside the cluster to a backend exposed


We use the backend term only internally in the Cilium codebase. The k8s term is service endpoint.

Good catch. I'll change it. There's quite few other mentions of backend in the same document in similar context (e.g. in DSR section). Wonder if we should do pass over the document and make sure the naming is consistent?

Wonder if we should do pass over the document and make sure the naming is consistent?

👍

brb · 2021-04-14T14:53:07Z

bpf/lib/lb.h

 static __always_inline bool __lb_svc_is_routable(__u8 flags)
 {
-	return (flags & svc_is_routable_mask()) > SVC_FLAG_ROUTABLE;
+	return (flags & SVC_FLAG_ROUTABLE) != 0;


Thinking loud about the upgrade path. When cilium-agent starts, services are updated from kube-apiserver before the datapath gets regenerated. So, it means that for some time, the old code will be running with the new service flags. I think this should be fine, as for previously routable services nothing should change.

During the downgrade, we will have the old service flags (= pre your changes) with the new datapath (= with your changes) for awhile. This will allow ClusterIP access from outside. But I guess this is tolerable.

brb · 2021-04-14T14:54:02Z

Adding @borkmann to the reviewers list, as he introduced the routable concept.

Documentation/cmdref/cilium-agent.md

Documentation/gettingstarted/kubeproxy-free.rst

install/kubernetes/cilium/templates/cilium-configmap.yaml

borkmann · 2021-04-19T22:51:40Z

Documentation/gettingstarted/kubeproxy-free.rst

+
+By default Cilium does not route packets coming from outside the cluster to a service endpoint 
+exposed with a ``ClusterIP`` service. Routing to ``ClusterIP`` services can be enabled using the 
+by setting ``config.enableClusterIPExternalAccess=true``.


Could we try to elaborate more on the rationale here on why we don't expose it by default? Back then we didn't expose it due to the assumption that the range would typically be non-routable, and given that, as a security measure, we also do not expect such traffic on the phys iface. (Plus there's NodePort which would accomplish similar things just in different IP/port range (if not backed by e.g. LoadBalancer). Perhaps there are additional pointers from K8s doc.)

I added the link to k8s documentation about this (which we already had below so I just moved it here).

borkmann · 2021-04-19T22:52:04Z

(Overall, code wise looks good to me, just few minor nits.)

joamaki · 2021-04-28T08:36:17Z

test-net-next

borkmann · 2021-04-28T12:29:27Z

Documentation/cmdref/cilium-agent.md

@@ -37,6 +37,7 @@ cilium-agent [flags]
      --bpf-lb-dev-ip-addr-inherit string                    Device name which IP addr is inherited by devices running LB BPF program (--devices)
      --bpf-lb-dsr-dispatch string                           BPF load balancing DSR dispatch method ("opt", "ipip") (default "opt")
      --bpf-lb-dsr-l4-xlate string                           BPF load balancing DSR L4 DNAT method for IPIP ("frontend", "backend") (default "frontend")
+      --bpf-lb-external-clusterip                            Enable external access to ClusterIP services


small doc nit, but no need to rerun ci: could we append here: (default false) ?

Add the option --bpf-lb-external-clusterip to enable routing to ClusterIP services from outside the cluster. The loadbalancer routing logic is modified to only look at the SVC_FLAG_ROUTABLE flag instead of expecting an additional higher bit in addition to it. The SVC_FLAG_ROUTABLE is only set for ClusterIP services when the flag is set. Fixes: cilium#14581 Signed-off-by: Jussi Maki <jussi@isovalent.com>

joamaki · 2021-04-28T13:41:14Z

Passing test-net-next here: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.16-net-next/398/. Only changed the agent flag help so I'll kick off the other checks, but not test-net-next.

joamaki · 2021-04-28T13:56:04Z

test-runtime

joamaki · 2021-04-28T13:56:15Z

test-1.20-4.19

joamaki · 2021-04-28T13:56:24Z

test-gke

joamaki · 2021-04-28T13:56:46Z

test-1.21-4.9

rpthms · 2021-06-27T13:30:40Z

This PR is marked for 1.10.0 but I don't see this option in the 1.10.1 Cilium image. Are there any plans to backport this PR?

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 12, 2021

maintainer-s-little-helper bot added this to In progress in 1.10.0 Apr 12, 2021

joamaki added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Apr 12, 2021

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 12, 2021

joamaki requested a review from brb April 12, 2021 12:51

maintainer-s-little-helper bot assigned brb Apr 12, 2021

joamaki force-pushed the pr/joamaki/flag-external-cluster-ip-access branch 4 times, most recently from 71535e7 to 15263b0 Compare April 13, 2021 13:52

joamaki commented Apr 13, 2021

View reviewed changes

aditighag requested changes Apr 13, 2021

View reviewed changes

joamaki force-pushed the pr/joamaki/flag-external-cluster-ip-access branch from 15263b0 to 39b6b7a Compare April 14, 2021 12:33

brb requested changes Apr 14, 2021

View reviewed changes

brb requested a review from borkmann April 14, 2021 14:53

maintainer-s-little-helper bot assigned borkmann Apr 14, 2021

joamaki force-pushed the pr/joamaki/flag-external-cluster-ip-access branch from 39b6b7a to 725ce3d Compare April 15, 2021 12:27

borkmann reviewed Apr 19, 2021

View reviewed changes

Documentation/cmdref/cilium-agent.md Outdated Show resolved Hide resolved

borkmann reviewed Apr 19, 2021

View reviewed changes

Documentation/gettingstarted/kubeproxy-free.rst Outdated Show resolved Hide resolved

borkmann reviewed Apr 19, 2021

View reviewed changes

install/kubernetes/cilium/templates/cilium-configmap.yaml Outdated Show resolved Hide resolved

borkmann reviewed Apr 19, 2021

View reviewed changes

joamaki force-pushed the pr/joamaki/flag-external-cluster-ip-access branch 2 times, most recently from fc1f551 to 2437df0 Compare April 20, 2021 12:53

christarazi removed their assignment Apr 27, 2021

joamaki force-pushed the pr/joamaki/flag-external-cluster-ip-access branch from 62e13c2 to 8922607 Compare April 28, 2021 07:42

borkmann reviewed Apr 28, 2021

View reviewed changes

joamaki force-pushed the pr/joamaki/flag-external-cluster-ip-access branch from 8922607 to 215945d Compare April 28, 2021 13:40

joamaki added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 28, 2021

rolinh merged commit c7a4272 into cilium:master Apr 29, 2021

1.10.0 automation moved this from In progress to Done Apr 29, 2021

pchaigno mentioned this pull request May 10, 2021

CI: K8sServicesTest Checks ClusterIP Connectivity With ClusterIP external access ClusterIP can be accessed when external access is enabled #15998

Closed

aanm added the needs-backport/1.10 label Jun 30, 2021

twpayne mentioned this pull request Jul 1, 2021

v1.10 backports 2021-07-01 #16731

Closed

twpayne added backport-pending/1.10 and removed needs-backport/1.10 labels Jul 1, 2021

twpayne mentioned this pull request Jul 5, 2021

v1.10 backports 2021-07-05 #16774

Merged

twpayne added backport-pending/1.10 and removed needs-backport/1.10 labels Jul 5, 2021

kkourt added backport-done/1.10 and removed backport-pending/1.10 labels Jul 8, 2021

aanm mentioned this pull request Jul 15, 2021

Prepare for release v1.10.3 #16898

Merged

borkmann mentioned this pull request Nov 22, 2021

IPv6 TCP connections broken / ACK packet not forwarded in case of service cluster-ip node/remote-pod #17941

Closed

2 tasks

brb mentioned this pull request Jan 17, 2022

IPv6 cluster IP not routable from external to the cluster #18494

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

daemon: Add option --bpf-lb-external-clusterip #15650

daemon: Add option --bpf-lb-external-clusterip #15650

joamaki commented Apr 12, 2021 •

edited

joamaki commented Apr 12, 2021

joamaki commented Apr 13, 2021

joamaki commented Apr 13, 2021

joamaki Apr 13, 2021

aditighag Apr 13, 2021

joamaki Apr 14, 2021

joamaki Apr 13, 2021

brb Apr 14, 2021

aditighag Apr 13, 2021

joamaki Apr 14, 2021 •

edited

qmonnet Apr 22, 2021

joamaki Apr 22, 2021

qmonnet Apr 22, 2021

joamaki commented Apr 14, 2021

brb left a comment

brb Apr 14, 2021

joamaki Apr 15, 2021

brb Apr 14, 2021

joamaki Apr 15, 2021

brb Apr 22, 2021

brb Apr 14, 2021

brb commented Apr 14, 2021

borkmann Apr 19, 2021

joamaki Apr 20, 2021

borkmann commented Apr 19, 2021 •

edited

joamaki commented Apr 28, 2021

borkmann Apr 28, 2021

joamaki commented Apr 28, 2021

joamaki commented Apr 28, 2021

joamaki commented Apr 28, 2021

joamaki commented Apr 28, 2021

joamaki commented Apr 28, 2021

rpthms commented Jun 27, 2021

daemon: Add option --bpf-lb-external-clusterip #15650

daemon: Add option --bpf-lb-external-clusterip #15650

Conversation

joamaki commented Apr 12, 2021 • edited

joamaki commented Apr 12, 2021

joamaki commented Apr 13, 2021

joamaki commented Apr 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joamaki Apr 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joamaki commented Apr 14, 2021

brb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brb commented Apr 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borkmann commented Apr 19, 2021 • edited

joamaki commented Apr 28, 2021

Choose a reason for hiding this comment

joamaki commented Apr 28, 2021

joamaki commented Apr 28, 2021

joamaki commented Apr 28, 2021

joamaki commented Apr 28, 2021

joamaki commented Apr 28, 2021

rpthms commented Jun 27, 2021

joamaki commented Apr 12, 2021 •

edited

joamaki Apr 14, 2021 •

edited

borkmann commented Apr 19, 2021 •

edited