From 6dba98cf38bc4ac42ce935b8ebc0a8129537ab6a Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 12 Aug 2023 14:38:43 +0200 Subject: [PATCH] libnetwork: implement Controller.GetSandbox(containerID) Various parts of the code were using "walkers" to iterate over the controller's sandboxes, and the only condition for all of them was to find the sandbox for a given container-ID. Iterating over all sandboxes was also sub-optimal, because on Windows, the ContainerID is used as Sandbox-ID, which can be used to lookup the sandbox from the "sandboxes" map on the controller. This patch implements a GetSandbox method on the controller that looks up the sandbox for a given container-ID, using the most optimal approach (depending on the platform). The new method can return errors for invalid (empty) container-IDs, and a "not found" error to allow consumers to detect non-existing sandboxes, or potentially invalid IDs. This new method replaces the (non-exported) Daemon.getNetworkSandbox(), which was only used internally, in favor of directly accessing the controller's method. Signed-off-by: Sebastiaan van Stijn --- daemon/container_operations.go | 33 +++++++++------------- libnetwork/controller.go | 26 +++++++++++++++++ libnetwork/libnetwork_linux_test.go | 7 ++--- libnetwork/sandbox_dns_unix.go | 5 +++- libnetwork/sandbox_externalkey_unix.go | 12 +++----- libnetwork/sandbox_unix_test.go | 39 ++++++++++++++++++++++++++ 6 files changed, 89 insertions(+), 33 deletions(-) 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 35b2169d1c..a25c6f103c 100644 --- a/libnetwork/controller.go +++ b/libnetwork/controller.go @@ -997,6 +997,32 @@ func (c *Controller) WalkSandboxes(walker SandboxWalker) { } } +// 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() + 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 + } + } 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. // If not found, a [types.NotFoundError] is returned. func (c *Controller) SandboxByID(id string) (*Sandbox, error) { diff --git a/libnetwork/libnetwork_linux_test.go b/libnetwork/libnetwork_linux_test.go index a845e912fc..c57a9e4a92 100644 --- a/libnetwork/libnetwork_linux_test.go +++ b/libnetwork/libnetwork_linux_test.go @@ -2109,11 +2109,10 @@ func (pt parallelTester) Do(t *testing.T, thrNumber int) error { return errors.New("got nil ep with no error") } - var sb *libnetwork.Sandbox 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)