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

add route to gateway, so that we dont blackhole it #1171

Merged
merged 2 commits into from
Nov 9, 2020

Conversation

nonsense
Copy link
Member

@nonsense nonsense commented Nov 6, 2020

This PR is explicitly adding a route to the gateway, because otherwise packets to DNS (and other services) are dropped:

/ # ip route
28.116.0.0/16 dev eth1 scope link  src 28.116.128.0 # data network
30.0.0.0 dev eth1 scope link # data network
blackhole 100.64.0.0/10 # we want to drop packets to the services CIDR apart from DNS
100.64.0.10 via 100.96.4.1 dev eth0 # this is DNS
100.96.1.9 via 100.96.4.1 dev eth0 # this is influxdb
100.96.3.3 via 100.96.4.1 dev eth0 # this is redis
blackhole 100.96.4.0/24 # we want to drop packets to all destinations at 100.96.4.x except 100.96.4.1
100.96.4.1 dev eth0 scope link # the route we are adding in this PR

This problem got triggered when upgrading Kubernetes, weave and flannel, as well as host OS from debian 9 stretch to ubuntu Ubuntu 20.04.1 LTS (kernel from 4.9.0-11-amd64 to 5.4.0-1028-aws) at testground/infra#64

I am not sure why packets going through 100.96.4.1 (i.e. the gateway) were not being dropped until now.

@nonsense nonsense force-pushed the sidecar-add-route-to-gateway branch 2 times, most recently from 89085eb to c85f806 Compare November 6, 2020 16:49
@nonsense nonsense marked this pull request as ready for review November 6, 2020 17:28
Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -313,6 +315,25 @@ func (d *K8sReactor) manageContainer(ctx context.Context, container *docker.Cont
routeDst = r.Dst.String()
}

// detect the gateway => we know that we are going to delete the services CIDR, which are routed via the gateway,
// so we use they to figure out the gateway IP as well as the link index
if !addedGwRoute && routeDst == "100.64.0.0/10" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this out to a configuration variable and make it settable through a CLI flag?

Copy link
Member Author

Choose a reason for hiding this comment

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

Totally agree about extracting it - done.

I don't think we need a CLI flag for this - this configuration is highly coupled with the exact configuration of the Kubernetes cluster, so chances are it won't work if you have a different configuration (this PR is a good example of how fragile networking updates are).

@nonsense nonsense merged commit 1364c54 into master Nov 9, 2020
@nonsense nonsense deleted the sidecar-add-route-to-gateway branch November 9, 2020 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants