libnetwork: Controller.NewNetwork: use named error-return

It's used in various defers, but was using `err` as name, which can be
confusing, and increases the risk of accidentally shadowing the error.

This patch:

- introduces a `retErr` output variable, to be used in defer statements.
- explicitly changes some `err` uses to locally-scoped variables.
- moves some variable definitions closer to where they're used (where possible).

While working on this change, there was one point in the code where
error handling was ambiguous. I added a note for that, in case this
was not a bug:

> This code was previously assigning the error to the global "err"
> variable (before it was renamed to "retErr"), but in case of a
> "MaskableError" did not *return* the error:
> https://github.com/moby/moby/blob/b325dcbff60a04cedbe40eb627465fc7379d05bf/libnetwork/controller.go#L566-L573
>
> Depending on code paths further down, that meant that this error
> was either overwritten by other errors (and thus not handled in
> defer statements) or handled (if no other code was overwriting it.
>
> I suspect this was a bug (but possible without effect), but it could
> have been intentional. This logic is confusing at least, and even
> more so combined with the handling in defer statements that check for
> both the "err" return AND "skipCfgEpCount":
> https://github.com/moby/moby/blob/b325dcbff60a04cedbe40eb627465fc7379d05bf/libnetwork/controller.go#L586-L602
>
> To save future visitors some time to dig up history:
>
> - config-only networks were added in 25082206df465d1c11dd1276a65b4a1dc701bd43
> - the special error-handling and "skipCfgEpcoung" was added in ddd22a819867faa0cd7d12b0c3fad1099ac3eb26
> - and updated in 87b082f3659f9ec245ab15d781e6bfffced0af83 to don't use string-matching

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

libnetwork/controller.go

@@ -465,19 +465,12 @@ const overlayDSROptionString = "dsr"
 
 // NewNetwork creates a new network of the specified network type. The options
 // are network specific and modeled in a generic way.
-func (c *Controller) NewNetwork(networkType, name string, id string, options ...NetworkOption) (*Network, error) {
-	var (
-		caps           driverapi.Capability
-		err            error
-		t              *Network
-		skipCfgEpCount bool
-	)
-
+func (c *Controller) NewNetwork(networkType, name string, id string, options ...NetworkOption) (_ *Network, retErr error) {
 	if id != "" {
 		c.networkLocker.Lock(id)
 		defer c.networkLocker.Unlock(id) //nolint:errcheck
 
-		if _, err = c.NetworkByID(id); err == nil {
+		if _, err := c.NetworkByID(id); err == nil {
 			return nil, NetworkNameError(id)
 		}
 	}
@@ -506,10 +499,19 @@ func (c *Controller) NewNetwork(networkType, name string, id string, options ...
 	}
 
 	nw.processOptions(options...)
-	if err = nw.validateConfiguration(); err != nil {
+	if err := nw.validateConfiguration(); err != nil {
 		return nil, err
 	}
 
+	// These variables must be defined here, as declaration would otherwise
+	// be skipped by the "goto addToStore"
+	var (
+		caps driverapi.Capability
+		err  error
+
+		skipCfgEpCount bool
+	)
+
 	// Reset network types, force local scope and skip allocation and
 	// plumbing for configuration networks. Reset of the config-only
 	// network drivers is needed so that this special network is not
@@ -555,36 +557,57 @@ func (c *Controller) NewNetwork(networkType, name string, id string, options ...
 	// From this point on, we need the network specific configuration,
 	// which may come from a configuration-only network
 	if nw.configFrom != "" {
-		t, err = c.getConfigNetwork(nw.configFrom)
+		configNetwork, err := c.getConfigNetwork(nw.configFrom)
 		if err != nil {
 			return nil, types.NotFoundErrorf("configuration network %q does not exist", nw.configFrom)
 		}
-		if err = t.applyConfigurationTo(nw); err != nil {
+		if err := configNetwork.applyConfigurationTo(nw); err != nil {
 			return nil, types.InternalErrorf("Failed to apply configuration: %v", err)
 		}
 		nw.generic[netlabel.Internal] = nw.internal
 		defer func() {
-			if err == nil && !skipCfgEpCount {
-				if err := t.getEpCnt().IncEndpointCnt(); err != nil {
-					log.G(context.TODO()).Warnf("Failed to update reference count for configuration network %q on creation of network %q: %v",
-						t.Name(), nw.Name(), err)
+			if retErr == nil && !skipCfgEpCount {
+				if err := configNetwork.getEpCnt().IncEndpointCnt(); err != nil {
+					log.G(context.TODO()).Warnf("Failed to update reference count for configuration network %q on creation of network %q: %v", configNetwork.Name(), nw.name, err)
 				}
 			}
 		}()
 	}
 
-	err = nw.ipamAllocate()
-	if err != nil {
+	if err := nw.ipamAllocate(); err != nil {
 		return nil, err
 	}
 	defer func() {
-		if err != nil {
+		if retErr != nil {
 			nw.ipamRelease()
 		}
 	}()
 
-	err = c.addNetwork(nw)
-	if err != nil {
+	// Note from thaJeztah to future code visitors, or "future self".
+	//
+	// This code was previously assigning the error to the global "err"
+	// variable (before it was renamed to "retErr"), but in case of a
+	// "MaskableError" did not *return* the error:
+	// https://github.com/moby/moby/blob/b325dcbff60a04cedbe40eb627465fc7379d05bf/libnetwork/controller.go#L566-L573
+	//
+	// Depending on code paths further down, that meant that this error
+	// was either overwritten by other errors (and thus not handled in
+	// defer statements) or handled (if no other code was overwriting it.
+	//
+	// I suspect this was a bug (but possible without effect), but it could
+	// have been intentional. This logic is confusing at least, and even
+	// more so combined with the handling in defer statements that check for
+	// both the "err" return AND "skipCfgEpCount":
+	// https://github.com/moby/moby/blob/b325dcbff60a04cedbe40eb627465fc7379d05bf/libnetwork/controller.go#L586-L602
+	//
+	// To save future visitors some time to dig up history:
+	//
+	// - config-only networks were added in 25082206df465d1c11dd1276a65b4a1dc701bd43
+	// - the special error-handling and "skipCfgEpcoung" was added in ddd22a819867faa0cd7d12b0c3fad1099ac3eb26
+	// - and updated in 87b082f3659f9ec245ab15d781e6bfffced0af83 to don't use string-matching
+	//
+	// To cut a long story short: if this broke anything, you know who to blame :)
+	if err := c.addNetwork(nw); err != nil {
 		if _, ok := err.(types.MaskableError); ok { //nolint:gosimple
 			// This error can be ignored and set this boolean
 			// value to skip a refcount increment for configOnly networks
@@ -594,9 +617,9 @@ func (c *Controller) NewNetwork(networkType, name string, id string, options ...
 		}
 	}
 	defer func() {
-		if err != nil {
-			if e := nw.deleteNetwork(); e != nil {
-				log.G(context.TODO()).Warnf("couldn't roll back driver network on network %s creation failure: %v", nw.name, err)
+		if retErr != nil {
+			if err := nw.deleteNetwork(); err != nil {
+				log.G(context.TODO()).Warnf("couldn't roll back driver network on network %s creation failure: %v", nw.name, retErr)
 			}
 		}
 	}()
@@ -621,25 +644,25 @@ addToStore:
 	// end up with a datastore containing a network and not an epCnt,
 	// in case of an ungraceful shutdown during this function call.
 	epCnt := &endpointCnt{n: nw}
-	if err = c.updateToStore(epCnt); err != nil {
+	if err := c.updateToStore(epCnt); err != nil {
 		return nil, err
 	}
 	defer func() {
-		if err != nil {
-			if e := c.deleteFromStore(epCnt); e != nil {
-				log.G(context.TODO()).Warnf("could not rollback from store, epCnt %v on failure (%v): %v", epCnt, err, e)
+		if retErr != nil {
+			if err := c.deleteFromStore(epCnt); err != nil {
+				log.G(context.TODO()).Warnf("could not rollback from store, epCnt %v on failure (%v): %v", epCnt, retErr, err)
 			}
 		}
 	}()
 
 	nw.epCnt = epCnt
-	if err = c.updateToStore(nw); err != nil {
+	if err := c.updateToStore(nw); err != nil {
 		return nil, err
 	}
 	defer func() {
-		if err != nil {
-			if e := c.deleteFromStore(nw); e != nil {
-				log.G(context.TODO()).Warnf("could not rollback from store, network %v on failure (%v): %v", nw, err, e)
+		if retErr != nil {
+			if err := c.deleteFromStore(nw); err != nil {
+				log.G(context.TODO()).Warnf("could not rollback from store, network %v on failure (%v): %v", nw, retErr, err)
 			}
 		}
 	}()
@@ -650,16 +673,16 @@ addToStore:
 
 	joinCluster(nw)
 	defer func() {
-		if err != nil {
+		if retErr != nil {
 			nw.cancelDriverWatches()
-			if e := nw.leaveCluster(); e != nil {
-				log.G(context.TODO()).Warnf("Failed to leave agent cluster on network %s on failure (%v): %v", nw.name, err, e)
+			if err := nw.leaveCluster(); err != nil {
+				log.G(context.TODO()).Warnf("Failed to leave agent cluster on network %s on failure (%v): %v", nw.name, retErr, err)
 			}
 		}
 	}()
 
 	if nw.hasLoadBalancerEndpoint() {
-		if err = nw.createLoadBalancerSandbox(); err != nil {
+		if err := nw.createLoadBalancerSandbox(); err != nil {
 			return nil, err
 		}
 	}