Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to specify a bridge be placed into promiscuous mode. #441

Closed
wants to merge 2 commits into from

Conversation

dnardo
Copy link
Contributor

@dnardo dnardo commented Apr 25, 2017

This is a work around for kubernetes issue 20096 which it allows
the bridge to accept hairpin packets.
kubernetes/kubernetes#20096

Update documentation

Update test to reflect new changes.

@bboreham
Copy link
Contributor

You specifically need to use those kernels which crash?

@dcbw
Copy link
Member

dcbw commented Apr 25, 2017

You specifically need to use those kernels which crash?

@bboreham I think so, yes. It seems the affected kernels are mostly Ubuntu LTS kernels used in GCE by Google, and nobody was able to track down the kernel bug. I tried to reproduce this a year ago without success on Fedora and RHEL kernels.

@dnardo
Copy link
Contributor Author

dnardo commented Apr 25, 2017

This was a work around done originally in "kubenet". It's an alternative to using hairpin mode. We do this to avoid the issues seen in pr/20096 but for folks that need to hit their own service vip from within the pod. I'm working on deprecating kubenet in favor of using CNI only which means we need to get some of the kubenet stuff into CNI so that we can have parity with kubenet.

I also just realized that we should probably throw an error if someone tries to set hairpin and promisc at the same time, so let me also add that.

@bboreham
Copy link
Contributor

I am very aware what it is; I was checking to see if you are definitely targeting those buggy kernels.

@dnardo
Copy link
Contributor Author

dnardo commented Apr 25, 2017

@bboreham Yes.

@dcbw
Copy link
Member

dcbw commented Apr 25, 2017

@dnardo can you change your commit shortlog to conform to the "Format of the Commit Message"? I'd suggest something like:

bridge: add support for promiscuous mode
bridge: don't allow promiscuous mode and hairpin to be set at the same time

bridge: don't allow promiscuous mode and hairpin to be set at the same time

This is a work around for kubernetes issue 20096 which it allows
the bridge to accept hairpin packets.
kubernetes/kubernetes#20096
@dnardo
Copy link
Contributor Author

dnardo commented Apr 25, 2017

Thanks for pointing that out, done.

@@ -39,4 +39,5 @@ If the bridge is missing, the plugin will create one on first use and, if gatewa
* `ipMasq` (boolean, optional): set up IP Masquerade on the host for traffic originating from this network and destined outside of it. Defaults to false.
* `mtu` (integer, optional): explicitly set MTU to the specified value. Defaults to the value chosen by the kernel.
* `hairpinMode` (boolean, optional): set hairpin mode for interfaces on the bridge. Defaults to false.
* `promiscMode` (boolean, optional): set promiscuous mode on the bridge. Defaults to false.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you mention that this conflicts with hairpinMode?

@dnardo
Copy link
Contributor Author

dnardo commented Apr 27, 2017

@squeed Good point, done. Let me know if you think further context needs to be added to the doc. I can give a brief summary and refer to the issue# if you think that is necessary. I realize also that it may not be obvious why anyone may want to set this from the simple description that was given.

@dnardo
Copy link
Contributor Author

dnardo commented May 2, 2017

Any other comments on this?

@squeed
Copy link
Member

squeed commented May 2, 2017

Oh, I just thought of something else (sorry for swooping in, as it were). What happens when you try and set promiscuous mode on a bridge that already exists? I seem to recall this only used to be possible for empty bridges on Linux, but I can't remember if that restriction was lifted.

@dnardo
Copy link
Contributor Author

dnardo commented May 2, 2017

@squeed By exists you mean has member ports? Or are you referring to the idempotent behavior of setting promisc mode? This seems to work ok for me on my workstation kernel. Was there a particular issue you can point me to?

→ brctl show mynet0
bridge name bridge id STP enabled interfaces
mynet0 8000.0a580a0a0001 no veth157107e2
vethebc72d32

→ sudo ip link set dev mynet0 promisc off

→ sudo ip link set dev mynet0 promisc on
→ sudo ip link show dev mynet0
14: mynet0: <BROADCAST,MULTICAST,PROMISC,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
link/ether 0a:58:0a:0a:00:01 brd ff:ff:ff:ff:ff:ff

→ sudo ip link set dev mynet0 promisc on

→ sudo ip link set dev mynet0 promisc off
14: mynet0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
link/ether 0a:58:0a:0a:00:01 brd ff:ff:ff:ff:ff:ff

@dnardo
Copy link
Contributor Author

dnardo commented May 4, 2017

@squeed ping

@squeed
Copy link
Member

squeed commented May 5, 2017

Ah, thanks for the clarification. I must be misremembering. LGTM.

@dnardo
Copy link
Contributor Author

dnardo commented May 25, 2017

@dcbw I take it I need to submit a new PR to the plugins repo now that it's moved?

I also noticed that the vendor netlink lib in plugins doesn't have fixes I made and then re-vendored in this repo. I suppose I need to recreate the netlink updaet PR again as well?

@dcbw
Copy link
Member

dcbw commented Jun 28, 2017

@dnardo sorry, this got lost a bit. Now that the repo is split, yes, it would be great if you could move hte PR over to the plugins repo. And for netlink, it's fine to just vendor the new netlink as the first commit in your PR, then do the promisc changes as the second commit. No need for single-commit-per-PR.

@dnardo
Copy link
Contributor Author

dnardo commented Jul 5, 2017

/close

@dnardo dnardo closed this Jul 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants