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

hostport: Don't masquerade localhost-to-localhost traffic #80591

Merged
merged 1 commit into from Aug 1, 2019

Conversation

@danwinship
Copy link
Contributor

commented Jul 25, 2019

What type of PR is this?
/kind bug

What this PR does / why we need it:
When you call HostPortManager.Add(id, mapping, natInterfaceName), it creates/ensures a rule:

-t nat -A POSTROUTING -o ${natInterfaceName} -s 127.0.0.0/8 -j MASQUERADE

This is supposed to ensure that traffic to the hostPort works from localhost; natInterfaceName is supposed to be the interface that traffic directed to pods would go through. Unfortunately, there's no way for the caller to say that it doesn't know what the right interface name is, and the existing code will error out if you pass an invalid interface name. CRI-O (which vendors this code, and doesn't know what the network plugin's "bridge" interface is) handles this by passing "lo" for the interface name, which results in a nonsensical rule insisting that localhost-to-localhost traffic needs to be masqueraded. Remarkably, this seems to not completely break the entire universe, although it does break a few small pieces of it (#66067).

This fixes the code to (a) explicitly allow passing "", and (b) create no rule if natInterfaceName is "" or "lo". (If the caller does this, that means that in theory localhost-to-hostport traffic may not work, but that was already the case before this patch as well, and CRI implementations currently have no way of knowing what the correct interface to pass is. The network plugin can try to fix this up by creating an appropriate rule itself. FTR, note that if "lo" actually is the correct value of natInterfaceName then no NAT rule would be needed anyway, so this is also actually correct for that case.)

(I guess HostPortManager could deal with this by analyzing the route table to figure out what interface would be used for traffic directed to the pod... but given that this was basically broken before, it seems likely that network plugins are already working around it by creating the rule themselves if they need it anyway.)

Which issue(s) this PR fixes:
Fixes #66067
(although not really; it won't be fixed until CRI-O, etc, vendor the new code)
Obsoletes #74665

Does this PR introduce a user-facing change?:

Fixes problems with connecting to services on localhost on some systems; in particular, DNS queries to systemd-resolved on Ubuntu.

/sig network
/priority important-soon
/cc @dcbw @erhudy

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

@danwinship: GitHub didn't allow me to request PR reviews from the following users: erhudy.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What type of PR is this?
/kind bug

What this PR does / why we need it:
When you call HostPortManager.Add(id, mapping, natInterfaceName), it creates/ensures a rule:

-t nat -A POSTROUTING -o ${natInterfaceName} -s 127.0.0.0/8 -j MASQUERADE

This is supposed to ensure that traffic to the hostPort works from localhost; natInterfaceName is supposed to be the interface that traffic directed to pods would go through. Unfortunately, there's no way for the caller to say that it doesn't know what the right interface name is, and the existing code will error out if you pass an invalid interface name. CRI-O (which vendors this code, and doesn't know what the network plugin's "bridge" interface is) handles this by passing "lo" for the interface name, which results in a nonsensical rule insisting that localhost-to-localhost traffic needs to be masqueraded. Remarkably, this seems to not completely break the entire universe, although it does break a few small pieces of it (#66067).

This fixes the code to (a) explicitly allow passing "", and (b) create no rule if natInterfaceName is "" or "lo". (If the caller does this, that means that in theory localhost-to-hostport traffic may not work, but that was already the case before this patch as well, and CRI implementations currently have no way of knowing what the correct interface to pass is. The network plugin can try to fix this up by creating an appropriate rule itself. FTR, note that if "lo" actually is the correct value of natInterfaceName then no NAT rule would be needed anyway, so this is also actually correct for that case.)

(I guess HostPortManager could deal with this by analyzing the route table to figure out what interface would be used for traffic directed to the pod... but given that this was basically broken before, it seems likely that network plugins are already working around it by creating the rule themselves if they need it anyway.)

Which issue(s) this PR fixes:
Fixes #66067
(although not really; it won't be fixed until CRI-O, etc, vendor the new code)
Obsoletes #74665

Does this PR introduce a user-facing change?:

Fixes problems with connecting to services on localhost on some systems; in particular, DNS queries to systemd-resolved on Ubuntu.

/sig network
/priority important-soon
/cc @dcbw @erhudy

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@danwinship danwinship force-pushed the danwinship:no-localhost-snat branch from 40d1347 to bf077b1 Jul 25, 2019

@danwinship

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

/retest

@dcbw

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, dcbw

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 0775e6b into kubernetes:master Aug 1, 2019

23 checks passed

cla/linuxfoundation danwinship authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.