Only check if route overlaps routes with scope: LINK#42598
Conversation
There was a problem hiding this comment.
Curious; should this check only be performed in this case, or also in other cases where NetworkOverlaps is used? (mostly wondering if this should be done in NetworkOverlaps() itself)?
There was a problem hiding this comment.
I think adding it to NetworkOverlaps() risks surprising someone else glancing at the code, the networks are overlapping so NetworkOverlaps() is performing exactly as expected, but during this specific comparison we only care about specific routes.
So without renaming it I'd be reluctant to put it there
Though looking at deleteInterfaceBySubnet() from ov_utils.go maybe it should be added, unfortunately I don't know anything about the overlay network so I have some code and documentation reading before I dare comment on that.
However, checkOverlap() from ov_network.go which uses NetworkOverlaps() should probably inherit this.
There was a problem hiding this comment.
Having had a closer look, I don't think it should be added to NetworkOverlaps(), this specific check is only being done in CheckRouteOverlaps() the other places where this is used it's being used to literally compare networks.
So for direct usages of this, all looks fine:
deleteInterfaceBySubnet()fromoverlayis checking subnetsCheckNameserverOverlaps()fromnetutilschecks if a given IP is in a specific subnetCheckRouteOverlaps()fromnetutilsis the only one looking at routes and that's being addressed in this PR
I've also pushed updated tests that verify the routes (checking both that the overlap works and that it does not overlap with routes with the wrong scope)
|
As for the test failure on Jenkins, I'm not sure that's related to my changes but I don't seem to have any way to rerun the tests and verify it |
tianon
left a comment
tianon
left a comment
There was a problem hiding this comment.
This seems relatively sane to me (this network picking code is best-effort anyhow).
I think the easiest way to restart the CI is to do a rebase/force push (probably even worth squashing your commits, since they're all tightly related).
95668d9 to
7245904
Compare
Signed-off-by: Alex Nordlund <[email protected]>
7245904 to
ee9e526
Compare
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
Whoops, this one dropped off my radar. Thanks for checking my comment, and the additional info.
LGTM
|
Thanks for creating a fix for the issue I reported a while back. Jan Hugo Prins |
|
@thaJeztah Could this be merged to the 20.10.10 release also? |
|
@thaJeztah Could this be backported to the 20.10.13 release? This problem make Docker unusable when using VPN connections! |
5 participants
closes #41525
partially addresses #33925
Signed-off-by: Alex Nordlund [email protected]
This implements the solution mentioned in #33925, and while it doesn't close that specific issue (since it's technically two different issues) it does remove one of the issues which may lead people there. and should close #41525
See #33925 (comment) for a great explanation.
But in my case we are using split VPN and we have routes set by our network team to make sure that docker works, unfortunately the fact that they set the routes also makes docker think the network is currently in use and prevents us from using it in the
default-address-pools.- What I did
I added a
network.Scope == netlink.SCOPE_LINKcheck to the route overlapping check.- How I did it
- How to verify it
- Description for the changelog
When checking for overlapping routes on Linux only consider ones where the scope is
LINK- A picture of a cute animal (not mandatory but encouraged)
Here's the dog I live with silently judging our VPN situation at a safe distance