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

masquerade and route forwarded packets from host's loopback to the pod #2027

Merged
merged 5 commits into from Feb 1, 2016

Conversation

Projects
None yet
4 participants
@steveeJ
Contributor

steveeJ commented Jan 24, 2016

This implementation is not ideal because it uses heuristics to find interface names.

The host's veth interface has to be found with the only thing that's known, it's IP address. The names are randomly generated by CNI and are not returned in the CNI result. Further, due to the way the PTP plugin works, multiple veth interfaces have the same address, so they are all candidates for needing route_localnet enabled.

Fixes #1256.

  • Implementation
  • Tests, we have no port forwarding tests at all!

Follow-Up:

  • Improve IPv4 / IPv6 address detection code
  • Cleanup network tests comments
  • Retrieve interface names by CNI instead of using heuristics
@iaguis

This comment has been minimized.

Show comment
Hide comment
@iaguis

iaguis Jan 25, 2016

Member

A future improvement should bump to a CNI version which contains more information in the result which can be used instead of the heuristics.

Is this already implemented on CNI?

Member

iaguis commented Jan 25, 2016

A future improvement should bump to a CNI version which contains more information in the result which can be used instead of the heuristics.

Is this already implemented on CNI?

@steveeJ

This comment has been minimized.

Show comment
Hide comment
@steveeJ

steveeJ Jan 25, 2016

Contributor

No, otherwise I would've made use of that.

Contributor

steveeJ commented Jan 25, 2016

No, otherwise I would've made use of that.

@alban alban added this to the v1.0.0 milestone Jan 25, 2016

@iaguis iaguis added the priority/P0 label Jan 26, 2016

@steveeJ steveeJ changed the title from [WIP] masquerade and route forwarded packets from host's loopback to the pod to masquerade and route forwarded packets from host's loopback to the pod Jan 27, 2016

@steveeJ steveeJ added needs/review and removed do not merge labels Jan 27, 2016

defaultHostIPstring := defaultHostIP.String()
switch {
case strings.Contains(defaultHostIPstring, "."):

This comment has been minimized.

@alban

alban Jan 28, 2016

Member

Can it use case net.ParseIP(defaultHostIPstring).To4() != nil?

@alban

alban Jan 28, 2016

Member

Can it use case net.ParseIP(defaultHostIPstring).To4() != nil?

This comment has been minimized.

@steveeJ

steveeJ Jan 28, 2016

Contributor

I will try that, thanks!

@steveeJ

steveeJ Jan 28, 2016

Contributor

I will try that, thanks!

This comment has been minimized.

@steveeJ

steveeJ Jan 28, 2016

Contributor

Unfortunately .ToIPv16() is a false positive in this case. I'm digging for the origin but I'm leaving the code as is (working).

        2016/01/28 17:41:39 unexpected IPv6 Address returned for default host interface: "172.16.28.1"
        Failed to setup network: unexpected IPv6 Address returned for default host interface: "172.16.28.1"

FAIL
exit status 1
FAIL    github.com/coreos/rkt/tests 3.150s
@steveeJ

steveeJ Jan 28, 2016

Contributor

Unfortunately .ToIPv16() is a false positive in this case. I'm digging for the origin but I'm leaving the code as is (working).

        2016/01/28 17:41:39 unexpected IPv6 Address returned for default host interface: "172.16.28.1"
        Failed to setup network: unexpected IPv6 Address returned for default host interface: "172.16.28.1"

FAIL
exit status 1
FAIL    github.com/coreos/rkt/tests 3.150s

This comment has been minimized.

@jonboulle

jonboulle Jan 29, 2016

Contributor

A TODO to follow up would be nice.

@jonboulle

jonboulle Jan 29, 2016

Contributor

A TODO to follow up would be nice.

This comment has been minimized.

@steveeJ

steveeJ Jan 29, 2016

Contributor

I'd prefer to put it in an issue since we need a follow-up for this already. Are you okay with that?

@steveeJ

steveeJ Jan 29, 2016

Contributor

I'd prefer to put it in an issue since we need a follow-up for this already. Are you okay with that?

This comment has been minimized.

@jonboulle

jonboulle Jan 29, 2016

Contributor

Sure

Stefan Junker notifications@github.com schrieb am Fr., 29. Jan. 2016
15:37:

In networking/networking.go
#2027 (comment):

@@ -116,6 +125,54 @@ func Setup(podRoot string, podID types.UUID, fps []ForwardedPort, netList common
return &n, nil
}

+// enableDefaultLocalnetRouting enables the route_localnet attribute on the supposedly default network interface.
+// This allows to setup a loopback NAT to access port forwardings to Pods on the host with the localhost address.
+func (n *Networking) enableDefaultLocalnetRouting() error {

  • routeLocalnetFormat := ""
  • defaultHostIP, err := n.GetDefaultHostIP()
  • if err != nil {
  •   return err
    
  • }
  • defaultHostIPstring := defaultHostIP.String()
  • switch {
  • case strings.Contains(defaultHostIPstring, "."):

I'd prefer to put it in an issue since we need a follow-up for this
already. Are you okay with that?


Reply to this email directly or view it on GitHub
https://github.com/coreos/rkt/pull/2027/files#r51266774.

@jonboulle

jonboulle Jan 29, 2016

Contributor

Sure

Stefan Junker notifications@github.com schrieb am Fr., 29. Jan. 2016
15:37:

In networking/networking.go
#2027 (comment):

@@ -116,6 +125,54 @@ func Setup(podRoot string, podID types.UUID, fps []ForwardedPort, netList common
return &n, nil
}

+// enableDefaultLocalnetRouting enables the route_localnet attribute on the supposedly default network interface.
+// This allows to setup a loopback NAT to access port forwardings to Pods on the host with the localhost address.
+func (n *Networking) enableDefaultLocalnetRouting() error {

  • routeLocalnetFormat := ""
  • defaultHostIP, err := n.GetDefaultHostIP()
  • if err != nil {
  •   return err
    
  • }
  • defaultHostIPstring := defaultHostIP.String()
  • switch {
  • case strings.Contains(defaultHostIPstring, "."):

I'd prefer to put it in an issue since we need a follow-up for this
already. Are you okay with that?


Reply to this email directly or view it on GitHub
https://github.com/coreos/rkt/pull/2027/files#r51266774.

if err != nil {
return err
}
if string(routeLocalnetValue) != "1" {

This comment has been minimized.

@alban

alban Jan 28, 2016

Member

I guess it is ok to write "1" if it is already set to "1"? If so, no need to read it before and just write it. That would be less code.

@alban

alban Jan 28, 2016

Member

I guess it is ok to write "1" if it is already set to "1"? If so, no need to read it before and just write it. That would be less code.

This comment has been minimized.

@steveeJ

steveeJ Jan 28, 2016

Contributor

The read can save a syscall that is designed to mutate the in-kernel state. I think the accuracy is worth a couple LOC.

@steveeJ

steveeJ Jan 28, 2016

Contributor

The read can save a syscall that is designed to mutate the in-kernel state. I think the accuracy is worth a couple LOC.

This comment has been minimized.

@alban

alban Jan 28, 2016

Member

Reading the current value also take syscalls (open, read, close). And setting the value to 1 if it is already set to 1 should not be costly: ipv4_doint_and_flush() skips the more expensive flush if the value is not changed:

        if (write && *valp != val)
                rt_cache_flush(net);
@alban

alban Jan 28, 2016

Member

Reading the current value also take syscalls (open, read, close). And setting the value to 1 if it is already set to 1 should not be costly: ipv4_doint_and_flush() skips the more expensive flush if the value is not changed:

        if (write && *valp != val)
                rt_cache_flush(net);

This comment has been minimized.

@steveeJ

steveeJ Jan 28, 2016

Contributor

Going down to http://lxr.free-electrons.com/source/kernel/sysctl.c#L2066 it will make a more significant difference if write != 0. This might not even be worth discussing but it's also not worth changing IMHO.

@steveeJ

steveeJ Jan 28, 2016

Contributor

Going down to http://lxr.free-electrons.com/source/kernel/sysctl.c#L2066 it will make a more significant difference if write != 0. This might not even be worth discussing but it's also not worth changing IMHO.

@jonboulle

This comment has been minimized.

Show comment
Hide comment
@jonboulle

jonboulle Jan 28, 2016

Contributor

@steveeJ could you document the potential scaling issue (?) with this we discussed in the sync?

Contributor

jonboulle commented Jan 28, 2016

@steveeJ could you document the potential scaling issue (?) with this we discussed in the sync?

@steveeJ

This comment has been minimized.

Show comment
Hide comment
@steveeJ

steveeJ Jan 28, 2016

Contributor

@jonboulle yes, I'll create an issue as soon as this gets merged!

Contributor

steveeJ commented Jan 28, 2016

@jonboulle yes, I'll create an issue as soon as this gets merged!

Show outdated Hide outdated networking/networking.go Outdated
Show outdated Hide outdated networking/networking.go Outdated
Show outdated Hide outdated networking/networking.go Outdated
Show outdated Hide outdated networking/portfwd.go Outdated
Show outdated Hide outdated networking/portfwd.go Outdated
Show outdated Hide outdated networking/networking.go Outdated
Show outdated Hide outdated networking/networking.go Outdated
@jonboulle

This comment has been minimized.

Show comment
Hide comment
@jonboulle

jonboulle Jan 29, 2016

Contributor

style nits, but LGTM wenn es funktioniert.

Contributor

jonboulle commented Jan 29, 2016

style nits, but LGTM wenn es funktioniert.

Show outdated Hide outdated networking/networking.go Outdated
* Default net port forwarding connectivity
* ---
* Container launches http server on all its interfaces
* Host must be able to connect to container's http server on it's own interfaces

This comment has been minimized.

@jonboulle

jonboulle Jan 29, 2016

Contributor

it's/its
also how did we get into this block comment habit?!

@jonboulle

jonboulle Jan 29, 2016

Contributor

it's/its
also how did we get into this block comment habit?!

This comment has been minimized.

@steveeJ

steveeJ Jan 29, 2016

Contributor

should I just clean up the whole file comment-wise (separate PR)? I wasn't familiar with Go style guidelines and thought that was a good idea.

@steveeJ

steveeJ Jan 29, 2016

Contributor

should I just clean up the whole file comment-wise (separate PR)? I wasn't familiar with Go style guidelines and thought that was a good idea.

This comment has been minimized.

@jonboulle

jonboulle Jan 29, 2016

Contributor

SGTM

@jonboulle

jonboulle Jan 29, 2016

Contributor

SGTM

@jonboulle

This comment has been minimized.

Show comment
Hide comment
@jonboulle

jonboulle Jan 29, 2016

Contributor

LGTM modulo previous requests for (two?) follow ups
maybe worth another pair of eyes

Contributor

jonboulle commented Jan 29, 2016

LGTM modulo previous requests for (two?) follow ups
maybe worth another pair of eyes

Show outdated Hide outdated networking/networking.go Outdated
Show outdated Hide outdated networking/networking.go Outdated
Show outdated Hide outdated networking/networking.go Outdated
@iaguis

This comment has been minimized.

Show comment
Hide comment
@iaguis

iaguis Jan 30, 2016

Member

LGTM after fixing a couple of nits.

Member

iaguis commented Jan 30, 2016

LGTM after fixing a couple of nits.

@steveeJ steveeJ removed the needs/review label Feb 1, 2016

}{
{"POSTROUTING", chainRuleSNAT}, // traffic originating on this host
{"PREROUTING", chainRuleDNAT}, // outside traffic hitting this host
{"OUTPUT", chainRuleDNAT}, // traffic originating on this host

This comment has been minimized.

@alban

alban Feb 1, 2016

Member

s/on this host/from this host? (x2)

@alban

alban Feb 1, 2016

Member

s/on this host/from this host? (x2)

for _, addr := range addrs {
currentAddr := strings.Split(addr.String(), "/")[0]
if searchAddr == currentAddr {
resultInterfaces = append(resultInterfaces, iface)

This comment has been minimized.

@alban

alban Feb 1, 2016

Member

we can add a break here: no need to iterate on other addresses if we found that the interface matches.

@alban

alban Feb 1, 2016

Member

we can add a break here: no need to iterate on other addresses if we found that the interface matches.

This comment has been minimized.

@steveeJ

steveeJ Feb 1, 2016

Contributor

It could be more than one interface for the way ptp is currently implemented.

@steveeJ

steveeJ Feb 1, 2016

Contributor

It could be more than one interface for the way ptp is currently implemented.

This comment has been minimized.

@alban

alban Feb 1, 2016

Member

I meant break in the inner for loop, not break in the outer for loop.

@alban

alban Feb 1, 2016

Member

I meant break in the inner for loop, not break in the outer for loop.

This comment has been minimized.

@steveeJ

steveeJ Feb 1, 2016

Contributor

thanks 👍

@steveeJ

steveeJ Feb 1, 2016

Contributor

thanks 👍

This comment has been minimized.

@alban

alban Feb 1, 2016

Member

done

@alban

alban Feb 1, 2016

Member

done

ga.Wait()
}
f("172.16.28.1", "--net=default", true)

This comment has been minimized.

@alban

alban Feb 1, 2016

Member

Where does this IP come from? 172.16.28.1
I guess it is the first IP available in the default network. What happens if another rkt pod is running with this IP while the tests are being run?

@alban

alban Feb 1, 2016

Member

Where does this IP come from? 172.16.28.1
I guess it is the first IP available in the default network. What happens if another rkt pod is running with this IP while the tests are being run?

This comment has been minimized.

@steveeJ

steveeJ Feb 1, 2016

Contributor

This is the IP of the host in the default network, it's not affected by other rkt instances.

@steveeJ

steveeJ Feb 1, 2016

Contributor

This is the IP of the host in the default network, it's not affected by other rkt instances.

This comment has been minimized.

@alban

alban Feb 1, 2016

Member

Oh, right. Thanks for the explanation. You can use a comment to say that, or a constant if it already exists.

@alban

alban Feb 1, 2016

Member

Oh, right. Thanks for the explanation. You can use a comment to say that, or a constant if it already exists.

iaguis added a commit that referenced this pull request Feb 1, 2016

Merge pull request #2027 from steveeJ/portfwd-loopback
masquerade and route forwarded packets from host's loopback to the pod

@iaguis iaguis merged commit f7303e4 into rkt:master Feb 1, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
semaphoreci The build passed on Semaphore.
Details
@jonboulle

This comment has been minimized.

Show comment
Hide comment
@jonboulle

jonboulle Feb 1, 2016

Contributor

sick

On Mon, Feb 1, 2016 at 6:30 PM, Iago López Galeiras <
notifications@github.com> wrote:

Merged #2027 #2027.


Reply to this email directly or view it on GitHub
#2027 (comment).

Contributor

jonboulle commented Feb 1, 2016

sick

On Mon, Feb 1, 2016 at 6:30 PM, Iago López Galeiras <
notifications@github.com> wrote:

Merged #2027 #2027.


Reply to this email directly or view it on GitHub
#2027 (comment).

@steveeJ steveeJ deleted the steveeJ:portfwd-loopback branch Feb 1, 2016

@steveeJ steveeJ referenced this pull request Feb 1, 2016

Open

masquerading code and tests need improvement #2058

0 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment