Merge pull request #808 from julia-stripe/retry-creating-iptables-rule

Ensure that the iptables masquerade rule is always installed

Makefile

@@ -8,7 +8,7 @@ TAG?=$(shell git describe --tags --dirty)
 ARCH?=amd64
 
 # These variables can be overridden by setting an environment variable.
-TEST_PACKAGES?=pkg/ip subnet subnet/etcdv2
+TEST_PACKAGES?=pkg/ip subnet subnet/etcdv2 network
 TEST_PACKAGES_EXPANDED=$(TEST_PACKAGES:%=github.com/coreos/flannel/%)
 PACKAGES?=$(TEST_PACKAGES) network
 PACKAGES_EXPANDED=$(PACKAGES:%=github.com/coreos/flannel/%)

main.go

@@ -28,6 +28,7 @@ import (
 	"strings"
 	"syscall"
 
+	"github.com/coreos/go-iptables/iptables"
 	"github.com/coreos/pkg/flagutil"
 	log "github.com/golang/glog"
 	"golang.org/x/net/context"
@@ -284,17 +285,7 @@ func main() {
 
 	// Set up ipMasq if needed
 	if opts.ipMasq {
-		err = network.SetupIPMasq(config.Network, bn.Lease())
-		if err != nil {
-			// Continue, even though it failed.
-			log.Errorf("Failed to set up IP Masquerade: %v", err)
-		}
-
-		defer func() {
-			if err := network.TeardownIPMasq(config.Network, bn.Lease()); err != nil {
-				log.Errorf("Failed to tear down IP Masquerade: %v", err)
-			}
-		}()
+		go setupIPMasq(config, bn)
 	}
 
 	if err := WriteSubnetFile(opts.subnetFile, config.Network, opts.ipMasq, bn); err != nil {
@@ -562,6 +553,26 @@ func mustRunHealthz() {
 	}
 }
 
+func setupIPMasq(config *subnet.Config, bn backend.Network) {
+	ipt, err := iptables.New()
+	if err != nil {
+		// if we can't find iptables, give up and return
+		log.Errorf("Failed to set up IP Masquerade. iptables was not found: %v", err)
+		return
+	}
+	defer func() {
+		network.TeardownIPMasq(ipt, config.Network, bn.Lease())
+	}()
+	for {
+		// Ensure that all the rules exist every 5 seconds
+		if err := network.EnsureIPMasq(ipt, config.Network, bn.Lease()); err != nil {
+			log.Errorf("Failed to ensure IP Masquerade: %v", err)
+		}
+		time.Sleep(5 * time.Second)
+	}
+
+}
+
 func ReadSubnetFromSubnetFile(path string) ip.IP4Net {
 	var prevSubnet ip.IP4Net
 	if _, err := os.Stat(path); !os.IsNotExist(err) {

network/ipmasq.go

@@ -18,13 +18,18 @@ import (
 	"fmt"
 	"strings"
 
-	"github.com/coreos/go-iptables/iptables"
 	log "github.com/golang/glog"
 
 	"github.com/coreos/flannel/pkg/ip"
 	"github.com/coreos/flannel/subnet"
 )
 
+type IPTablesRules interface {
+	AppendUnique(table string, chain string, rulespec ...string) error
+	Delete(table string, chain string, rulespec ...string) error
+	Exists(table string, chain string, rulespec ...string) (bool, error)
+}
+
 func rules(ipn ip.IP4Net, lease *subnet.Lease) [][]string {
 	n := ipn.String()
 	sn := lease.Subnet.String()
@@ -41,36 +46,57 @@ func rules(ipn ip.IP4Net, lease *subnet.Lease) [][]string {
 	}
 }
 
-func SetupIPMasq(ipn ip.IP4Net, lease *subnet.Lease) error {
-	ipt, err := iptables.New()
-	if err != nil {
-		return fmt.Errorf("failed to set up IP Masquerade. iptables was not found")
-	}
-
+func ipMasqRulesExist(ipt IPTablesRules, ipn ip.IP4Net, lease *subnet.Lease) (bool, error) {
 	for _, rule := range rules(ipn, lease) {
-		log.Info("Adding iptables rule: ", strings.Join(rule, " "))
-		err = ipt.AppendUnique("nat", "POSTROUTING", rule...)
+		exists, err := ipt.Exists("nat", "POSTROUTING", rule...)
 		if err != nil {
-			return fmt.Errorf("failed to insert IP masquerade rule: %v", err)
+			// this shouldn't ever happen
+			return false, fmt.Errorf("failed to check rule existence", err)
+		}
+		if !exists {
+			return false, nil
 		}
 	}
 
-	return nil
+	return true, nil
 }
 
-func TeardownIPMasq(ipn ip.IP4Net, lease *subnet.Lease) error {
-	ipt, err := iptables.New()
+func EnsureIPMasq(ipt IPTablesRules, ipn ip.IP4Net, lease *subnet.Lease) error {
+	exists, err := ipMasqRulesExist(ipt, ipn, lease)
 	if err != nil {
-		return fmt.Errorf("failed to teardown IP Masquerade. iptables was not found")
+		return fmt.Errorf("Error checking rule existence: %v", err)
+	}
+	if exists {
+		// if all the rules already exist, no need to do anything
+		return nil
+	}
+	// Otherwise, teardown all the rules and set them up again
+	// We do this because the order of the rules is important
+	log.Info("Some iptables rules are missing; deleting and recreating rules")
+	TeardownIPMasq(ipt, ipn, lease)
+	if err = SetupIPMasq(ipt, ipn, lease); err != nil {
+		return fmt.Errorf("Error setting up rules: %v", err)
 	}
+	return nil
+}
 
+func SetupIPMasq(ipt IPTablesRules, ipn ip.IP4Net, lease *subnet.Lease) error {
 	for _, rule := range rules(ipn, lease) {
-		log.Info("Deleting iptables rule: ", strings.Join(rule, " "))
-		err = ipt.Delete("nat", "POSTROUTING", rule...)
+		log.Info("Adding iptables rule: ", strings.Join(rule, " "))
+		err := ipt.AppendUnique("nat", "POSTROUTING", rule...)
 		if err != nil {
-			return fmt.Errorf("failed to delete IP masquerade rule: %v", err)
+			return fmt.Errorf("failed to insert IP masquerade rule: %v", err)
 		}
 	}
 
 	return nil
 }
+
+func TeardownIPMasq(ipt IPTablesRules, ipn ip.IP4Net, lease *subnet.Lease) {
+	for _, rule := range rules(ipn, lease) {
+		log.Info("Deleting iptables rule: ", strings.Join(rule, " "))
+		// We ignore errors here because if there's an error it's almost certainly because the rule
+		// doesn't exist, which is fine (we don't need to delete rules that don't exist)
+		ipt.Delete("nat", "POSTROUTING", rule...)
+	}
+}

network/ipmasq_test.go

@@ -0,0 +1,99 @@
+// Copyright 2015 flannel authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package network
+
+import (
+	"github.com/coreos/flannel/pkg/ip"
+	"github.com/coreos/flannel/subnet"
+	"net"
+	"reflect"
+	"testing"
+)
+
+func lease() *subnet.Lease {
+	_, net, _ := net.ParseCIDR("192.168.0.0/16")
+	return &subnet.Lease{
+		Subnet: ip.FromIPNet(net),
+	}
+}
+
+type MockIPTablesRule struct {
+	table    string
+	chain    string
+	rulespec []string
+}
+
+type MockIPTables struct {
+	rules []MockIPTablesRule
+}
+
+func (mock *MockIPTables) ruleIndex(table string, chain string, rulespec []string) int {
+	for i, rule := range mock.rules {
+		if rule.table == table && rule.chain == chain && reflect.DeepEqual(rule.rulespec, rulespec) {
+			return i
+		}
+	}
+	return -1
+}
+
+func (mock *MockIPTables) Delete(table string, chain string, rulespec ...string) error {
+	var ruleIndex = mock.ruleIndex(table, chain, rulespec)
+	if ruleIndex != -1 {
+		mock.rules = append(mock.rules[:ruleIndex], mock.rules[ruleIndex+1:]...)
+	}
+	return nil
+}
+
+func (mock *MockIPTables) Exists(table string, chain string, rulespec ...string) (bool, error) {
+	var ruleIndex = mock.ruleIndex(table, chain, rulespec)
+	if ruleIndex != -1 {
+		return true, nil
+	}
+	return false, nil
+}
+
+func (mock *MockIPTables) AppendUnique(table string, chain string, rulespec ...string) error {
+	var ruleIndex = mock.ruleIndex(table, chain, rulespec)
+	if ruleIndex == -1 {
+		mock.rules = append(mock.rules, MockIPTablesRule{table: table, chain: chain, rulespec: rulespec})
+	}
+	return nil
+}
+
+func TestDeleteRules(t *testing.T) {
+	ipt := &MockIPTables{}
+	SetupIPMasq(ipt, ip.IP4Net{}, lease())
+	if len(ipt.rules) != 4 {
+		t.Errorf("Should be 4 rules, there are actually %d: %#v", len(ipt.rules), ipt.rules)
+	}
+	TeardownIPMasq(ipt, ip.IP4Net{}, lease())
+	if len(ipt.rules) != 0 {
+		t.Errorf("Should be 0 rules, there are actually %d: %#v", len(ipt.rules), ipt.rules)
+	}
+}
+
+func TestEnsureRules(t *testing.T) {
+	// If any rules are missing, they should be all deleted and recreated in the correct order
+	ipt_correct := &MockIPTables{}
+	SetupIPMasq(ipt_correct, ip.IP4Net{}, lease())
+	// setup a mock instance where we delete some rules and run `EnsureIPMasq`
+	ipt_recreate := &MockIPTables{}
+	SetupIPMasq(ipt_recreate, ip.IP4Net{}, lease())
+	ipt_recreate.rules = ipt_recreate.rules[0:2]
+	EnsureIPMasq(ipt_recreate, ip.IP4Net{}, lease())
+	if !reflect.DeepEqual(ipt_recreate.rules, ipt_correct.rules) {
+		t.Errorf("iptables rules after EnsureIPMasq are incorrected. Expected: %#v, Actual: %#v", ipt_recreate.rules, ipt_correct.rules)
+	}
+}