-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
Conntrack flush support #32505
Conntrack flush support #32505
Conversation
- Added conntrack support Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>
- adding conntrack flush fix for moby#8795 Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>
@fcrisciani thanks for resolving this long standing issue and a good test to catch it as well. LGTM |
LGTM |
c.Assert(err, check.IsNil) | ||
var flowMatch int | ||
for _, flow := range flows { | ||
if flow.Forward.Protocol == 17 && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use constant for this magic number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do
@fcrisciani the windowsRS1 failure seems genuine -
|
When a container was being destroyed was possible to have flows in conntrack left behind on the host. If a flow is present into the conntrack table, the packet processing will skip the POSTROUTING table of iptables and will use the information in conntrack to do the translation. For this reason is possible that long lived flows created towards a container that is destroyed, will actually affect new flows incoming to the host, creating erroneous conditions where traffic cannot reach new containers. The fix takes care of cleaning them up when a container is destroyed. The test of this commit is actually reproducing the condition where an UDP flow is established towards a container that is then destroyed. The test verifies that the flow established is gone after the container is destroyed. Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>
8b8dfbe
to
1c4286b
Compare
adding |
Thanks! I'll go ahead and merge 👍 |
|
||
// Launch the server, this will remain listening on an exposed port and reply to any request in a ping/pong fashion | ||
cmd := "while true; do echo hello | nc -w 1 -lu 8080; done" | ||
_, _, err := dockerCmdWithError("run", "-d", "--name", "server", "--net", "testbind", "-p", "8080:8080/udp", "appropriate/nc", "sh", "-c", cmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fcrisciani I think @seemethere was refering to the use of a 3rd party image, and it that case it was not updated in 2 years. could you replace it with a simple alpine
for the official library ? it includes nc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vieux @seemethere Using that image is not accidental but is intended. The nc version in alpine does not support for example the option -q. There is a bug in nc that let the command never return also when specify the -w option.
Feel free to try to change it to the alpine removing the -q option to let the command work, but most likely the test will fail timing out because the nc command will hang forever.
There is a race condition between the local proxy and iptables rule setting. When we have a lot of UDP traffic, the kernel will create conntrack entries to the local proxy and will ignore the iptables rules set after that. Related to PR #32505. Fix #8795. Signed-off-by: Vincent Bernat <vincent@bernat.ch>
- What I did
Fixes #8795
Fixes #31610
Fixes #31414
On external connectivity removal, the bridge driver will take care of erasing all the conntrack flows relative to the endpoint that is going down
- How I did it
Introduced the conntrack support in the netlink library and then from libnetwork will cleanup the flows passing the IP address of the endpoint that is going down
- How to verify it
Added a test that creates a server container and a client container and establishes an UDP flow (using UDP for simplicity) between them.
The test verifies the presence of the flow fetching its from the conntrack table.
The server container is then destroyed and the tests validates that the previous flow got purged from conntrack.
- Description for the changelog
Conntrack flush on external connectivity removal
- A picture of a cute animal (not mandatory but encouraged)
![15](https://cloud.githubusercontent.com/assets/1823625/24888362/ae6f0384-1e17-11e7-956b-c67013cf8823.jpg)