Browse code

DOCKER-USER chain not created when IPTableEnable=false.

This fix addresses https://docker.atlassian.net/browse/ENGCORE-1115
Expected behaviors upon docker engine restarts:
1. IPTableEnable=true, DOCKER-USER chain present
-- no change to DOCKER-USER chain
2. IPTableEnable=true, DOCKER-USER chain not present
-- DOCKER-USER chain created and inserted top of FORWARD
chain.
3. IPTableEnable=false, DOCKER-USER chain present
-- no change to DOCKER-USER chain
the rational is that DOCKER-USER is populated
and may be used by end-user for purpose other than
filtering docker container traffic. Thus even if
IPTableEnable=false, docker engine does not touch
pre-existing DOCKER-USER chain.
4. IPTableEnable=false, DOCKER-USER chain not present
-- DOCKER-USER chain is not created.

Signed-off-by: Su Wang <su.wang@docker.com>

Su Wang authored on 2019/10/13 13:54:49
Showing 4 changed files
... ...
@@ -67,6 +67,7 @@ import (
67 67
 	"github.com/docker/libnetwork/hostdiscovery"
68 68
 	"github.com/docker/libnetwork/ipamapi"
69 69
 	"github.com/docker/libnetwork/netlabel"
70
+	"github.com/docker/libnetwork/options"
70 71
 	"github.com/docker/libnetwork/osl"
71 72
 	"github.com/docker/libnetwork/types"
72 73
 	"github.com/pkg/errors"
... ...
@@ -252,6 +253,7 @@ func New(cfgOptions ...config.Option) (NetworkController, error) {
252 252
 		return nil, err
253 253
 	}
254 254
 
255
+	setupArrangeUserFilterRule(c)
255 256
 	return c, nil
256 257
 }
257 258
 
... ...
@@ -909,8 +911,7 @@ addToStore:
909 909
 		arrangeIngressFilterRule()
910 910
 		c.Unlock()
911 911
 	}
912
-
913
-	c.arrangeUserFilterRule()
912
+	arrangeUserFilterRule()
914 913
 
915 914
 	return network, nil
916 915
 }
... ...
@@ -1363,3 +1364,27 @@ func (c *controller) IsDiagnosticEnabled() bool {
1363 1363
 	defer c.Unlock()
1364 1364
 	return c.DiagnosticServer.IsDiagnosticEnabled()
1365 1365
 }
1366
+
1367
+func (c *controller) iptablesEnabled() bool {
1368
+	c.Lock()
1369
+	defer c.Unlock()
1370
+
1371
+	if c.cfg == nil {
1372
+		return false
1373
+	}
1374
+	// parse map cfg["bridge"]["generic"]["EnableIPTable"]
1375
+	cfgBridge, ok := c.cfg.Daemon.DriverCfg["bridge"].(map[string]interface{})
1376
+	if !ok {
1377
+		return false
1378
+	}
1379
+	cfgGeneric, ok := cfgBridge[netlabel.GenericData].(options.Generic)
1380
+	if !ok {
1381
+		return false
1382
+	}
1383
+	enabled, ok := cfgGeneric["EnableIPTables"].(bool)
1384
+	if !ok {
1385
+		// unless user explicitly stated, assume iptable is enabled
1386
+		enabled = true
1387
+	}
1388
+	return enabled
1389
+}
... ...
@@ -7,21 +7,25 @@ import (
7 7
 
8 8
 const userChain = "DOCKER-USER"
9 9
 
10
-func (c *controller) arrangeUserFilterRule() {
11
-	c.Lock()
12
-	arrangeUserFilterRule()
13
-	c.Unlock()
14
-	iptables.OnReloaded(func() {
15
-		c.Lock()
16
-		arrangeUserFilterRule()
17
-		c.Unlock()
18
-	})
10
+var (
11
+	ctrl *controller = nil
12
+)
13
+
14
+func setupArrangeUserFilterRule(c *controller) {
15
+	ctrl = c
16
+	iptables.OnReloaded(arrangeUserFilterRule)
19 17
 }
20 18
 
21 19
 // This chain allow users to configure firewall policies in a way that persists
22 20
 // docker operations/restarts. Docker will not delete or modify any pre-existing
23 21
 // rules from the DOCKER-USER filter chain.
22
+// Note once DOCKER-USER chain is created, docker engine does not remove it when
23
+// IPTableForwarding is disabled, because it contains rules configured by user that
24
+// are beyond docker engine's control.
24 25
 func arrangeUserFilterRule() {
26
+	if ctrl == nil || !ctrl.iptablesEnabled() {
27
+		return
28
+	}
25 29
 	_, err := iptables.NewChain(userChain, iptables.Filter, false)
26 30
 	if err != nil {
27 31
 		logrus.Warnf("Failed to create %s chain: %v", userChain, err)
... ...
@@ -2,5 +2,5 @@
2 2
 
3 3
 package libnetwork
4 4
 
5
-func (c *controller) arrangeUserFilterRule() {
6
-}
5
+func setupArrangeUserFilterRule(c *controller) {}
6
+func arrangeUserFilterRule()                   {}
7 7
new file mode 100644
... ...
@@ -0,0 +1,97 @@
0
+package libnetwork
1
+
2
+import (
3
+	"fmt"
4
+	"gotest.tools/assert"
5
+	"strings"
6
+	"testing"
7
+
8
+	"github.com/docker/libnetwork/iptables"
9
+	"github.com/docker/libnetwork/netlabel"
10
+	"github.com/docker/libnetwork/options"
11
+)
12
+
13
+const (
14
+	fwdChainName = "FORWARD"
15
+	usrChainName = userChain
16
+)
17
+
18
+func TestUserChain(t *testing.T) {
19
+	nc, err := New()
20
+	assert.NilError(t, err)
21
+
22
+	tests := []struct {
23
+		iptables  bool
24
+		insert    bool // insert other rules to FORWARD
25
+		fwdChain  []string
26
+		userChain []string
27
+	}{
28
+		{
29
+			iptables: false,
30
+			insert:   false,
31
+			fwdChain: []string{"-P FORWARD ACCEPT"},
32
+		},
33
+		{
34
+			iptables:  true,
35
+			insert:    false,
36
+			fwdChain:  []string{"-P FORWARD ACCEPT", "-A FORWARD -j DOCKER-USER"},
37
+			userChain: []string{"-N DOCKER-USER", "-A DOCKER-USER -j RETURN"},
38
+		},
39
+		{
40
+			iptables:  true,
41
+			insert:    true,
42
+			fwdChain:  []string{"-P FORWARD ACCEPT", "-A FORWARD -j DOCKER-USER", "-A FORWARD -j DROP"},
43
+			userChain: []string{"-N DOCKER-USER", "-A DOCKER-USER -j RETURN"},
44
+		},
45
+	}
46
+
47
+	resetIptables(t)
48
+	for _, tc := range tests {
49
+		tc := tc
50
+		t.Run(fmt.Sprintf("iptables=%v,insert=%v", tc.iptables, tc.insert), func(t *testing.T) {
51
+			c := nc.(*controller)
52
+			c.cfg.Daemon.DriverCfg["bridge"] = map[string]interface{}{
53
+				netlabel.GenericData: options.Generic{
54
+					"EnableIPTables": tc.iptables,
55
+				},
56
+			}
57
+
58
+			// init. condition, FORWARD chain empty DOCKER-USER not exist
59
+			assert.DeepEqual(t, getRules(t, fwdChainName), []string{"-P FORWARD ACCEPT"})
60
+
61
+			if tc.insert {
62
+				_, err = iptables.Raw("-A", fwdChainName, "-j", "DROP")
63
+				assert.NilError(t, err)
64
+			}
65
+			arrangeUserFilterRule()
66
+
67
+			assert.DeepEqual(t, getRules(t, fwdChainName), tc.fwdChain)
68
+			if tc.userChain != nil {
69
+				assert.DeepEqual(t, getRules(t, usrChainName), tc.userChain)
70
+			} else {
71
+				_, err := iptables.Raw("-S", usrChainName)
72
+				assert.Assert(t, err != nil, "chain %v: created unexpectedly", usrChainName)
73
+			}
74
+		})
75
+		resetIptables(t)
76
+	}
77
+}
78
+
79
+func getRules(t *testing.T, chain string) []string {
80
+	t.Helper()
81
+	output, err := iptables.Raw("-S", chain)
82
+	assert.NilError(t, err, "chain %s: failed to get rules", chain)
83
+
84
+	rules := strings.Split(string(output), "\n")
85
+	if len(rules) > 0 {
86
+		rules = rules[:len(rules)-1]
87
+	}
88
+	return rules
89
+}
90
+
91
+func resetIptables(t *testing.T) {
92
+	t.Helper()
93
+	_, err := iptables.Raw("-F", fwdChainName)
94
+	assert.NilError(t, err)
95
+	_ = iptables.RemoveExistingChain(usrChainName, "")
96
+}