Selectively disable iptables proxy's bridge-nf-call-iptables=1 behavior #20647

dcbw · 2016-02-04T15:11:20Z

bridge-nf-call-iptables appears to only be relevant when the containers are
attached to a Linux bridge, which is usually the case with default Kubernetes
setups, docker, and flannel. That ensures that the container traffic is
actually subject to the iptables rules since it traverses a Linux bridge
and bridged traffic is only subject to iptables when bridge-nf-call-iptables=1.

But with other networking solutions (like openshift-sdn) that don't use Linux
bridges, bridge-nf-call-iptables may not be not relevant, because iptables is
invoked at other points not involving a Linux bridge.

The decision to set bridge-nf-call-iptables should be influenced by networking
plugins in some way, but until that's plumbed through somehow, allow it to
be disabled easily.

dcbw · 2016-02-04T15:14:23Z

@thockin you added the bridge-nf-call-iptables=1 stuff originally in 3a5c23d, so I assume you might have some comments here.

The back-story is that openshift-sdn uses an OVS vswitch not a Linux bridge, but at the moment we still use docker for IPAM, and the IP address assigned to the docker bridge (from which containers are IPAMed) is the same as the IP address assigned to the vswitch port where container traffic exits the OVS vswitch. openshift-sdn removes the container veth from the docker bridge and puts it into the OVS vswitch, so the docker bridge is no longer involved, and thus bridge-nf-call-iptables=1 is no longer useful. But it confuses iptables horribly since two interfaces have the same IP address.

This really should be a plugin-by-plugin decision, but we need some way in the future to plumb this through better. For now though, this PR will allow us to remove some awful hacks in openshift.

k8s-github-robot · 2016-02-04T15:16:12Z

Labelling this PR as size/S

k8s-bot · 2016-02-04T15:56:19Z

GCE e2e build/test failed for commit bed53f09f9cbed13c694ebb94131162fd6a6be48.

dcbw · 2016-02-04T16:19:05Z

Test flake is #20633

thockin · 2016-02-04T17:22:14Z

pkg/proxy/iptables/proxier.go

+	// to a bridge, as is usually the case.  But when containers are attached
+	// to an SDN switch this is not useful (since there is no bridge involved)
+	// and it may interfere with that SDN's operation
+	if containersBridged {


What if we moved this to kubelet and looked at the network plugin instead? Or otherwise - what is the evolution plan for this? Kube-proxy doesn't really understand network plugins.

if this depends on the network plugin, it seems like something the plugin interface should be able to answer, and something that has the network plugin should ask before setting. is the kubelet the right place for that to happen currently?

k8s-bot · 2016-02-04T17:35:48Z

GCE e2e build/test failed for commit 3cb0cedac03be7f2e4f6f729576bf4c0bcbc0653.

k8s-github-robot · 2016-02-08T19:02:11Z

PR needs rebase

dcbw · 2016-02-09T17:57:12Z

@thockin @liggitt yeah it should definitely have some interaction with networking plugins. I'll rework for that.

k8s-github-robot · 2016-02-09T22:49:29Z

Labelling this PR as size/M

k8s-bot · 2016-02-09T23:22:25Z

GCE e2e test build/test passed for commit fbf39fe10888767efede618f6704a2498a581059.

k8s-github-robot · 2016-02-09T23:33:04Z

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

dcbw · 2016-02-10T16:56:47Z

@thockin how about this approach?

dcbw · 2016-02-19T16:32:10Z

@thockin @bprashanth PTAL, thanks!

bprashanth · 2016-02-19T16:41:47Z

pkg/proxy/iptables/proxier.go

-	// Load the module.  It's OK if this fails (e.g. the module is not present)
-	// because we'll catch the error on the sysctl, which is what we actually
-	// care about.
-	exec.Command("modprobe", "br-netfilter").CombinedOutput()


Can you log something here in case someone already has a plugin that needs br-netfilter and they're getting it for free?

Can you log something here in case someone already has a plugin that needs br-netfilter and they're getting it for free?

Ok, but not quite sure what you're asking for... logging that the load failed? it's expected to fail in a bunch of systems because br-netfilter used to be built-in to the bridge module, but got split sometime in the late 3.1x timeframe. So new systems need this, older ones don't.

Can you just run modinfo and log something like: br-netfilter module [not] loaded?

Yeah, I'll doing that.

bprashanth · 2016-02-19T16:42:22Z

LGTM but for the logging nit

dcbw · 2016-02-20T16:10:35Z

@bprashanth updated with more logging as requested, PTAL, thanks!

liggitt · 2016-02-20T16:12:45Z

pkg/proxy/iptables/proxier.go

@@ -190,14 +189,6 @@ func NewProxier(ipt utiliptables.Interface, exec utilexec.Interface, syncPeriod
 		return nil, fmt.Errorf("can't set sysctl %s: %v", sysctlRouteLocalnet, err)


is this sysctl call still appropriate for the proxier to set, rather than the network plugin?

We can move it to a plugin when we start using plugins by default. For now I'd like to keep things as they are unless absolutely necessary. the no-op plugin should really no-op.

Worse, there was a separate PR, somewhere, that made the no-op plugin go away and be a nil pointer.

k8s-bot · 2016-02-20T16:41:33Z

GCE e2e test build/test passed for commit efe2ee902334ce59b1eb013439eab78bd10e9e09.

bprashanth · 2016-02-22T02:12:51Z

pkg/kubelet/network/plugins.go

+	if _, err := utilexec.New().Command("modprobe", "br-netfilter").CombinedOutput(); err != nil {
+		glog.V(3).Infof("Module br-netfilter not loaded")
+	} else {
+		glog.V(3).Infof("Module br-netfilter loaded")


This is actually not what I meant. I meant if someone has a CNI plugin that needs br-netfilter, and it currently works because kube-proxy is invoking the load, it will not work after this change. I was asking for the output of modinfo in kube-proxy as a clue that it isn't loaded. This logging statement won't get invoked in the case I'm talking about because we'll init CNI.

bprashanth · 2016-02-22T02:13:54Z

I'm kind of sad that the no-op plugin has side effects now. But this appears to be the easiest work around for a potentially disruptive scenario, so LGTM modulo that logging comment (or you can answer with why you don't want to do it).

dcbw · 2016-02-22T22:19:24Z

@bprashanth @thockin is the latest more in line with what you were thinking?

k8s-bot · 2016-02-22T22:45:15Z

GCE e2e test build/test passed for commit 32f6f9db0faf362dfb12e73323129c492e889a99.

bprashanth · 2016-02-22T23:03:48Z

pkg/proxy/iptables/proxier.go

+		warnBrNetfilter = true
+	}
+	if warnBrNetfilter {
+		glog.V(3).Infof("missing br-netfilter module or unset br-nf-call-iptables; proxy may not work as intended")


Can you make this an infof? it should be a one time thing and no one runs V(3) in prod

@bprashanth done, PTAL

…ugins bridge-nf-call-iptables appears to only be relevant when the containers are attached to a Linux bridge, which is usually the case with default Kubernetes setups, docker, and flannel. That ensures that the container traffic is actually subject to the iptables rules since it traverses a Linux bridge and bridged traffic is only subject to iptables when bridge-nf-call-iptables=1. But with other networking solutions (like openshift-sdn) that don't use Linux bridges, bridge-nf-call-iptables may not be not relevant, because iptables is invoked at other points not involving a Linux bridge. The decision to set bridge-nf-call-iptables should be influenced by networking plugins, so push the responsiblity out to them. If no network plugin is specified, fall back to the existing bridge-nf-call-iptables=1 behavior.

k8s-bot · 2016-02-23T16:05:46Z

GCE e2e test build/test passed for commit 6248939.

bprashanth · 2016-02-23T16:53:06Z

LGTM

k8s-github-robot · 2016-02-25T08:54:34Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-02-25T09:24:42Z

GCE e2e test build/test passed for commit 6248939.

k8s-github-robot · 2016-02-25T09:27:47Z

Automatic merge from submit-queue

…tables Auto commit by PR queue bot

https://trello.com/c/vnvUCQPG/112-3-remove-the-bridge-nf-call-iptables-hack-sdn-techdebt Effectively reverts d510a76 "Fix up net.bridge.bridge-nf-call-iptables after kubernetes breaks it" now that upstream kube PR kubernetes/kubernetes#20647 got merged. The hack is no longer necessary.

googlebot added the cla: yes label Feb 4, 2016

dcbw mentioned this pull request Feb 4, 2016

Ensure bridge-nf-call-iptables is always disabled on startup openshift/openshift-sdn#251

Closed

k8s-github-robot assigned bgrant0607 Feb 4, 2016

k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 4, 2016

dcbw force-pushed the allow-disabling-bridge-nf-call-iptables branch from bed53f0 to 3cb0ced Compare February 4, 2016 17:21

thockin reviewed Feb 4, 2016
View reviewed changes

thockin added this to the v1.2 milestone Feb 4, 2016

thockin assigned thockin and unassigned bgrant0607 Feb 4, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 8, 2016

dcbw force-pushed the allow-disabling-bridge-nf-call-iptables branch from 3cb0ced to fbf39fe Compare February 9, 2016 22:40

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 9, 2016

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 9, 2016

k8s-github-robot added the needs-ok-to-merge label Feb 9, 2016

bprashanth reviewed Feb 19, 2016
View reviewed changes

dcbw force-pushed the allow-disabling-bridge-nf-call-iptables branch from fbf39fe to efe2ee9 Compare February 20, 2016 16:10

liggitt reviewed Feb 20, 2016
View reviewed changes

bprashanth reviewed Feb 22, 2016
View reviewed changes

dcbw force-pushed the allow-disabling-bridge-nf-call-iptables branch from efe2ee9 to 32f6f9d Compare February 22, 2016 22:18

bprashanth reviewed Feb 22, 2016
View reviewed changes

dcbw force-pushed the allow-disabling-bridge-nf-call-iptables branch from 32f6f9d to 6248939 Compare February 23, 2016 15:35

bprashanth added lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-merge and removed needs-ok-to-merge labels Feb 23, 2016

k8s-github-robot pushed a commit that referenced this pull request Feb 25, 2016

Merge pull request #20647 from dcbw/allow-disabling-bridge-nf-call-ip…

33ef7a9

…tables Auto commit by PR queue bot

k8s-github-robot merged commit 33ef7a9 into kubernetes:master Feb 25, 2016

dcbw mentioned this pull request Feb 25, 2016

Revert SDN bridge-nf-call-iptables=0 hack openshift/origin#7627

Merged

danwinship mentioned this pull request Sep 21, 2023

Remove an old compatibility check/warning #120818

Merged

dhruvmehtaaa mentioned this pull request Sep 24, 2023

Remove an old compatibility check/warning #120851

Merged

		@@ -190,14 +189,6 @@ func NewProxier(ipt utiliptables.Interface, exec utilexec.Interface, syncPeriod
		return nil, fmt.Errorf("can't set sysctl %s: %v", sysctlRouteLocalnet, err)

Selectively disable iptables proxy's bridge-nf-call-iptables=1 behavior #20647

Selectively disable iptables proxy's bridge-nf-call-iptables=1 behavior #20647

Conversation

dcbw commented Feb 4, 2016

dcbw commented Feb 4, 2016

k8s-github-robot commented Feb 4, 2016

k8s-bot commented Feb 4, 2016

dcbw commented Feb 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-bot commented Feb 4, 2016

k8s-github-robot commented Feb 8, 2016

dcbw commented Feb 9, 2016

k8s-github-robot commented Feb 9, 2016

k8s-bot commented Feb 9, 2016

k8s-github-robot commented Feb 9, 2016

dcbw commented Feb 10, 2016

dcbw commented Feb 19, 2016

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

bprashanth commented Feb 19, 2016

dcbw commented Feb 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-bot commented Feb 20, 2016

Choose a reason for hiding this comment

bprashanth commented Feb 22, 2016

dcbw commented Feb 22, 2016

k8s-bot commented Feb 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-bot commented Feb 23, 2016

bprashanth commented Feb 23, 2016

k8s-github-robot commented Feb 25, 2016

k8s-bot commented Feb 25, 2016

k8s-github-robot commented Feb 25, 2016