From 9b90e119e5da75a0f27ffae355abd423096b0ba6 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Fri, 24 Oct 2014 11:38:10 +0100 Subject: [PATCH 1/9] Say 'name' instead of 'identity' in log and status --- router/router.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/router/router.go b/router/router.go index 34541035..89ea43ea 100644 --- a/router/router.go +++ b/router/router.go @@ -34,7 +34,7 @@ func NewRouter(iface *net.Interface, name PeerName, password []byte, connLimit i ourself := NewPeer(name, 0, 0, router) router.Ourself = router.Peers.FetchWithDefault(ourself) router.Ourself.StartLocalPeer() - log.Println("Local identity is", router.Ourself.Name) + log.Println("Our name is", router.Ourself.Name) return router } @@ -58,7 +58,7 @@ func (router *Router) Start() { func (router *Router) Status() string { var buf bytes.Buffer - buf.WriteString(fmt.Sprintln("Local identity is", router.Ourself.Name)) + buf.WriteString(fmt.Sprintln("Our name is", router.Ourself.Name)) buf.WriteString(fmt.Sprintln("Sniffing traffic on", router.Iface)) buf.WriteString(fmt.Sprintf("MACs:\n%s", router.Macs)) buf.WriteString(fmt.Sprintf("Peers:\n%s", router.Peers)) From 34e240c4a5e2a8dad869efe03e90929ffa05ad95 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Fri, 24 Oct 2014 12:49:04 +0100 Subject: [PATCH 2/9] remove misleading comment... attach() and detach() are never invoked directly, plus it's really not that hard to figure out what they do. --- weave | 5 ----- 1 file changed, 5 deletions(-) diff --git a/weave b/weave index ea6d9a9f..b91bbc6a 100755 --- a/weave +++ b/weave @@ -158,9 +158,6 @@ with_container_netns () { return $STATUS } -# Attach Container-ID $1 to the weave network at CIDR address $2, -# creating a virtual ethernet device if necessary. -# Also turn off tx offload on device $3 inside the namespace. attach() { if ip link show $LOCAL_IFNAME >/dev/null 2>&1 ; then # veth name already exists, so assume we set it up already; @@ -206,8 +203,6 @@ attach() { ip netns exec $NETNS ip route add 224.0.0.0/4 dev $CONTAINER_IFNAME } -# Detach Container-ID $1 at CIDR address $2 from the weave network -# removing the virtual ethernet device if there are no more addresses detach() { if ! ip netns exec $NETNS ip addr show dev $CONTAINER_IFNAME | grep -F $2 >/dev/null ; then # address is not there, leave the device alone From 6501934a15e9ddf3ec2b2a2971ccdb80c4001769 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Fri, 24 Oct 2014 12:50:29 +0100 Subject: [PATCH 3/9] make with_container_netns take the container as the first arg and invoke the subsequent command. This is closer to typical 'with' semantics. --- weave | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/weave b/weave index b91bbc6a..c5b460fa 100755 --- a/weave +++ b/weave @@ -119,7 +119,7 @@ stop_masquerading() { # the following borrows from https://github.com/jpetazzo/pipework with_container_netns () { - CONTAINER="$2" + CONTAINER="$1" CONTAINER_PID=$(docker inspect --format='{{ .State.Pid }}' $CONTAINER) if [ "$CONTAINER_PID" = 0 ] ; then @@ -144,6 +144,7 @@ with_container_netns () { # Run the wrapped command STATUS=0 + shift 1 if ! "$@" >$IP_TMPFILE 2>&1 ; then STATUS=1 if [ "$(docker inspect --format='{{ .State.Pid }}' $CONTAINER)" != "$CONTAINER_PID" ] ; then @@ -162,12 +163,12 @@ attach() { if ip link show $LOCAL_IFNAME >/dev/null 2>&1 ; then # veth name already exists, so assume we set it up already; # just add the IP address. - if ip netns exec $NETNS ip addr show dev $CONTAINER_IFNAME | grep -F $2 >/dev/null ; then + if ip netns exec $NETNS ip addr show dev $CONTAINER_IFNAME | grep -F $1 >/dev/null ; then # address was there already return 0 fi - if ! ip netns exec $NETNS ip addr add $2 dev $CONTAINER_IFNAME ; then + if ! ip netns exec $NETNS ip addr add $1 dev $CONTAINER_IFNAME ; then return 1 fi @@ -188,9 +189,9 @@ attach() { fi if ! ip netns exec $NETNS ip link set $GUEST_IFNAME name $CONTAINER_IFNAME || - ! ip netns exec $NETNS ip addr add $2 dev $CONTAINER_IFNAME || + ! ip netns exec $NETNS ip addr add $1 dev $CONTAINER_IFNAME || ! ip netns exec $NETNS ip link set $CONTAINER_IFNAME up || - ! check_command_and_run ethtool ip netns exec $NETNS ethtool -K $3 tx off >/dev/null ; then + ! check_command_and_run ethtool ip netns exec $NETNS ethtool -K $2 tx off >/dev/null ; then return 1 fi @@ -204,12 +205,12 @@ attach() { } detach() { - if ! ip netns exec $NETNS ip addr show dev $CONTAINER_IFNAME | grep -F $2 >/dev/null ; then + if ! ip netns exec $NETNS ip addr show dev $CONTAINER_IFNAME | grep -F $1 >/dev/null ; then # address is not there, leave the device alone return 0 fi - if ! ip netns exec $NETNS ip addr del $2 dev $CONTAINER_IFNAME ; then + if ! ip netns exec $NETNS ip addr del $1 dev $CONTAINER_IFNAME ; then return 1 fi @@ -258,7 +259,7 @@ case "$COMMAND" in # additional parameters, such as resource limits, to docker # when launching the weave container. CONTAINER=$(docker run --privileged -d --name=weave -p 6783:6783/tcp -p 6783:6783/udp $WEAVE_DOCKER_ARGS $IMAGE -name $MACADDR "$@" | tail -n 1) - with_container_netns attach $CONTAINER $IPADDR eth0 + with_container_netns $CONTAINER attach $IPADDR eth0 echo $CONTAINER ;; status) @@ -295,7 +296,7 @@ case "$COMMAND" in shift 1 create_bridge CONTAINER=$(docker run -d "$@" | tail -n 1) - with_container_netns attach $CONTAINER $IPADDR $CONTAINER_IFNAME + with_container_netns $CONTAINER attach $IPADDR $CONTAINER_IFNAME echo $CONTAINER ;; start) @@ -303,17 +304,17 @@ case "$COMMAND" in IPADDR=$1 create_bridge CONTAINER=$(docker start $2) - with_container_netns attach $CONTAINER $IPADDR $CONTAINER_IFNAME + with_container_netns $CONTAINER attach $IPADDR $CONTAINER_IFNAME echo $CONTAINER ;; attach) [ $# -gt 1 ] || usage create_bridge - with_container_netns attach $2 $1 $CONTAINER_IFNAME + with_container_netns $2 attach $1 $CONTAINER_IFNAME ;; detach) [ $# -gt 1 ] || usage - with_container_netns detach $2 $1 + with_container_netns $2 detach $1 ;; expose) [ $# -gt 0 ] || usage From 5b6a892c38ec926d0c4c1b28debfc9a242f03989 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Fri, 24 Oct 2014 12:57:35 +0100 Subject: [PATCH 4/9] refactor: don't extract param to var and shift unless we have to --- weave | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/weave b/weave index c5b460fa..5f987f0d 100755 --- a/weave +++ b/weave @@ -301,10 +301,9 @@ case "$COMMAND" in ;; start) [ $# -gt 1 ] || usage - IPADDR=$1 create_bridge CONTAINER=$(docker start $2) - with_container_netns $CONTAINER attach $IPADDR $CONTAINER_IFNAME + with_container_netns $CONTAINER attach $1 $CONTAINER_IFNAME echo $CONTAINER ;; attach) @@ -318,22 +317,18 @@ case "$COMMAND" in ;; expose) [ $# -gt 0 ] || usage - IPADDR=$1 - shift 1 create_bridge - if ! ip addr show dev $BRIDGE | grep -qF $IPADDR + if ! ip addr show dev $BRIDGE | grep -qF $1 then - ip addr add dev $BRIDGE $IPADDR + ip addr add dev $BRIDGE $1 fi ;; hide) [ $# -gt 0 ] || usage - IPADDR=$1 - shift 1 create_bridge - if ip addr show dev $BRIDGE | grep -qF $IPADDR + if ip addr show dev $BRIDGE | grep -qF $1 then - ip addr del dev $BRIDGE $IPADDR + ip addr del dev $BRIDGE $1 fi ;; stop) From 093b23fa83b2d591c883c7d48a36a4a70220c4b8 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Fri, 24 Oct 2014 12:59:47 +0100 Subject: [PATCH 5/9] refactor: it's not an IPADDR but a CIDR --- weave | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/weave b/weave index 5f987f0d..f99c47bf 100755 --- a/weave +++ b/weave @@ -226,7 +226,7 @@ detach() { case "$COMMAND" in launch) [ $# -gt 0 ] || usage - IPADDR=$1 + CIDR=$1 shift 1 case $(docker inspect --format '{{ .State.Running }} {{ .Config.Image }}' weave 2>/dev/null) in "true $IMAGE") @@ -247,7 +247,7 @@ case "$COMMAND" in esac create_bridge stop_masquerading - setup_masquerading $IPADDR + setup_masquerading $CIDR # We set the router name to the bridge mac since that is # stable across re-creations of the containers. # @@ -259,7 +259,7 @@ case "$COMMAND" in # additional parameters, such as resource limits, to docker # when launching the weave container. CONTAINER=$(docker run --privileged -d --name=weave -p 6783:6783/tcp -p 6783:6783/udp $WEAVE_DOCKER_ARGS $IMAGE -name $MACADDR "$@" | tail -n 1) - with_container_netns $CONTAINER attach $IPADDR eth0 + with_container_netns $CONTAINER attach $CIDR eth0 echo $CONTAINER ;; status) @@ -292,11 +292,11 @@ case "$COMMAND" in ;; run) [ $# -gt 0 ] || usage - IPADDR=$1 + CIDR=$1 shift 1 create_bridge CONTAINER=$(docker run -d "$@" | tail -n 1) - with_container_netns $CONTAINER attach $IPADDR $CONTAINER_IFNAME + with_container_netns $CONTAINER attach $CIDR $CONTAINER_IFNAME echo $CONTAINER ;; start) From de96742935c306da6d27f8ff562f6040e7713dfc Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Fri, 24 Oct 2014 13:03:28 +0100 Subject: [PATCH 6/9] refactor: remove add_nat_rule it did too little to be worthwhile --- weave | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/weave b/weave index f99c47bf..ecb8d416 100755 --- a/weave +++ b/weave @@ -73,10 +73,6 @@ add_iptables_rule() { fi } -add_nat_rule() { - add_iptables_rule nat "$@" -} - create_bridge() { [ ! -d /sys/class/net/$BRIDGE ] && { ip link add name $BRIDGE type bridge @@ -106,9 +102,9 @@ destroy_bridge() { setup_masquerading() { run_iptables -t nat -N WEAVE >/dev/null 2>&1 || true - add_nat_rule POSTROUTING -j WEAVE - add_nat_rule WEAVE -o $BRIDGE ! -s $1 -j MASQUERADE - add_nat_rule WEAVE -s $1 ! -o $BRIDGE -j MASQUERADE + add_iptables_rule nat POSTROUTING -j WEAVE + add_iptables_rule nat WEAVE -o $BRIDGE ! -s $1 -j MASQUERADE + add_iptables_rule nat WEAVE -s $1 ! -o $BRIDGE -j MASQUERADE } stop_masquerading() { From 560cbabb54dfd95008242f3972c304df555f7b43 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Fri, 24 Oct 2014 13:13:34 +0100 Subject: [PATCH 7/9] refactor: simplify 'attach' Move the interface into the container network NS asap, which simplifies error handling. --- weave | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/weave b/weave index ecb8d416..8afcf009 100755 --- a/weave +++ b/weave @@ -175,16 +175,16 @@ attach() { return 1 fi - if ! ip link set $LOCAL_IFNAME master $BRIDGE || - ! ip link set $LOCAL_IFNAME up || - ! ip link set $GUEST_IFNAME netns $NETNS ; then + if ! ip link set $GUEST_IFNAME netns $NETNS ; then # failed before we assigned the veth to the container's # namespace - delete it ip link del $LOCAL_IFNAME type veth || true return 1 fi - if ! ip netns exec $NETNS ip link set $GUEST_IFNAME name $CONTAINER_IFNAME || + if ! ip link set $LOCAL_IFNAME master $BRIDGE || + ! ip link set $LOCAL_IFNAME up || + ! ip netns exec $NETNS ip link set $GUEST_IFNAME name $CONTAINER_IFNAME || ! ip netns exec $NETNS ip addr add $1 dev $CONTAINER_IFNAME || ! ip netns exec $NETNS ip link set $CONTAINER_IFNAME up || ! check_command_and_run ethtool ip netns exec $NETNS ethtool -K $2 tx off >/dev/null ; then From 00a4a5e056a565e81822af201987ff65bae00450 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Fri, 24 Oct 2014 13:15:15 +0100 Subject: [PATCH 8/9] refactor: simplify multicast route addition in 'attach' --- weave | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/weave b/weave index 8afcf009..176461c5 100755 --- a/weave +++ b/weave @@ -191,13 +191,10 @@ attach() { return 1 fi - if ip netns exec $NETNS ip route show | grep '^224\.0\.0\.0/4' >/dev/null ; then - # There's a multicast route already present, leave it alone. - return 0 - fi - # Route multicast packets across the weave network. - ip netns exec $NETNS ip route add 224.0.0.0/4 dev $CONTAINER_IFNAME + if ! ip netns exec $NETNS ip route show | grep '^224\.0\.0\.0/4' >/dev/null ; then + ip netns exec $NETNS ip route add 224.0.0.0/4 dev $CONTAINER_IFNAME + fi } detach() { From 83464a38eb5ed6aef0a1cd40342782fcb9093bce Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Fri, 24 Oct 2014 14:30:58 +0100 Subject: [PATCH 9/9] remove explicit multicast detection We don't need. Multicast addresses never appear as sources and hence never end up in the peer_cache. And the router always broadcasts frames with destination addresses that aren't in the peer cache. --- router/ethernet_decoder.go | 14 ++------------ router/router.go | 5 +++-- 2 files changed, 5 insertions(+), 14 deletions(-) diff --git a/router/ethernet_decoder.go b/router/ethernet_decoder.go index 62177383..65051909 100644 --- a/router/ethernet_decoder.go +++ b/router/ethernet_decoder.go @@ -67,24 +67,14 @@ func (dec *EthernetDecoder) formICMPMTUPacket(mtu int) ([]byte, error) { var ( // see http://en.wikipedia.org/wiki/Multicast_address#Ethernet - broadcastMAC, _ = net.ParseMAC("ff:ff:ff:ff:ff:ff") - zeroMAC, _ = net.ParseMAC("00:00:00:00:00:00") - stpMACPrefix = []byte{0x01, 0x80, 0xC2, 0x00, 0x00} - ipv4MulticastMACPrefix = []byte{0x01, 0x00, 0x5E} - ipv6MulticastMACPrefix = []byte{0x33, 0x33} + stpMACPrefix = []byte{0x01, 0x80, 0xC2, 0x00, 0x00} + zeroMAC, _ = net.ParseMAC("00:00:00:00:00:00") ) func (dec *EthernetDecoder) DropFrame() bool { return bytes.Equal(stpMACPrefix, dec.eth.DstMAC[:len(stpMACPrefix)]) } -func (dec *EthernetDecoder) BroadcastFrame() bool { - return bytes.Equal(broadcastMAC, dec.eth.DstMAC) || - // treat multicast frames as broadcast - bytes.Equal(ipv4MulticastMACPrefix, dec.eth.DstMAC[:len(ipv4MulticastMACPrefix)]) || - bytes.Equal(ipv6MulticastMACPrefix, dec.eth.DstMAC[:len(ipv6MulticastMACPrefix)]) -} - func (dec *EthernetDecoder) IsPMTUVerify() bool { return bytes.Equal(zeroMAC, dec.eth.SrcMAC) && bytes.Equal(zeroMAC, dec.eth.DstMAC) diff --git a/router/router.go b/router/router.go index 89ea43ea..5d29ff70 100644 --- a/router/router.go +++ b/router/router.go @@ -124,7 +124,8 @@ func (router *Router) handleCapturedPacket(frameData []byte, dec *EthernetDecode frameLen := len(frameData) frameCopy := make([]byte, frameLen, frameLen) copy(frameCopy, frameData) - if !found || dec.BroadcastFrame() { + + if !found { return checkFrameTooBig(router.Ourself.Broadcast(df, frameCopy, dec)) } else { return checkFrameTooBig(router.Ourself.Forward(dstPeer, df, frameCopy, dec)) @@ -287,7 +288,7 @@ func (router *Router) handleUDPPacketFunc(dec *EthernetDecoder, po PacketSink) F checkWarn(po.WritePacket(frame)) dstPeer, found = router.Macs.Lookup(dec.eth.DstMAC) - if !found || dec.BroadcastFrame() || dstPeer != router.Ourself { + if !found || dstPeer != router.Ourself { return checkFrameTooBig(router.Ourself.RelayBroadcast(srcPeer, df, frame, dec), srcPeer) }