From 6203e3660d37e401833a6aeea710ecd09afcf624 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 27 Aug 2023 11:15:42 +0200 Subject: [PATCH] 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 --- libnetwork/agent.go | 44 +++++++++++++++++++++++++------------------- 1 file changed, 25 insertions(+), 19 deletions(-) diff --git a/libnetwork/agent.go b/libnetwork/agent.go index 918e248dbd..07d4c60e94 100644 --- a/libnetwork/agent.go +++ b/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 {