Add new ipip backend #842

chenchun · 2017-10-18T08:04:03Z

Signed-off-by: forrestchen forrestchen@tencent.com
Signed-off-by: Chun Chen ramichen@tencent.com

tomdee

Thanks for your contribution! I've taken a quick look and I have a few comments. I'll take a more thorough look next week.

Could you also add IPIP to the end to end tests. It should be as simple as just adding a few lines like this to https://github.com/coreos/flannel/blob/master/dist/functional-test.sh#L88 and to functional-test-k8s.sh

tomdee · 2017-10-20T20:52:57Z

backend/ipip/ipip.go

+}
+
+func New(sm subnet.Manager, extIface *backend.ExternalInterface) (backend.Backend, error) {
+	if !extIface.ExtAddr.Equal(extIface.IfaceAddr) {


Are you sure you want this check?

I don't have an environment to test this and currently removed this check.

tomdee · 2017-10-20T20:57:06Z

backend/ipip/ipip.go

+
+const (
+	backendType  = "ipip"
+	tunnelName   = "tunl0"


Vxlan uses flannel.<VNI> for the tunnel device and UDP uses flannel0 I think it would be good if IPIP also used flannel in the tunnel device name. Maybe just flannel.ipip

Instead of using tunl0 device, created a new flanneld.ipip ipip device. As a effect of this we should set flanneld.ipip‘s local ip to public ip, or kernel will forward packets to tunl0.

I don't understand your comment about setting the IP address for flanneld.ipip to the public. Could you explain some more?

tunl0 is an namespace default device with IPIP attributes local=any and remote=any.
If we create another IPIP device flannel.ipip with local=any and remote=any. It seems when receiving IPIP protocol packets, kernel will forward them to tunl0 as a fallback device instead of flannel.ipip. See https://github.com/torvalds/linux/blob/master/net/ipv4/ip_tunnel.c#L85-L96 .

So we have two options here, either rename tunl0 to flannel.ipip or setting local attribute of flannel.ipip to distinguish these two devices. Considering tunl0 might be used by user, so I choose the later option.

tomdee · 2017-10-20T20:58:07Z

backend/ipip/ipip.go

+const (
+	backendType  = "ipip"
+	tunnelName   = "tunl0"
+	tunnelMaxMTU = 1480


Why do you have a maximum MTU?

Made 1480 as a default MTU and made MTU configurable.

tomdee · 2017-10-28T00:22:54Z

Documentation/backends.md

@@ -115,4 +115,5 @@ IPIP kind of tunnels is the simplest one. It has the lowest overhead, but can in

 Type:
 * `Type` (string): `ipip`
+* `MTU` (number): The mtu of ipip device. Defaults to 1480.


Can you write a little more here to help guide users. Why is this this value? Why and when would a user want to change it? And what value would they change it to?

tomdee · 2017-10-28T00:24:05Z

dist/functional-test.sh

@@ -110,6 +116,12 @@ test_udp_perf() {
    perf
 }

+test_ipip_perf() {
+    write_config_etcd ipip
+	create_ping_dest


tomdee · 2017-10-28T00:24:58Z

I added a few more comments (including this one which might be hidden #842 (comment))

chenchun · 2017-11-09T11:44:16Z

@tomdee PTAL

tomdee · 2017-11-10T02:08:24Z

backend/ipip/ipip.go

@@ -0,0 +1,176 @@
+// Copyright 2015 flannel authors


Can you update the copyright lines to 2017 please.

tomdee · 2017-11-10T02:26:48Z

Hi @chenchun I want to make sure I understand the design for this fully before merging it. If I understand your comment (#842 (comment)) and the code correctly then you're not setting a "remote" but you are setting the "local" address to the public IP. Can you confirm what effects this has? Which address will be used as the source IP address for packets that originate from the host network namespace? And what if the public IP isn't the same as an IP address on the host, then you can't actually use it?

I also have some questions about the MTU settings. On the vxlan backend, the vxlan tunnel device is associated with a physical ethernet device on the host. This is what is used to determine the MTU value to use. Flanneld will always have an interface that it chooses (specified with the --iface option), so maybe you should be getting the MTU value from that device? WDYT?

So maybe the best option would be to "local" address to the address of the interface that flannel selects and also use that as the for the MTU?

tomdee · 2017-11-10T02:32:00Z

When I run your code I'm seeing two IPIP devices, is that expected?

6: tunl0@NONE: <NOARP> mtu 1480 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ipip 0.0.0.0 brd 0.0.0.0
7: flannel.ipip@NONE: <NOARP,UP,LOWER_UP> mtu 1480 qdisc noqueue state UNKNOWN mode DEFAULT group default 
    link/ipip 192.168.5.188 brd 0.0.0.0

tomdee · 2017-11-10T02:34:16Z

Also, one final request: Would you mind adding some comments to your code explain why it's written like it is. For some examples of the style, see the vxlan code - e.g. https://github.com/coreos/flannel/blob/master/backend/vxlan/device.go#L108

chenchun · 2017-11-10T03:36:00Z

If I understand your comment (#842 (comment)) and the code correctly then you're not setting a "remote" but you are setting the "local" address to the public IP. Can you confirm what effects this has? Which address will be used as the source IP address for packets that originate from the host network namespace? And what if the public IP isn't the same as an IP address on the host, then you can't actually use it?

My mistake. I originally had a check if PublicIP is the same as IfaceAddr and I assumed they are equal. Now that I removed this check, I will change the "local" address to the IfaceAddr.

if !extIface.ExtAddr.Equal(extIface.IfaceAddr) {
		return nil, fmt.Errorf("your PublicIP differs from interface IP, meaning that probably you're on a NAT, which is not supported by host-gw backend")
	}

maybe you should be getting the MTU value from that device? WDYT?

I agree.

When I run your code I'm seeing two IPIP devices, is that expected?

Yes, this is expected. tunl0 is created by the ipip kernel module itself and it is not used by this ipip backend.

chenchun · 2017-11-15T09:33:56Z

The last update removed MTU param from backend configuration. We may not need it since we calculate it based on --iface device's MTU.

tomdee · 2017-11-24T04:32:56Z

backend/ipip/ipip.go

+	}
+
+	// Don't set remote attribute making flannel.ipip an one to many tunnel device.
+	if (ipip.Local != nil && !ipip.Local.Equal(be.extIface.IfaceAddr)) || (ipip.Remote != nil && ipip.Remote.String() != "0.0.0.0") {


I don't think that just returning an error here is good enough. If there's a flannel.ipip that isn't an ipip device then it wasn't created by flannel, so in that case it's best to just return an error. But in this case, the local address can change between flannel restarts, so flannel should be able to recreate the device and not just return an error. You should follow similar logic to vxlan (see the ensureLink function), it should delete and recreate the device if the config changes

tomdee · 2017-11-24T04:35:35Z

backend/ipip/ipip.go

+	oldMTU := link.Attrs().MTU
+	if oldMTU > expectMTU || oldMTU == 0 {
+		glog.Infof("current MTU of %s is %d, setting it to %d", tunnelName, oldMTU, expectMTU)
+		err := netlink.LinkSetMTU(link, expectMTU)


Does this need to explicitly set the link to down first?

I couldn't find any reference that saying setting down the link first. I think changing MTU on the fly is ok.

From http://linux-ip.net/html/tools-ip-link.html

B.3.5. Using ip link set to change the MTU

The remaining options to the ip link command cannot be used while the interface is in an UP state.

tomdee · 2017-11-24T04:40:07Z

backend/ipip/ipip.go

+		return nil, fmt.Errorf("failed to set %v UP: %v", tunnelName, err)
+	}
+
+	existingAddrs, err := netlink.AddrList(link, netlink.FAMILY_V4)


I think this next section of code is identical to the vxlan code, could you extract the code into a common package?

tomdee · 2017-11-24T04:54:44Z

backend/ipip/ipip_network.go

@@ -0,0 +1,216 @@
+// Copyright 2017 flannel authors


Much of this file is the same as the host-gw code. It would be great if the common code could be pulled out (and then it could be shared by the vxlan code too but I would do that in a followup PR).

I'll try to do it

Added RouteNetwork in common.go.

tomdee · 2017-11-24T04:58:39Z

Hi @chenchun I've looked at this code some more and realized how similar so much of is it to existing flannel code. It would be great to refactor and pull out the common code.

A few other things that need to happen before this can be released (but not necessarily before this PR is merged)

Documentation update (maybe in the troubleshooting guide too) to descrieb that the tunl0 device will be created but that's not a bug (!) - users will get confused about this.
a ..._windows.go stub needs to be created.
Some manual functional and scale testing.

Signed-off-by: forrestchen <forrestchen@tencent.com> Signed-off-by: Chun Chen <ramichen@tencent.com> IPIP backend update - Rename ipip device to flannel.ipip - Add tests for ipip backend - Add mtu config to ipip backend - Fix invalid use of EEXIST - Remove the equal check of `IfaceAddr` and `ExtAddr` - Set MTU to 20 less than the MTU of the selected iface (specified with the --iface option) Update according to the review - Recreate flannel.ipip device if local address changes - Add `EnsureV4AddressOnLink` - Update ipip document to describe that the tunl0 device will be created but that's not a bug Merge hostgw network with ipip network

tomdee · 2017-12-13T00:38:50Z

@chenchun and @ChenLingPeng I rebased and squashed your commits and added another commit to the PR just to make some small markups. Your original commits are at https://github.com/tomdee/flannel/tree/iptun-orig if you still want them.

chenchun · 2017-12-13T01:32:34Z

Ausome! Thanks for making this happen.

chenchun force-pushed the iptun branch from 66c7176 to 2b8bb12 Compare October 18, 2017 08:05

tomdee suggested changes Oct 20, 2017

View reviewed changes

chenchun force-pushed the iptun branch from dc191e2 to 798ba9b Compare October 24, 2017 08:54

tomdee reviewed Oct 28, 2017

View reviewed changes

chenchun force-pushed the iptun branch 3 times, most recently from fb2550f to 399f1ce Compare November 1, 2017 01:13

tomdee reviewed Nov 10, 2017

View reviewed changes

chenchun force-pushed the iptun branch 2 times, most recently from e039ff7 to eb0f9b4 Compare November 10, 2017 09:21

klausenbusk mentioned this pull request Nov 14, 2017

IPSEC backend, continued #637

Closed

chenchun force-pushed the iptun branch from eb0f9b4 to 73fac54 Compare November 15, 2017 09:16

tomdee reviewed Nov 24, 2017

View reviewed changes

chenchun force-pushed the iptun branch 5 times, most recently from 517827a to 0444147 Compare November 30, 2017 14:07

chenchun force-pushed the iptun branch 3 times, most recently from efadee3 to df5617a Compare December 1, 2017 12:14

ChenLingPeng and others added 2 commits December 10, 2017 14:54

Code markups and minor refactorings

34be77d

tomdee force-pushed the iptun branch from df5617a to 34be77d Compare December 13, 2017 00:35

tomdee approved these changes Dec 13, 2017

View reviewed changes

tomdee merged commit 9ac5988 into flannel-io:master Dec 13, 2017

chenchun deleted the iptun branch January 13, 2018 02:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new ipip backend #842

Add new ipip backend #842

chenchun commented Oct 18, 2017

tomdee left a comment

tomdee Oct 20, 2017

chenchun Oct 24, 2017

tomdee Oct 20, 2017

chenchun Oct 24, 2017 •

edited

Loading

tomdee Oct 28, 2017

chenchun Oct 28, 2017

tomdee Oct 20, 2017

chenchun Oct 24, 2017 •

edited

Loading

tomdee Oct 28, 2017

chenchun Oct 28, 2017

tomdee Oct 28, 2017

chenchun Oct 28, 2017

tomdee commented Oct 28, 2017

chenchun commented Nov 9, 2017

tomdee Nov 10, 2017

tomdee commented Nov 10, 2017

tomdee commented Nov 10, 2017

tomdee commented Nov 10, 2017

chenchun commented Nov 10, 2017

chenchun commented Nov 15, 2017

tomdee Nov 24, 2017

chenchun Nov 29, 2017

tomdee Nov 24, 2017

chenchun Nov 29, 2017 •

edited

Loading

tomdee Nov 24, 2017

chenchun Nov 29, 2017

tomdee Nov 24, 2017

chenchun Nov 29, 2017

chenchun Dec 1, 2017

tomdee commented Nov 24, 2017

tomdee commented Dec 13, 2017

chenchun commented Dec 13, 2017

Add new ipip backend #842

Add new ipip backend #842

Conversation

chenchun commented Oct 18, 2017

tomdee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenchun Oct 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenchun Oct 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomdee commented Oct 28, 2017

chenchun commented Nov 9, 2017

Choose a reason for hiding this comment

tomdee commented Nov 10, 2017

tomdee commented Nov 10, 2017

tomdee commented Nov 10, 2017

chenchun commented Nov 10, 2017

chenchun commented Nov 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenchun Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

B.3.5. Using ip link set to change the MTU

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomdee commented Nov 24, 2017

tomdee commented Dec 13, 2017

chenchun commented Dec 13, 2017

chenchun Oct 24, 2017 •

edited

Loading

chenchun Oct 24, 2017 •

edited

Loading

chenchun Nov 29, 2017 •

edited

Loading