Bladeren bron

libnetwork: Endpoint: return early if no agent was found

This removes redundant nil-checks in Endpoint.deleteServiceInfoFromCluster
and Endpoint.addServiceInfoToCluster.

These functions return early if the network is not ["cluster eligible"][1],
and the function used for that (`Network.isClusterEligible`) requires the
[agent to not be `nil`][2].

This check moved around a few times ([3][3], [4][4]), but was originally
added in [libnetwork 1570][5] which, among others, tried to avoid a nil-pointer
exception reported in [moby 28712][6], which accessed the `Controller.agent`
[without locking][7]. That issue was addressed by adding locks, adding a
`Controller.getAgent` accessor, and updating deleteServiceInfoFromCluster
to use a local var. It also sprinkled this `nil` check to be on the safe
side, but as `Network.isClusterEligible` already checks for the agent
to not be `nil`, this should not be redundant.

[1]: https://github.com/moby/moby/blob/5b53ddfcdd1c0721f875d3e237078ab7de891a57/libnetwork/agent.go#L529-L534
[2]: https://github.com/moby/moby/blob/5b53ddfcdd1c0721f875d3e237078ab7de891a57/libnetwork/agent.go#L688-L696
[3]: https://github.com/moby/libnetwork/commit/f2307265c77fd6db5d92310857c3ffad1184ad24
[4]: https://github.com/moby/libnetwork/commit/6426d1e66f33c0b0c8bb135b7ee547447f54d043
[5]: https://github.com/moby/libnetwork/commit/8dcf9960aa81767376e580f20d9ecea3e68f62ad
[6]: https://github.com/moby/moby/issues/28712
[7]: https://github.com/moby/moby/blob/75fd88ba8938f120ce1f2a21b19cf13c12071b16/vendor/github.com/docker/libnetwork/agent.go#L452

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Sebastiaan van Stijn 1 jaar geleden
bovenliggende
commit
6203e3660d
1 gewijzigde bestanden met toevoegingen van 25 en 19 verwijderingen
  1. 25 19
      libnetwork/agent.go

+ 25 - 19
libnetwork/agent.go

@@ -618,6 +618,14 @@ func (ep *Endpoint) addServiceInfoToCluster(sb *Sandbox) error {
 	if !n.isClusterEligible() {
 		return nil
 	}
+	c := n.getController()
+	agent := c.getAgent()
+	if agent == nil {
+		// this should not happen normally, as the network is not considered
+		// "ClusterEligible" if the agent is nil (in which case we wouldn't
+		// reach this code).
+		return nil
+	}
 
 	sb.service.Lock()
 	defer sb.service.Unlock()
@@ -639,9 +647,6 @@ func (ep *Endpoint) addServiceInfoToCluster(sb *Sandbox) error {
 		return nil
 	}
 
-	c := n.getController()
-	agent := c.getAgent()
-
 	name := ep.Name()
 	if ep.isAnonymous() {
 		name = ep.MyAliases()[0]
@@ -679,11 +684,9 @@ func (ep *Endpoint) addServiceInfoToCluster(sb *Sandbox) error {
 		return err
 	}
 
-	if agent != nil {
-		if err := agent.networkDB.CreateEntry(libnetworkEPTable, n.ID(), ep.ID(), buf); err != nil {
-			log.G(context.TODO()).Warnf("addServiceInfoToCluster NetworkDB CreateEntry failed for %s %s err:%s", ep.id, n.id, err)
-			return err
-		}
+	if err := agent.networkDB.CreateEntry(libnetworkEPTable, n.ID(), ep.ID(), buf); err != nil {
+		log.G(context.TODO()).Warnf("addServiceInfoToCluster NetworkDB CreateEntry failed for %s %s err:%s", ep.id, n.id, err)
+		return err
 	}
 
 	log.G(context.TODO()).Debugf("addServiceInfoToCluster END for %s %s", ep.svcName, ep.ID())
@@ -700,6 +703,14 @@ func (ep *Endpoint) deleteServiceInfoFromCluster(sb *Sandbox, fullRemove bool, m
 	if !n.isClusterEligible() {
 		return nil
 	}
+	c := n.getController()
+	agent := c.getAgent()
+	if agent == nil {
+		// this should not happen normally, as the network is not considered
+		// "ClusterEligible" if the agent is nil (in which case we wouldn't
+		// reach this code).
+		return nil
+	}
 
 	sb.service.Lock()
 	defer sb.service.Unlock()
@@ -714,23 +725,18 @@ func (ep *Endpoint) deleteServiceInfoFromCluster(sb *Sandbox, fullRemove bool, m
 		return nil
 	}
 
-	c := n.getController()
-	agent := c.getAgent()
-
 	name := ep.Name()
 	if ep.isAnonymous() {
 		name = ep.MyAliases()[0]
 	}
 
-	if agent != nil {
-		// First update the networkDB then locally
-		if fullRemove {
-			if err := agent.networkDB.DeleteEntry(libnetworkEPTable, n.ID(), ep.ID()); err != nil {
-				log.G(context.TODO()).Warnf("deleteServiceInfoFromCluster NetworkDB DeleteEntry failed for %s %s err:%s", ep.id, n.id, err)
-			}
-		} else {
-			disableServiceInNetworkDB(agent, n, ep)
+	// First update the networkDB then locally
+	if fullRemove {
+		if err := agent.networkDB.DeleteEntry(libnetworkEPTable, n.ID(), ep.ID()); err != nil {
+			log.G(context.TODO()).Warnf("deleteServiceInfoFromCluster NetworkDB DeleteEntry failed for %s %s err:%s", ep.id, n.id, err)
 		}
+	} else {
+		disableServiceInNetworkDB(agent, n, ep)
 	}
 
 	if ep.Iface() != nil && ep.Iface().Address() != nil {