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

Layer 2: Metallb shouldn't reply on interface with NOARP flag #351

Closed
kvaps opened this issue Nov 28, 2018 · 9 comments

Comments

@kvaps
Copy link
Contributor

commented Nov 28, 2018

Is this a bug report or a feature request?:

Feature request

What happened:

Speaker sends replies to any interface, even if arp disabled for it

What you expected to happen:

If interface have NOARP flag set, speaker should ignore it.

How to reproduce it (as minimally and precisely as possible):

ip link set dev <interface> arp off

Then try to arping

Anything else we need to know?:

Related #349

Environment:

  • MetalLB version:
  • Kubernetes version:
  • BGP router type/version:
  • OS (e.g. from /etc/os-release):
  • Kernel (e.g. uname -a):
@kvaps

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

@danderson , what do you thinking about using https://github.com/mickep76/netlink for identify flags on interfaces such as FlagNoArp and FlagMaster?

@danderson

This comment has been minimized.

Copy link
Owner

commented Nov 28, 2018

I'm not a huge fan of adding a dependency, especially one that uses syscall and unsafe. Those are hard to get right, even for experienced Go programmers.

The one netlink library I trust is the one by @mdlayher . It's very well tested, and Matt's got a bunch of experience with kernel interfacing. But AFAICT, he hasn't implemented interface reading, so we'd have to add that on top of the base netlink (or genetlink? Not sure) package.

I mean, with that said, if I get a PR with anything, I'll probably merge it. If it's not using mdlayher's netlink packages, I'll just file an extra bug to remind me to change it later ;)

@danderson danderson added the bug label Nov 28, 2018

@danderson danderson added this to To Do in Layer 2 mode via automation Nov 28, 2018

@kvaps

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

Ok, I'm trying to parse noarp from /sys/class/net/<interface>/flags using only standard libs, right now

@danderson

This comment has been minimized.

Copy link
Owner

commented Nov 28, 2018

Hmm, actually, it looks like that repository is mostly just copy/pasting the linux implementation of net.Interfaces, and adding support for more flags. If we're going to just do that anyway, I would prefer it if we just copy/paste our own implementation using https://golang.org/src/net/interface_linux.go as a reference. That way we can tailor it to exactly the data we need.

But like I said, if you're sending a PR, I'll merge it if it's not hugely unsafe. Worst case I'll just file a bug to think about improving it later, but that's not a big deal.

@kvaps

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

HI, I have success with another netlink library: github.com/vishvananda/netlink. It's well tested too.
Kubernetes uses it in ipvs proxier and kubenet projects.

Can you agree to using it in MetalLB?

@mdlayher

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2018

I would have assumed the stdlib would have the "No ARP" flag, but apparently not.

vishvananda/netlink is pretty widely used. I personally am not a big fan of some of the design choices it has made, but it seems reliable enough. The equivalent in my netlink ecosystem would be https://godoc.org/github.com/jsimonetti/rtnetlink, but this one is currently less mature and less battle tested.

That said, if you can read that flag from /sys, I'd just do that instead of mucking with (route) netlink for this one tiny thing.

@kvaps

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

Unfortunately it can't be simple readed from /sys/class/net/<interface>/flags, because all flags are encoded as hex values:

sysIFF_UP          = 0x1
sysIFF_BROADCAST   = 0x2
sysIFF_DEBUG       = 0x4
sysIFF_LOOPBACK    = 0x8
sysIFF_POINTOPOINT = 0x10
sysIFF_NOTRAILERS  = 0x20
sysIFF_RUNNING     = 0x40
sysIFF_NOARP       = 0x80
sysIFF_PROMISC     = 0x100
sysIFF_ALLMULTI    = 0x200
sysIFF_INTELLIGENT = 0x400
sysIFF_MULTICAST   = 0x800
sysIFF_MULTI_BCAST = 0x1000
sysIFF_UNNUMBERED  = 0x2000
sysIFF_PRIVATE     = 0x8000

The result is combination of all of them is written in /sys/class/net/<interface>/flags file.

I have no better idea for parse this value for and apply bitwise OR for all of them.

In bash I would like to use some workaround like:

grep '[8-f].$' /sys/class/net/*/flags
@mdlayher

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2018

But all you'd need to do is read that hex value and then check that flag, right? That sounds like an easy solution to the problem rather than bringing netlink into the picture.

@kvaps

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

You're right, I have an idea, I'll try to implement it now

Layer 2 mode automation moved this from To Do to Done Nov 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.