From 0c249fa126ec6d6ba4673e69cb8ed0a65d51e3c3 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Thu, 14 Dec 2017 16:56:59 +0100 Subject: [PATCH 1/9] Update netlink dependency To include LinkNotFoundError. --- vendor/github.com/vishvananda/netlink | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vendor/github.com/vishvananda/netlink b/vendor/github.com/vishvananda/netlink index c19091b1..f67b75ed 160000 --- a/vendor/github.com/vishvananda/netlink +++ b/vendor/github.com/vishvananda/netlink @@ -1 +1 @@ -Subproject commit c19091b1c6fb954a80f9f67abed2e2d780bb485b +Subproject commit f67b75edbf5e3bb7dfe70bb788610693a71be3d1 From a71e90ddb8be4a5c990486a1dd6f67de1f618b4e Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Sat, 16 Dec 2017 20:52:24 +0100 Subject: [PATCH 2/9] Do not create bridge upon "weave expose" or "weave hide" Both cmds depend on running weaver which creates the bridge during its initialization. So, calling "create_bridge" during execution of the cmds is redundant. --- weave | 2 -- 1 file changed, 2 deletions(-) diff --git a/weave b/weave index 30f556c9..f3f5a9c7 100755 --- a/weave +++ b/weave @@ -1494,7 +1494,6 @@ case "$COMMAND" in [ $# -eq 2 -a "$1" = "-h" ] || usage FQDN="$2" fi - create_bridge expose_ip show_addrs $ALL_CIDRS ;; @@ -1502,7 +1501,6 @@ case "$COMMAND" in collect_cidr_args "$@" shift $CIDR_ARG_COUNT ipam_cidrs lookup weave:expose $CIDR_ARGS - create_bridge for CIDR in $ALL_CIDRS ; do if ip addr show dev $BRIDGE | grep -qF $CIDR ; then ip addr del dev $BRIDGE $CIDR From 541adf6bb1504d37448a890cde8a3621a9f650ac Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Sat, 16 Dec 2017 20:57:58 +0100 Subject: [PATCH 3/9] Make "weave attach" to depend on running weaver "weave attach $CIDR $CONTAINER_ID" is no longer possible w/o running weaver. This feature was undocumented and complicated the weave script, as "weave attach" required to call "create_bridge". Removing the feature allows us to eliminate the bridge creation from the script. Running "weave attach" w/o weaver results in the following err: bridge "weave" not present; did you launch weave? Also, this commit simplifies the "attach-container" cmd from weaveutil by removing "mtu" as a param. The cmd is used only by "weave attach", and a mtu is value for an attached container interface is exactly the same as one of the weave bridge. --- prog/weaveutil/attach.go | 13 ++++--------- weave | 7 ++----- 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/prog/weaveutil/attach.go b/prog/weaveutil/attach.go index 8ffce5f2..e4cfe0b9 100644 --- a/prog/weaveutil/attach.go +++ b/prog/weaveutil/attach.go @@ -4,7 +4,6 @@ import ( "fmt" "net" "os" - "strconv" "strings" "syscall" @@ -15,8 +14,8 @@ import ( ) func attach(args []string) error { - if len(args) < 4 { - cmdUsage("attach-container", "[--no-multicast-route] [--keep-tx-on] [--hairpin-mode=true|false] ...") + if len(args) < 3 { + cmdUsage("attach-container", "[--no-multicast-route] [--keep-tx-on] [--hairpin-mode=true|false] ...") } keepTXOn := false @@ -52,16 +51,12 @@ func attach(args []string) error { } else if nsHost.Equal(nsContainer) { return fmt.Errorf("Container is running in the host network namespace, and therefore cannot be\nconnected to weave - perhaps the container was started with --net=host") } - mtu, err := strconv.Atoi(args[2]) - if err != nil && args[3] != "" { - return fmt.Errorf("unable to parse mtu %q: %s", args[2], err) - } - cidrs, err := parseCIDRs(args[3:]) + cidrs, err := parseCIDRs(args[2:]) if err != nil { return err } - err = weavenet.AttachContainer(weavenet.NSPathByPid(pid), fmt.Sprint(pid), weavenet.VethName, args[1], mtu, withMulticastRoute, cidrs, keepTXOn, hairpinMode) + err = weavenet.AttachContainer(weavenet.NSPathByPid(pid), fmt.Sprint(pid), weavenet.VethName, args[1], 0, withMulticastRoute, cidrs, keepTXOn, hairpinMode) // If we detected an error but the container has died, tell the user that instead. if err != nil && !processExists(pid) { err = fmt.Errorf("Container %s died", args[0]) diff --git a/weave b/weave index f3f5a9c7..eccad81e 100755 --- a/weave +++ b/weave @@ -918,9 +918,7 @@ check_overlap() { } detect_awsvpc() { - # Ignoring errors here: if we cannot detect AWSVPC we will skip the relevant - # steps, because "attach" should work without the weave router running. - [ "$(call_weave GET /ipinfo/tracker)" != "awsvpc" ] || AWSVPC=1 + [ "$(call_weave GET /ipinfo/tracker 2>/dev/null)" != "awsvpc" ] || AWSVPC=1 } # Call IPAM as necessary to lookup or allocate addresses @@ -1436,7 +1434,6 @@ case "$COMMAND" in done [ $# -eq 1 ] || usage CONTAINER=$(util_op container-id $1) - create_bridge ipam_cidrs allocate $CONTAINER $CIDR_ARGS if [ -n "$REWRITE_HOSTS" ] ; then # rewrite /etc/hosts, unlinking the file (so Docker does @@ -1444,7 +1441,7 @@ case "$COMMAND" in util_op rewrite-etc-hosts "$CONTAINER" "$EXEC_IMAGE" "$ALL_CIDRS" $DNS_EXTRA_HOSTS fi [ -n "$AWSVPC" ] && ATTACH_ARGS="--no-multicast-route --keep-tx-on" - util_op attach-container $ATTACH_ARGS $CONTAINER $BRIDGE $MTU $ALL_CIDRS >/dev/null + util_op attach-container $ATTACH_ARGS $CONTAINER $BRIDGE $ALL_CIDRS >/dev/null [ -n "$WITHOUT_DNS" ] || when_weave_running with_container_fqdn $CONTAINER put_dns_fqdn $ALL_CIDRS show_addrs $ALL_CIDRS ;; From 1860f21efb2b4b33ecefbab7d6589a04c76982f2 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Sat, 16 Dec 2017 21:19:25 +0100 Subject: [PATCH 4/9] Remove "create_bridge" routines No longer needed in the weave script and thus, in weaveutil. --- prog/weaveutil/bridge.go | 35 ----------------------------------- prog/weaveutil/main.go | 1 - weave | 21 --------------------- 3 files changed, 57 deletions(-) diff --git a/prog/weaveutil/bridge.go b/prog/weaveutil/bridge.go index 930d1e9d..ccb3a4dc 100644 --- a/prog/weaveutil/bridge.go +++ b/prog/weaveutil/bridge.go @@ -2,9 +2,7 @@ package main import ( "fmt" - "strconv" - "github.com/weaveworks/weave/common" weavenet "github.com/weaveworks/weave/net" ) @@ -22,36 +20,3 @@ func detectBridgeType(args []string) error { } return nil } - -func createBridge(args []string) error { - if len(args) != 10 { - cmdUsage("create-bridge", " ") - } - - mtu, err := strconv.Atoi(args[3]) - if err != nil && args[3] != "" { - return err - } - port, err := strconv.Atoi(args[4]) - if err != nil { - return err - } - config := weavenet.BridgeConfig{ - DockerBridgeName: args[0], - WeaveBridgeName: args[1], - DatapathName: args[2], - MTU: mtu, - Port: port, - Mac: args[5], - NoFastdp: args[6] != "", - NoBridgedFastdp: args[7] != "", - NPC: args[9] == "--expect-npc", - } - procPath := args[8] - bridgeType, err := weavenet.EnsureBridge(procPath, &config, common.Log) - if err != nil { - return err - } - fmt.Println(bridgeType.String()) - return nil -} diff --git a/prog/weaveutil/main.go b/prog/weaveutil/main.go index deb45d9d..45592e7d 100644 --- a/prog/weaveutil/main.go +++ b/prog/weaveutil/main.go @@ -17,7 +17,6 @@ func init() { "help": help, "netcheck": netcheck, "docker-tls-args": dockerTLSArgs, - "create-bridge": createBridge, "detect-bridge-type": detectBridgeType, "create-datapath": createDatapath, "delete-datapath": deleteDatapath, diff --git a/weave b/weave index eccad81e..98913edf 100755 --- a/weave +++ b/weave @@ -405,27 +405,6 @@ detect_bridge_type() { MTU=${WEAVE_MTU:-$(cat /sys/class/net/$BRIDGE/mtu)} } -try_create_bridge() { - # Pass /dev/null because we do not have access to the weavedb files when this runs; BoltDB - # cannot open the file anyway if weaver is running, which is likely for attach|expose|hide. - MAC=$(util_op unique-id "/dev/null" "$HOST_ROOT") - BRIDGE_TYPE=$(util_op create-bridge "$DOCKER_BRIDGE" "$BRIDGE" "$DATAPATH" "$WEAVE_MTU" "$PORT" "$MAC" "$WEAVE_NO_FASTDP" "$WEAVE_NO_BRIDGED_FASTDP" "$PROC_PATH" "$1") - # Set some variables that are expected by code that comes later - [ "$BRIDGE_TYPE" != "fastdp" ] || DATAPATH="$BRIDGE" - MTU=${WEAVE_MTU:-$(cat /sys/class/net/$BRIDGE/mtu)} -} - -create_bridge() { - validate_bridge_type - if ! try_create_bridge "$@" ; then - echo "Creating bridge '$BRIDGE' failed" >&2 - # reset to original value so we destroy both kinds - DATAPATH=datapath - destroy_bridge - exit 1 - fi -} - validate_bridge_type() { detect_bridge_type || return 0 From 2a120531223361f33b2c4d2387b965cbe87a4527 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Sun, 17 Dec 2017 18:51:05 +0100 Subject: [PATCH 5/9] Do not skip bridge initialization if such exists EnsureBridge used to skip creation and initialization of the weave bridge and friends if such existed. This caused problems when EnsureBridge failed before completing all init steps and so, subsequent weaver could not work without "weave reset". This change makes EnsureBridge idempotent. --- common/odp/odp.go | 12 ++++++++++++ net/bridge.go | 35 +++++++++++++++++++++++++---------- net/netlink.go | 16 ++++++++++++++++ net/veth.go | 11 ++++++++--- plugin/net/driver.go | 2 +- 5 files changed, 62 insertions(+), 14 deletions(-) create mode 100644 net/netlink.go diff --git a/common/odp/odp.go b/common/odp/odp.go index 0ea0b8b1..de9a527d 100644 --- a/common/odp/odp.go +++ b/common/odp/odp.go @@ -75,6 +75,14 @@ func DeleteDatapath(dpname string) error { } func AddDatapathInterface(dpname string, ifname string) error { + return addDatapathInterface(dpname, ifname, false) +} + +func AddDatapathInterfaceIfNotExist(dpname string, ifname string) error { + return addDatapathInterface(dpname, ifname, true) +} + +func addDatapathInterface(dpname, ifname string, ignoreIfExist bool) error { dpif, err := odp.NewDpif() if err != nil { return err @@ -87,5 +95,9 @@ func AddDatapathInterface(dpname string, ifname string) error { } _, err = dp.CreateVport(odp.NewNetdevVportSpec(ifname)) + if ignoreIfExist && err == odp.NetlinkError(syscall.EEXIST) { + return nil + } + return err } diff --git a/net/bridge.go b/net/bridge.go index 9359fa86..0c87bcbe 100644 --- a/net/bridge.go +++ b/net/bridge.go @@ -227,12 +227,19 @@ func (config *BridgeConfig) configuredBridgeType() Bridge { } func EnsureBridge(procPath string, config *BridgeConfig, log *logrus.Logger) (Bridge, error) { - bridgeType, err := ExistingBridgeType(config.WeaveBridgeName, config.DatapathName) - if bridgeType != nil || err != nil { - return bridgeType, err + existingBridgeType, err := ExistingBridgeType(config.WeaveBridgeName, config.DatapathName) + if err != nil { + return nil, err + } + + bridgeType := config.configuredBridgeType() + + if existingBridgeType != nil && bridgeType.String() != existingBridgeType.String() { + return nil, + fmt.Errorf("Existing bridge type %q is different than requested %q. Please do 'weave reset' and try again", + existingBridgeType, bridgeType) } - bridgeType = config.configuredBridgeType() for { if err := bridgeType.init(config); err != nil { if errors.Cause(err) == errBridgeNotSupported { @@ -288,7 +295,7 @@ func (b bridgeImpl) initPrep(config *BridgeConfig) error { config.MTU = 65535 } b.bridge = &netlink.Bridge{LinkAttrs: linkAttrs} - if err = netlink.LinkAdd(b.bridge); err != nil { + if err := LinkAddIfNotExist(b.bridge); err != nil { return errors.Wrapf(err, "creating bridge %q", config.WeaveBridgeName) } if err := netlink.LinkSetHardwareAddr(b.bridge, mac); err != nil { @@ -320,7 +327,7 @@ func (b bridgeImpl) init(config *BridgeConfig) error { if err := b.initPrep(config); err != nil { return err } - if _, err := CreateAndAttachVeth(BridgeIfName, PcapIfName, config.WeaveBridgeName, config.MTU, true, func(veth netlink.Link) error { + if _, err := CreateAndAttachVeth(BridgeIfName, PcapIfName, config.WeaveBridgeName, config.MTU, true, false, func(veth netlink.Link) error { return netlink.LinkSetUp(veth) }); err != nil { return errors.Wrap(err, "creating pcap veth pair") @@ -370,11 +377,11 @@ func (bf bridgedFastdpImpl) init(config *BridgeConfig) error { if err := bf.bridgeImpl.initPrep(config); err != nil { return err } - if _, err := CreateAndAttachVeth(BridgeIfName, DatapathIfName, config.WeaveBridgeName, config.MTU, true, func(veth netlink.Link) error { + if _, err := CreateAndAttachVeth(BridgeIfName, DatapathIfName, config.WeaveBridgeName, config.MTU, true, false, func(veth netlink.Link) error { if err := netlink.LinkSetUp(veth); err != nil { return errors.Wrapf(err, "setting link up on %q", veth.Attrs().Name) } - if err := odp.AddDatapathInterface(bf.datapathName, veth.Attrs().Name); err != nil { + if err := odp.AddDatapathInterfaceIfNotExist(bf.datapathName, veth.Attrs().Name); err != nil { return errors.Wrapf(err, "adding interface %q to datapath %q", veth.Attrs().Name, bf.datapathName) } return nil @@ -394,7 +401,7 @@ func (bf bridgedFastdpImpl) attach(veth *netlink.Veth) error { } func (f fastdpImpl) attach(veth *netlink.Veth) error { - return odp.AddDatapathInterface(f.datapathName, veth.Attrs().Name) + return odp.AddDatapathInterfaceIfNotExist(f.datapathName, veth.Attrs().Name) } func configureIPTables(config *BridgeConfig) error { @@ -404,10 +411,18 @@ func configureIPTables(config *BridgeConfig) error { } if config.DockerBridgeName != "" { if config.WeaveBridgeName != config.DockerBridgeName { - err := ipt.Insert("filter", "FORWARD", 1, "-i", config.DockerBridgeName, "-o", config.WeaveBridgeName, "-j", "DROP") + // This check is not ideal, as the rule should be at the very top + // of the chain. + found, err := ipt.Exists("filter", "FORWARD", "-i", config.DockerBridgeName, "-o", config.WeaveBridgeName, "-j", "DROP") if err != nil { return err } + if !found { + err := ipt.Insert("filter", "FORWARD", 1, "-i", config.DockerBridgeName, "-o", config.WeaveBridgeName, "-j", "DROP") + if err != nil { + return err + } + } } dockerBridgeIP, err := FindBridgeIP(config.DockerBridgeName, nil) diff --git a/net/netlink.go b/net/netlink.go new file mode 100644 index 00000000..a6b45d2f --- /dev/null +++ b/net/netlink.go @@ -0,0 +1,16 @@ +package net + +import ( + "syscall" + + "github.com/pkg/errors" + "github.com/vishvananda/netlink" +) + +func LinkAddIfNotExist(link netlink.Link) error { + err := netlink.LinkAdd(link) + if err != nil && err == syscall.EEXIST { + return nil + } + return errors.Wrapf(err, "creating link %q", link) +} diff --git a/net/veth.go b/net/veth.go index f7f44c2a..6c188fb5 100644 --- a/net/veth.go +++ b/net/veth.go @@ -13,7 +13,7 @@ import ( ) // create and attach a veth to the Weave bridge -func CreateAndAttachVeth(name, peerName, bridgeName string, mtu int, keepTXOn bool, init func(peer netlink.Link) error) (*netlink.Veth, error) { +func CreateAndAttachVeth(name, peerName, bridgeName string, mtu int, keepTXOn bool, errIfLinkExist bool, init func(peer netlink.Link) error) (*netlink.Veth, error) { bridge, err := netlink.LinkByName(bridgeName) if err != nil { return nil, fmt.Errorf(`bridge "%s" not present; did you launch weave?`, bridgeName) @@ -28,7 +28,12 @@ func CreateAndAttachVeth(name, peerName, bridgeName string, mtu int, keepTXOn bo MTU: mtu}, PeerName: peerName, } - if err := netlink.LinkAdd(veth); err != nil { + + linkAdd := LinkAddIfNotExist + if errIfLinkExist { + linkAdd = netlink.LinkAdd + } + if err := linkAdd(veth); err != nil { return nil, fmt.Errorf(`could not create veth pair %s-%s: %s`, name, peerName, err) } @@ -116,7 +121,7 @@ func AttachContainer(netNSPath, id, ifName, bridgeName string, mtu int, withMult id = id[:maxIDLen] // trim passed ID if too long } name, peerName := vethPrefix+"pl"+id, vethPrefix+"pg"+id - veth, err := CreateAndAttachVeth(name, peerName, bridgeName, mtu, keepTXOn, func(veth netlink.Link) error { + veth, err := CreateAndAttachVeth(name, peerName, bridgeName, mtu, keepTXOn, true, func(veth netlink.Link) error { if err := netlink.LinkSetNsFd(veth, int(ns)); err != nil { return fmt.Errorf("failed to move veth to container netns: %s", err) } diff --git a/plugin/net/driver.go b/plugin/net/driver.go index e5f78124..70de3efa 100644 --- a/plugin/net/driver.go +++ b/plugin/net/driver.go @@ -134,7 +134,7 @@ func (driver *driver) CreateEndpoint(create *api.CreateEndpointRequest) (*api.Cr // create veths. note we assume endpoint IDs are unique in the first 9 chars name, peerName := vethPair(create.EndpointID) - if _, err := weavenet.CreateAndAttachVeth(name, peerName, weavenet.WeaveBridgeName, 0, false, nil); err != nil { + if _, err := weavenet.CreateAndAttachVeth(name, peerName, weavenet.WeaveBridgeName, 0, false, true, nil); err != nil { return nil, driver.error("JoinEndpoint", "%s", err) } From a0c8fc2ddb663a9ed65d87f1881f30dcf3b47b5c Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Sun, 17 Dec 2017 19:05:12 +0100 Subject: [PATCH 6/9] Pass WEAVE_NO_BRIDGED_FASTDP to weaver --- prog/weaver/main.go | 1 + weave | 1 + 2 files changed, 2 insertions(+) diff --git a/prog/weaver/main.go b/prog/weaver/main.go index c12b4c03..89074327 100644 --- a/prog/weaver/main.go +++ b/prog/weaver/main.go @@ -197,6 +197,7 @@ func main() { mflag.StringVar(&dnsConfig.ResolvConf, []string{"-resolv-conf"}, "", "path to resolver configuration for fallback DNS lookups") mflag.StringVar(&bridgeConfig.DatapathName, []string{"-datapath"}, "", "ODP datapath name") mflag.BoolVar(&bridgeConfig.NoFastdp, []string{"-no-fastdp"}, false, "Disable Fast Datapath") + mflag.BoolVar(&bridgeConfig.NoBridgedFastdp, []string{"-no-bridged-fastdp"}, false, "Disable Bridged Fast Datapath") mflag.StringVar(&trustedSubnetStr, []string{"-trusted-subnets"}, "", "comma-separated list of trusted subnets in CIDR notation") mflag.StringVar(&dbPrefix, []string{"-db-prefix"}, "/weavedb/weave", "pathname/prefix of filename to store data") mflag.StringVar(&procPath, []string{"-proc-path"}, "/proc", "path to reach host /proc filesystem") diff --git a/weave b/weave index 98913edf..db80efe5 100755 --- a/weave +++ b/weave @@ -536,6 +536,7 @@ router_bridge_opts() { echo --docker-bridge "$DOCKER_BRIDGE" --weave-bridge "$BRIDGE" --datapath "$DATAPATH" [ -z "$WEAVE_MTU" ] || echo --mtu "$WEAVE_MTU" [ -z $WEAVE_NO_FASTDP ] || echo --no-fastdp + [ -z $WEAVE_NO_BRIDGED_FASTDP ] || echo --no-bridged-fastdp } ###################################################################### From 14768d6947fd620d7faf49b9d0d06d4a805c9462 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Mon, 18 Dec 2017 16:38:36 +0100 Subject: [PATCH 7/9] Add smoke test for bridge re-creation after restart --- test/191_create_bridge_2_test.sh | 43 ++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100755 test/191_create_bridge_2_test.sh diff --git a/test/191_create_bridge_2_test.sh b/test/191_create_bridge_2_test.sh new file mode 100755 index 00000000..c8b06cb3 --- /dev/null +++ b/test/191_create_bridge_2_test.sh @@ -0,0 +1,43 @@ +#! /bin/bash + +. "$(dirname "$0")/config.sh" + +C1=10.32.0.1 +C2=10.32.0.2 + +kill_weaver() { + run_on $HOST1 sudo ip link set weave down + WEAVER_PID=$(container_pid $HOST1 weave) + run_on $HOST1 sudo kill -9 $WEAVER_PID +} + +start_suite "Re-create bridge after restart" + +# Should create a bridge of the "bridge" type +WEAVE_NO_FASTDP=1 weave_on $HOST1 launch +WEAVE_NO_FASTDP=1 weave_on $HOST2 launch $HOST1 + +start_container $HOST1 $C1/24 --name=c1 +start_container $HOST2 $C2/24 --name=c2 +assert_raises "exec_on $HOST1 c1 $PING $C2" + +kill_weaver # should re-create the bridge + +sleep 3 + +assert_raises "exec_on $HOST1 c1 $PING $C2" + +# Should create a bridge of the "bridged_fastdp" type +weave_on $HOST1 reset +weave_on $HOST1 launch $HOST2 +weave_on $HOST1 attach $C1/24 c1 + +assert_raises "exec_on $HOST1 c1 $PING $C2" + +kill_weaver # should re-create the bridge + +sleep 3 + +assert_raises "exec_on $HOST1 c1 $PING $C2" + +end_suite From 921ffefd9092dadb84fed993151f04b1bf6e29fa Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Thu, 4 Jan 2018 23:12:55 +0100 Subject: [PATCH 8/9] Ensure the order of filter/FORWARD iptables rules --- net/bridge.go | 83 ++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 59 insertions(+), 24 deletions(-) diff --git a/net/bridge.go b/net/bridge.go index 0c87bcbe..f46c08c9 100644 --- a/net/bridge.go +++ b/net/bridge.go @@ -411,7 +411,7 @@ func configureIPTables(config *BridgeConfig) error { } if config.DockerBridgeName != "" { if config.WeaveBridgeName != config.DockerBridgeName { - // This check is not ideal, as the rule should be at the very top + // This is not ideal, as it does not check whether the rule is at the top // of the chain. found, err := ipt.Exists("filter", "FORWARD", "-i", config.DockerBridgeName, "-o", config.WeaveBridgeName, "-j", "DROP") if err != nil { @@ -450,48 +450,46 @@ func configureIPTables(config *BridgeConfig) error { } } + // The order among weave filter/FORWARD rules is important! + fwdRules := make([][]string, 0) + if config.NPC { // Steer traffic via the NPC _ = ipt.NewChain("filter", "WEAVE-NPC") // ignore error because it might already exist - // If WEAVE-NPC chain doesn't exist then next line will fail - if err = ipt.AppendUnique("filter", "FORWARD", "-o", config.WeaveBridgeName, "-j", "WEAVE-NPC"); err != nil { - return err - } - if err = ipt.AppendUnique("filter", "FORWARD", "-o", config.WeaveBridgeName, "-m", "state", "--state", "NEW", "-j", "NFLOG", "--nflog-group", "86"); err != nil { - return err - } - if err = ipt.AppendUnique("filter", "FORWARD", "-o", config.WeaveBridgeName, "-j", "DROP"); err != nil { - return err - } + // If WEAVE-NPC chain doesn't exist then creating a rule in the chain will fail + fwdRules = append(fwdRules, + [][]string{ + {"-o", config.WeaveBridgeName, "-j", "WEAVE-NPC"}, + {"-o", config.WeaveBridgeName, "-m", "state", "--state", "NEW", "-j", "NFLOG", "--nflog-group", "86"}, + {"-o", config.WeaveBridgeName, "-j", "DROP"}, + }...) } else { // Work around the situation where there are no rules allowing traffic // across our bridge. E.g. ufw - if err = ipt.AppendUnique("filter", "FORWARD", "-i", config.WeaveBridgeName, "-o", config.WeaveBridgeName, "-j", "ACCEPT"); err != nil { - return err - } + fwdRules = append(fwdRules, []string{"-i", config.WeaveBridgeName, "-o", config.WeaveBridgeName, "-j", "ACCEPT"}) } if !config.NPC { // Create a chain for allowing ingress traffic when the bridge is exposed _ = ipt.NewChain("filter", "WEAVE-EXPOSE") - if err = ipt.AppendUnique("filter", "FORWARD", "-o", config.WeaveBridgeName, "-j", "WEAVE-EXPOSE"); err != nil { - return err - } + fwdRules = append(fwdRules, []string{"-o", config.WeaveBridgeName, "-j", "WEAVE-EXPOSE"}) } // Forward from weave to the rest of the world - if err = ipt.AppendUnique("filter", "FORWARD", "-i", config.WeaveBridgeName, "!", "-o", config.WeaveBridgeName, "-j", "ACCEPT"); err != nil { - return err - } + fwdRules = append(fwdRules, []string{"-i", config.WeaveBridgeName, "!", "-o", config.WeaveBridgeName, "-j", "ACCEPT"}) // and allow replies back - if err = ipt.AppendUnique("filter", "FORWARD", "-o", config.WeaveBridgeName, "-m", "conntrack", "--ctstate", "RELATED,ESTABLISHED", "-j", "ACCEPT"); err != nil { + fwdRules = append(fwdRules, []string{"-o", config.WeaveBridgeName, "-m", "conntrack", "--ctstate", "RELATED,ESTABLISHED", "-j", "ACCEPT"}) + + if err := ensureRules("filter", "FORWARD", fwdRules, ipt); err != nil { return err } // create a chain for masquerading - if err = ipt.ClearChain("nat", "WEAVE"); err != nil { - return errors.Wrap(err, "clearing WEAVE chain") - } + // + // NB: we do not clear the chain to preserve existing rules + // inserted by "weave expose". + _ = ipt.NewChain("nat", "WEAVE") + return ipt.AppendUnique("nat", "POSTROUTING", "-j", "WEAVE") } @@ -502,3 +500,40 @@ func linkSetUpByName(linkName string) error { } return netlink.LinkSetUp(link) } + +// ensureRules ensures the presence of given iptables rules. +// +// If any rule from the list is missing, the function deletes all given +// rules and re-appends them to ensure the order of the rules. +func ensureRules(table, chain string, rulespecs [][]string, ipt *iptables.IPTables) error { + allFound := true + + for _, rs := range rulespecs { + found, err := ipt.Exists(table, chain, rs...) + if err != nil { + return errors.Wrapf(err, "ipt.Exists(%s, %s, %s)", table, chain, rs) + } + if !found { + allFound = false + break + } + } + + // All rules exist, do nothing. + if allFound { + return nil + } + + for _, rs := range rulespecs { + // If any is missing, then delete all, as we need to preserve the order of + // given rules. Ignore errors, as rule might not exist. + if !allFound { + ipt.Delete(table, chain, rs...) + } + if err := ipt.Append(table, chain, rs...); err != nil { + return errors.Wrapf(err, "ipt.Append(%s, %s, %s)", table, chain, rs) + } + } + + return nil +} From 1d67e2f95524c3459c6685f2a63e8bb07492e81e Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Thu, 4 Jan 2018 23:13:38 +0100 Subject: [PATCH 9/9] Add test case for restoring iptables rules after weaver restart --- test/191_create_bridge_2_test.sh | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/test/191_create_bridge_2_test.sh b/test/191_create_bridge_2_test.sh index c8b06cb3..fa26c780 100755 --- a/test/191_create_bridge_2_test.sh +++ b/test/191_create_bridge_2_test.sh @@ -2,18 +2,19 @@ . "$(dirname "$0")/config.sh" -C1=10.32.0.1 -C2=10.32.0.2 +C1=10.32.0.2 +C2=10.32.0.3 kill_weaver() { run_on $HOST1 sudo ip link set weave down WEAVER_PID=$(container_pid $HOST1 weave) run_on $HOST1 sudo kill -9 $WEAVER_PID + sleep 3 } start_suite "Re-create bridge after restart" -# Should create a bridge of the "bridge" type +# Should create a bridge of the "bridge" type. WEAVE_NO_FASTDP=1 weave_on $HOST1 launch WEAVE_NO_FASTDP=1 weave_on $HOST2 launch $HOST1 @@ -23,21 +24,36 @@ assert_raises "exec_on $HOST1 c1 $PING $C2" kill_weaver # should re-create the bridge -sleep 3 - assert_raises "exec_on $HOST1 c1 $PING $C2" # Should create a bridge of the "bridged_fastdp" type weave_on $HOST1 reset weave_on $HOST1 launch $HOST2 weave_on $HOST1 attach $C1/24 c1 - assert_raises "exec_on $HOST1 c1 $PING $C2" kill_weaver # should re-create the bridge -sleep 3 - assert_raises "exec_on $HOST1 c1 $PING $C2" +# test restore of iptables + +weave_on $HOST1 reset +# `--expect-npc` to trigger creation of WEAVE-NPC iptables chain. +weave_on $HOST1 launch --expect-npc +# To create POSTROUTING rules. +weave_on $HOST1 expose + +IPT_BEFORE=$(mktemp) +IPT_AFTER=$(mktemp) +run_on $HOST1 "sudo iptables-save | grep -i weave | grep -v '\[.*:.*\]' > $IPT_BEFORE" + +run_on $HOST1 sudo iptables -t filter -D FORWARD -o weave -j WEAVE-NPC +run_on $HOST1 sudo iptables -t nat -D POSTROUTING -j WEAVE +kill_weaver # should re-create the bridge and iptables friends + +# Rudimentary check that weave related iptables rules has been restored +run_on $HOST1 "sudo iptables-save | grep -i weave | grep -v '\[.*:.*\]' > $IPT_AFTER" +assert_raises "run_on $HOST1 diff $IPT_BEFORE $IPT_AFTER" + end_suite