Merge pull request #2471 from suwang48404/master

DOCKER-USER chain not created when IPTableEnable=false.

controller.go

@@ -67,6 +67,7 @@ import (
 	"github.com/docker/libnetwork/hostdiscovery"
 	"github.com/docker/libnetwork/ipamapi"
 	"github.com/docker/libnetwork/netlabel"
+	"github.com/docker/libnetwork/options"
 	"github.com/docker/libnetwork/osl"
 	"github.com/docker/libnetwork/types"
 	"github.com/pkg/errors"
@@ -252,6 +253,7 @@ func New(cfgOptions ...config.Option) (NetworkController, error) {
 		return nil, err
 	}
 
+	setupArrangeUserFilterRule(c)
 	return c, nil
 }
 
@@ -909,8 +911,7 @@ addToStore:
 		arrangeIngressFilterRule()
 		c.Unlock()
 	}
-
-	c.arrangeUserFilterRule()
+	arrangeUserFilterRule()
 
 	return network, nil
 }
@@ -1367,3 +1368,27 @@ func (c *controller) IsDiagnosticEnabled() bool {
 	defer c.Unlock()
 	return c.DiagnosticServer.IsDiagnosticEnabled()
 }
+
+func (c *controller) iptablesEnabled() bool {
+	c.Lock()
+	defer c.Unlock()
+
+	if c.cfg == nil {
+		return false
+	}
+	// parse map cfg["bridge"]["generic"]["EnableIPTable"]
+	cfgBridge, ok := c.cfg.Daemon.DriverCfg["bridge"].(map[string]interface{})
+	if !ok {
+		return false
+	}
+	cfgGeneric, ok := cfgBridge[netlabel.GenericData].(options.Generic)
+	if !ok {
+		return false
+	}
+	enabled, ok := cfgGeneric["EnableIPTables"].(bool)
+	if !ok {
+		// unless user explicitly stated, assume iptable is enabled
+		enabled = true
+	}
+	return enabled
+}

firewall_linux.go

@@ -7,21 +7,25 @@ import (
 
 const userChain = "DOCKER-USER"
 
-func (c *controller) arrangeUserFilterRule() {
-	c.Lock()
-	arrangeUserFilterRule()
-	c.Unlock()
-	iptables.OnReloaded(func() {
-		c.Lock()
-		arrangeUserFilterRule()
-		c.Unlock()
-	})
+var (
+	ctrl *controller = nil
+)
+
+func setupArrangeUserFilterRule(c *controller) {
+	ctrl = c
+	iptables.OnReloaded(arrangeUserFilterRule)
 }
 
 // This chain allow users to configure firewall policies in a way that persists
 // docker operations/restarts. Docker will not delete or modify any pre-existing
 // rules from the DOCKER-USER filter chain.
+// Note once DOCKER-USER chain is created, docker engine does not remove it when
+// IPTableForwarding is disabled, because it contains rules configured by user that
+// are beyond docker engine's control.
 func arrangeUserFilterRule() {
+	if ctrl == nil || !ctrl.iptablesEnabled() {
+		return
+	}
 	_, err := iptables.NewChain(userChain, iptables.Filter, false)
 	if err != nil {
 		logrus.Warnf("Failed to create %s chain: %v", userChain, err)

firewall_others.go

@@ -2,5 +2,5 @@
 
 package libnetwork
 
-func (c *controller) arrangeUserFilterRule() {
-}
+func setupArrangeUserFilterRule(c *controller) {}
+func arrangeUserFilterRule()                   {}

firewall_test.go

@@ -0,0 +1,97 @@
+package libnetwork
+
+import (
+	"fmt"
+	"gotest.tools/assert"
+	"strings"
+	"testing"
+
+	"github.com/docker/libnetwork/iptables"
+	"github.com/docker/libnetwork/netlabel"
+	"github.com/docker/libnetwork/options"
+)
+
+const (
+	fwdChainName = "FORWARD"
+	usrChainName = userChain
+)
+
+func TestUserChain(t *testing.T) {
+	nc, err := New()
+	assert.NilError(t, err)
+
+	tests := []struct {
+		iptables  bool
+		insert    bool // insert other rules to FORWARD
+		fwdChain  []string
+		userChain []string
+	}{
+		{
+			iptables: false,
+			insert:   false,
+			fwdChain: []string{"-P FORWARD ACCEPT"},
+		},
+		{
+			iptables:  true,
+			insert:    false,
+			fwdChain:  []string{"-P FORWARD ACCEPT", "-A FORWARD -j DOCKER-USER"},
+			userChain: []string{"-N DOCKER-USER", "-A DOCKER-USER -j RETURN"},
+		},
+		{
+			iptables:  true,
+			insert:    true,
+			fwdChain:  []string{"-P FORWARD ACCEPT", "-A FORWARD -j DOCKER-USER", "-A FORWARD -j DROP"},
+			userChain: []string{"-N DOCKER-USER", "-A DOCKER-USER -j RETURN"},
+		},
+	}
+
+	resetIptables(t)
+	for _, tc := range tests {
+		tc := tc
+		t.Run(fmt.Sprintf("iptables=%v,insert=%v", tc.iptables, tc.insert), func(t *testing.T) {
+			c := nc.(*controller)
+			c.cfg.Daemon.DriverCfg["bridge"] = map[string]interface{}{
+				netlabel.GenericData: options.Generic{
+					"EnableIPTables": tc.iptables,
+				},
+			}
+
+			// init. condition, FORWARD chain empty DOCKER-USER not exist
+			assert.DeepEqual(t, getRules(t, fwdChainName), []string{"-P FORWARD ACCEPT"})
+
+			if tc.insert {
+				_, err = iptables.Raw("-A", fwdChainName, "-j", "DROP")
+				assert.NilError(t, err)
+			}
+			arrangeUserFilterRule()
+
+			assert.DeepEqual(t, getRules(t, fwdChainName), tc.fwdChain)
+			if tc.userChain != nil {
+				assert.DeepEqual(t, getRules(t, usrChainName), tc.userChain)
+			} else {
+				_, err := iptables.Raw("-S", usrChainName)
+				assert.Assert(t, err != nil, "chain %v: created unexpectedly", usrChainName)
+			}
+		})
+		resetIptables(t)
+	}
+}
+
+func getRules(t *testing.T, chain string) []string {
+	t.Helper()
+	output, err := iptables.Raw("-S", chain)
+	assert.NilError(t, err, "chain %s: failed to get rules", chain)
+
+	rules := strings.Split(string(output), "\n")
+	if len(rules) > 0 {
+		rules = rules[:len(rules)-1]
+	}
+	return rules
+}
+
+func resetIptables(t *testing.T) {
+	t.Helper()
+	_, err := iptables.Raw("-F", fwdChainName)
+	assert.NilError(t, err)
+	_ = iptables.RemoveExistingChain(usrChainName, "")
+}