Fix detection of mac address on IPv6 only systems #695
Conversation
| end | ||
| else | ||
| Ohai::Log.debug("Unable to determine ideal default route, not setting ipaddress/ip6address/macaddress. Should have considered #{default_route[:dev]}?") |
There was a problem hiding this comment.
why not pull it from default_route?
There was a problem hiding this comment.
I'd rather not change the behavior of ipaddress/ip6address/macaddress without a specific use case that we're solving for. Everyone wants the address attributes to be exactly the IP/mac address that they want in their use case, which is really hard to do without breaking someone else's use case.
We weren't previously setting the address attributes in this case, and we don't appear to need to for the ipv6 only case here. On the surface, it seems fine, but a whole pile of tests are broken by the change and would need to be updated. If we make a presumption here about a use case that nobody has asked for, we might find ourselves making a breaking change for another use case down the road. So I don't see the value in making that change until it is needed.
There was a problem hiding this comment.
I may be mis-understanding the code... but you end up doing the exact same thing in plugins/network.rb anyway, so why not do it here? Also, it's one thing to change the behavior for an existing case, but I can't imagine it's ever OK to just ... NOT pick a macaddress. That seems just broken in any and all use-cases.
There was a problem hiding this comment.
Can you identify any real scenario where we would get to this code path? If not, we're solving a problem that doesn't exist. I don't think we can get here, but I don't feel certain enough about that to fail to make someone report that to me.
It is absolutely okay to not pick a macaddress in some network configurations.
You need to be able to leave macaddress set to nil if the interface you're picking ipaddress from has no MAC. For a contrived case, if you run on a linux system with no routes and no external interfaces, you'll get nothing from linux/network because we have no routes, and fall back to the network plugin where you will get ipaddress => 127.0.0.1 and macaddress => nil because the localhost interface has no need for layer 2 addressing. You wouldn't hit the code path we're discussing though because you don't have any routes.
If you go way back to OHAI-362 we were breaking people who were using tun interfaces because we assumed macaddress would never be nil. Part of the fix of OHAI-362 is visible in the conditional check for the NOARP flag that still exists:
macaddress iface[route[:dev]][:addresses].select{|k,v| v["family"]=="lladdr"}.first.first unless iface[route[:dev]][:flags].include? "NOARP"
A point-to-point link isn't going to have a mac address because there's no addressing required. There's only one other host on the link, so anything we put on the wire is destined for that host and it expects everything it reads on the wire came from us.
There was a problem hiding this comment.
What? I get to that code!! That's the whole point of the bug.
There was a problem hiding this comment.
Are you sure? Have you run this branch and you're not getting a macaddress?
There was a problem hiding this comment.
Allow me to prove the point I laid out in the original bug:
# pwd
/opt/chef/embedded/apps/ohai/lib/ohai/plugins/linux
# diff -u network.rb.orig network.rb
--- network.rb.orig 2015-12-23 15:00:11.551300264 -0800
+++ network.rb 2015-12-23 14:59:39.864068269 -0800
@@ -332,6 +332,8 @@
else
ip6address route[:src]
end
+ else
+ puts "PHILD: I am here"
end
end
end
# ohai | grep PHILD
PHILD: I am here
#
I assure I while I said my patch wasn't good enough for production, I didn't just throw unreachable code in....
There was a problem hiding this comment.
Are you sure? Have you run this branch and you're not getting a macaddress?
I get a mac address with this branch for sure... but I also hit that code. When I use your branch:
[2015-12-23T15:25:39-08:00] DEBUG: Unable to determine ideal default route, not setting ipaddress/ip6address/macaddress. Should have considered eth0?
|
@chef/client-core put down that cookie and review please. |
|
Just to be clear here... the PR does in fact give me a mac address, but it's I'm very unclear in general why both of those files try to determine |
|
I understand the bug you're experiencing in v6 route detection, you shouldn't be falling through. I'll replicate and then fix that, standby. |
|
Standing by. Ish --- I'm on a plane, so connection isn't terribly reliable, especially ssh over ssh over vpn over ATG wifi. :) |
Previously we almost always set macaddress to the value from the interface we took ipaddress from, unless we didn't get any ipaddress, at which point we would accept the macaddress from the interface we took ip6address from. We're seeing ipv6 only hosts now, we need to be able to take macaddress from an ip6address interface if that's the most logical choice so we can ignore loopback or random/meaningless virtual interfaces that don't go anywhere.
We were previously ignoring ipv6 routes because we expected all routes to have a 'src' attribute. However, ipv6 uses an algorthim (RFC6724) to select the source address rather than having it fixed in the route. This change applies a different selection criteria for ipv6 routes, allowing ipv6 only linux hosts with the 'ip' binary to make better choices about setting ip6address and macaddress.
|
@jaymzh I've got to go back and work on tests now, but please take a look at this if you're around today. |
|
So I don't have time to test today, but looking over the code it's far far cleaner... though the |
|
@btm following up here now that the holiday is past. |
|
@jaymzh WB. I got distracted by the |
|
Ah - you're right. I saw the OK, initial testing looks like this is working as expected. I'll do some more testing and hopefully by the time you have the tests fixed by able to give full thumbs-up |
|
OK here's my findings:
Which I'm pretty sure is not the desired behavior? |
|
Not desired, but expected due to the use of src there. Do you need to get ip6address from linux/network instead of network? I started writing an RFC6724 implementation for when src isn't available, but realized I didn't think I needed that for macaddress because we take the device from the default route. We would need it to pick an ip6address from a route without a src. |
|
Much the same as the previous round where we made |
|
So I'm entirely sure you need a full RFC6724 implementation. The non-linux-specific plugin is already managing to pick a sane default address. On top of that, if I only have one (global scope) address, that should be pretty clear. You're already picking a "default interface", right? |
|
The network plugin returns the first address it finds on If we wanted to do better than that, I'd write a RFC6724 implementation in the linux/network plugin to make sure that the address being chosen for ip6address is more likely to be the one used as the source address when accessing the default gateway. Does allowing the network plugin to set ip6address for you not work? |
|
This totally works for me. I'm good with this (at least for now). That said I don't understand your logic. That RFC has nothing to do with Linux - if it should be anywhere, it should be in the non-OS-specific plugin... it's also weird to me that there's TWO plugins which can potentially fill in the SAME data, that seems a bit broken to me... |
|
Ping? |
|
I don't disagree with you on the ideal design. The That said, it's been like that for almost 4 years. There's significant risk in changing the algorithm for {ip,ip6,mac}address. So that refactor would start with rewriting the tests for My scope right now is fixing the original bug: choosing the "correct" macaddress on an ipv6 only host. I think we've done that (plus fixing the tests). I don't want to creep scope to a rewrite of the plugins unless we need to, which would necessitate triggering a prioritization conversation. I'll work on cleaning up the tests so we can get this merged. |
|
yup, I'm on board. Once tests pass, I look it all over again and thumbs up... but this looks.... way cleaner. |
|
Ping? |
|
these tests are extremely difficult to grok. working on it. not confident On Wed, Jan 6, 2016 at 2:48 PM, Phil Dibowitz notifications@github.com
|
|
this looks sane to me... though it definitely needs a lookover from someone else. Perhaps the Ohai queen? @mcquin |
|
Oh, and to be clear, I'm 👍 |
|
this looks correct 👍 |
|
there's not enough +1's for a non-employee to merge... but you guys wanna do this? or do we want more reviewers? |
Fix detection of mac address on IPv6 only systems
|
there's no distinction in the RFC between employee and non-employee, so with 2 +1's its always mergeable. |
|
@lamont-granquist did we change that? I thought it was at least half? |
|
@jaymzh yeah, two 👍 from maintainers. We backed off the "majority" thing because it was untenable once we ended up with a dozen maintainers. https://github.com/chef/chef-rfc/blob/master/rfc030-maintenance-policy.md#how-a-patch-gets-merged Some day we'll be explicit about what repositories that applies to other than chef/chef. |
|
ah... I do remember that PR now... totally forgot! thanks for the reminder. |
Fixes #685.
Builds on and replaces #689.
@jaymzh @tas50