Merge pull request #46039 from thaJeztah/cleanup_bridge

libnetwork/drivers/bridge: assorted cleanups

libnetwork/drivers/bridge/bridge.go

@@ -310,13 +310,12 @@ func (n *bridgeNetwork) getNetworkBridgeName() string {
 }
 
 func (n *bridgeNetwork) getEndpoint(eid string) (*bridgeEndpoint, error) {
-	n.Lock()
-	defer n.Unlock()
-
 	if eid == "" {
 		return nil, InvalidEndpointIDError(eid)
 	}
 
+	n.Lock()
+	defer n.Unlock()
 	if ep, ok := n.endpoints[eid]; ok {
 		return ep, nil
 	}
@@ -1349,43 +1348,45 @@ func (d *driver) RevokeExternalConnectivity(nid, eid string) error {
 	return nil
 }
 
-func (d *driver) link(network *bridgeNetwork, endpoint *bridgeEndpoint, enable bool) error {
-	var err error
-
+func (d *driver) link(network *bridgeNetwork, endpoint *bridgeEndpoint, enable bool) (retErr error) {
 	cc := endpoint.containerConfig
-	if cc == nil {
-		return nil
-	}
 	ec := endpoint.extConnConfig
-	if ec == nil {
+	if cc == nil || ec == nil || (len(cc.ParentEndpoints) == 0 && len(cc.ChildEndpoints) == 0) {
+		// nothing to do
 		return nil
 	}
 
+	// Try to keep things atomic. addedLinks keeps track of links that were
+	// successfully added. If any error occurred, then roll back all.
+	var addedLinks []*link
+	defer func() {
+		if retErr == nil {
+			return
+		}
+		for _, l := range addedLinks {
+			l.Disable()
+		}
+	}()
+
 	if ec.ExposedPorts != nil {
 		for _, p := range cc.ParentEndpoints {
-			var parentEndpoint *bridgeEndpoint
-			parentEndpoint, err = network.getEndpoint(p)
+			parentEndpoint, err := network.getEndpoint(p)
 			if err != nil {
 				return err
 			}
 			if parentEndpoint == nil {
-				err = InvalidEndpointIDError(p)
-				return err
+				return InvalidEndpointIDError(p)
 			}
 
-			l := newLink(parentEndpoint.addr.IP.String(),
-				endpoint.addr.IP.String(),
-				ec.ExposedPorts, network.config.BridgeName)
+			l, err := newLink(parentEndpoint.addr.IP, endpoint.addr.IP, ec.ExposedPorts, network.config.BridgeName)
+			if err != nil {
+				return err
+			}
 			if enable {
-				err = l.Enable()
-				if err != nil {
+				if err := l.Enable(); err != nil {
 					return err
 				}
-				defer func() {
-					if err != nil {
-						l.Disable()
-					}
-				}()
+				addedLinks = append(addedLinks, l)
 			} else {
 				l.Disable()
 			}
@@ -1393,32 +1394,26 @@ func (d *driver) link(network *bridgeNetwork, endpoint *bridgeEndpoint, enable b
 	}
 
 	for _, c := range cc.ChildEndpoints {
-		var childEndpoint *bridgeEndpoint
-		childEndpoint, err = network.getEndpoint(c)
+		childEndpoint, err := network.getEndpoint(c)
 		if err != nil {
 			return err
 		}
 		if childEndpoint == nil {
-			err = InvalidEndpointIDError(c)
-			return err
+			return InvalidEndpointIDError(c)
 		}
 		if childEndpoint.extConnConfig == nil || childEndpoint.extConnConfig.ExposedPorts == nil {
 			continue
 		}
 
-		l := newLink(endpoint.addr.IP.String(),
-			childEndpoint.addr.IP.String(),
-			childEndpoint.extConnConfig.ExposedPorts, network.config.BridgeName)
+		l, err := newLink(endpoint.addr.IP, childEndpoint.addr.IP, childEndpoint.extConnConfig.ExposedPorts, network.config.BridgeName)
+		if err != nil {
+			return err
+		}
 		if enable {
-			err = l.Enable()
-			if err != nil {
+			if err := l.Enable(); err != nil {
 				return err
 			}
-			defer func() {
-				if err != nil {
-					l.Disable()
-				}
-			}()
+			addedLinks = append(addedLinks, l)
 		} else {
 			l.Disable()
 		}

libnetwork/drivers/bridge/link.go

@@ -13,8 +13,8 @@ import (
 )
 
 type link struct {
-	parentIP string
-	childIP  string
+	parentIP net.IP
+	childIP  net.IP
 	ports    []types.TransportPort
 	bridge   string
 }
@@ -23,61 +23,52 @@ func (l *link) String() string {
 	return fmt.Sprintf("%s <-> %s [%v] on %s", l.parentIP, l.childIP, l.ports, l.bridge)
 }
 
-func newLink(parentIP, childIP string, ports []types.TransportPort, bridge string) *link {
+func newLink(parentIP, childIP net.IP, ports []types.TransportPort, bridge string) (*link, error) {
+	if parentIP == nil {
+		return nil, fmt.Errorf("cannot link to a container with an empty parent IP address")
+	}
+	if childIP == nil {
+		return nil, fmt.Errorf("cannot link to a container with an empty child IP address")
+	}
+
 	return &link{
 		childIP:  childIP,
 		parentIP: parentIP,
 		ports:    ports,
 		bridge:   bridge,
-	}
+	}, nil
 }
 
 func (l *link) Enable() error {
-	// -A == iptables append flag
 	linkFunction := func() error {
-		return linkContainers("-A", l.parentIP, l.childIP, l.ports, l.bridge, false)
+		return linkContainers(iptables.Append, l.parentIP, l.childIP, l.ports, l.bridge, false)
+	}
+	if err := linkFunction(); err != nil {
+		return err
 	}
 
-	iptables.OnReloaded(func() { linkFunction() })
-	return linkFunction()
+	iptables.OnReloaded(func() { _ = linkFunction() })
+	return nil
 }
 
 func (l *link) Disable() {
-	// -D == iptables delete flag
-	err := linkContainers("-D", l.parentIP, l.childIP, l.ports, l.bridge, true)
-	if err != nil {
-		log.G(context.TODO()).Errorf("Error removing IPTables rules for a link %s due to %s", l.String(), err.Error())
+	if err := linkContainers(iptables.Delete, l.parentIP, l.childIP, l.ports, l.bridge, true); err != nil {
+		// @TODO: Return error once we have the iptables package return typed errors.
+		log.G(context.TODO()).WithError(err).Errorf("Error removing IPTables rules for link: %s", l.String())
 	}
-	// Return proper error once we move to use a proper iptables package
-	// that returns typed errors
 }
 
-func linkContainers(action, parentIP, childIP string, ports []types.TransportPort, bridge string, ignoreErrors bool) error {
-	var nfAction iptables.Action
-
-	switch action {
-	case "-A":
-		nfAction = iptables.Append
-	case "-I":
-		nfAction = iptables.Insert
-	case "-D":
-		nfAction = iptables.Delete
-	default:
-		return fmt.Errorf("invalid iptables action: %s", action)
-	}
-
-	ip1 := net.ParseIP(parentIP)
-	if ip1 == nil {
-		return fmt.Errorf("cannot link to a container with an invalid parent IP address %q", parentIP)
+func linkContainers(action iptables.Action, parentIP, childIP net.IP, ports []types.TransportPort, bridge string, ignoreErrors bool) error {
+	if parentIP == nil {
+		return fmt.Errorf("cannot link to a container with an empty parent IP address")
 	}
-	ip2 := net.ParseIP(childIP)
-	if ip2 == nil {
-		return fmt.Errorf("cannot link to a container with an invalid child IP address %q", childIP)
+	if childIP == nil {
+		return fmt.Errorf("cannot link to a container with an empty child IP address")
 	}
 
 	chain := iptables.ChainInfo{Name: DockerChain}
 	for _, port := range ports {
-		err := chain.Link(nfAction, ip1, ip2, int(port.Port), port.Proto.String(), bridge)
+		err := chain.Link(action, parentIP, childIP, int(port.Port), port.Proto.String(), bridge)
 		if !ignoreErrors && err != nil {
 			return err
 		}

libnetwork/drivers/bridge/link_test.go

@@ -3,6 +3,7 @@
 package bridge
 
 import (
+	"net"
 	"testing"
 
 	"github.com/docker/docker/libnetwork/types"
@@ -19,23 +20,34 @@ func getPorts() []types.TransportPort {
 func TestLinkNew(t *testing.T) {
 	ports := getPorts()
 
-	link := newLink("172.0.17.3", "172.0.17.2", ports, "docker0")
+	const (
+		pIP        = "172.0.17.3"
+		cIP        = "172.0.17.2"
+		bridgeName = "docker0"
+	)
 
-	if link == nil {
+	parentIP := net.ParseIP(pIP)
+	childIP := net.ParseIP(cIP)
+
+	l, err := newLink(parentIP, childIP, ports, bridgeName)
+	if err != nil {
+		t.Errorf("unexpected error from newlink(): %v", err)
+	}
+	if l == nil {
 		t.FailNow()
 	}
-	if link.parentIP != "172.0.17.3" {
+	if l.parentIP.String() != pIP {
 		t.Fail()
 	}
-	if link.childIP != "172.0.17.2" {
+	if l.childIP.String() != cIP {
 		t.Fail()
 	}
-	for i, p := range link.ports {
+	for i, p := range l.ports {
 		if p != ports[i] {
 			t.Fail()
 		}
 	}
-	if link.bridge != "docker0" {
+	if l.bridge != bridgeName {
 		t.Fail()
 	}
 }