fix(kuma-dp): fix conntrack collisions #3459
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #3459 +/- ##
==========================================
+ Coverage 50.72% 50.76% +0.03%
==========================================
Files 920 920
Lines 56610 56691 +81
==========================================
+ Hits 28717 28778 +61
- Misses 25594 25612 +18
- Partials 2299 2301 +2
Continue to review full report at Codecov.
|
Vendors this fix (istio/istio#33572) from Istio Signed-off-by: John Harris <john.harris@konghq.com>
johnharris85
force-pushed
the
fix/conntrack-zones
branch
from
047042c
to
569c12d
Compare
johnharris85
changed the title
|
@Mergifyio update |
✅ Branch has been successfully updated |
|
@johnharris85 thanks! Do you want to look at reenabling the tests? |
|
@johnharris85 Hi John, as we discussed yesterday I want to help with finishing this PR. Would you be able to allow me to contribute to your fork please? |
|
As our e2e tests for universal are done from inside of a docker I also fixed the code to include our flag for capturing all dns |
As our e2e tests for universal are done from inside of a docker container, to make the networking work, we are bridging docker network to the host, which results in additional iptables rules inside every container within this network. Problem with these rules is that it's doing some NAT'ing for DNS udp datagrams with addition of randomly picked (during the container startup) port. It's problematic for this conntrack change as it works inside the `raw` table and among others `PREROUTING` chain and expects the datagrams from known port (53), which the earlier described NAT'ing is changing. This probably could be fixed by rethinking some of the rules, but as it's an edge case, after consultation with the team I decided it's not worth the time needed to properly solve it and instead I introduced to a `--skip-dns-conntrack-zone-split` flag for `kumactl install transparent-proxy`, which allows us to skip attaching the conntrack-collision iptables rules. This change was necessary for making some of the tests to work (univeral). I also fixed the code to include our flag for capturing all dns traffic and instead of hardcoding port `15053`, to use the one from the configuration. Signed-off-by: Bart Smykla <bartek@smykla.com>
bartsmykla
force-pushed
the
fix/conntrack-zones
branch
from
2966bba
to
b78edcd
Compare
lahabana
approved these changes
Jan 19, 2022
mergify bot
pushed a commit
that referenced
this pull request
Jan 20, 2022
* fix(kuma-dp): fix conntrack collisions Vendors this fix (istio/istio#33572) from Istio Signed-off-by: John Harris <john.harris@konghq.com> * chore(*): adjust changes to kuma environment As our e2e tests for universal are done from inside of a docker container, to make the networking work, we are bridging docker network to the host, which results in additional iptables rules inside every container within this network. Problem with these rules is that it's doing some NAT'ing for DNS udp datagrams with addition of randomly picked (during the container startup) port. It's problematic for this conntrack change as it works inside the `raw` table and among others `PREROUTING` chain and expects the datagrams from known port (53), which the earlier described NAT'ing is changing. This probably could be fixed by rethinking some of the rules, but as it's an edge case, after consultation with the team I decided it's not worth the time needed to properly solve it and instead I introduced to a `--skip-dns-conntrack-zone-split` flag for `kumactl install transparent-proxy`, which allows us to skip attaching the conntrack-collision iptables rules. This change was necessary for making some of the tests to work (univeral). I also fixed the code to include our flag for capturing all dns traffic and instead of hardcoding port `15053`, to use the one from the configuration. Signed-off-by: Bart Smykla <bartek@smykla.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Bart Smykla <bartek@smykla.com> (cherry picked from commit 45a6d5a)
bartsmykla
pushed a commit
that referenced
this pull request
Jan 20, 2022
* fix(kuma-dp): fix conntrack collisions Vendors this fix (istio/istio#33572) from Istio Signed-off-by: John Harris <john.harris@konghq.com> * chore(*): adjust changes to kuma environment As our e2e tests for universal are done from inside of a docker container, to make the networking work, we are bridging docker network to the host, which results in additional iptables rules inside every container within this network. Problem with these rules is that it's doing some NAT'ing for DNS udp datagrams with addition of randomly picked (during the container startup) port. It's problematic for this conntrack change as it works inside the `raw` table and among others `PREROUTING` chain and expects the datagrams from known port (53), which the earlier described NAT'ing is changing. This probably could be fixed by rethinking some of the rules, but as it's an edge case, after consultation with the team I decided it's not worth the time needed to properly solve it and instead I introduced to a `--skip-dns-conntrack-zone-split` flag for `kumactl install transparent-proxy`, which allows us to skip attaching the conntrack-collision iptables rules. This change was necessary for making some of the tests to work (univeral). I also fixed the code to include our flag for capturing all dns traffic and instead of hardcoding port `15053`, to use the one from the configuration. Signed-off-by: Bart Smykla <bartek@smykla.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Bart Smykla <bartek@smykla.com> (cherry picked from commit 45a6d5a) Co-authored-by: John Harris <johnharris85@users.noreply.github.com>
This was referenced Sep 24, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
When there are a lot of DNS queries and a smaller set of ephemeral ports, conntrack collisions could cause DNS traffic to bypass the mesh. The original fix is in Istio's transparentproxy but came in after we vendored that code.
Original fix here: (istio/istio#33572).
I also pulled across the Istio package's test changes but it looks like we skip tests for this vendored package anyway? Should we bring these back in?
Should probably also backport this.
Full changelog
Implement conntrack zones in the iptables rules based on a fix in Istio
Issues resolved
N / A
Documentation
N / A
Testing
Backwards compatibility
UPGRADE.mdwith any steps users will need to takewhen upgrading.
backport-to-stablelabel if the code is backwards compatible. Otherwise, list breaking changes.Signed-off-by: John Harris john.harris@konghq.com