From 94e75e45af8264e9478e719e7e19c03f73d37485 Mon Sep 17 00:00:00 2001
From: Simon Howe <footless@gmail.com>
Date: Tue, 12 May 2020 17:43:05 +0200
Subject: [PATCH 1/8] Make ConfigureIPTables idempotent and poll it

- Instead of EnsureBridge
- Adds ipset.ExistEntry method
- Refactor ipset method names Exist <-> EntryExists
- Turn on iptable-refresh by default (10s)
- Adds a test that checks iptables are refreshed, check EXPOSE recovers
---
 net/bridge.go                       | 64 +++++++++++++++++------
 net/ipset/ipset.go                  | 19 ++++++-
 npc/controller_test.go              | 81 +++++++++++++++--------------
 npc/namespace.go                    |  2 +-
 prog/weave-npc/main.go              | 18 +------
 prog/weaver/main.go                 | 28 ++++++++++
 test/130_expose_2_test.sh           | 21 +++++++-
 test/135_expose_iptables_refresh.sh | 69 ++++++++++++++++++++++++
 8 files changed, 228 insertions(+), 74 deletions(-)
 create mode 100755 test/135_expose_iptables_refresh.sh

diff --git a/net/bridge.go b/net/bridge.go
index f882ebb1..3397f883 100644
--- a/net/bridge.go
+++ b/net/bridge.go
@@ -262,7 +262,7 @@ func EnsureBridge(procPath string, config *BridgeConfig, log *logrus.Logger, ips
 		break
 	}
 
-	if err := configureIPTables(config, ips); err != nil {
+	if err := ConfigureIPTables(config, ips); err != nil {
 		return bridgeType, errors.Wrap(err, "configuring iptables")
 	}
 
@@ -297,9 +297,7 @@ func EnsureBridge(procPath string, config *BridgeConfig, log *logrus.Logger, ips
 		return bridgeType, errors.Wrapf(err, "configuring ARP cache on bridge %q", config.WeaveBridgeName)
 	}
 
-	// NB: No concurrent call to Expose is possible, as EnsureBridge is called
-	// before any service has been started.
-	if err := reexpose(config, log); err != nil {
+	if err := Reexpose(config, log); err != nil {
 		return bridgeType, err
 	}
 
@@ -431,10 +429,40 @@ func (f fastdpImpl) attach(veth *netlink.Veth) error {
 	return odp.AddDatapathInterfaceIfNotExist(f.datapathName, veth.Attrs().Name)
 }
 
-func configureIPTables(config *BridgeConfig, ips ipset.Interface) error {
+// ResetIPTables resets IPTables if they're in a strange state from a previous run.
+func ResetIPTables(config *BridgeConfig, ips ipset.Interface) error {
 	ipt, err := iptables.New()
 	if err != nil {
-		return errors.Wrap(err, "creating iptables object")
+		return errors.Wrap(err, "creating iptables object while resetting")
+	}
+
+	if !config.NPC {
+		// Create/Flush a chain for allowing ingress traffic when the bridge is exposed
+		if err := ipt.ClearChain("filter", "WEAVE-EXPOSE"); err != nil {
+			return errors.Wrap(err, "failed to clear/create filter/WEAVE-EXPOSE chain")
+		}
+	}
+
+	if err := ipt.ClearChain("nat", "WEAVE"); err != nil {
+		return errors.Wrap(err, "failed to clear/create nat/WEAVE chain")
+	}
+
+	if config.NoMasqLocal {
+		ips := ipset.New(common.LogLogger(), 0)
+		_ = ips.Destroy(NoMasqLocalIpset)
+		if err := ips.Create(NoMasqLocalIpset, ipset.HashNet); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+// ConfigureIPTables idempotently configures all the iptables!
+func ConfigureIPTables(config *BridgeConfig, ips ipset.Interface) error {
+	ipt, err := iptables.New()
+	if err != nil {
+		return errors.Wrap(err, "creating iptables object while configuring")
 	}
 
 	// The order among weave filter/FORWARD rules is important!
@@ -515,8 +543,8 @@ func configureIPTables(config *BridgeConfig, ips ipset.Interface) error {
 
 	if !config.NPC {
 		// Create/Flush a chain for allowing ingress traffic when the bridge is exposed
-		if err := ipt.ClearChain("filter", "WEAVE-EXPOSE"); err != nil {
-			return errors.Wrap(err, "failed to clear/create filter/WEAVE-EXPOSE chain")
+		if err := ensureChains(ipt, "filter", "WEAVE-EXPOSE"); err != nil {
+			return errors.Wrap(err, "failed to ensure existence of filter/WEAVE-EXPOSE chain")
 		}
 
 		fwdRules = append(fwdRules, []string{"-o", config.WeaveBridgeName, "-j", "WEAVE-EXPOSE"})
@@ -532,8 +560,8 @@ func configureIPTables(config *BridgeConfig, ips ipset.Interface) error {
 	}
 
 	// Create a chain for masquerading
-	if err := ipt.ClearChain("nat", "WEAVE"); err != nil {
-		return errors.Wrap(err, "failed to clear/create nat/WEAVE chain")
+	if err := ensureChains(ipt, "nat", "WEAVE"); err != nil {
+		return errors.Wrap(err, "failed to ensure existence of nat/WEAVE chain")
 	}
 	if err := ipt.AppendUnique("nat", "POSTROUTING", "-j", "WEAVE"); err != nil {
 		return err
@@ -547,11 +575,17 @@ func configureIPTables(config *BridgeConfig, ips ipset.Interface) error {
 	// src IP addr.
 	if config.NoMasqLocal {
 		ips := ipset.New(common.LogLogger(), 0)
-		_ = ips.Destroy(NoMasqLocalIpset)
-		if err := ips.Create(NoMasqLocalIpset, ipset.HashNet); err != nil {
-			return err
+
+		noMasqLocalExists, err := ips.Exists(NoMasqLocalIpset)
+		if err != nil {
+			common.Log.Errorf("Failed to look if ipset '%s' exists: %s", NoMasqLocalIpset, err)
+		} else if !noMasqLocalExists {
+			if err := ips.Create(NoMasqLocalIpset, ipset.HashNet); err != nil {
+				return err
+			}
 		}
-		if err := ipt.Insert("nat", "WEAVE", 1,
+
+		if err := ipt.AppendUnique("nat", "WEAVE",
 			"-m", "set", "--match-set", string(NoMasqLocalIpset), "dst",
 			"-m", "comment", "--comment", "Prevent SNAT to locally running containers",
 			"-j", "RETURN"); err != nil {
@@ -609,7 +643,7 @@ func linkSetUpByName(linkName string) error {
 	return netlink.LinkSetUp(link)
 }
 
-func reexpose(config *BridgeConfig, log *logrus.Logger) error {
+func Reexpose(config *BridgeConfig, log *logrus.Logger) error {
 	// Get existing IP addrs of the weave bridge.
 	// If the bridge hasn't been exposed, then this functions does nothing.
 	//
diff --git a/net/ipset/ipset.go b/net/ipset/ipset.go
index b538a6c9..4c0b7481 100644
--- a/net/ipset/ipset.go
+++ b/net/ipset/ipset.go
@@ -26,7 +26,8 @@ type Interface interface {
 	Create(ipsetName Name, ipsetType Type) error
 	AddEntry(user types.UID, ipsetName Name, entry string, comment string) error
 	DelEntry(user types.UID, ipsetName Name, entry string) error
-	Exist(user types.UID, ipsetName Name, entry string) bool
+	EntryExists(user types.UID, ipsetName Name, entry string) bool
+	Exists(ipsetName Name) (bool, error)
 	Flush(ipsetName Name) error
 	Destroy(ipsetName Name) error
 
@@ -121,10 +122,24 @@ func (i *ipset) DelEntry(user types.UID, ipsetName Name, entry string) error {
 	return doExec("del", string(ipsetName), entry)
 }
 
-func (i *ipset) Exist(user types.UID, ipsetName Name, entry string) bool {
+func (i *ipset) EntryExists(user types.UID, ipsetName Name, entry string) bool {
 	return i.existUser(user, ipsetName, entry)
 }
 
+// Dummy way to check whether a given ipset exists.
+func (i *ipset) Exists(name Name) (bool, error) {
+	sets, err := i.List(string(name))
+	if err != nil {
+		return false, err
+	}
+	for _, s := range sets {
+		if s == name {
+			return true, nil
+		}
+	}
+	return false, nil
+}
+
 func (i *ipset) Flush(ipsetName Name) error {
 	i.removeSetFromUsers(ipsetName)
 	return doExec("flush", string(ipsetName))
diff --git a/npc/controller_test.go b/npc/controller_test.go
index 860b5e5c..618e453c 100644
--- a/npc/controller_test.go
+++ b/npc/controller_test.go
@@ -72,12 +72,17 @@ func (i *mockIPSet) DelEntry(user types.UID, ipsetName ipset.Name, entry string)
 	return nil
 }
 
-func (i *mockIPSet) Exist(user types.UID, ipsetName ipset.Name, entry string) bool {
+func (i *mockIPSet) EntryExists(user types.UID, ipsetName ipset.Name, entry string) bool {
 	_, ok := i.sets[string(ipsetName)].subSets[entry][user]
 	return ok
 }
 
-func (i *mockIPSet) entryExists(ipsetName ipset.Name, entry string) bool {
+func (i *mockIPSet) Exists(ipsetName ipset.Name) (bool, error) {
+	_, ok := i.sets[string(ipsetName)]
+	return ok, nil
+}
+
+func (i *mockIPSet) entriesExist(ipsetName ipset.Name, entry string) bool {
 	return len(i.sets[string(ipsetName)].subSets[entry]) > 0
 }
 
@@ -262,8 +267,8 @@ func TestDefaultAllow(t *testing.T) {
 	controller.AddPod(podFoo)
 
 	// Should add the foo pod to default-allow
-	require.True(t, m.entryExists(ingressDefaultAllowIPSetName, fooPodIP))
-	require.True(t, m.entryExists(egressDefaultAllowIPSetName, fooPodIP))
+	require.True(t, m.entriesExist(ingressDefaultAllowIPSetName, fooPodIP))
+	require.True(t, m.entriesExist(egressDefaultAllowIPSetName, fooPodIP))
 
 	podBar := &coreapi.Pod{
 		ObjectMeta: metav1.ObjectMeta{
@@ -278,8 +283,8 @@ func TestDefaultAllow(t *testing.T) {
 	controller.UpdatePod(podBarNoIP, podBar)
 
 	// Should add the bar pod to default-allow
-	require.True(t, m.entryExists(ingressDefaultAllowIPSetName, barPodIP))
-	require.True(t, m.entryExists(egressDefaultAllowIPSetName, barPodIP))
+	require.True(t, m.entriesExist(ingressDefaultAllowIPSetName, barPodIP))
+	require.True(t, m.entriesExist(egressDefaultAllowIPSetName, barPodIP))
 
 	// Allow access from the bar pod to the foo pod
 	netpol := &networkingv1.NetworkPolicy{
@@ -310,48 +315,48 @@ func TestDefaultAllow(t *testing.T) {
 	controller.AddNetworkPolicy(netpol)
 
 	// Should remove the foo pod from default-allow as the netpol selects it
-	require.False(t, m.entryExists(ingressDefaultAllowIPSetName, fooPodIP))
-	require.False(t, m.entryExists(egressDefaultAllowIPSetName, fooPodIP))
-	require.True(t, m.entryExists(ingressDefaultAllowIPSetName, barPodIP))
-	require.True(t, m.entryExists(egressDefaultAllowIPSetName, barPodIP))
+	require.False(t, m.entriesExist(ingressDefaultAllowIPSetName, fooPodIP))
+	require.False(t, m.entriesExist(egressDefaultAllowIPSetName, fooPodIP))
+	require.True(t, m.entriesExist(ingressDefaultAllowIPSetName, barPodIP))
+	require.True(t, m.entriesExist(egressDefaultAllowIPSetName, barPodIP))
 
 	podBarWithNewIP := *podBar
 	podBarWithNewIP.Status.PodIP = barPodNewIP
 	controller.UpdatePod(podBar, &podBarWithNewIP)
 
 	// Should update IP addr of the bar pod in default-allow
-	require.False(t, m.entryExists(ingressDefaultAllowIPSetName, barPodIP))
-	require.False(t, m.entryExists(egressDefaultAllowIPSetName, barPodIP))
-	require.True(t, m.entryExists(ingressDefaultAllowIPSetName, barPodNewIP))
-	require.True(t, m.entryExists(egressDefaultAllowIPSetName, barPodNewIP))
+	require.False(t, m.entriesExist(ingressDefaultAllowIPSetName, barPodIP))
+	require.False(t, m.entriesExist(egressDefaultAllowIPSetName, barPodIP))
+	require.True(t, m.entriesExist(ingressDefaultAllowIPSetName, barPodNewIP))
+	require.True(t, m.entriesExist(egressDefaultAllowIPSetName, barPodNewIP))
 
 	controller.UpdatePod(&podBarWithNewIP, podBarNoIP)
 	// Should remove the bar pod from default-allow as it does not have any IP addr
-	require.False(t, m.entryExists(ingressDefaultAllowIPSetName, barPodNewIP))
-	require.False(t, m.entryExists(egressDefaultAllowIPSetName, barPodNewIP))
+	require.False(t, m.entriesExist(ingressDefaultAllowIPSetName, barPodNewIP))
+	require.False(t, m.entriesExist(egressDefaultAllowIPSetName, barPodNewIP))
 
 	podFooWithNewLabel := *podFoo
 	podFooWithNewLabel.ObjectMeta.Labels = map[string]string{"run": "new-foo"}
 	controller.UpdatePod(podFoo, &podFooWithNewLabel)
 
 	// Should bring back the foo pod to default-allow as it does not match dst of any netpol
-	require.True(t, m.entryExists(ingressDefaultAllowIPSetName, fooPodIP))
-	require.True(t, m.entryExists(egressDefaultAllowIPSetName, fooPodIP))
+	require.True(t, m.entriesExist(ingressDefaultAllowIPSetName, fooPodIP))
+	require.True(t, m.entriesExist(egressDefaultAllowIPSetName, fooPodIP))
 
 	controller.UpdatePod(&podFooWithNewLabel, podFoo)
 	// Should remove from default-allow as it matches the netpol after the update
-	require.False(t, m.entryExists(ingressDefaultAllowIPSetName, fooPodIP))
-	require.False(t, m.entryExists(egressDefaultAllowIPSetName, fooPodIP))
+	require.False(t, m.entriesExist(ingressDefaultAllowIPSetName, fooPodIP))
+	require.False(t, m.entriesExist(egressDefaultAllowIPSetName, fooPodIP))
 
 	controller.DeleteNetworkPolicy(netpol)
 	// Should bring back the foo pod to default-allow as no netpol selects it
-	require.True(t, m.entryExists(ingressDefaultAllowIPSetName, fooPodIP))
-	require.True(t, m.entryExists(egressDefaultAllowIPSetName, fooPodIP))
+	require.True(t, m.entriesExist(ingressDefaultAllowIPSetName, fooPodIP))
+	require.True(t, m.entriesExist(egressDefaultAllowIPSetName, fooPodIP))
 
 	controller.DeletePod(podFoo)
 	// Should remove foo pod from default-allow
-	require.False(t, m.entryExists(ingressDefaultAllowIPSetName, fooPodIP))
-	require.False(t, m.entryExists(egressDefaultAllowIPSetName, fooPodIP))
+	require.False(t, m.entriesExist(ingressDefaultAllowIPSetName, fooPodIP))
+	require.False(t, m.entriesExist(egressDefaultAllowIPSetName, fooPodIP))
 
 	controller.DeleteNamespace(defaultNamespace)
 	// Should remove default ipset
@@ -388,7 +393,7 @@ func TestOutOfOrderPodEvents(t *testing.T) {
 	controller.AddPod(podFoo)
 
 	// Should be in default-allow as no netpol selects podFoo
-	require.True(t, m.entryExists(ingressDefaultAllowIPSetName, podIP))
+	require.True(t, m.entriesExist(ingressDefaultAllowIPSetName, podIP))
 
 	netpol := &networkingv1.NetworkPolicy{
 		ObjectMeta: metav1.ObjectMeta{
@@ -410,7 +415,7 @@ func TestOutOfOrderPodEvents(t *testing.T) {
 	controller.AddNetworkPolicy(netpol)
 
 	// Shouldn't be in default-allow as netpol above selects podFoo
-	require.False(t, m.entryExists(ingressDefaultAllowIPSetName, podIP))
+	require.False(t, m.entriesExist(ingressDefaultAllowIPSetName, podIP))
 
 	podBar := &coreapi.Pod{
 		ObjectMeta: metav1.ObjectMeta{
@@ -422,10 +427,10 @@ func TestOutOfOrderPodEvents(t *testing.T) {
 	controller.AddPod(podBar)
 
 	// Should be in default-allow as no netpol selects podBar
-	require.True(t, m.entryExists(ingressDefaultAllowIPSetName, podIP))
-	require.True(t, m.Exist(podBar.ObjectMeta.UID, ingressDefaultAllowIPSetName, podIP))
+	require.True(t, m.entriesExist(ingressDefaultAllowIPSetName, podIP))
+	require.True(t, m.EntryExists(podBar.ObjectMeta.UID, ingressDefaultAllowIPSetName, podIP))
 	// Should be in run=bar ipset
-	require.True(t, m.entryExists(runBarIPSetName, podIP))
+	require.True(t, m.entriesExist(runBarIPSetName, podIP))
 
 	controller.DeletePod(podFoo)
 	// Multiple duplicate events should not affect npc state
@@ -434,13 +439,13 @@ func TestOutOfOrderPodEvents(t *testing.T) {
 
 	// Should be in default-allow as no netpol selects podBar and podFoo removal
 	// should not affect podBar in default-allow
-	require.True(t, m.entryExists(ingressDefaultAllowIPSetName, podIP))
+	require.True(t, m.entriesExist(ingressDefaultAllowIPSetName, podIP))
 
 	controller.DeletePod(podBar)
 
 	// Should remove from default-allow and run=bar ipsets
 	require.Equal(t, 0, len(m.sets[ingressDefaultAllowIPSetName].subSets))
-	require.False(t, m.entryExists(runBarIPSetName, podIP))
+	require.False(t, m.entriesExist(runBarIPSetName, podIP))
 }
 
 // Test case for https://github.com/weaveworks/weave/issues/3222
@@ -471,7 +476,7 @@ func TestNewTargetSelector(t *testing.T) {
 		Status: coreapi.PodStatus{PodIP: podIP}}
 	controller.AddPod(podFoo)
 
-	require.True(t, m.entryExists(ingressDefaultAllowIPSetName, podIP))
+	require.True(t, m.entriesExist(ingressDefaultAllowIPSetName, podIP))
 
 	netpolBar := &networkingv1.NetworkPolicy{
 		ObjectMeta: metav1.ObjectMeta{
@@ -492,7 +497,7 @@ func TestNewTargetSelector(t *testing.T) {
 	controller.AddNetworkPolicy(netpolBar)
 
 	// netpolBar target selector selects podFoo
-	require.False(t, m.entryExists(ingressDefaultAllowIPSetName, podIP))
+	require.False(t, m.entriesExist(ingressDefaultAllowIPSetName, podIP))
 
 	netpolFoo := &networkingv1.NetworkPolicy{
 		ObjectMeta: metav1.ObjectMeta{
@@ -514,10 +519,10 @@ func TestNewTargetSelector(t *testing.T) {
 
 	controller.DeleteNetworkPolicy(netpolBar)
 	// netpolFoo target-selects podFoo
-	require.False(t, m.entryExists(ingressDefaultAllowIPSetName, podIP))
+	require.False(t, m.entriesExist(ingressDefaultAllowIPSetName, podIP))
 	controller.DeleteNetworkPolicy(netpolFoo)
 	// No netpol target-selects podFoo
-	require.True(t, m.entryExists(ingressDefaultAllowIPSetName, podIP))
+	require.True(t, m.entriesExist(ingressDefaultAllowIPSetName, podIP))
 }
 
 func TestEgressPolicyWithIPBlock(t *testing.T) {
@@ -573,8 +578,8 @@ func TestEgressPolicyWithIPBlock(t *testing.T) {
 	require.NoError(t, err)
 
 	require.Equal(t, 2, len(m.sets[exceptIPSetName].subSets))
-	require.True(t, m.entryExists(exceptIPSetName, "192.168.48.1/32"))
-	require.True(t, m.entryExists(exceptIPSetName, "192.168.48.2/32"))
+	require.True(t, m.entriesExist(exceptIPSetName, "192.168.48.1/32"))
+	require.True(t, m.entriesExist(exceptIPSetName, "192.168.48.2/32"))
 
 	// Each egress rule is represented as two iptables rules (-J MARK and -J RETURN).
 	require.Equal(t, 2, len(ipt.rules[EgressCustomChain]))
@@ -683,7 +688,7 @@ func TestIngressPolicyWithIPBlockAndPortSpecified(t *testing.T) {
 	defer controller.DeleteNetworkPolicy(netpolicty)
 
 	require.Equal(t, 1, len(m.sets[runBarIPSetName].subSets))
-	require.True(t, m.entryExists(runBarIPSetName, barPodIP))
+	require.True(t, m.entriesExist(runBarIPSetName, barPodIP))
 
 	require.Equal(t, 1, len(ipt.rules[IngressChain]))
 	for rule := range ipt.rules[IngressChain] {
diff --git a/npc/namespace.go b/npc/namespace.go
index 3c061111..6423b4cd 100644
--- a/npc/namespace.go
+++ b/npc/namespace.go
@@ -441,7 +441,7 @@ func (ns *ns) updateDefaultAllowIPSetEntry(oldObj, newObj *coreapi.Pod, ipsetNam
 	// Instead of iterating over all selectors we check whether old pod IP
 	// has been inserted into default-allow ipset to decide whether the IP
 	// in the ipset has to be updated.
-	if ns.ips.Exist(oldObj.ObjectMeta.UID, ipsetName, oldObj.Status.PodIP) {
+	if ns.ips.EntryExists(oldObj.ObjectMeta.UID, ipsetName, oldObj.Status.PodIP) {
 
 		if err := ns.ips.DelEntry(oldObj.ObjectMeta.UID, ipsetName, oldObj.Status.PodIP); err != nil {
 			return err
diff --git a/prog/weave-npc/main.go b/prog/weave-npc/main.go
index 4da4a1de..22e4e556 100644
--- a/prog/weave-npc/main.go
+++ b/prog/weave-npc/main.go
@@ -196,7 +196,7 @@ func createBaseRules(ipt *iptables.IPTables, ips ipset.Interface) error {
 	}
 
 	// delete `weave-local-pods` ipset which is no longer used by weave-npc
-	weaveLocalPodExist, err := ipsetExist(ips, npc.LocalIpset)
+	weaveLocalPodExist, err := ips.Exists(npc.LocalIpset)
 	if err != nil {
 		common.Log.Errorf("Failed to look if ipset '%s' exists", npc.LocalIpset)
 	} else if weaveLocalPodExist {
@@ -209,22 +209,6 @@ func createBaseRules(ipt *iptables.IPTables, ips ipset.Interface) error {
 	return nil
 }
 
-// Dummy way to check whether a given ipset exists.
-// TODO(brb) Use "ipset -exist create <..>" for our purpose instead (for some reasons
-// creating an ipset with -exist fails).
-func ipsetExist(ips ipset.Interface, name ipset.Name) (bool, error) {
-	sets, err := ips.List(string(name))
-	if err != nil {
-		return false, err
-	}
-	for _, s := range sets {
-		if s == name {
-			return true, nil
-		}
-	}
-	return false, nil
-}
-
 func root(cmd *cobra.Command, args []string) {
 	var npController cache.Controller
 
diff --git a/prog/weaver/main.go b/prog/weaver/main.go
index 3edaed16..04624bd8 100644
--- a/prog/weaver/main.go
+++ b/prog/weaver/main.go
@@ -168,6 +168,7 @@ func main() {
 		procPath           string
 		discoveryEndpoint  string
 		token              string
+		iptablesRefresh    time.Duration
 		advertiseAddress   string
 		pluginConfig       plugin.Config
 		defaultDockerHost  = getenvOrDefault("DOCKER_HOST", "unix:///var/run/docker.sock")
@@ -216,6 +217,7 @@ func main() {
 	mflag.StringVar(&discoveryEndpoint, []string{"-peer-discovery-url"}, "https://cloud.weave.works/api/net", "url for peer discovery")
 	mflag.StringVar(&token, []string{"-token"}, "", "token for peer discovery")
 	mflag.StringVar(&advertiseAddress, []string{"-advertise-address"}, "", "address to advertise for peer discovery")
+	mflag.DurationVar(&iptablesRefresh, []string{"-iptables-refresh-interval"}, 10*time.Second, "Interval to reapply iptables. 0 to only apply on launch.")
 
 	mflag.BoolVar(&pluginConfig.Enable, []string{"-plugin"}, false, "enable Docker plugin (v1)")
 	mflag.BoolVar(&pluginConfig.EnableV2, []string{"-plugin-v2"}, false, "enable Docker plugin (v2)")
@@ -313,8 +315,11 @@ func main() {
 		}
 	}
 	ips := ipset.New(common.LogLogger(), 0)
+	err = weavenet.ResetIPTables(&bridgeConfig, ips)
+	checkFatal(err)
 	bridgeType, err := weavenet.EnsureBridge(procPath, &bridgeConfig, Log, ips)
 	checkFatal(err)
+
 	Log.Println("Bridge type is", bridgeType)
 
 	config.Password = determinePassword(password)
@@ -512,6 +517,29 @@ func main() {
 		go exposeForAWSVPC(allocator, defaultSubnet, bridgeConfig.WeaveBridgeName, waitReady.Add())
 	}
 
+	if iptablesRefresh > 0 {
+		Log.Printf("Entering iptable refresh loop (interval %v)", iptablesRefresh)
+		ticker := time.NewTicker(iptablesRefresh).C
+		go func() {
+			for {
+				select {
+				case <-ticker:
+					Log.Debug("Re-configuring iptables")
+					err := weavenet.ConfigureIPTables(&bridgeConfig, ips)
+					if err != nil {
+						Log.Errorf("Error configuring iptables: %s", err)
+					}
+
+					Log.Debug("Beginning re-exposure...")
+					weavenet.Reexpose(&bridgeConfig, Log)
+					Log.Debug("... re-exposure complete.")
+				}
+			}
+		}()
+	} else {
+		Log.Printf("Not refreshing iptables (interval: %v)", iptablesRefresh)
+	}
+
 	signals.SignalHandlerLoop(common.Log, router)
 }
 
diff --git a/test/130_expose_2_test.sh b/test/130_expose_2_test.sh
index e738da50..dd819f0d 100755
--- a/test/130_expose_2_test.sh
+++ b/test/130_expose_2_test.sh
@@ -27,6 +27,10 @@ exec_on1() {
     assert_raises "exec_on  $HOST1 $@"
 }
 
+wait_for_iptable_refresh() {
+    sleep 2
+}
+
 # Containers in the same subnet should be able to talk; different subnet not.
 check_container_connectivity() {
     exec_on1 "c1 $PING $C2"
@@ -39,7 +43,7 @@ check_container_connectivity() {
 
 start_suite "exposing weave network to host"
 
-weave_on $HOST1 launch --ipalloc-range $UNIVERSE
+weave_on $HOST1 launch --ipalloc-range $UNIVERSE --iptables-refresh-interval=1s
 
 start_container $HOST1 $C1/24 --name=c1
 start_container $HOST1 $C2/24 --name=c2
@@ -61,11 +65,20 @@ weave_on1 "expose $EXP/24"
 run_on1   "! $PING $C1"
 run_on1   "  $PING $C3"
 run_on1   "! $PING $C5"
+wait_for_iptable_refresh
+run_on1   "! $PING $C1"
+run_on1   "  $PING $C3"
+run_on1   "! $PING $C5"
+
 weave_on1 "expose"
 weave_on1 "expose"
 run_on1   "! $PING $C1"
 run_on1   "  $PING $C3"
 run_on1   "  $PING $C5"
+wait_for_iptable_refresh
+run_on1   "! $PING $C1"
+run_on1   "  $PING $C3"
+run_on1   "  $PING $C5"
 check_container_connectivity
 
 # no host connectivity after 'hide', and idempotence of 'hide'
@@ -73,9 +86,15 @@ weave_on1 "hide $EXP/24"
 weave_on1 "hide $EXP/24"
 run_on1   "! $PING $C3"
 run_on1   "  $PING $C5"
+wait_for_iptable_refresh
+run_on1   "! $PING $C3"
+run_on1   "  $PING $C5"
+
 weave_on1 "hide"
 weave_on1 "hide"
 run_on1   "! $PING $C5"
+wait_for_iptable_refresh
+run_on1   "! $PING $C5"
 
 # remote host connectivity after 'expose'
 docker_on $HOST1 exec -d c3 nc -l -p $PORT
diff --git a/test/135_expose_iptables_refresh.sh b/test/135_expose_iptables_refresh.sh
new file mode 100755
index 00000000..e19145c2
--- /dev/null
+++ b/test/135_expose_iptables_refresh.sh
@@ -0,0 +1,69 @@
+#!/bin/bash
+
+. "$(dirname "$0")/config.sh"
+
+run_on1() {
+    assert_raises "run_on $HOST1 $@"
+}
+
+weave_on1() {
+    assert_raises "weave_on $HOST1 $@"
+}
+
+stop_weave_on1() {
+    assert_raises "stop_weave_on $HOST1 $@"
+}
+
+get_weave_iptable_rules() {
+    get_command_output_on $HOST1 "sudo iptables-save | grep -i weave"
+}
+
+wait_for_iptable_refresh() {
+    sleep 2
+}
+
+start_suite "exposing weave network to host"
+
+# Launch
+
+## Check no refreshing
+weave_on1 "launch --iptables-refresh-interval=0s"
+IPT_BEFORE=$(get_weave_iptable_rules)
+run_on1 "sudo iptables -t nat -D POSTROUTING -j WEAVE"
+wait_for_iptable_refresh
+IPT_AFTER=$(get_weave_iptable_rules)
+assert_raises "diff <(echo $IPT_BEFORE) <(echo $IPT_AFTER)" 1
+stop_weave_on1
+
+## Check refreshing
+weave_on1 "launch --iptables-refresh-interval=1s"
+IPT_BEFORE=$(get_weave_iptable_rules)
+run_on1 "sudo iptables -t nat -D POSTROUTING -j WEAVE"
+wait_for_iptable_refresh
+IPT_AFTER=$(get_weave_iptable_rules)
+assert_raises "diff <(echo $IPT_BEFORE) <(echo $IPT_AFTER)" 0
+stop_weave_on1
+
+# Expose
+
+## Check no refreshing
+weave_on1 "launch --iptables-refresh-interval=0s"
+weave_on1 "expose"
+IPT_BEFORE=$(get_weave_iptable_rules)
+run_on1 "sudo iptables -D FORWARD -o weave -j WEAVE-EXPOSE"
+wait_for_iptable_refresh
+IPT_AFTER=$(get_weave_iptable_rules)
+assert_raises "diff <(echo $IPT_BEFORE) <(echo $IPT_AFTER)" 1
+stop_weave_on1
+
+## Check refreshing
+weave_on1 "launch --iptables-refresh-interval=1s"
+weave_on1 "expose"
+IPT_BEFORE=$(get_weave_iptable_rules)
+run_on1 "sudo iptables -D FORWARD -o weave -j WEAVE-EXPOSE"
+wait_for_iptable_refresh
+IPT_AFTER=$(get_weave_iptable_rules)
+assert_raises "diff <(echo $IPT_BEFORE) <(echo $IPT_AFTER)" 0
+stop_weave_on1
+
+end_suite

From 66eb80d8416f271bf628f5b0e70a711ad96fc962 Mon Sep 17 00:00:00 2001
From: Simon Howe <footless@gmail.com>
Date: Mon, 18 May 2020 14:55:02 +0200
Subject: [PATCH 2/8] Adds a CANARY chain and Monitor loop instead of
 reapplying iptables

---
 net/expose.go                       |   9 +--
 net/iptables.go                     | 118 ++++++++++++++++++++++++++++
 prog/weaver/main.go                 |  26 +++---
 test/130_expose_2_test.sh           |  21 +----
 test/135_expose_iptables_refresh.sh |   8 +-
 5 files changed, 135 insertions(+), 47 deletions(-)

diff --git a/net/expose.go b/net/expose.go
index 76ba41a1..d35f781e 100644
--- a/net/expose.go
+++ b/net/expose.go
@@ -103,13 +103,8 @@ func addNatRule(ipt *iptables.IPTables, rulespec ...string) error {
 	// Loop until we get an exit code other than "temporarily unavailable"
 	for {
 		if err := ipt.AppendUnique("nat", "WEAVE", rulespec...); err != nil {
-			if ierr, ok := err.(*iptables.Error); ok {
-				if status, ok := ierr.ExitError.Sys().(syscall.WaitStatus); ok {
-					// (magic exit code 4 found in iptables source code; undocumented)
-					if status.ExitStatus() == 4 {
-						continue
-					}
-				}
+			if isResourceError(err) {
+				continue
 			}
 			return err
 		}
diff --git a/net/iptables.go b/net/iptables.go
index a11ca533..ba555e0d 100644
--- a/net/iptables.go
+++ b/net/iptables.go
@@ -2,9 +2,13 @@ package net
 
 import (
 	"strings"
+	"syscall"
+	"time"
 
 	"github.com/coreos/go-iptables/iptables"
 	"github.com/pkg/errors"
+	"github.com/sirupsen/logrus"
+	utilwait "k8s.io/apimachinery/pkg/util/wait"
 )
 
 // AddChainWithRules creates a chain and appends given rules to it.
@@ -104,3 +108,117 @@ func ensureRulesAtTop(table, chain string, rulespecs [][]string, ipt *iptables.I
 
 	return nil
 }
+
+func chainExists(ipt *iptables.IPTables, table string, chain string) (bool, error) {
+	existingChains, err := ipt.ListChains(table)
+	if err != nil {
+		return false, errors.Wrapf(err, "ipt.ListChains(%s)", table)
+	}
+	chainMap := make(map[string]struct{})
+	for _, c := range existingChains {
+		chainMap[c] = struct{}{}
+	}
+
+	_, found := chainMap[chain]
+	return found, nil
+}
+
+const (
+	// Max time we wait for an iptables flush to complete after we notice it has started
+	iptablesFlushTimeout = 5 * time.Second
+	// How often we poll while waiting for an iptables flush to complete
+	iptablesFlushPollTime = 100 * time.Millisecond
+)
+
+// Monitor is part of Interface
+func Monitor(log *logrus.Logger, canary string, tables []string, reloadFunc func(), interval time.Duration, stopCh <-chan struct{}) {
+	ipt, err := iptables.New()
+	if err != nil {
+		log.Errorf("creating iptables object while initializing iptable Monitoring %s", err)
+		return
+	}
+
+	for {
+		_ = PollImmediateUntil(interval, func() (bool, error) {
+			for _, table := range tables {
+				if err := ensureChains(ipt, table, canary); err != nil {
+					log.Warningf("Could not set up iptables canary %s/%s: %v", table, canary, err)
+					return false, nil
+				}
+			}
+			return true, nil
+		}, stopCh)
+
+		// Poll until stopCh is closed or iptables is flushed
+		err = utilwait.PollUntil(interval, func() (bool, error) {
+			if exists, err := chainExists(ipt, tables[0], canary); exists {
+				return false, nil
+			} else if isResourceError(err) {
+				log.Warningf("Could not check for iptables canary %s/%s: %v", tables[0], canary, err)
+				return false, nil
+			}
+			log.Infof("iptables canary %s/%s deleted", tables[0], canary)
+
+			// Wait for the other canaries to be deleted too before returning
+			// so we don't start reloading too soon.
+			err := utilwait.PollImmediate(iptablesFlushPollTime, iptablesFlushTimeout, func() (bool, error) {
+				for i := 1; i < len(tables); i++ {
+					if exists, err := chainExists(ipt, tables[i], canary); exists || isResourceError(err) {
+						return false, nil
+					}
+				}
+				return true, nil
+			})
+			if err != nil {
+				log.Warning("Inconsistent iptables state detected.")
+			}
+			return true, nil
+		}, stopCh)
+
+		if err != nil {
+			// stopCh was closed
+			for _, table := range tables {
+				_ = ipt.DeleteChain(table, canary)
+			}
+			return
+		}
+
+		log.Infof("Reloading after iptables flush")
+		reloadFunc()
+	}
+}
+
+const iptablesStatusResourceProblem = 4
+
+// isResourceError returns true if the error indicates that iptables ran into a "resource
+// problem" and was unable to attempt the request. In particular, this will be true if it
+// times out trying to get the iptables lock.
+func isResourceError(err error) bool {
+	if ierr, ok := err.(*iptables.Error); ok {
+		if status, ok := ierr.ExitError.Sys().(syscall.WaitStatus); ok {
+			return status.ExitStatus() == iptablesStatusResourceProblem
+		}
+	}
+
+	return false
+}
+
+// PollImmediateUntil tries a condition func until it returns true, an error or stopCh is closed.
+//
+// PollImmediateUntil runs the 'condition' before waiting for the interval.
+// 'condition' will always be invoked at least once.
+func PollImmediateUntil(interval time.Duration, condition utilwait.ConditionFunc, stopCh <-chan struct{}) error {
+	done, err := condition()
+	if err != nil {
+		return err
+	}
+	if done {
+		return nil
+	}
+	select {
+	case <-stopCh:
+		return utilwait.ErrWaitTimeout
+	default:
+		return utilwait.PollUntil(interval, condition, stopCh)
+	}
+}
diff --git a/prog/weaver/main.go b/prog/weaver/main.go
index 04624bd8..acf59413 100644
--- a/prog/weaver/main.go
+++ b/prog/weaver/main.go
@@ -17,6 +17,7 @@ import (
 	"github.com/weaveworks/common/mflagext"
 	"github.com/weaveworks/common/signals"
 	"github.com/weaveworks/mesh"
+	"k8s.io/apimachinery/pkg/util/wait"
 
 	"github.com/weaveworks/weave/common"
 	"github.com/weaveworks/weave/common/docker"
@@ -519,23 +520,16 @@ func main() {
 
 	if iptablesRefresh > 0 {
 		Log.Printf("Entering iptable refresh loop (interval %v)", iptablesRefresh)
-		ticker := time.NewTicker(iptablesRefresh).C
-		go func() {
-			for {
-				select {
-				case <-ticker:
-					Log.Debug("Re-configuring iptables")
-					err := weavenet.ConfigureIPTables(&bridgeConfig, ips)
-					if err != nil {
-						Log.Errorf("Error configuring iptables: %s", err)
-					}
-
-					Log.Debug("Beginning re-exposure...")
-					weavenet.Reexpose(&bridgeConfig, Log)
-					Log.Debug("... re-exposure complete.")
-				}
+		applyIPTables := func() {
+			Log.Info("Re-configuring iptables")
+			err := weavenet.ConfigureIPTables(&bridgeConfig, ips)
+			if err != nil {
+				Log.Errorf("Error configuring iptables: %s", err)
 			}
-		}()
+
+			weavenet.Reexpose(&bridgeConfig, Log)
+		}
+		go weavenet.Monitor(Log, "WEAVE-CANARY", []string{"mangle", "nat", "filter"}, applyIPTables, iptablesRefresh, wait.NeverStop)
 	} else {
 		Log.Printf("Not refreshing iptables (interval: %v)", iptablesRefresh)
 	}
diff --git a/test/130_expose_2_test.sh b/test/130_expose_2_test.sh
index dd819f0d..e738da50 100755
--- a/test/130_expose_2_test.sh
+++ b/test/130_expose_2_test.sh
@@ -27,10 +27,6 @@ exec_on1() {
     assert_raises "exec_on  $HOST1 $@"
 }
 
-wait_for_iptable_refresh() {
-    sleep 2
-}
-
 # Containers in the same subnet should be able to talk; different subnet not.
 check_container_connectivity() {
     exec_on1 "c1 $PING $C2"
@@ -43,7 +39,7 @@ check_container_connectivity() {
 
 start_suite "exposing weave network to host"
 
-weave_on $HOST1 launch --ipalloc-range $UNIVERSE --iptables-refresh-interval=1s
+weave_on $HOST1 launch --ipalloc-range $UNIVERSE
 
 start_container $HOST1 $C1/24 --name=c1
 start_container $HOST1 $C2/24 --name=c2
@@ -65,20 +61,11 @@ weave_on1 "expose $EXP/24"
 run_on1   "! $PING $C1"
 run_on1   "  $PING $C3"
 run_on1   "! $PING $C5"
-wait_for_iptable_refresh
-run_on1   "! $PING $C1"
-run_on1   "  $PING $C3"
-run_on1   "! $PING $C5"
-
 weave_on1 "expose"
 weave_on1 "expose"
 run_on1   "! $PING $C1"
 run_on1   "  $PING $C3"
 run_on1   "  $PING $C5"
-wait_for_iptable_refresh
-run_on1   "! $PING $C1"
-run_on1   "  $PING $C3"
-run_on1   "  $PING $C5"
 check_container_connectivity
 
 # no host connectivity after 'hide', and idempotence of 'hide'
@@ -86,15 +73,9 @@ weave_on1 "hide $EXP/24"
 weave_on1 "hide $EXP/24"
 run_on1   "! $PING $C3"
 run_on1   "  $PING $C5"
-wait_for_iptable_refresh
-run_on1   "! $PING $C3"
-run_on1   "  $PING $C5"
-
 weave_on1 "hide"
 weave_on1 "hide"
 run_on1   "! $PING $C5"
-wait_for_iptable_refresh
-run_on1   "! $PING $C5"
 
 # remote host connectivity after 'expose'
 docker_on $HOST1 exec -d c3 nc -l -p $PORT
diff --git a/test/135_expose_iptables_refresh.sh b/test/135_expose_iptables_refresh.sh
index e19145c2..f354bbac 100755
--- a/test/135_expose_iptables_refresh.sh
+++ b/test/135_expose_iptables_refresh.sh
@@ -29,7 +29,7 @@ start_suite "exposing weave network to host"
 ## Check no refreshing
 weave_on1 "launch --iptables-refresh-interval=0s"
 IPT_BEFORE=$(get_weave_iptable_rules)
-run_on1 "sudo iptables -t nat -D POSTROUTING -j WEAVE"
+run_on1 "sudo iptables -t nat -X WEAVE-CANARY"
 wait_for_iptable_refresh
 IPT_AFTER=$(get_weave_iptable_rules)
 assert_raises "diff <(echo $IPT_BEFORE) <(echo $IPT_AFTER)" 1
@@ -38,7 +38,7 @@ stop_weave_on1
 ## Check refreshing
 weave_on1 "launch --iptables-refresh-interval=1s"
 IPT_BEFORE=$(get_weave_iptable_rules)
-run_on1 "sudo iptables -t nat -D POSTROUTING -j WEAVE"
+run_on1 "sudo iptables -t nat -X WEAVE-CANARY"
 wait_for_iptable_refresh
 IPT_AFTER=$(get_weave_iptable_rules)
 assert_raises "diff <(echo $IPT_BEFORE) <(echo $IPT_AFTER)" 0
@@ -50,7 +50,7 @@ stop_weave_on1
 weave_on1 "launch --iptables-refresh-interval=0s"
 weave_on1 "expose"
 IPT_BEFORE=$(get_weave_iptable_rules)
-run_on1 "sudo iptables -D FORWARD -o weave -j WEAVE-EXPOSE"
+run_on1 "sudo iptables -t nat -X WEAVE-CANARY"
 wait_for_iptable_refresh
 IPT_AFTER=$(get_weave_iptable_rules)
 assert_raises "diff <(echo $IPT_BEFORE) <(echo $IPT_AFTER)" 1
@@ -60,7 +60,7 @@ stop_weave_on1
 weave_on1 "launch --iptables-refresh-interval=1s"
 weave_on1 "expose"
 IPT_BEFORE=$(get_weave_iptable_rules)
-run_on1 "sudo iptables -D FORWARD -o weave -j WEAVE-EXPOSE"
+run_on1 "sudo iptables -t nat -X WEAVE-CANARY"
 wait_for_iptable_refresh
 IPT_AFTER=$(get_weave_iptable_rules)
 assert_raises "diff <(echo $IPT_BEFORE) <(echo $IPT_AFTER)" 0

From ca7103d0075c54308474ee0e3c3508d9a80955ec Mon Sep 17 00:00:00 2001
From: Simon Howe <footless@gmail.com>
Date: Fri, 3 Jul 2020 16:20:00 +0200
Subject: [PATCH 3/8] Review feedback: don't recreate NoMasqLocalIpset
 onCanaryDeleted

---
 net/bridge.go | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/net/bridge.go b/net/bridge.go
index 3397f883..268e70c1 100644
--- a/net/bridge.go
+++ b/net/bridge.go
@@ -429,7 +429,7 @@ func (f fastdpImpl) attach(veth *netlink.Veth) error {
 	return odp.AddDatapathInterfaceIfNotExist(f.datapathName, veth.Attrs().Name)
 }
 
-// ResetIPTables resets IPTables if they're in a strange state from a previous run.
+// ResetIPTables resets IPTables in case they're in a strange state from a previous run.
 func ResetIPTables(config *BridgeConfig, ips ipset.Interface) error {
 	ipt, err := iptables.New()
 	if err != nil {
@@ -568,23 +568,13 @@ func ConfigureIPTables(config *BridgeConfig, ips ipset.Interface) error {
 	}
 
 	// For the cases where the weave bridge is the default gateway for
-	// containers (e.g. Kubernetes): create the ipset to store CIDRs allocated
-	// by IPAM for local containers. In the case of Kubernetes, external traffic
-	// sent to these CIDRs avoids SNAT'ing so that NodePort with
-	// `"externalTrafficPolicy":"Local"` would receive packets with correct
-	// src IP addr.
+	// containers (e.g. Kubernetes): In `ResetIPTables` (which we assume
+	// to have been called at this point) we create an ipset to store CIDRs
+	// allocated by IPAM for local containers.
+	// In the case of Kubernetes, external traffic sent to these CIDRs
+	// avoids SNAT'ing so that NodePort with `"externalTrafficPolicy":"Local"`
+	// would receive packets with correct src IP addr.
 	if config.NoMasqLocal {
-		ips := ipset.New(common.LogLogger(), 0)
-
-		noMasqLocalExists, err := ips.Exists(NoMasqLocalIpset)
-		if err != nil {
-			common.Log.Errorf("Failed to look if ipset '%s' exists: %s", NoMasqLocalIpset, err)
-		} else if !noMasqLocalExists {
-			if err := ips.Create(NoMasqLocalIpset, ipset.HashNet); err != nil {
-				return err
-			}
-		}
-
 		if err := ipt.AppendUnique("nat", "WEAVE",
 			"-m", "set", "--match-set", string(NoMasqLocalIpset), "dst",
 			"-m", "comment", "--comment", "Prevent SNAT to locally running containers",

From 709b434f55eebd2a4a44a1c7eeb345521146a24c Mon Sep 17 00:00:00 2001
From: Simon Howe <footless@gmail.com>
Date: Fri, 3 Jul 2020 16:33:34 +0200
Subject: [PATCH 4/8] Clean up the Monitor go routine

---
 prog/weaver/main.go | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/prog/weaver/main.go b/prog/weaver/main.go
index acf59413..81acb67d 100644
--- a/prog/weaver/main.go
+++ b/prog/weaver/main.go
@@ -17,7 +17,6 @@ import (
 	"github.com/weaveworks/common/mflagext"
 	"github.com/weaveworks/common/signals"
 	"github.com/weaveworks/mesh"
-	"k8s.io/apimachinery/pkg/util/wait"
 
 	"github.com/weaveworks/weave/common"
 	"github.com/weaveworks/weave/common/docker"
@@ -529,7 +528,9 @@ func main() {
 
 			weavenet.Reexpose(&bridgeConfig, Log)
 		}
-		go weavenet.Monitor(Log, "WEAVE-CANARY", []string{"mangle", "nat", "filter"}, applyIPTables, iptablesRefresh, wait.NeverStop)
+		stopChan := make(chan struct{})
+		go weavenet.Monitor(Log, "WEAVE-CANARY", []string{"mangle", "nat", "filter"}, applyIPTables, iptablesRefresh, stopChan)
+		defer close(stopChan)
 	} else {
 		Log.Printf("Not refreshing iptables (interval: %v)", iptablesRefresh)
 	}

From 899490c3f9b69a351be90a6b3a382bedf2dcacb3 Mon Sep 17 00:00:00 2001
From: Simon Howe <footless@gmail.com>
Date: Wed, 8 Jul 2020 10:54:18 +0200
Subject: [PATCH 5/8] Removes disabling iptable canary

- Should always be on. Simpler.
---
 prog/weaver/main.go | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/prog/weaver/main.go b/prog/weaver/main.go
index 81acb67d..36a95246 100644
--- a/prog/weaver/main.go
+++ b/prog/weaver/main.go
@@ -168,7 +168,6 @@ func main() {
 		procPath           string
 		discoveryEndpoint  string
 		token              string
-		iptablesRefresh    time.Duration
 		advertiseAddress   string
 		pluginConfig       plugin.Config
 		defaultDockerHost  = getenvOrDefault("DOCKER_HOST", "unix:///var/run/docker.sock")
@@ -217,7 +216,6 @@ func main() {
 	mflag.StringVar(&discoveryEndpoint, []string{"-peer-discovery-url"}, "https://cloud.weave.works/api/net", "url for peer discovery")
 	mflag.StringVar(&token, []string{"-token"}, "", "token for peer discovery")
 	mflag.StringVar(&advertiseAddress, []string{"-advertise-address"}, "", "address to advertise for peer discovery")
-	mflag.DurationVar(&iptablesRefresh, []string{"-iptables-refresh-interval"}, 10*time.Second, "Interval to reapply iptables. 0 to only apply on launch.")
 
 	mflag.BoolVar(&pluginConfig.Enable, []string{"-plugin"}, false, "enable Docker plugin (v1)")
 	mflag.BoolVar(&pluginConfig.EnableV2, []string{"-plugin-v2"}, false, "enable Docker plugin (v2)")
@@ -517,23 +515,17 @@ func main() {
 		go exposeForAWSVPC(allocator, defaultSubnet, bridgeConfig.WeaveBridgeName, waitReady.Add())
 	}
 
-	if iptablesRefresh > 0 {
-		Log.Printf("Entering iptable refresh loop (interval %v)", iptablesRefresh)
-		applyIPTables := func() {
-			Log.Info("Re-configuring iptables")
-			err := weavenet.ConfigureIPTables(&bridgeConfig, ips)
-			if err != nil {
-				Log.Errorf("Error configuring iptables: %s", err)
-			}
-
-			weavenet.Reexpose(&bridgeConfig, Log)
+	applyIPTables := func() {
+		Log.Info("Re-configuring iptables")
+		err := weavenet.ConfigureIPTables(&bridgeConfig, ips)
+		if err != nil {
+			Log.Errorf("Error configuring iptables: %s", err)
 		}
-		stopChan := make(chan struct{})
-		go weavenet.Monitor(Log, "WEAVE-CANARY", []string{"mangle", "nat", "filter"}, applyIPTables, iptablesRefresh, stopChan)
-		defer close(stopChan)
-	} else {
-		Log.Printf("Not refreshing iptables (interval: %v)", iptablesRefresh)
+		weavenet.Reexpose(&bridgeConfig, Log)
 	}
+	stopChan := make(chan struct{})
+	go weavenet.Monitor(Log, "WEAVE-CANARY", []string{"mangle", "nat", "filter"}, applyIPTables, 10*time.Second, stopChan)
+	defer close(stopChan)
 
 	signals.SignalHandlerLoop(common.Log, router)
 }

From c0e50082027e8cc4dce48b3b891446e20e80f66a Mon Sep 17 00:00:00 2001
From: Simon Howe <footless@gmail.com>
Date: Wed, 8 Jul 2020 11:50:36 +0200
Subject: [PATCH 6/8] Update tests after removing canary refresh flag

---
 test/135_expose_iptables_refresh.sh | 32 +++++------------------------
 1 file changed, 5 insertions(+), 27 deletions(-)

diff --git a/test/135_expose_iptables_refresh.sh b/test/135_expose_iptables_refresh.sh
index f354bbac..a844e7cc 100755
--- a/test/135_expose_iptables_refresh.sh
+++ b/test/135_expose_iptables_refresh.sh
@@ -19,24 +19,14 @@ get_weave_iptable_rules() {
 }
 
 wait_for_iptable_refresh() {
-    sleep 2
+    # Canary is checked every 10s
+    sleep 12
 }
 
 start_suite "exposing weave network to host"
 
 # Launch
-
-## Check no refreshing
-weave_on1 "launch --iptables-refresh-interval=0s"
-IPT_BEFORE=$(get_weave_iptable_rules)
-run_on1 "sudo iptables -t nat -X WEAVE-CANARY"
-wait_for_iptable_refresh
-IPT_AFTER=$(get_weave_iptable_rules)
-assert_raises "diff <(echo $IPT_BEFORE) <(echo $IPT_AFTER)" 1
-stop_weave_on1
-
-## Check refreshing
-weave_on1 "launch --iptables-refresh-interval=1s"
+weave_on1 "launch"
 IPT_BEFORE=$(get_weave_iptable_rules)
 run_on1 "sudo iptables -t nat -X WEAVE-CANARY"
 wait_for_iptable_refresh
@@ -44,20 +34,8 @@ IPT_AFTER=$(get_weave_iptable_rules)
 assert_raises "diff <(echo $IPT_BEFORE) <(echo $IPT_AFTER)" 0
 stop_weave_on1
 
-# Expose
-
-## Check no refreshing
-weave_on1 "launch --iptables-refresh-interval=0s"
-weave_on1 "expose"
-IPT_BEFORE=$(get_weave_iptable_rules)
-run_on1 "sudo iptables -t nat -X WEAVE-CANARY"
-wait_for_iptable_refresh
-IPT_AFTER=$(get_weave_iptable_rules)
-assert_raises "diff <(echo $IPT_BEFORE) <(echo $IPT_AFTER)" 1
-stop_weave_on1
-
-## Check refreshing
-weave_on1 "launch --iptables-refresh-interval=1s"
+# After exposing
+weave_on1 "launch"
 weave_on1 "expose"
 IPT_BEFORE=$(get_weave_iptable_rules)
 run_on1 "sudo iptables -t nat -X WEAVE-CANARY"

From 1d7ca17c3ecc3c38a7e4cbd27fbd7ee6dcc1e022 Mon Sep 17 00:00:00 2001
From: Simon Howe <footless@gmail.com>
Date: Fri, 24 Jul 2020 15:01:45 +0200
Subject: [PATCH 7/8] Improve Monitor function description

---
 net/iptables.go | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/iptables.go b/net/iptables.go
index ba555e0d..315fac02 100644
--- a/net/iptables.go
+++ b/net/iptables.go
@@ -130,7 +130,9 @@ const (
 	iptablesFlushPollTime = 100 * time.Millisecond
 )
 
-// Monitor is part of Interface
+// Monitor periodically checks for a canary chain in iptables. If this canary chain goes missing it calls the reloadFunc.
+// This is a more efficient way of detecting whether firewalld or another process has been removing rules that we rely on.
+// The reloadFunc can then check whether other chains that should exist are still there, fix things and restore the canary.
 func Monitor(log *logrus.Logger, canary string, tables []string, reloadFunc func(), interval time.Duration, stopCh <-chan struct{}) {
 	ipt, err := iptables.New()
 	if err != nil {

From 341bac996833d28a019dc5f66c3f0d00f292af7e Mon Sep 17 00:00:00 2001
From: Simon Howe <footless@gmail.com>
Date: Fri, 24 Jul 2020 15:04:28 +0200
Subject: [PATCH 8/8] Rename Monitor -> MonitorForIptablesFlush

- Makes it all a bit more explicit
---
 net/iptables.go     | 4 ++--
 prog/weaver/main.go | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/iptables.go b/net/iptables.go
index 315fac02..cda4655a 100644
--- a/net/iptables.go
+++ b/net/iptables.go
@@ -130,10 +130,10 @@ const (
 	iptablesFlushPollTime = 100 * time.Millisecond
 )
 
-// Monitor periodically checks for a canary chain in iptables. If this canary chain goes missing it calls the reloadFunc.
+// MonitorForIptablesFlush periodically checks for a canary chain in iptables. If this canary chain goes missing it calls the reloadFunc.
 // This is a more efficient way of detecting whether firewalld or another process has been removing rules that we rely on.
 // The reloadFunc can then check whether other chains that should exist are still there, fix things and restore the canary.
-func Monitor(log *logrus.Logger, canary string, tables []string, reloadFunc func(), interval time.Duration, stopCh <-chan struct{}) {
+func MonitorForIptablesFlush(log *logrus.Logger, canary string, tables []string, reloadFunc func(), interval time.Duration, stopCh <-chan struct{}) {
 	ipt, err := iptables.New()
 	if err != nil {
 		log.Errorf("creating iptables object while initializing iptable Monitoring %s", err)
diff --git a/prog/weaver/main.go b/prog/weaver/main.go
index 36a95246..811fc5d9 100644
--- a/prog/weaver/main.go
+++ b/prog/weaver/main.go
@@ -524,7 +524,7 @@ func main() {
 		weavenet.Reexpose(&bridgeConfig, Log)
 	}
 	stopChan := make(chan struct{})
-	go weavenet.Monitor(Log, "WEAVE-CANARY", []string{"mangle", "nat", "filter"}, applyIPTables, 10*time.Second, stopChan)
+	go weavenet.MonitorForIptablesFlush(Log, "WEAVE-CANARY", []string{"mangle", "nat", "filter"}, applyIPTables, 10*time.Second, stopChan)
 	defer close(stopChan)
 
 	signals.SignalHandlerLoop(common.Log, router)