daemon/cluster: only fail a task on a pull error if the image is missing

A task whose image cannot be pulled is rejected outright when the registry
answers with an "unauthorized" error, even when the image is already present
on the node. A service created without registry credentials carries no auth in
its spec and cannot have its digest resolved, so its tasks always attempt an
unauthenticated pull; on nodes that already hold the image those tasks are
rejected instead of started.

Tolerating a failed pull when the image is available locally is long-standing
behaviour, described in the comment above the check, and what the executor did
for every pull error before a7cf7eac0a ("fix: propagate registry auth error in
swarm image pull").

Keep reporting the pull failure, as it is far more useful than the "No such
image" the container create would otherwise produce, but only when the image
really is unavailable. Whether a task can run should not depend on how the
registry error happened to be classified, and that classification is not
consistent: the same registry 401 surfaces as ErrUnauthenticated from the
graphdriver pull path, as ErrNotFound from the containerd image store (via
docker.ErrInvalidAuthorization), and as ErrUnknown when the failure happens
while fetching a token, since the error is then a *url.Error that
translatePullError does not classify. The check therefore only fires for some
combinations of image store and registry.

The test needs DOCKER_SERVICE_PREFER_OFFLINE_IMAGE=0 on the daemon under test:
internal/testutil/daemon sets it to 1, which makes the executor skip the pull
altogether.

Signed-off-by: Sopho Merkviladze <smerkviladze@mirantis.com>

daemon/cluster/executor/container/adapter.go

@@ -12,6 +12,7 @@ import (
 	"syscall"
 	"time"
 
+	cerrdefs "github.com/containerd/errdefs"
 	"github.com/containerd/log"
 	"github.com/distribution/reference"
 	gogotypes "github.com/gogo/protobuf/types"
@@ -65,6 +66,15 @@ func newContainerAdapter(b executorpkg.Backend, i executorpkg.ImageBackend, v ex
 	}, nil
 }
 
+// imageExists reports whether the image the container is configured with is
+// present in the local image store. An error other than "not found" is not
+// conclusive, so the image is reported as present and the container create
+// is left to surface the error.
+func (c *containerAdapter) imageExists(ctx context.Context) bool {
+	_, err := c.imageBackend.GetImage(ctx, c.container.image(), imagebackend.GetImageOpts{})
+	return !cerrdefs.IsNotFound(err)
+}
+
 func (c *containerAdapter) pullImage(ctx context.Context) error {
 	spec := c.container.spec()
 

daemon/cluster/executor/container/controller.go

@@ -175,11 +175,14 @@ func (r *controller) Prepare(ctx context.Context) error {
 				// immutable tag or digest.
 				log.G(ctx).WithError(r.pullErr).Error("pulling image failed")
 
-				// If the pull failed with an authentication error, return it
-				// so the actual cause is propagated instead of the misleading
+				// If the image is not available locally, the pull error is the
+				// actual cause, so return it instead of the misleading
 				// "No such image" error that would otherwise result from the
-				// container create below.
-				if cerrdefs.IsUnauthorized(r.pullErr) {
+				// container create below. If the image _is_ available, keep
+				// going: a task whose image is already on the node must not be
+				// failed just because the registry could not be reached, for
+				// example when the task carries no registry credentials.
+				if !r.adapter.imageExists(ctx) {
 					return r.pullErr
 				}
 			}

integration/service/create_pull_test.go

@@ -0,0 +1,126 @@
+package service
+
+import (
+	"context"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	swarmtypes "github.com/moby/moby/api/types/swarm"
+	"github.com/moby/moby/client"
+	"github.com/moby/moby/v2/integration/internal/swarm"
+	"github.com/moby/moby/v2/internal/testutil/daemon"
+	"gotest.tools/v3/assert"
+	"gotest.tools/v3/poll"
+	"gotest.tools/v3/skip"
+)
+
+// unauthorizedRegistry returns the address of a registry that rejects every
+// request with an UNAUTHORIZED error code. The response deliberately carries no
+// WWW-Authenticate challenge: the pull then fails at the manifest request with
+// the registry's own error, which is the case this test is about. With a Bearer
+// challenge the pull fails while fetching a token instead, and that error is
+// reported differently.
+func unauthorizedRegistry(t *testing.T, repo string) string {
+	t.Helper()
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		w.Header().Set("Docker-Distribution-Api-Version", "registry/2.0")
+		w.WriteHeader(http.StatusUnauthorized)
+		_, _ = fmt.Fprintf(w,
+			`{"errors":[{"code":"UNAUTHORIZED","message":"unauthorized to access repository: %s, action: pull"}]}`,
+			repo,
+		)
+	}))
+	t.Cleanup(srv.Close)
+
+	// The registry is served over plain HTTP, which the daemon allows without
+	// configuration only for loopback addresses; httptest binds to 127.0.0.1.
+	return strings.TrimPrefix(srv.URL, "http://")
+}
+
+// TestServiceCreateUnauthorizedRegistry verifies how a task whose image cannot
+// be pulled is handled: the task runs if the image is already present on the
+// node, and is rejected with the registry error if it is not.
+//
+// A service created without registry credentials (no "--with-registry-auth")
+// cannot have its digest resolved and cannot be pulled by the node, so an
+// image that is already present locally is the only thing the task can run.
+func TestServiceCreateUnauthorizedRegistry(t *testing.T) {
+	skip.If(t, testEnv.IsRemoteDaemon, "the test registry is only reachable from the test host")
+	skip.If(t, testEnv.DaemonInfo.OSType == "windows", "the test uses a Linux busybox image")
+	ctx := setupTest(t)
+
+	// Test daemons are started with DOCKER_SERVICE_PREFER_OFFLINE_IMAGE=1,
+	// which makes the executor skip the pull altogether; opt out of it, as the
+	// pull is what is being tested here.
+	d := swarm.NewSwarm(ctx, t, testEnv, daemon.WithEnvVars("DOCKER_SERVICE_PREFER_OFFLINE_IMAGE=0"))
+	defer d.Stop(t)
+	apiClient := d.NewClientT(t)
+	defer apiClient.Close()
+
+	t.Run("image present locally", func(t *testing.T) {
+		const repo = "testing/present"
+		img := unauthorizedRegistry(t, repo) + "/" + repo + ":latest"
+
+		// Make the image available on the node under a name that resolves to
+		// the unauthorized registry. It is never pushed there: the task has to
+		// run from the local image store.
+		d.LoadBusybox(ctx, t)
+		_, err := apiClient.ImageTag(ctx, client.ImageTagOptions{Source: "busybox:latest", Target: img})
+		assert.NilError(t, err)
+
+		serviceID := swarm.CreateService(ctx, t, d,
+			swarm.ServiceWithImage(img),
+			swarm.ServiceWithCommand([]string{"top"}),
+			swarm.ServiceWithReplicas(1),
+		)
+		poll.WaitOn(t, swarm.RunningTasksCount(ctx, apiClient, serviceID, 1), swarm.ServicePoll)
+	})
+
+	t.Run("image missing locally", func(t *testing.T) {
+		const repo = "testing/missing"
+		img := unauthorizedRegistry(t, repo) + "/" + repo + ":latest"
+
+		serviceID := swarm.CreateService(ctx, t, d,
+			swarm.ServiceWithImage(img),
+			swarm.ServiceWithCommand([]string{"top"}),
+			swarm.ServiceWithReplicas(1),
+		)
+		// The pull failure is the actual cause of the task not starting, so it
+		// is what the task status must report.
+		poll.WaitOn(t, taskRejectedWithPullError(ctx, apiClient, serviceID, repo), swarm.ServicePoll)
+	})
+}
+
+// taskRejectedWithPullError waits for a task of serviceID to be rejected, and
+// checks that it reports the failure to pull repo rather than the misleading
+// "No such image" from the container create that follows the pull. The wording
+// of the pull error itself is left to the image store: the graphdriver reports
+// the registry's "unauthorized" error, the containerd store reports a
+// reference it could not resolve.
+func taskRejectedWithPullError(ctx context.Context, apiClient client.TaskAPIClient, serviceID, repo string) func(poll.LogT) poll.Result {
+	return func(log poll.LogT) poll.Result {
+		taskList, err := apiClient.TaskList(ctx, client.TaskListOptions{
+			Filters: make(client.Filters).Add("service", serviceID),
+		})
+		if err != nil {
+			return poll.Error(err)
+		}
+		for _, task := range taskList.Items {
+			if task.Status.State != swarmtypes.TaskStateRejected {
+				continue
+			}
+			switch {
+			case strings.Contains(task.Status.Err, "No such image"):
+				return poll.Error(fmt.Errorf("task %s reports the container create failure %q instead of the pull failure", task.ID, task.Status.Err))
+			case !strings.Contains(task.Status.Err, repo):
+				return poll.Error(fmt.Errorf("task %s rejected with %q, expected it to name %s", task.ID, task.Status.Err, repo))
+			}
+			return poll.Success()
+		}
+		return poll.Continue("waiting for a task of service %s to be rejected", serviceID)
+	}
+}