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

Only check if route overlaps routes with scope: LINK #42598

Merged
merged 1 commit into from
Aug 26, 2021

Conversation

deepy
Copy link
Contributor

@deepy deepy commented Jul 5, 2021

closes #41525
partially addresses #33925

Signed-off-by: Alex Nordlund alexander.nordlund@nasdaq.com

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_LINK check 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
image

@@ -31,7 +31,7 @@ func CheckRouteOverlaps(toCheck *net.IPNet) error {
return err
}
for _, network := range networks {
if network.Dst != nil && NetworkOverlaps(toCheck, network.Dst) {
if network.Dst != nil && network.Scope == netlink.SCOPE_LINK && NetworkOverlaps(toCheck, network.Dst) {
Copy link
Member

Choose a reason for hiding this comment

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

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@deepy deepy Jul 12, 2021

Choose a reason for hiding this comment

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

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() from overlay is checking subnets
  • CheckNameserverOverlaps() from netutils checks if a given IP is in a specific subnet
  • CheckRouteOverlaps() from netutils is 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)

@deepy
Copy link
Contributor Author

deepy commented Jul 12, 2021

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

Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

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

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).

@deepy deepy force-pushed the linux-routeoverlaps-link-only branch from 95668d9 to 7245904 Compare August 25, 2021 08:56
Signed-off-by: Alex Nordlund <alexander.nordlund@nasdaq.com>
@deepy deepy force-pushed the linux-routeoverlaps-link-only branch from 7245904 to ee9e526 Compare August 25, 2021 08:58
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Whoops, this one dropped off my radar. Thanks for checking my comment, and the additional info.

LGTM

@thaJeztah thaJeztah added this to the 21.xx milestone Aug 26, 2021
@thaJeztah thaJeztah merged commit 2bb21b8 into moby:master Aug 26, 2021
@deepy deepy deleted the linux-routeoverlaps-link-only branch August 26, 2021 09:19
@jhaprins
Copy link

Thanks for creating a fix for the issue I reported a while back.
My dog also approves :-)

Jan Hugo Prins

@Nossnevs
Copy link

@thaJeztah Could this be merged to the 20.10.10 release also?

@Nossnevs
Copy link

Nossnevs commented Feb 1, 2022

@thaJeztah Could this be backported to the 20.10.13 release? This problem make Docker unusable when using VPN connections!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error initializing network controller: list bridge addresses failed: no available network
5 participants