ipaddress on Linux - default route pointing to unaddressed interface, with route src #682
Conversation
| if route_entry[:src] | ||
| addr = iface[route_int][:addresses] | ||
| unless addr.nil? or addr.has_key? route_entry[:src] or | ||
| not addr.values.any? { |a| a['family'] == family[:name] } |
There was a problem hiding this comment.
Please use symbolic operators || and ! for boolean calculations.
There was a problem hiding this comment.
Sorry - I was following the convention of the existing code. Will rework my diffs.
There was a problem hiding this comment.
Thank you! =)
(I'll likely submit a rework of this code once your changes go through to remove the remaining instances.)
glennmatthews
force-pushed
the
linux_network_ipaddress_cornercase
branch
from
bbee7fa
to
9051150
Compare
|
Changed to boolean operators as requested. |
|
Hi, is anything needed to move this along? |
|
Sorry for the delay @glennmatthews . It looks like there have been some recent changes to these files and we cannot cleanly merge. Would you please rebase this branch on latest master? |
|
Yup looks like a conflict with the refactoring done by 58605b2. I'll see about getting it fixed. |
glennmatthews
force-pushed
the
linux_network_ipaddress_cornercase
branch
from
9051150
to
6daaca8
Compare
|
@mcquin Rebased and reworked the diffs; tests are again passing. Thanks! |
|
@btm you touched it last ;) |
| ) | ||
| ) |
There was a problem hiding this comment.
this logic is kind of horrible to read. the comments are useful.
could we extract some helpers out of this though in order to make the logic more readable?
a trivial example:
def iface_has_no_addresses?(iface)
iface[iface][:addresses].nil?
end
|
@lamont-granquist Sorry - were you asking me to refactor the code for readability? Or was that a general observation that this logic is quite messy (which I agree with, but predates my changes)? |
|
yeah refactoring the code for readability would be good. |
|
essentially, if you look where you have comments, if you could make the code read like the comments via helpers... then the helpers become bite-size chunks that you can look at and validate that they do what it says on the tin. then you look at the overall algorithm and make sure it reads correctly... |
|
OK, let me ask a different way - can this code be merged as-is, or will it not be accepted until I find the time to refactor it and resubmit? 😄 |
|
yeah, i'd really like to see it more readable before accepting. otherwise i strongly suspect it won't get cleaned up, and then since this is the linux networking plugin, in 2-6 months after i've forgotten it, i'll have to waste a lot of time deciphering it when the next major PR comes in against it to fix some bug in the code... |
|
@thom fwiw, I left it better than I got it. :) I'll put this on the side of my desk and will help if it's still here when I find some time. |
|
@adamleff will also be looking into the refactor later this week. Thanks Adam! |
adamleff
force-pushed
the
linux_network_ipaddress_cornercase
branch
from
6daaca8
to
764623d
Compare
|
@mcquin / @lamont-granquist : would you mind giving this another look? I cleaned up /cc: @glennmatthews |
|
👍 Nice work Adam! |
adamleff
force-pushed
the
linux_network_ipaddress_cornercase
branch
from
764623d
to
37a6334
Compare
|
Rebased on master again due to the conflict created by the chefstyle work yesterday. Should be good to go for another review, @mcquin and @lamont-granquist -- thank you! |
| ) | ||
| ) | ||
| # the route must have a source field | ||
| return false unless defined?(r[:src]) |
There was a problem hiding this comment.
r[:src] will return nil if :src is not a key in the hash. It appears that defined?(nil) is truthy. Maybe you want unless r[:src].nil? or .empty?.
There was a problem hiding this comment.
@mcquin: That's a great find, because it actually uncovered an unrelated issue. The "returns" I was using was returning the whole method, not the individual #select eval block. When I implemented your fix, the tests uncovered unexpected results.
So thanks for that! I changed the logic and we should be good-to-go.
|
Code looks good to me. Thanks for the refactor @adamleff |
|
@lamont-granquist if you get a chance today, can you take a peek at this and make sure it aligns with what you were thinking in terms of a refactor? thank you :) |
adamleff
force-pushed
the
linux_network_ipaddress_cornercase
branch
from
6b21984
to
53620d4
Compare
adamleff
force-pushed
the
linux_network_ipaddress_cornercase
branch
from
53620d4
to
1163126
Compare
- we have a default route to an interface - the route has a configured src - the src address is not configured on the default interface, but the default interface has no address at all In this case we will choose the src as the ipaddress to report.
adamleff
force-pushed
the
linux_network_ipaddress_cornercase
branch
from
1163126
to
5e228f0
Compare
|
yes, substantially more readable 👍 |
|
@chef/client-core any moar eyeballs on this? |
|
Looks like a great refactor, thanks! 👍 |
…rnercase ipaddress on Linux - default route pointing to unaddressed interface, with route src
Version: 8.5.1
Environment: Cisco IOS XR (Windriver Linux 7)
Scenario:
Expected Result:
ohai ipaddressshould report 192.168.122.222 (the configuredsrcfor the default route) andohai networkshould include"default_interface": "fwdintf"Actual Result:
ohai ipaddressignores the default route (because of existing logic that says 'ignore default route if its src is not an address configured on the default interface') and reports 1.1.1.1 (lowest global scope IP address found). No value is reported fordefault_interface.Fix
If the default route src is not a configured address on the default interface, but the default interface has no addresses of this type, then do not ignore the default route when determining the ipaddress.