From cc89b30d35edc02fc598a0b26fe7a1ed002238e4 Mon Sep 17 00:00:00 2001 From: Porjo Date: Fri, 27 Jun 2014 17:29:55 +1000 Subject: [PATCH 1/5] Move per-container forward rules to DOCKER chain Docker-DCO-1.1-Signed-off-by: Ian Bishop (github: porjo) --- daemon/networkdriver/bridge/driver.go | 58 ++++++++++------- daemon/networkdriver/portmapper/mapper.go | 2 +- links/links.go | 4 +- pkg/iptables/iptables.go | 77 ++++++++++++++++++++--- 4 files changed, 109 insertions(+), 32 deletions(-) diff --git a/daemon/networkdriver/bridge/driver.go b/daemon/networkdriver/bridge/driver.go index e0467b6bd..880eaefef 100644 --- a/daemon/networkdriver/bridge/driver.go +++ b/daemon/networkdriver/bridge/driver.go @@ -6,6 +6,7 @@ import ( "net" "os" "strconv" + "strings" "sync" log "github.com/Sirupsen/logrus" @@ -501,35 +502,48 @@ func AllocatePort(job *engine.Job) engine.Status { func LinkContainers(job *engine.Job) engine.Status { var ( action = job.Args[0] + nfAction iptables.Action childIP = job.Getenv("ChildIP") parentIP = job.Getenv("ParentIP") ignoreErrors = job.GetenvBool("IgnoreErrors") ports = job.GetenvList("Ports") + chain = iptables.Chain{} ) - for _, value := range ports { - port := nat.Port(value) - if output, err := iptables.Raw(action, "FORWARD", - "-i", bridgeIface, "-o", bridgeIface, - "-p", port.Proto(), - "-s", parentIP, - "--dport", strconv.Itoa(port.Int()), - "-d", childIP, - "-j", "ACCEPT"); !ignoreErrors && err != nil { - return job.Error(err) - } else if len(output) != 0 { - return job.Errorf("Error toggle iptables forward: %s", output) - } + split := func(p string) (string, string) { + parts := strings.Split(p, "/") + return parts[0], parts[1] + } - if output, err := iptables.Raw(action, "FORWARD", - "-i", bridgeIface, "-o", bridgeIface, - "-p", port.Proto(), - "-s", childIP, - "--sport", strconv.Itoa(port.Int()), - "-d", parentIP, - "-j", "ACCEPT"); !ignoreErrors && err != nil { + switch action { + case "-A": + nfAction = iptables.Append + case "-I": + nfAction = iptables.Insert + case "-D": + nfAction = iptables.Delete + default: + return job.Errorf("Invalid action '%s' specified", action) + } + + ip1 := net.ParseIP(parentIP) + if ip1 == nil { + return job.Errorf("parent IP '%s' is invalid", parentIP) + } + ip2 := net.ParseIP(childIP) + if ip2 == nil { + return job.Errorf("child IP '%s' is invalid", childIP) + } + + chain.Name = "DOCKER" + chain.Bridge = bridgeIface + for _, p := range ports { + portStr, proto := split(p) + port, err := strconv.Atoi(portStr) + if !ignoreErrors && err != nil { + return job.Errorf("port '%s' is invalid", portStr) + } + if err := chain.Link(nfAction, ip1, ip2, port, proto); !ignoreErrors && err != nil { return job.Error(err) - } else if len(output) != 0 { - return job.Errorf("Error toggle iptables forward: %s", output) } } return engine.StatusOK diff --git a/daemon/networkdriver/portmapper/mapper.go b/daemon/networkdriver/portmapper/mapper.go index 4bf8cd142..9f2ca5a75 100644 --- a/daemon/networkdriver/portmapper/mapper.go +++ b/daemon/networkdriver/portmapper/mapper.go @@ -93,7 +93,7 @@ func Map(container net.Addr, hostIP net.IP, hostPort int) (host net.Addr, err er } containerIP, containerPort := getIPAndPort(m.container) - if err := forward(iptables.Add, m.proto, hostIP, allocatedHostPort, containerIP.String(), containerPort); err != nil { + if err := forward(iptables.Append, m.proto, hostIP, allocatedHostPort, containerIP.String(), containerPort); err != nil { return nil, err } diff --git a/links/links.go b/links/links.go index fc4d95ab0..ab03eadf5 100644 --- a/links/links.go +++ b/links/links.go @@ -138,7 +138,8 @@ func (l *Link) getDefaultPort() *nat.Port { } func (l *Link) Enable() error { - if err := l.toggle("-I", false); err != nil { + // -A == iptables append flag + if err := l.toggle("-A", false); err != nil { return err } l.IsEnabled = true @@ -148,6 +149,7 @@ func (l *Link) Enable() error { func (l *Link) Disable() { // We do not care about errors here because the link may not // exist in iptables + // -D == iptables delete flag l.toggle("-D", true) l.IsEnabled = false diff --git a/pkg/iptables/iptables.go b/pkg/iptables/iptables.go index b783347fa..a7d216a97 100644 --- a/pkg/iptables/iptables.go +++ b/pkg/iptables/iptables.go @@ -15,8 +15,9 @@ import ( type Action string const ( - Add Action = "-A" + Append Action = "-A" Delete Action = "-D" + Insert Action = "-I" ) var ( @@ -54,10 +55,10 @@ func NewChain(name, bridge string) (*Chain, error) { Bridge: bridge, } - if err := chain.Prerouting(Add, "-m", "addrtype", "--dst-type", "LOCAL"); err != nil { + if err := chain.Prerouting(Append, "-m", "addrtype", "--dst-type", "LOCAL"); err != nil { return nil, fmt.Errorf("Failed to inject docker in PREROUTING chain: %s", err) } - if err := chain.Output(Add, "-m", "addrtype", "--dst-type", "LOCAL", "!", "--dst", "127.0.0.0/8"); err != nil { + if err := chain.Output(Append, "-m", "addrtype", "--dst-type", "LOCAL", "!", "--dst", "127.0.0.0/8"); err != nil { return nil, fmt.Errorf("Failed to inject docker in OUTPUT chain: %s", err) } return chain, nil @@ -78,7 +79,7 @@ func (c *Chain) Forward(action Action, ip net.IP, port int, proto, dest_addr str // value" by both iptables and ip6tables. daddr = "0/0" } - if output, err := Raw("-t", "nat", fmt.Sprint(action), c.Name, + if output, err := Raw("-t", "nat", string(action), c.Name, "-p", proto, "-d", daddr, "--dport", strconv.Itoa(port), @@ -90,11 +91,13 @@ func (c *Chain) Forward(action Action, ip net.IP, port int, proto, dest_addr str return &ChainError{Chain: "FORWARD", Output: output} } - fAction := action - if fAction == Add { - fAction = "-I" + if action != Delete { + if err := c.createForwardChain(); err != nil { + return err + } } - if output, err := Raw(string(fAction), "FORWARD", + + if output, err := Raw(string(action), c.Name, "!", "-i", c.Bridge, "-o", c.Bridge, "-p", proto, @@ -109,6 +112,39 @@ func (c *Chain) Forward(action Action, ip net.IP, port int, proto, dest_addr str return nil } +func (c *Chain) Link(action Action, ip1, ip2 net.IP, port int, proto string) error { + if action != Delete { + if err := c.createForwardChain(); err != nil { + return err + } + } + if output, err := Raw(string(action), c.Name, + "-i", c.Bridge, "-o", c.Bridge, + "-p", proto, + "-s", ip1.String(), + "--dport", strconv.Itoa(port), + "-d", ip2.String(), + "-j", "ACCEPT"); err != nil { + return err + } else if len(output) != 0 { + return fmt.Errorf("Error toggle iptables forward: %s", output) + } + + if output, err := Raw(string(action), c.Name, + "-i", c.Bridge, "-o", c.Bridge, + "-p", proto, + "-s", ip2.String(), + "--dport", strconv.Itoa(port), + "-d", ip1.String(), + "-j", "ACCEPT"); err != nil { + return err + } else if len(output) != 0 { + return fmt.Errorf("Error toggle iptables forward: %s", output) + } + + return nil +} + func (c *Chain) Prerouting(action Action, args ...string) error { a := append(nat, fmt.Sprint(action), "PREROUTING") if len(args) > 0 { @@ -199,3 +235,28 @@ func Raw(args ...string) ([]byte, error) { return output, err } + +func (c *Chain) createForwardChain() error { + // Add chain if doesn't exist + if _, err := Raw("-n", "-L", c.Name); err != nil { + output, err := Raw("-N", c.Name) + if err != nil { + return err + } else if len(output) != 0 { + return fmt.Errorf("Error iptables forward: %s", output) + } + } + // Add linking rule if it doesn't exist + if !Exists("FORWARD", + "-o", c.Bridge, + "-j", c.Name) { + if output2, err := Raw(string(Insert), "FORWARD", + "-o", c.Bridge, + "-j", c.Name); err != nil { + return err + } else if len(output2) != 0 { + return fmt.Errorf("Error iptables forward: %s", output2) + } + } + return nil +} From 2865373894f1532fa725481e8f04db4a5d7a0aa8 Mon Sep 17 00:00:00 2001 From: Porjo Date: Wed, 16 Jul 2014 22:03:01 +1000 Subject: [PATCH 2/5] Create DOCKER forward chain on driver init Docker-DCO-1.1-Signed-off-by: Ian Bishop (github: porjo) --- daemon/networkdriver/bridge/driver.go | 8 +- pkg/iptables/iptables.go | 157 ++++++++++++++------------ 2 files changed, 90 insertions(+), 75 deletions(-) diff --git a/daemon/networkdriver/bridge/driver.go b/daemon/networkdriver/bridge/driver.go index 880eaefef..9272d9ddd 100644 --- a/daemon/networkdriver/bridge/driver.go +++ b/daemon/networkdriver/bridge/driver.go @@ -145,12 +145,16 @@ func InitDriver(job *engine.Job) engine.Status { } // We can always try removing the iptables - if err := iptables.RemoveExistingChain("DOCKER"); err != nil { + if err := iptables.RemoveExistingChain("DOCKER", iptables.Nat); err != nil { return job.Error(err) } if enableIPTables { - chain, err := iptables.NewChain("DOCKER", bridgeIface) + _, err := iptables.NewChain("DOCKER", bridgeIface, iptables.Nat) + if err != nil { + return job.Error(err) + } + chain, err := iptables.NewChain("DOCKER", bridgeIface, iptables.Filter) if err != nil { return job.Error(err) } diff --git a/pkg/iptables/iptables.go b/pkg/iptables/iptables.go index a7d216a97..c58db9c68 100644 --- a/pkg/iptables/iptables.go +++ b/pkg/iptables/iptables.go @@ -13,15 +13,17 @@ import ( ) type Action string +type Table string const ( Append Action = "-A" Delete Action = "-D" Insert Action = "-I" + Nat Table = "nat" + Filter Table = "filter" ) var ( - nat = []string{"-t", "nat"} supportsXlock = false ErrIptablesNotFound = errors.New("Iptables not found") ) @@ -29,6 +31,7 @@ var ( type Chain struct { Name string Bridge string + Table Table } type ChainError struct { @@ -44,33 +47,73 @@ func init() { supportsXlock = exec.Command("iptables", "--wait", "-L", "-n").Run() == nil } -func NewChain(name, bridge string) (*Chain, error) { - if output, err := Raw("-t", "nat", "-N", name); err != nil { - return nil, err - } else if len(output) != 0 { - return nil, fmt.Errorf("Error creating new iptables chain: %s", output) - } - chain := &Chain{ +func NewChain(name, bridge string, table Table) (*Chain, error) { + c := &Chain{ Name: name, Bridge: bridge, + Table: table, } - if err := chain.Prerouting(Append, "-m", "addrtype", "--dst-type", "LOCAL"); err != nil { - return nil, fmt.Errorf("Failed to inject docker in PREROUTING chain: %s", err) + if string(c.Table) == "" { + c.Table = Filter } - if err := chain.Output(Append, "-m", "addrtype", "--dst-type", "LOCAL", "!", "--dst", "127.0.0.0/8"); err != nil { - return nil, fmt.Errorf("Failed to inject docker in OUTPUT chain: %s", err) + + // Add chain if it doesn't exist + if _, err := Raw("-t", string(c.Table), "-n", "-L", c.Name); err != nil { + if output, err := Raw("-t", string(c.Table), "-N", c.Name); err != nil { + return nil, err + } else if len(output) != 0 { + return nil, fmt.Errorf("Could not create %s/%s chain: %s", c.Table, c.Name, output) + } } - return chain, nil + + switch table { + case Nat: + preroute := []string{ + "-m", "addrtype", + "--dst-type", "LOCAL"} + if !Exists(preroute...) { + if err := c.Prerouting(Append, preroute...); err != nil { + return nil, fmt.Errorf("Failed to inject docker in PREROUTING chain: %s", err) + } + } + output := []string{ + "-m", "addrtype", + "--dst-type", "LOCAL", + "!", "--dst", "127.0.0.0/8"} + if !Exists(output...) { + if err := c.Output(Append, output...); err != nil { + return nil, fmt.Errorf("Failed to inject docker in OUTPUT chain: %s", err) + } + } + case Filter: + link := []string{"FORWARD", + "-o", c.Bridge, + "-j", c.Name} + if !Exists(link...) { + insert := append([]string{string(Insert)}, link...) + if output, err := Raw(insert...); err != nil { + return nil, err + } else if len(output) != 0 { + return nil, fmt.Errorf("Could not create linking rule to %s/%s: %s", c.Table, c.Name, output) + } + } + } + return c, nil } -func RemoveExistingChain(name string) error { - chain := &Chain{ - Name: name, +func RemoveExistingChain(name string, table Table) error { + c := &Chain{ + Name: name, + Table: table, } - return chain.Remove() + if string(c.Table) == "" { + c.Table = Filter + } + return c.Remove() } +// Add forwarding rule to 'filter' table and corresponding nat rule to 'nat' table func (c *Chain) Forward(action Action, ip net.IP, port int, proto, dest_addr string, dest_port int) error { daddr := ip.String() if ip.IsUnspecified() { @@ -79,7 +122,7 @@ func (c *Chain) Forward(action Action, ip net.IP, port int, proto, dest_addr str // value" by both iptables and ip6tables. daddr = "0/0" } - if output, err := Raw("-t", "nat", string(action), c.Name, + if output, err := Raw("-t", string(Nat), string(action), c.Name, "-p", proto, "-d", daddr, "--dport", strconv.Itoa(port), @@ -91,13 +134,7 @@ func (c *Chain) Forward(action Action, ip net.IP, port int, proto, dest_addr str return &ChainError{Chain: "FORWARD", Output: output} } - if action != Delete { - if err := c.createForwardChain(); err != nil { - return err - } - } - - if output, err := Raw(string(action), c.Name, + if output, err := Raw("-t", string(Filter), string(action), c.Name, "!", "-i", c.Bridge, "-o", c.Bridge, "-p", proto, @@ -112,13 +149,10 @@ func (c *Chain) Forward(action Action, ip net.IP, port int, proto, dest_addr str return nil } +// Add reciprocal ACCEPT rule for two supplied IP addresses. +// Traffic is allowed from ip1 to ip2 and vice-versa func (c *Chain) Link(action Action, ip1, ip2 net.IP, port int, proto string) error { - if action != Delete { - if err := c.createForwardChain(); err != nil { - return err - } - } - if output, err := Raw(string(action), c.Name, + if output, err := Raw("-t", string(Filter), string(action), c.Name, "-i", c.Bridge, "-o", c.Bridge, "-p", proto, "-s", ip1.String(), @@ -127,10 +161,9 @@ func (c *Chain) Link(action Action, ip1, ip2 net.IP, port int, proto string) err "-j", "ACCEPT"); err != nil { return err } else if len(output) != 0 { - return fmt.Errorf("Error toggle iptables forward: %s", output) + return fmt.Errorf("Error iptables forward: %s", output) } - - if output, err := Raw(string(action), c.Name, + if output, err := Raw("-t", string(Filter), string(action), c.Name, "-i", c.Bridge, "-o", c.Bridge, "-p", proto, "-s", ip2.String(), @@ -139,14 +172,14 @@ func (c *Chain) Link(action Action, ip1, ip2 net.IP, port int, proto string) err "-j", "ACCEPT"); err != nil { return err } else if len(output) != 0 { - return fmt.Errorf("Error toggle iptables forward: %s", output) + return fmt.Errorf("Error iptables forward: %s", output) } - return nil } +// Add linking rule to nat/PREROUTING chain. func (c *Chain) Prerouting(action Action, args ...string) error { - a := append(nat, fmt.Sprint(action), "PREROUTING") + a := []string{"-t", string(Nat), string(action), "PREROUTING"} if len(args) > 0 { a = append(a, args...) } @@ -158,8 +191,9 @@ func (c *Chain) Prerouting(action Action, args ...string) error { return nil } +// Add linking rule to an OUTPUT chain func (c *Chain) Output(action Action, args ...string) error { - a := append(nat, fmt.Sprint(action), "OUTPUT") + a := []string{"-t", string(c.Table), string(action), "OUTPUT"} if len(args) > 0 { a = append(a, args...) } @@ -172,21 +206,22 @@ func (c *Chain) Output(action Action, args ...string) error { } func (c *Chain) Remove() error { - // Ignore errors - This could mean the chains were never set up - c.Prerouting(Delete, "-m", "addrtype", "--dst-type", "LOCAL") - c.Output(Delete, "-m", "addrtype", "--dst-type", "LOCAL", "!", "--dst", "127.0.0.0/8") - c.Output(Delete, "-m", "addrtype", "--dst-type", "LOCAL") // Created in versions <= 0.1.6 + if c.Table == Nat { + // Ignore errors - This could mean the chains were never set up + c.Prerouting(Delete, "-m", "addrtype", "--dst-type", "LOCAL") + c.Output(Delete, "-m", "addrtype", "--dst-type", "LOCAL", "!", "--dst", "127.0.0.0/8") + c.Output(Delete, "-m", "addrtype", "--dst-type", "LOCAL") // Created in versions <= 0.1.6 - c.Prerouting(Delete) - c.Output(Delete) - - Raw("-t", "nat", "-F", c.Name) - Raw("-t", "nat", "-X", c.Name) + c.Prerouting(Delete) + c.Output(Delete) + Raw("-t", string(Nat), "-F", c.Name) + Raw("-t", string(Nat), "-X", c.Name) + } return nil } -// Check if an existing rule exists +// Check if a rule exists func Exists(args ...string) bool { // iptables -C, --check option was added in v.1.4.11 // http://ftp.netfilter.org/pub/iptables/changes-iptables-1.4.11.txt @@ -211,6 +246,7 @@ func Exists(args ...string) bool { ) } +// Call 'iptables' system command, passing supplied arguments func Raw(args ...string) ([]byte, error) { path, err := exec.LookPath("iptables") if err != nil { @@ -235,28 +271,3 @@ func Raw(args ...string) ([]byte, error) { return output, err } - -func (c *Chain) createForwardChain() error { - // Add chain if doesn't exist - if _, err := Raw("-n", "-L", c.Name); err != nil { - output, err := Raw("-N", c.Name) - if err != nil { - return err - } else if len(output) != 0 { - return fmt.Errorf("Error iptables forward: %s", output) - } - } - // Add linking rule if it doesn't exist - if !Exists("FORWARD", - "-o", c.Bridge, - "-j", c.Name) { - if output2, err := Raw(string(Insert), "FORWARD", - "-o", c.Bridge, - "-j", c.Name); err != nil { - return err - } else if len(output2) != 0 { - return fmt.Errorf("Error iptables forward: %s", output2) - } - } - return nil -} From 0da92633b4161ed1f8babe5ec4a9fe98257d34b5 Mon Sep 17 00:00:00 2001 From: Ian Bishop Date: Sat, 15 Nov 2014 11:36:38 +1000 Subject: [PATCH 3/5] Create tests for pkg/iptables Docker-DCO-1.1-Signed-off-by: Ian Bishop (github: porjo) --- daemon/networkdriver/bridge/driver.go | 16 +- daemon/networkdriver/bridge/driver_test.go | 41 +++++ integration-cli/docker_cli_links_test.go | 4 +- integration-cli/docker_cli_run_test.go | 2 +- pkg/iptables/iptables.go | 30 ++- pkg/iptables/iptables_test.go | 204 +++++++++++++++++++++ 6 files changed, 272 insertions(+), 25 deletions(-) create mode 100644 pkg/iptables/iptables_test.go diff --git a/daemon/networkdriver/bridge/driver.go b/daemon/networkdriver/bridge/driver.go index 9272d9ddd..228bd479f 100644 --- a/daemon/networkdriver/bridge/driver.go +++ b/daemon/networkdriver/bridge/driver.go @@ -5,8 +5,6 @@ import ( "io/ioutil" "net" "os" - "strconv" - "strings" "sync" log "github.com/Sirupsen/logrus" @@ -513,10 +511,6 @@ func LinkContainers(job *engine.Job) engine.Status { ports = job.GetenvList("Ports") chain = iptables.Chain{} ) - split := func(p string) (string, string) { - parts := strings.Split(p, "/") - return parts[0], parts[1] - } switch action { case "-A": @@ -541,13 +535,11 @@ func LinkContainers(job *engine.Job) engine.Status { chain.Name = "DOCKER" chain.Bridge = bridgeIface for _, p := range ports { - portStr, proto := split(p) - port, err := strconv.Atoi(portStr) - if !ignoreErrors && err != nil { - return job.Errorf("port '%s' is invalid", portStr) - } - if err := chain.Link(nfAction, ip1, ip2, port, proto); !ignoreErrors && err != nil { + port := nat.Port(p) + if err := chain.Link(nfAction, ip1, ip2, port.Int(), port.Proto()); !ignoreErrors && err != nil { + fmt.Print(err) return job.Error(err) + } } return engine.StatusOK diff --git a/daemon/networkdriver/bridge/driver_test.go b/daemon/networkdriver/bridge/driver_test.go index 1bda2f437..02bea9ce1 100644 --- a/daemon/networkdriver/bridge/driver_test.go +++ b/daemon/networkdriver/bridge/driver_test.go @@ -7,6 +7,7 @@ import ( "github.com/docker/docker/daemon/networkdriver/portmapper" "github.com/docker/docker/engine" + "github.com/docker/docker/pkg/iptables" ) func init() { @@ -118,3 +119,43 @@ func TestMacAddrGeneration(t *testing.T) { t.Fatal("Non-unique MAC address") } } + +func TestLinkContainers(t *testing.T) { + eng := engine.New() + eng.Logging = false + + // Init driver + job := eng.Job("initdriver") + if res := InitDriver(job); res != engine.StatusOK { + t.Fatal("Failed to initialize network driver") + } + + // Allocate interface + job = eng.Job("allocate_interface", "container_id") + if res := Allocate(job); res != engine.StatusOK { + t.Fatal("Failed to allocate network interface") + } + + job.Args[0] = "-I" + + job.Setenv("ChildIP", "172.17.0.2") + job.Setenv("ParentIP", "172.17.0.1") + job.SetenvBool("IgnoreErrors", false) + job.SetenvList("Ports", []string{"1234"}) + + bridgeIface = "lo" + _, err := iptables.NewChain("DOCKER", bridgeIface, iptables.Filter) + if err != nil { + t.Fatal(err) + } + + if res := LinkContainers(job); res != engine.StatusOK { + t.Fatalf("LinkContainers failed") + } + + // flush rules + if _, err = iptables.Raw([]string{"-F", "DOCKER"}...); err != nil { + t.Fatal(err) + } + +} diff --git a/integration-cli/docker_cli_links_test.go b/integration-cli/docker_cli_links_test.go index 5b81b7fec..49d46ed94 100644 --- a/integration-cli/docker_cli_links_test.go +++ b/integration-cli/docker_cli_links_test.go @@ -83,8 +83,8 @@ func TestLinksIpTablesRulesWhenLinkAndUnlink(t *testing.T) { childIP := findContainerIP(t, "child") parentIP := findContainerIP(t, "parent") - sourceRule := []string{"FORWARD", "-i", "docker0", "-o", "docker0", "-p", "tcp", "-s", childIP, "--sport", "80", "-d", parentIP, "-j", "ACCEPT"} - destinationRule := []string{"FORWARD", "-i", "docker0", "-o", "docker0", "-p", "tcp", "-s", parentIP, "--dport", "80", "-d", childIP, "-j", "ACCEPT"} + sourceRule := []string{"DOCKER", "-i", "docker0", "-o", "docker0", "-p", "tcp", "-s", childIP, "--sport", "80", "-d", parentIP, "-j", "ACCEPT"} + destinationRule := []string{"DOCKER", "-i", "docker0", "-o", "docker0", "-p", "tcp", "-s", parentIP, "--dport", "80", "-d", childIP, "-j", "ACCEPT"} if !iptables.Exists(sourceRule...) || !iptables.Exists(destinationRule...) { t.Fatal("Iptables rules not found") } diff --git a/integration-cli/docker_cli_run_test.go b/integration-cli/docker_cli_run_test.go index fafb31d89..cb5556bbc 100644 --- a/integration-cli/docker_cli_run_test.go +++ b/integration-cli/docker_cli_run_test.go @@ -2070,7 +2070,7 @@ func TestRunDeallocatePortOnMissingIptablesRule(t *testing.T) { if err != nil { t.Fatal(err) } - iptCmd := exec.Command("iptables", "-D", "FORWARD", "-d", fmt.Sprintf("%s/32", ip), + iptCmd := exec.Command("iptables", "-D", "DOCKER", "-d", fmt.Sprintf("%s/32", ip), "!", "-i", "docker0", "-o", "docker0", "-p", "tcp", "-m", "tcp", "--dport", "23", "-j", "ACCEPT") out, _, err = runCommandWithOutput(iptCmd) if err != nil { diff --git a/pkg/iptables/iptables.go b/pkg/iptables/iptables.go index c58db9c68..90ccbeff5 100644 --- a/pkg/iptables/iptables.go +++ b/pkg/iptables/iptables.go @@ -114,7 +114,7 @@ func RemoveExistingChain(name string, table Table) error { } // Add forwarding rule to 'filter' table and corresponding nat rule to 'nat' table -func (c *Chain) Forward(action Action, ip net.IP, port int, proto, dest_addr string, dest_port int) error { +func (c *Chain) Forward(action Action, ip net.IP, port int, proto, destAddr string, destPort int) error { daddr := ip.String() if ip.IsUnspecified() { // iptables interprets "0.0.0.0" as "0.0.0.0/32", whereas we @@ -128,7 +128,7 @@ func (c *Chain) Forward(action Action, ip net.IP, port int, proto, dest_addr str "--dport", strconv.Itoa(port), "!", "-i", c.Bridge, "-j", "DNAT", - "--to-destination", net.JoinHostPort(dest_addr, strconv.Itoa(dest_port))); err != nil { + "--to-destination", net.JoinHostPort(destAddr, strconv.Itoa(destPort))); err != nil { return err } else if len(output) != 0 { return &ChainError{Chain: "FORWARD", Output: output} @@ -138,14 +138,25 @@ func (c *Chain) Forward(action Action, ip net.IP, port int, proto, dest_addr str "!", "-i", c.Bridge, "-o", c.Bridge, "-p", proto, - "-d", dest_addr, - "--dport", strconv.Itoa(dest_port), + "-d", destAddr, + "--dport", strconv.Itoa(destPort), "-j", "ACCEPT"); err != nil { return err } else if len(output) != 0 { return &ChainError{Chain: "FORWARD", Output: output} } + if output, err := Raw("-t", string(Nat), string(action), "POSTROUTING", + "-p", proto, + "-s", destAddr, + "-d", destAddr, + "--dport", strconv.Itoa(destPort), + "-j", "MASQUERADE"); err != nil { + return err + } else if len(output) != 0 { + return &ChainError{Chain: "FORWARD", Output: output} + } + return nil } @@ -156,8 +167,8 @@ func (c *Chain) Link(action Action, ip1, ip2 net.IP, port int, proto string) err "-i", c.Bridge, "-o", c.Bridge, "-p", proto, "-s", ip1.String(), - "--dport", strconv.Itoa(port), "-d", ip2.String(), + "--dport", strconv.Itoa(port), "-j", "ACCEPT"); err != nil { return err } else if len(output) != 0 { @@ -167,8 +178,8 @@ func (c *Chain) Link(action Action, ip1, ip2 net.IP, port int, proto string) err "-i", c.Bridge, "-o", c.Bridge, "-p", proto, "-s", ip2.String(), - "--dport", strconv.Itoa(port), "-d", ip1.String(), + "--sport", strconv.Itoa(port), "-j", "ACCEPT"); err != nil { return err } else if len(output) != 0 { @@ -206,18 +217,17 @@ func (c *Chain) Output(action Action, args ...string) error { } func (c *Chain) Remove() error { + // Ignore errors - This could mean the chains were never set up if c.Table == Nat { - // Ignore errors - This could mean the chains were never set up c.Prerouting(Delete, "-m", "addrtype", "--dst-type", "LOCAL") c.Output(Delete, "-m", "addrtype", "--dst-type", "LOCAL", "!", "--dst", "127.0.0.0/8") c.Output(Delete, "-m", "addrtype", "--dst-type", "LOCAL") // Created in versions <= 0.1.6 c.Prerouting(Delete) c.Output(Delete) - - Raw("-t", string(Nat), "-F", c.Name) - Raw("-t", string(Nat), "-X", c.Name) } + Raw("-t", string(c.Table), "-F", c.Name) + Raw("-t", string(c.Table), "-X", c.Name) return nil } diff --git a/pkg/iptables/iptables_test.go b/pkg/iptables/iptables_test.go new file mode 100644 index 000000000..8aaf429c9 --- /dev/null +++ b/pkg/iptables/iptables_test.go @@ -0,0 +1,204 @@ +package iptables + +import ( + "net" + "os/exec" + "strconv" + "strings" + "testing" +) + +const chainName = "DOCKERTEST" + +var natChain *Chain +var filterChain *Chain + +func TestNewChain(t *testing.T) { + var err error + + natChain, err = NewChain(chainName, "lo", Nat) + if err != nil { + t.Fatal(err) + } + + filterChain, err = NewChain(chainName, "lo", Filter) + if err != nil { + t.Fatal(err) + } +} + +func TestForward(t *testing.T) { + ip := net.ParseIP("192.168.1.1") + port := 1234 + dstAddr := "172.17.0.1" + dstPort := 4321 + proto := "tcp" + + err := natChain.Forward(Insert, ip, port, proto, dstAddr, dstPort) + if err != nil { + t.Fatal(err) + } + + dnatRule := []string{natChain.Name, + "-t", string(natChain.Table), + "!", "-i", filterChain.Bridge, + "-d", ip.String(), + "-p", proto, + "--dport", strconv.Itoa(port), + "-j", "DNAT", + "--to-destination", dstAddr + ":" + strconv.Itoa(dstPort), + } + + if !Exists(dnatRule...) { + t.Fatalf("DNAT rule does not exist") + } + + filterRule := []string{filterChain.Name, + "-t", string(filterChain.Table), + "!", "-i", filterChain.Bridge, + "-o", filterChain.Bridge, + "-d", dstAddr, + "-p", proto, + "--dport", strconv.Itoa(dstPort), + "-j", "ACCEPT", + } + + if !Exists(filterRule...) { + t.Fatalf("filter rule does not exist") + } + + masqRule := []string{"POSTROUTING", + "-t", string(natChain.Table), + "-d", dstAddr, + "-s", dstAddr, + "-p", proto, + "--dport", strconv.Itoa(dstPort), + "-j", "MASQUERADE", + } + + if !Exists(masqRule...) { + t.Fatalf("MASQUERADE rule does not exist") + } +} + +func TestLink(t *testing.T) { + var err error + + ip1 := net.ParseIP("192.168.1.1") + ip2 := net.ParseIP("192.168.1.2") + port := 1234 + proto := "tcp" + + err = filterChain.Link(Append, ip1, ip2, port, proto) + if err != nil { + t.Fatal(err) + } + + rule1 := []string{filterChain.Name, + "-t", string(filterChain.Table), + "-i", filterChain.Bridge, + "-o", filterChain.Bridge, + "-p", proto, + "-s", ip1.String(), + "-d", ip2.String(), + "--dport", strconv.Itoa(port), + "-j", "ACCEPT"} + + if !Exists(rule1...) { + t.Fatalf("rule1 does not exist") + } + + rule2 := []string{filterChain.Name, + "-t", string(filterChain.Table), + "-i", filterChain.Bridge, + "-o", filterChain.Bridge, + "-p", proto, + "-s", ip2.String(), + "-d", ip1.String(), + "--sport", strconv.Itoa(port), + "-j", "ACCEPT"} + + if !Exists(rule2...) { + t.Fatalf("rule2 does not exist") + } +} + +func TestPrerouting(t *testing.T) { + args := []string{ + "-i", "lo", + "-d", "192.168.1.1"} + + err := natChain.Prerouting(Insert, args...) + if err != nil { + t.Fatal(err) + } + + rule := []string{"PREROUTING", + "-t", string(Nat), + "-j", natChain.Name} + + rule = append(rule, args...) + + if !Exists(rule...) { + t.Fatalf("rule does not exist") + } + + delRule := append([]string{"-D"}, rule...) + if _, err = Raw(delRule...); err != nil { + t.Fatal(err) + } +} + +func TestOutput(t *testing.T) { + args := []string{ + "-o", "lo", + "-d", "192.168.1.1"} + + err := natChain.Output(Insert, args...) + if err != nil { + t.Fatal(err) + } + + rule := []string{"OUTPUT", + "-t", string(natChain.Table), + "-j", natChain.Name} + + rule = append(rule, args...) + + if !Exists(rule...) { + t.Fatalf("rule does not exist") + } + + delRule := append([]string{"-D"}, rule...) + if _, err = Raw(delRule...); err != nil { + t.Fatal(err) + } +} + +func TestCleanup(t *testing.T) { + var err error + var rules []byte + + // Cleanup filter/FORWARD first otherwise output of iptables-save is dirty + link := []string{"-t", string(filterChain.Table), + string(Delete), "FORWARD", + "-o", filterChain.Bridge, + "-j", filterChain.Name} + if _, err = Raw(link...); err != nil { + t.Fatal(err) + } + filterChain.Remove() + + err = RemoveExistingChain(chainName, Nat) + if err != nil { + t.Fatal(err) + } + + rules, err = exec.Command("iptables-save").Output() + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(rules), chainName) { + t.Fatalf("Removing chain failed. %s found in iptables-save", chainName) + } +} From 137ceae9138e535f8b3f4e76bda18f6db954c231 Mon Sep 17 00:00:00 2001 From: Ian Bishop Date: Mon, 1 Dec 2014 11:28:25 +1000 Subject: [PATCH 4/5] Update networking.md with new iptables behaviour Docker-DCO-1.1-Signed-off-by: Ian Bishop (github: porjo) --- docs/sources/articles/networking.md | 67 +++++++++++++++++++---------- 1 file changed, 44 insertions(+), 23 deletions(-) diff --git a/docs/sources/articles/networking.md b/docs/sources/articles/networking.md index 6587efc52..4bfbcfdad 100644 --- a/docs/sources/articles/networking.md +++ b/docs/sources/articles/networking.md @@ -184,30 +184,46 @@ running. The options then modify this default configuration. -Whether a container can talk to the world is governed by one main factor. +Whether a container can talk to the world is governed by two factors. -Is the host machine willing to forward IP packets? This is governed -by the `ip_forward` system parameter. Packets can only pass between -containers if this parameter is `1`. Usually you will simply leave -the Docker server at its default setting `--ip-forward=true` and -Docker will go set `ip_forward` to `1` for you when the server -starts up. To check the setting or turn it on manually: - - # Usually not necessary: turning on forwarding, - # on the host where your Docker server is running +1. Is the host machine willing to forward IP packets? This is governed + by the `ip_forward` system parameter. Packets can only pass between + containers if this parameter is `1`. Usually you will simply leave + the Docker server at its default setting `--ip-forward=true` and + Docker will go set `ip_forward` to `1` for you when the server + starts up. To check the setting or turn it on manually: + ``` $ cat /proc/sys/net/ipv4/ip_forward 0 - $ sudo echo 1 > /proc/sys/net/ipv4/ip_forward + $ echo 1 > /proc/sys/net/ipv4/ip_forward $ cat /proc/sys/net/ipv4/ip_forward 1 + ``` -Many using Docker will want `ip_forward` to be on, to at -least make communication *possible* between containers and -the wider world. + Many using Docker will want `ip_forward` to be on, to at + least make communication *possible* between containers and + the wider world. -May also be needed for inter-container communication if you are -in a multiple bridge setup. + May also be needed for inter-container communication if you are + in a multiple bridge setup. + +2. Do your `iptables` allow this particular connection? Docker will + never make changes to your system `iptables` rules if you set + `--iptables=false` when the daemon starts. Otherwise the Docker + server will append forwarding rules to the `DOCKER` filter chain. + +Docker will not delete or modify any pre-existing rules from the `DOCKER` +filter chain. This allows the user to create in advance any rules required +to further restrict access to the containers. + +Docker's forward rules permit all external source IPs by default. To allow +only a specific IP or network to access the containers, insert a negated +rule at the top of the `DOCKER` filter chain. For example, to restrict +external access such that *only* source IP 8.8.8.8 can access the +containers, the following rule could be added: + + $ iptables -I DOCKER -i ext_if ! -s 8.8.8.8 -j DROP ## Communication between containers @@ -222,12 +238,12 @@ system level, by two factors. between them. See the later sections of this document for other possible topologies. -2. Do your `iptables` allow this particular connection to be made? - Docker will never make changes to your system `iptables` rules if - you set `--iptables=false` when the daemon starts. Otherwise the - Docker server will add a default rule to the `FORWARD` chain with a - blanket `ACCEPT` policy if you retain the default `--icc=true`, or - else will set the policy to `DROP` if `--icc=false`. +2. Do your `iptables` allow this particular connection? Docker will never + make changes to your system `iptables` rules if you set + `--iptables=false` when the daemon starts. Otherwise the Docker server + will add a default rule to the `FORWARD` chain with a blanket `ACCEPT` + policy if you retain the default `--icc=true`, or else will set the + policy to `DROP` if `--icc=false`. It is a strategic question whether to leave `--icc=true` or change it to `--icc=false` (on Ubuntu, by editing the `DOCKER_OPTS` variable in @@ -267,6 +283,7 @@ the `FORWARD` chain has a default policy of `ACCEPT` or `DROP`: ... Chain FORWARD (policy ACCEPT) target prot opt source destination + DOCKER all -- 0.0.0.0/0 0.0.0.0/0 DROP all -- 0.0.0.0/0 0.0.0.0/0 ... @@ -278,9 +295,13 @@ the `FORWARD` chain has a default policy of `ACCEPT` or `DROP`: ... Chain FORWARD (policy ACCEPT) target prot opt source destination + DOCKER all -- 0.0.0.0/0 0.0.0.0/0 + DROP all -- 0.0.0.0/0 0.0.0.0/0 + + Chain DOCKER (1 references) + target prot opt source destination ACCEPT tcp -- 172.17.0.2 172.17.0.3 tcp spt:80 ACCEPT tcp -- 172.17.0.3 172.17.0.2 tcp dpt:80 - DROP all -- 0.0.0.0/0 0.0.0.0/0 > **Note**: > Docker is careful that its host-wide `iptables` rules fully expose From 38a595aec561b8a7b4325b6c1c4efd1b0b8e89c0 Mon Sep 17 00:00:00 2001 From: Ian Bishop Date: Sun, 21 Dec 2014 13:42:02 +1000 Subject: [PATCH 5/5] Tidy driver.go/LinkContainers Docker-DCO-1.1-Signed-off-by: Ian Bishop (github: porjo) --- daemon/networkdriver/bridge/driver.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/daemon/networkdriver/bridge/driver.go b/daemon/networkdriver/bridge/driver.go index 228bd479f..81624ad1d 100644 --- a/daemon/networkdriver/bridge/driver.go +++ b/daemon/networkdriver/bridge/driver.go @@ -509,7 +509,6 @@ func LinkContainers(job *engine.Job) engine.Status { parentIP = job.Getenv("ParentIP") ignoreErrors = job.GetenvBool("IgnoreErrors") ports = job.GetenvList("Ports") - chain = iptables.Chain{} ) switch action { @@ -532,14 +531,11 @@ func LinkContainers(job *engine.Job) engine.Status { return job.Errorf("child IP '%s' is invalid", childIP) } - chain.Name = "DOCKER" - chain.Bridge = bridgeIface + chain := iptables.Chain{Name: "DOCKER", Bridge: bridgeIface} for _, p := range ports { port := nat.Port(p) if err := chain.Link(nfAction, ip1, ip2, port.Int(), port.Proto()); !ignoreErrors && err != nil { - fmt.Print(err) return job.Error(err) - } } return engine.StatusOK