diff --git a/libnetwork/controller.go b/libnetwork/controller.go index 331c7f8e1b..b7d612aeef 100644 --- a/libnetwork/controller.go +++ b/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 } }