2924: Preserve the client source IP #3298

brb · 2018-05-13T13:09:19Z

This PR implements a mechanism for preserving the client source IP addr when accessing a Service annotated with service.spec.externalTrafficPolicy=Local.

The idea for preserving the source IP addr has been presented by #2924 (comment).

If the --no-masq-local flag is passed to weaver, it will dynamically maintain the weave-no-masq-local ipset containing CIDRs allocated by IPAM for the particular weaver. The ipset is used by the iptables rule: iptables -t nat -I WEAVE 1 -m set --match-set weave-no-masq-local dst -j RETURN. The rule prevents external traffic to be MASQUERADEd before being sent to a Pod.

I suggest to read each commit separately when reviewing this PR.

Fixes #2924.

bboreham · 2018-05-23T14:19:45Z

Just trying to understand this, without having been immersed in the details lately, I think the following explanation is useful:

Consider a packet arriving on node A from source S destined for pod P.
If P is on node A, then the reply can be sent straight to S. (however it got here, Linux and/or the network must know how to get it back)
If P is on node B, then a reply from B to S will be unexpected and/or not work.
Masquerading the packet as coming from node A means the reply from P on B will come via A, which is good.
So, this PR attempts to non-masquerade all cases where P is on A while still masquerading the cases where P is on B.

@brb can you confirm?

bboreham

Seems ok, if I understood the overall point.
A few comments below.

net/bridge.go

@@ -529,3 +537,27 @@ func ensureRulesAtTop(table, chain string, rulespecs [][]string, ipt *iptables.I

 	return nil
 }
+
+func reexpose(config *BridgeConfig, log *logrus.Logger) error {
+	// Get existing IP addrs of the weave bridge.


net/bridge.go

@@ -463,7 +469,10 @@ func configureIPTables(config *BridgeConfig) error {

 	if !config.NPC {
 		// Create a chain for allowing ingress traffic when the bridge is exposed
-		_ = ipt.NewChain("filter", "WEAVE-EXPOSE")
+		if err := ipt.ClearChain("filter", "WEAVE-EXPOSE"); err != nil {


net/bridge.go


-	return ipt.AppendUnique("nat", "POSTROUTING", "-j", "WEAVE")
+	// k8s-only: Create the ipset to store CIDRs allocated by IPAM for local pods.


prog/weave-npc/main.go

@@ -56,7 +57,7 @@ func resetIPTables(ipt *iptables.IPTables) error {
 }

 func resetIPSets(ips ipset.Interface) error {
-	// Remove ipsets prefixed `weave-` only
+	// Remove ipsets prefixed `weave-` excluding weave-no-masq-local (used by weaver).


net/bridge.go

+		if err := ips.Create(NoMasqLocalIpset, ipset.HashNet); err != nil {
+			return err
+		}
+		if err := ipt.Insert("nat", "WEAVE", 1, "-m", "set", "--match-set", string(NoMasqLocalIpset), "dst", "-j", "RETURN"); err != nil {


ipam/http.go

@@ -199,6 +199,6 @@ func (alloc *Allocator) HandleHTTP(router *mux.Router, defaultSubnet address.CID
 	})

 	router.Methods("GET").Path("/ipinfo/tracker").HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		fmt.Fprintf(w, tracker)
+		fmt.Fprintf(w, alloc.tracker.String())


net/bridge.go

+	return "no-masq-local"
+}
+
+func (t *NoMasqLocalTracker) HandleUpdate(prevRanges, currRanges []address.Range, local bool) error {


net/bridge.go

+
+func NewNoMasqLocalTracker() *NoMasqLocalTracker {
+	return &NoMasqLocalTracker{
+		// TODO(mp) Reuse ipset object as each time calling New creates a test set.


test/images/network-tester/webserver.go

@@ -186,6 +202,7 @@ func main() {
 	http.HandleFunc("/read", state.serveRead)
 	http.HandleFunc("/write", state.serveWrite)
 	http.HandleFunc("/status", state.serveStatus)
+	http.HandleFunc("/client_ip", state.serveClientIP)


test/840_weave_kube_3_test.sh

+
+CLIENT_IP_MASQ="$($SSH $HOST1 curl -sS http://$HOST2:31138/client_ip)"
+
+WEAVE_ENV_VARS="${WEAVE_ENV_VARS}\\n                - name: NO_MASQ_LOCAL\\n                  value: \"1\""


This ensures that all exposing-related iptables rules are created. As we do the re-expose, we need to clear the WEAVE-EXPOSE chain if such exists. Thus, we use ClearChain instead of NewChain to create the chain.

It's going to be used by net/bridge.go.

The ipset is used to track locally allocated CIDRs. If requested, external traffic sent to pods within these CIDR ranges will avoid SNAT'ing (NYI).

Cleaner approach than passing a tracker name to ipam/http.

It's going to be used by NoMasqLocalTracker.

The tracker updates the "no-masq-local" ipset with CIDR ranges allocated by IPAM.

One can enable the feature by setting NO_MASQ_LOCAL=1 among env vars of the weave-kube DaemonSet.

The endpoint is going to be used by --no-masq-local integration tests.

Also, increase timeout for smoke tests to 360 sec.

brb · 2018-06-04T12:35:48Z

@brb can you confirm?

ACK.

Additional useful info: as mentioned in the code, this works only on k8s, as in the vanilla Docker case, the Docker bridge acts as a default gw for a container, while in the k8s case - the weave bridge. So in the Docker case, if S sends a packet to P, a reply to it might be sent via different interface which can result in dropped reply due to rp_filter.

bboreham · 2018-06-04T13:12:41Z

It might be worth commenting that "Kubernetes only" is a shorthand for "containers where the Weave Net interface is the default gateway", since the latter can be achieved using vanilla Docker and weave attach, or via some other orchestrator using the Weave Net CNI plugin.

bboreham · 2018-06-04T13:27:08Z

Is this PR ready for final review now?

- Track only local changes. - Add comment to the iptables rule which prevents SNAT'ing. - Rename the ipset to weaver-no-masq-local to prevent NPC from destroying it. - Fix comment about create/flush WEAVE-EXPOSE. - Add comment about reexpose being no-op if bridge hasn't been exposed. - Add comment that the feature is not only for Kubernetes. - Reuse the ipset instance.

brb · 2018-06-04T17:33:22Z

Thanks for the review. Yep, it's ready for final review. Two last commits are relevant ones.

As ipset.New(...) can be called by two processes (weaver and weave-npc) at the same time, there is a possibility for a race when they both check the comment support. To prevent the race, we append a random nonce to the test ipset name.

bboreham · 2018-08-31T20:49:36Z

Some discussion on Twitter about whether we should default this on. Thoughts?

rade · 2018-08-31T20:55:38Z

Some discussion on Twitter about whether we should default this on.

See #3389

brb · 2018-09-03T08:59:40Z

Some discussion on Twitter about whether we should default this on. Thoughts?

Could you link to the discussion?

We can make it default only for cases where the weave bridge is a default gw for containers (do we know any k8s deployment where it is not the case?).

brb added this to the 2.4 milestone May 13, 2018

brb force-pushed the issues/2924-preserve-src-ip-on-local branch from bff0984 to 9792df4 Compare May 14, 2018 12:03

brb requested a review from bboreham May 22, 2018 14:31

bboreham reviewed May 23, 2018

View reviewed changes

brb force-pushed the issues/2924-preserve-src-ip-on-local branch 2 times, most recently from 20c1526 to ef42d9b Compare June 4, 2018 11:13

brb added 11 commits June 4, 2018 14:06

Re-expose weave bridge in EnsureBridge

039750a

This ensures that all exposing-related iptables rules are created. As we do the re-expose, we need to clear the WEAVE-EXPOSE chain if such exists. Thus, we use ClearChain instead of NewChain to create the chain.

Move npc/ipset package to net/ipset

27c9807

It's going to be used by net/bridge.go.

Create weave-no-masq-local ipset

21fb953

The ipset is used to track locally allocated CIDRs. If requested, external traffic sent to pods within these CIDR ranges will avoid SNAT'ing (NYI).

Extern LocalRangeTracker interface with String method

62c30d9

Cleaner approach than passing a tracker name to ipam/http.

Export tracker.RemoveCommon and tracker.Merge

4f99efa

It's going to be used by NoMasqLocalTracker.

Implement and start using NoMasqLocalTracker

ebf4b4b

The tracker updates the "no-masq-local" ipset with CIDR ranges allocated by IPAM.

Destroy weave-no-masq-local ipset upon "weave reset"

a0ec06a

Add NO_MASQ_LOCAL to weave-kube/launch.sh

6dbe95a

One can enable the feature by setting NO_MASQ_LOCAL=1 among env vars of the weave-kube DaemonSet.

Add /client_ip to network-tester

efd5d0e

The endpoint is going to be used by --no-masq-local integration tests.

Add k8s integration test case for preserving client IP addr

45a9ea6

Also, increase timeout for smoke tests to 360 sec.

Mention NO_MASQ_LOCAL opt in the docs

7fdf8fe

brb self-assigned this Jun 4, 2018

brb force-pushed the issues/2924-preserve-src-ip-on-local branch from ef42d9b to 4c13639 Compare June 4, 2018 17:28

brb removed their assignment Jun 4, 2018

brb force-pushed the issues/2924-preserve-src-ip-on-local branch from 4c13639 to 3daa028 Compare June 4, 2018 17:31

brb force-pushed the issues/2924-preserve-src-ip-on-local branch from 3daa028 to 2f8aa7a Compare June 5, 2018 06:57

brb mentioned this pull request Jun 6, 2018

Support "host-local" IPAM plugin in weave-kube #3311

Open

bboreham approved these changes Jun 11, 2018

View reviewed changes

bboreham merged commit 812c582 into master Jun 11, 2018

brb mentioned this pull request Sep 3, 2018

add --without-masquerade option to weave expose #3388

Merged

bboreham deleted the issues/2924-preserve-src-ip-on-local branch December 24, 2018 12:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2924: Preserve the client source IP #3298

2924: Preserve the client source IP #3298

brb commented May 13, 2018 •

edited

Loading

bboreham commented May 23, 2018

bboreham left a comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

brb commented Jun 4, 2018

bboreham commented Jun 4, 2018

bboreham commented Jun 4, 2018

brb commented Jun 4, 2018

bboreham commented Aug 31, 2018

rade commented Aug 31, 2018

brb commented Sep 3, 2018


		return ipt.AppendUnique("nat", "POSTROUTING", "-j", "WEAVE")
		// k8s-only: Create the ipset to store CIDRs allocated by IPAM for local pods.


		CLIENT_IP_MASQ="$($SSH $HOST1 curl -sS http://$HOST2:31138/client_ip)"

		WEAVE_ENV_VARS="${WEAVE_ENV_VARS}\\n - name: NO_MASQ_LOCAL\\n value: \"1\""

2924: Preserve the client source IP #3298

2924: Preserve the client source IP #3298

Conversation

brb commented May 13, 2018 • edited Loading

bboreham commented May 23, 2018

bboreham left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

brb commented Jun 4, 2018

bboreham commented Jun 4, 2018

bboreham commented Jun 4, 2018

brb commented Jun 4, 2018

bboreham commented Aug 31, 2018

rade commented Aug 31, 2018

brb commented Sep 3, 2018

brb commented May 13, 2018 •

edited

Loading