diff --git a/daemon/container_operations.go b/daemon/container_operations.go index 5134845668..a8a8a7aae2 100644 --- a/daemon/container_operations.go +++ b/daemon/container_operations.go @@ -532,7 +532,11 @@ func (daemon *Daemon) allocateNetwork(cfg *config.Config, container *container.C // If the container is not to be connected to any network, // create its network sandbox now if not present if len(networks) == 0 { - if nil == daemon.getNetworkSandbox(container) { + if _, err := daemon.netController.GetSandbox(container.ID); err != nil { + if !errdefs.IsNotFound(err) { + return err + } + sbOptions, err := daemon.buildSandboxOptions(cfg, container) if err != nil { return err @@ -557,18 +561,6 @@ func (daemon *Daemon) allocateNetwork(cfg *config.Config, container *container.C return nil } -func (daemon *Daemon) getNetworkSandbox(container *container.Container) *libnetwork.Sandbox { - var sb *libnetwork.Sandbox - daemon.netController.WalkSandboxes(func(s *libnetwork.Sandbox) bool { - if s.ContainerID() == container.ID { - sb = s - return true - } - return false - }) - return sb -} - // hasUserDefinedIPAddress returns whether the passed IPAM configuration contains IP address configuration func hasUserDefinedIPAddress(ipamConfig *networktypes.EndpointIPAMConfig) bool { return ipamConfig != nil && (len(ipamConfig.IPv4Address) > 0 || len(ipamConfig.IPv6Address) > 0) @@ -715,7 +707,8 @@ func (daemon *Daemon) connectToNetwork(cfg *config.Config, container *container. return err } - sb := daemon.getNetworkSandbox(container) + // TODO(thaJeztah): should this fail early if no sandbox was found? + sb, _ := daemon.netController.GetSandbox(container.ID) createOptions, err := buildCreateEndpointOptions(container, n, endpointConfig, sb, cfg.DNS) if err != nil { return err @@ -1072,9 +1065,9 @@ func (daemon *Daemon) ActivateContainerServiceBinding(containerName string) erro if err != nil { return err } - sb := daemon.getNetworkSandbox(ctr) - if sb == nil { - return fmt.Errorf("network sandbox does not exist for container %s", containerName) + sb, err := daemon.netController.GetSandbox(ctr.ID) + if err != nil { + return fmt.Errorf("failed to activate service binding for container %s: %w", containerName, err) } return sb.EnableService() } @@ -1085,10 +1078,10 @@ func (daemon *Daemon) DeactivateContainerServiceBinding(containerName string) er if err != nil { return err } - sb := daemon.getNetworkSandbox(ctr) - if sb == nil { + sb, err := daemon.netController.GetSandbox(ctr.ID) + if err != nil { // If the network sandbox is not found, then there is nothing to deactivate - log.G(context.TODO()).Debugf("Could not find network sandbox for container %s on service binding deactivation request", containerName) + log.G(context.TODO()).WithError(err).Debugf("Could not find network sandbox for container %s on service binding deactivation request", containerName) return nil } return sb.DisableService() diff --git a/libnetwork/controller.go b/libnetwork/controller.go index d54d13340a..cfb26dd816 100644 --- a/libnetwork/controller.go +++ b/libnetwork/controller.go @@ -78,18 +78,12 @@ import ( // When the function returns true, the walk will stop. type NetworkWalker func(nw *Network) bool -// SandboxWalker is a client provided function which will be used to walk the Sandboxes. -// When the function returns true, the walk will stop. -type SandboxWalker func(sb *Sandbox) bool - -type sandboxTable map[string]*Sandbox - // Controller manages networks. type Controller struct { id string drvRegistry drvregistry.Networks ipamRegistry drvregistry.IPAMs - sandboxes sandboxTable + sandboxes map[string]*Sandbox cfg *config.Config store *datastore.Store extKeyListener net.Listener @@ -115,7 +109,7 @@ func New(cfgOptions ...config.Option) (*Controller, error) { c := &Controller{ id: stringid.GenerateRandomID(), cfg: config.New(cfgOptions...), - sandboxes: sandboxTable{}, + sandboxes: map[string]*Sandbox{}, svcRecords: make(map[string]*svcInfo), serviceBindings: make(map[serviceKey]*service), agentInitDone: make(chan struct{}), @@ -972,31 +966,30 @@ func (c *Controller) NewSandbox(containerID string, options ...SandboxOption) (* return sb, nil } -// Sandboxes returns the list of Sandbox(s) managed by this controller. -func (c *Controller) Sandboxes() []*Sandbox { +// GetSandbox returns the Sandbox which has the passed id. +// +// It returns an [ErrInvalidID] when passing an invalid ID, or an +// [types.NotFoundError] if no Sandbox was found for the container. +func (c *Controller) GetSandbox(containerID string) (*Sandbox, error) { + if containerID == "" { + return nil, ErrInvalidID("id is empty") + } c.mu.Lock() defer c.mu.Unlock() - - list := make([]*Sandbox, 0, len(c.sandboxes)) - for _, s := range c.sandboxes { - // Hide stub sandboxes from libnetwork users - if s.isStub { - continue + if runtime.GOOS == "windows" { + // fast-path for Windows, which uses the container ID as sandbox ID. + if sb := c.sandboxes[containerID]; sb != nil && !sb.isStub { + return sb, nil } - - list = append(list, s) - } - - return list -} - -// WalkSandboxes uses the provided function to walk the Sandbox(s) managed by this controller. -func (c *Controller) WalkSandboxes(walker SandboxWalker) { - for _, sb := range c.Sandboxes() { - if walker(sb) { - return + } else { + for _, sb := range c.sandboxes { + if sb.containerID == containerID && !sb.isStub { + return sb, nil + } } } + + return nil, types.NotFoundErrorf("network sandbox for container %s not found", containerID) } // SandboxByID returns the Sandbox which has the passed id. @@ -1034,28 +1027,6 @@ func (c *Controller) SandboxDestroy(id string) error { return sb.Delete() } -// SandboxContainerWalker returns a Sandbox Walker function which looks for an existing Sandbox with the passed containerID -func SandboxContainerWalker(out **Sandbox, containerID string) SandboxWalker { - return func(sb *Sandbox) bool { - if sb.ContainerID() == containerID { - *out = sb - return true - } - return false - } -} - -// SandboxKeyWalker returns a Sandbox Walker function which looks for an existing Sandbox with the passed key -func SandboxKeyWalker(out **Sandbox, key string) SandboxWalker { - return func(sb *Sandbox) bool { - if sb.Key() == key { - *out = sb - return true - } - return false - } -} - func (c *Controller) loadDriver(networkType string) error { var err error diff --git a/libnetwork/libnetwork_linux_test.go b/libnetwork/libnetwork_linux_test.go index d292c18567..c57a9e4a92 100644 --- a/libnetwork/libnetwork_linux_test.go +++ b/libnetwork/libnetwork_linux_test.go @@ -2089,24 +2089,17 @@ type parallelTester struct { } func (pt parallelTester) Do(t *testing.T, thrNumber int) error { - var ( - ep *libnetwork.Endpoint - sb *libnetwork.Sandbox - err error - ) - teardown, err := pt.osctx.Set() if err != nil { return err } defer teardown(t) - epName := fmt.Sprintf("pep%d", thrNumber) - + var ep *libnetwork.Endpoint if thrNumber == 1 { - ep, err = pt.net1.EndpointByName(epName) + ep, err = pt.net1.EndpointByName(fmt.Sprintf("pep%d", thrNumber)) } else { - ep, err = pt.net2.EndpointByName(epName) + ep, err = pt.net2.EndpointByName(fmt.Sprintf("pep%d", thrNumber)) } if err != nil { @@ -2117,9 +2110,9 @@ func (pt parallelTester) Do(t *testing.T, thrNumber int) error { } cid := fmt.Sprintf("%drace", thrNumber) - pt.controller.WalkSandboxes(libnetwork.SandboxContainerWalker(&sb, cid)) - if sb == nil { - return errors.Errorf("got nil sandbox for container: %s", cid) + sb, err := pt.controller.GetSandbox(cid) + if err != nil { + return err } for i := 0; i < pt.iterCnt; i++ { diff --git a/libnetwork/sandbox_dns_unix.go b/libnetwork/sandbox_dns_unix.go index c1e4d6feb6..84177424e4 100644 --- a/libnetwork/sandbox_dns_unix.go +++ b/libnetwork/sandbox_dns_unix.go @@ -147,7 +147,10 @@ func (sb *Sandbox) updateParentHosts() error { var pSb *Sandbox for _, update := range sb.config.parentUpdates { - sb.controller.WalkSandboxes(SandboxContainerWalker(&pSb, update.cid)) + // TODO(thaJeztah): was it intentional for this loop to re-use prior results of pSB? If not, we should make pSb local and always replace here. + if s, _ := sb.controller.GetSandbox(update.cid); s != nil { + pSb = s + } if pSb == nil { continue } diff --git a/libnetwork/sandbox_externalkey_unix.go b/libnetwork/sandbox_externalkey_unix.go index 9fed223ac8..ad40b27194 100644 --- a/libnetwork/sandbox_externalkey_unix.go +++ b/libnetwork/sandbox_externalkey_unix.go @@ -164,15 +164,11 @@ func (c *Controller) processExternalKey(conn net.Conn) error { if err = json.Unmarshal(buf[0:nr], &s); err != nil { return err } - - var sandbox *Sandbox - search := SandboxContainerWalker(&sandbox, s.ContainerID) - c.WalkSandboxes(search) - if sandbox == nil { - return types.InvalidParameterErrorf("no sandbox present for %s", s.ContainerID) + sb, err := c.GetSandbox(s.ContainerID) + if err != nil { + return types.InvalidParameterErrorf("failed to get sandbox for %s", s.ContainerID) } - - return sandbox.SetKey(s.Key) + return sb.SetKey(s.Key) } func (c *Controller) stopExternalKeyListener() { diff --git a/libnetwork/sandbox_unix_test.go b/libnetwork/sandbox_unix_test.go index 4d77b05142..f0eb45db47 100644 --- a/libnetwork/sandbox_unix_test.go +++ b/libnetwork/sandbox_unix_test.go @@ -6,12 +6,15 @@ import ( "strconv" "testing" + "github.com/docker/docker/errdefs" "github.com/docker/docker/internal/testutils/netnsutils" "github.com/docker/docker/libnetwork/config" "github.com/docker/docker/libnetwork/ipamapi" "github.com/docker/docker/libnetwork/netlabel" "github.com/docker/docker/libnetwork/options" "github.com/docker/docker/libnetwork/osl" + "gotest.tools/v3/assert" + is "gotest.tools/v3/assert/cmp" ) func getTestEnv(t *testing.T, opts ...[]NetworkOption) (*Controller, []*Network) { @@ -51,6 +54,42 @@ func getTestEnv(t *testing.T, opts ...[]NetworkOption) (*Controller, []*Network) return c, nwList } +func TestControllerGetSandbox(t *testing.T) { + ctrlr, _ := getTestEnv(t) + t.Run("invalid id", func(t *testing.T) { + const cID = "" + sb, err := ctrlr.GetSandbox(cID) + _, ok := err.(ErrInvalidID) + assert.Check(t, ok, "expected ErrInvalidID, got %[1]v (%[1]T)", err) + assert.Check(t, is.Nil(sb)) + }) + t.Run("not found", func(t *testing.T) { + const cID = "container-id-with-no-sandbox" + sb, err := ctrlr.GetSandbox(cID) + assert.Check(t, errdefs.IsNotFound(err), "expected a ErrNotFound, got %[1]v (%[1]T)", err) + assert.Check(t, is.Nil(sb)) + }) + t.Run("existing sandbox", func(t *testing.T) { + const cID = "test-container-id" + expected, err := ctrlr.NewSandbox(cID) + assert.Check(t, err) + + sb, err := ctrlr.GetSandbox(cID) + assert.Check(t, err) + assert.Check(t, is.Equal(sb.ContainerID(), cID)) + assert.Check(t, is.Equal(sb.ID(), expected.ID())) + assert.Check(t, is.Equal(sb.Key(), expected.Key())) + assert.Check(t, is.Equal(sb.ContainerID(), expected.ContainerID())) + + err = sb.Delete() + assert.Check(t, err) + + sb, err = ctrlr.GetSandbox(cID) + assert.Check(t, errdefs.IsNotFound(err), "expected a ErrNotFound, got %[1]v (%[1]T)", err) + assert.Check(t, is.Nil(sb)) + }) +} + func TestSandboxAddEmpty(t *testing.T) { ctrlr, _ := getTestEnv(t)