Visitar URL original
daemon/cluster: Cluster.imageWithDigestString: include mirrors to resolve digest by thaJeztah · Pull Request #47024 · moby/moby · GitHub
Skip to content

daemon/cluster: Cluster.imageWithDigestString: include mirrors to resolve digest - #47024

Merged
thaJeztah merged 1 commit into
moby:masterfrom
thaJeztah:fix_daemon_side_digest_resolve
Jan 5, 2024
Merged

daemon/cluster: Cluster.imageWithDigestString: include mirrors to resolve digest#47024
thaJeztah merged 1 commit into
moby:masterfrom
thaJeztah:fix_daemon_side_digest_resolve

Conversation

@thaJeztah

@thaJeztah thaJeztah commented Jan 4, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

daemon/cluster: Cluster.imageWithDigestString: include mirrors to resolve digest

If the daemon is configured to use a mirror for the default (Docker Hub)
registry, the endpoint did not fall back to querying the upstream if the mirror
did not contain the given reference.

For pull-through registry-mirrors, this was not a problem, as in that case the
registry would forward the request, but for other mirrors, no fallback would
happen. This was inconsistent with how "pulling" images handled this situation;
when pulling images, both the mirror and upstream would be tried.

This patch brings the daemon-side lookup of image-manifests on-par with the
client-side lookup (the GET /distribution endpoint) as used in API 1.30 and
higher.

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah thaJeztah mentioned this pull request Jan 4, 2024
Merged
@thaJeztah
daemon/cluster: Cluster.imageWithDigestString: include mirrors to res…
07d2ad3 
…olve digest

If the daemon is configured to use a mirror for the default (Docker Hub)
registry, the endpoint did not fall back to querying the upstream if the mirror
did not contain the given reference.

For pull-through registry-mirrors, this was not a problem, as in that case the
registry would forward the request, but for other mirrors, no fallback would
happen. This was inconsistent with how "pulling" images handled this situation;
when pulling images, both the mirror and upstream would be tried.

This patch brings the daemon-side lookup of image-manifests on-par with the
client-side lookup (the GET /distribution endpoint) as used in API 1.30 and
higher.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
thaJeztah force-pushed the fix_daemon_side_digest_resolve branch from 9ffd11c to 07d2ad3 Compare January 4, 2024 15:53
@thaJeztah
thaJeztah marked this pull request as ready for review January 4, 2024 15:54
@thaJeztah

thaJeztah commented Jan 4, 2024

Copy link
Copy Markdown
Member Author

FWIW; I kept this one separate from #47023, as this is only fixing a code-path for API < 1.30, and I was on the fence if it was worth fixing / changing behavior for old API versions. We never received reports (that I'm aware of) for this case, but OTOH, it cleans up the extra function, and we have not yet deprecated API 1.30 in #46887).

If possible we should also move this part to the API (I think), and adjust behavior there, but I'd have to have a closer look if that's possible (it may depend on other parts of "service")

@thaJeztah
thaJeztah requested a review from corhere January 4, 2024 16:03
corhere
corhere approved these changes Jan 4, 2024
View reviewed changes

@corhere corhere left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a tough call. Someone might be depending on the buggy behaviour. Since there is no way to explicitly opt out of resolving the image to a digest on API <v1.30, they might even have intentionally crafted an environment which triggers the bug in order to prevent image tags from being resolved to digests. However I can't imagine that being terribly useful or desirable. You'd have to manually pull the updated images into each node's image cache to roll out an update with a moving tag.

A more plausible "breakage" scenario after fixing this bug would be users running docker service update triggering a rollout of an updated image for the first time in months/years and having the (unknowingly untested) new images break in production. They may blame the daemon upgrade because as far as they know the images were tested and worked previously. On the other hand, they'd run into issues sooner or later regardless, particularly if a task gets rescheduled onto a new node which doesn't already have the stale image cached.

In either scenario, they would also have been "affected" by a client upgrade.

Given that the API contract pre-v1.30 was to pin the image digest whenever possible, I think it's a fair argument that fixing the bug would not be a violation. The users in my second scenario would start to see the behaviour they had always expected, and the Hyrum's Law abusers in the first scenario would have a workaround in the form of upgrading their client and explicitly opting out of resolving images. I'm good with merging this so long as we call this change out in the release notes and describe how users could be impacted.

@thaJeztah

thaJeztah commented Jan 4, 2024

Copy link
Copy Markdown
Member Author

I think the docker service update scenario may only happen if they also pass either --force or --image to update the image, otherwise I think it stays pinned to the existing digest.

Or maybe I have my wires crossed there 🤔 (--no-resolve-image is also an option, but I think that's to be used when updating the image, and you don't want the image to be pinned).

@corhere

corhere commented Jan 4, 2024

Copy link
Copy Markdown
Contributor

Oh yeah, I now see the behaviour is a bit more subtle. Any update to a container-type task pre-v1.30 triggers a resolve of the image reference. Since the image reference gets overwritten in-place, any references which were previously resolved to digests will remain unchanged since the digest reference would be resolved back to the same digest. Any tag references would get resolved to digests on the first service update after the daemon upgrade.

moby/daemon/cluster/services.go

Lines 360 to 387 in 677d41a

// pin image by digest for API versions < 1.30
// TODO(nishanttotla): The check on "DOCKER_SERVICE_PREFER_OFFLINE_IMAGE"
// should be removed in the future. Since integration tests only use the
// latest API version, so this is no longer required.
if os.Getenv("DOCKER_SERVICE_PREFER_OFFLINE_IMAGE") != "1" && queryRegistry {
digestImage, err := c.imageWithDigestString(ctx, newCtnr.Image, authConfig)
if err != nil {
log.G(ctx).Warnf("unable to pin image %s to digest: %s", newCtnr.Image, err.Error())
// warning in the client response should be concise
resp.Warnings = append(resp.Warnings, digestWarning(newCtnr.Image))
} else if newCtnr.Image != digestImage {
log.G(ctx).Debugf("pinning image %s by digest: %s", newCtnr.Image, digestImage)
newCtnr.Image = digestImage
} else {
log.G(ctx).Debugf("updating service using supplied digest reference %s", newCtnr.Image)
}
// Replace the context with a fresh one.
// If we timed out while communicating with the
// registry, then "ctx" will already be expired, which
// would cause UpdateService below to fail. Reusing
// "ctx" could make it impossible to update a service
// if the registry is slow or unresponsive.
var cancel func()
ctx = compatcontext.WithoutCancel(ctx)
ctx, cancel = c.getRequestContext(ctx)
defer cancel()
}

@thaJeztah

thaJeztah commented Jan 5, 2024

Copy link
Copy Markdown
Member Author

Oh yeah, I now see the behaviour is a bit more subtle.

Yes, indeed! It's all a bit messy, which is also why I was pondering if that code should be moved to the API (considering it adjusting API response, similar to what we do for other cases), but I'd need to have a closer look if possible.

Looks like we're ok with this fix; let me bring this one in for v25;

I'll add cherry-pick labels if we consider backporting this fix (and #47023) to LTS branches, but we can hold-off doing so (consider the labels a "reminder" so that we don't forget).

@thaJeztah
thaJeztah merged commit c3c1ee0 into moby:master Jan 5, 2024
@thaJeztah
thaJeztah deleted the fix_daemon_side_digest_resolve branch January 5, 2024 11:13
@thaJeztah thaJeztah added this to the 25.0.0 milestone Jan 5, 2024
@ogulcanaydogan ogulcanaydogan mentioned this pull request Oct 12, 2026
Closed
@thaJeztah thaJeztah mentioned this pull request Oct 13, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@corhere corhere corhere approved these changes
@cpuguy83 cpuguy83 cpuguy83 approved these changes

Assignees

No one assigned

Labels

area/distribution Image Distribution area/swarm impact/changelog kind/bugfix PR's that fix bugs status/2-code-review

Projects

None yet

Milestone

25.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@thaJeztah @corhere @cpuguy83