Do not leave/delete gw endpoint twice

- On sandbox delete, the leave and delete of each
  endpoint is performed, regardless of whether the endpoint
  is the gw network endpoint. This endpoint is already
  automatically removed in endpoint.sbLeave() by
  sb.clearDefaultGW() when the sandbox is marked for
  deletion.
- Also restoring otiginal behavior where on disconnect
  from overlay network (only connected network), it also
  disconnects from default gw network.
- Also do not let internal network dictate container does
  not need external connectivity. Before this fix, if a container
  was connected to an overlay and an internal network, it may not
  get attached to the default gw network.
- needDefaultGw() takes now into account whether the sandbox
  is marked for deletion

Signed-off-by: Alessandro Boch <aboch@docker.com>

libnetwork/default_gateway.go

@@ -65,20 +65,13 @@ func (sb *sandbox) setupDefaultGW() error {
 	return nil
 }
 
-// If present, removes the endpoint connecting the sandbox to the default gw network.
-// Unless it is the endpoint designated to provide the external connectivity.
-// If the sandbox is being deleted, removes the endpoint unconditionally.
+// If present, detach and remove the endpoint connecting the sandbox to the default gw network.
 func (sb *sandbox) clearDefaultGW() error {
 	var ep *endpoint
 
 	if ep = sb.getEndpointInGWNetwork(); ep == nil {
 		return nil
 	}
-
-	if ep == sb.getGatewayEndpoint() && !sb.inDelete {
-		return nil
-	}
-
 	if err := ep.sbLeave(sb, false); err != nil {
 		return fmt.Errorf("container %s: endpoint leaving GW Network failed: %v", sb.containerID, err)
 	}
@@ -88,20 +81,27 @@ func (sb *sandbox) clearDefaultGW() error {
 	return nil
 }
 
+// Evaluate whether the sandbox needs to be attached to the default
+// gateway network.
 func (sb *sandbox) needDefaultGW() bool {
 	var needGW bool
 
+	if sb.inDelete {
+		return false
+	}
+
 	for _, ep := range sb.getConnectedEndpoints() {
 		if ep.endpointInGWNetwork() {
-			return false
+			continue
 		}
 		if ep.getNetwork().Type() == "null" || ep.getNetwork().Type() == "host" {
 			continue
 		}
 		if ep.getNetwork().Internal() {
-			return false
+			continue
 		}
-		if ep.joinInfo.disableGatewayService {
+		// During stale sandbox cleanup, joinInfo may be nil
+		if ep.joinInfo != nil && ep.joinInfo.disableGatewayService {
 			return false
 		}
 		// TODO v6 needs to be handled.
@@ -115,6 +115,7 @@ func (sb *sandbox) needDefaultGW() bool {
 		}
 		needGW = true
 	}
+
 	return needGW
 }
 

libnetwork/endpoint.go

@@ -479,7 +479,14 @@ func (ep *endpoint) sbJoin(sb *sandbox, options ...EndpointOption) error {
 		}
 	}
 
-	return sb.clearDefaultGW()
+	if !sb.needDefaultGW() {
+		if err := sb.clearDefaultGW(); err != nil {
+			log.Warnf("Failure while disconnecting sandbox %s (%s) from gateway network: %v",
+				sb.ID(), sb.ContainerID(), err)
+		}
+	}
+
+	return nil
 }
 
 func (ep *endpoint) rename(name string) error {
@@ -622,10 +629,7 @@ func (ep *endpoint) sbLeave(sb *sandbox, force bool, options ...EndpointOption)
 	}
 
 	sb.deleteHostsEntries(n.getSvcRecords(ep))
-	if !sb.inDelete && sb.needDefaultGW() {
-		if sb.getEPwithoutGateway() == nil {
-			return fmt.Errorf("endpoint without GW expected, but not found")
-		}
+	if sb.needDefaultGW() {
 		return sb.setupDefaultGW()
 	}
 
@@ -639,7 +643,14 @@ func (ep *endpoint) sbLeave(sb *sandbox, force bool, options ...EndpointOption)
 		}
 	}
 
-	return sb.clearDefaultGW()
+	if !sb.needDefaultGW() {
+		if err := sb.clearDefaultGW(); err != nil {
+			log.Warnf("Failure while disconnecting sandbox %s (%s) from gateway network: %v",
+				sb.ID(), sb.ContainerID(), err)
+		}
+	}
+
+	return nil
 }
 
 func (n *network) validateForceDelete(locator string) error {

libnetwork/sandbox.go

@@ -197,6 +197,10 @@ func (sb *sandbox) delete(force bool) error {
 	// Detach from all endpoints
 	retain := false
 	for _, ep := range sb.getConnectedEndpoints() {
+		// gw network endpoint detach and removal are automatic
+		if ep.endpointInGWNetwork() {
+			continue
+		}
 		// Retain the sanbdox if we can't obtain the network from store.
 		if _, err := c.getNetworkFromStore(ep.getNetwork().ID()); err != nil {
 			retain = true

libnetwork/test/integration/dnet/helpers.bash

@@ -345,10 +345,6 @@ function test_overlay() {
             # Disconnect from overlay network
             net_disconnect ${start} container_${start} multihost
 
-            # Make sure external connectivity works
-            runc $(dnet_container_name ${start} $dnet_suffix) $(get_sbox_id ${start} container_${start}) \
-                "ping -c 1 www.google.com"
-
             # Connect to overlay network again
             net_connect ${start} container_${start} multihost