Skip to content
This repository has been archived by the owner on Feb 24, 2020. It is now read-only.

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

Merged
merged 5 commits into from Feb 1, 2016
Merged

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

merged 5 commits into from Feb 1, 2016

Conversation

steveej
Copy link
Contributor

@steveej 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
Copy link
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
Copy link
Contributor Author

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
@steveej steveej changed the title [WIP] masquerade and route forwarded packets from host's loopback to the pod masquerade and route forwarded packets from host's loopback to the pod Jan 27, 2016

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try that, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

A TODO to follow up would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

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

@steveej
Copy link
Contributor Author

steveej commented Jan 28, 2016

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


if len(resultInterfaces) == 0 {
return nil, fmt.Errorf("no interface found with IP %q", ifaceIP)
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, such a bad habit.

@jonboulle
Copy link
Contributor

style nits, but LGTM wenn es funktioniert.

resultInterfaces := make([]net.Interface, 0)

for _, iface := range ifaces {
if iface.Flags&net.FlagLoopback == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

if != 0 continue maybe nicer..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, makes it a little easier to read

@jonboulle
Copy link
Contributor

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

@@ -183,6 +243,43 @@ func (n *Networking) GetDefaultHostIP() (net.IP, error) {
return n.nets[len(n.nets)-1].runtime.HostIP, nil
}

// GetIfacesByIP searches for and returns the interfaces with the given IP
// Disregards the subnet mask since not every net.IP object contains on
// On success it will return the list of found interfaces
Copy link
Member

Choose a reason for hiding this comment

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

double on

@iaguis
Copy link
Member

iaguis commented Jan 30, 2016

LGTM after fixing a couple of nits.

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

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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
masquerade and route forwarded packets from host's loopback to the pod
@iaguis iaguis merged commit f7303e4 into rkt:master Feb 1, 2016
@jonboulle
Copy link
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).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants