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

[WIP] Probe: Better mapping of NATted connections #3451

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Conversation

bboreham
Copy link
Collaborator

@bboreham bboreham commented Nov 28, 2018

Now that we get NAT information via netlink we have a flag to distinguish SRC_NAT from DST_NAT, so we can do a better job of mapping connections. See extensive comment in nat.go for explanation of the approach.

One example how we get a better result is that incoming connections to a Kubernetes NodePort are visible with their remote IP address - previously they did not get mapped. Unfortunately, when using an AWS ELB the source endpoint is excluded because its address is on the same network as the host where the probe runs.

Also stop special-casing the Docker bridge in Kubernetes mode. Most often, Kubernetes uses a bridge named cbr0 so the 'docker0' bridge should be locally-scoped, and exceptionally users can turn that off with --probe.docker.bridge=""

In the case targeted, Kubernetes uses a bridge named 'cbr0'.

The 'docker0' bridge should be locally-scoped in most cases, and
exceptionally users can turn that off with --probe.docker.bridge=""
Deal separately with SRC vs DST mapping.
Includes detailed rationale.
- Add SRC and DST NAT flag in status.
- The port number was wrong from host1's perspective - it should see
  the mapped port on host2 not the real port inside the container.
- Add nodes at the other end of the connection, like the real probe does.
- Pass the local host name to MakeEndpointNodeID as it would be on the real probe.
// Don't add the bridge in Kubernetes since container IPs are global and
// shouldn't be scoped
if flags.dockerBridge != "" && !flags.kubernetesEnabled {
if flags.dockerBridge != "" {
Copy link
Member

Choose a reason for hiding this comment

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

This is an improvement for when we have "unmanaged" containers running on a k8s host. However, it will also result in

  1. spurious error messages when docker isn't running on a k8s host (as will happen by default on some future k8s releases)

  2. incorrect treatment of the docker bridge as local if it is in fact the bridge used by k8s

We could avoid the first one by only erroring when the bridge name has been specified on the command line (rather than left to the default 'docker0' value).

Is the 2nd problem real?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. I don't think it is practical to use the docker0 bridge as the Kubernetes bridge nowadays except on a single-node cluster.
    I think Flannel used to connect one machine's Docker bridge to another, but nowadays that is driven via CNI with its own bridge. And Docker's own overlay networking will create a different bridge for each network.

probe/endpoint/nat.go Outdated Show resolved Hide resolved

All of the above can be satisfied by these rules:
For SRC_NAT either add NAT orig source as a copy of NAT reply destination
or add NAT reply destination as a copy of NAT original source
Copy link
Member

Choose a reason for hiding this comment

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

why do we not need to replace adjacencies here?

probe/endpoint/nat.go Outdated Show resolved Hide resolved
@bboreham bboreham changed the title Probe: Better mapping of NATted connections [WIP] Probe: Better mapping of NATted connections Jan 8, 2019
Matching up examples to rules to code was hard, so I added letters
(A), (B) etc.

This helped to track down various inconsistencies.  Now SRC and DST
NAT are symmetrical, and the code is quite similar, apart from
replacing the source vs destination of an adjacency.
@fbarl
Copy link
Contributor

fbarl commented Oct 25, 2019

@bboreham What's the status of this PR? Is there something blocking you from proceeding / merging it?

@bboreham bboreham marked this pull request as draft April 9, 2021 08:55
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.

3 participants