Moving docker service digest pinning to client side by nishanttotla · Pull Request #32388 · moby/moby

nishanttotla · 2017-04-05T22:49:36Z

This PR is an alternative proposal to #32384, based on discussions with @aluzzardi @dhiltgen @alexmavr.

The heavy lifting in this case happens in the client package, which is easier to use for third-party clients, and they get the benefit of client-side digest pinning without having to do it themselves.

cc @aaronlehmann

- What I did
Digest pinning was introduced for Docker services in #28173, and it was performed on the daemon. After #32061, I moved digest pinning to the client, simplifying it.

- How I did it
The client makes a call to /images/image_name/manifest to retrieve manifest information, which contains the image digest.

- How to verify it
docker service create --name test alpine sleep 1000 then docker service inspect test should contain an image with the digest.

- Description for the changelog

Docker service image pin by digest moved to client side.

- WIP

Merge Adding Docker API endpoint to inspect image manifest #32061
Return warning when pinning fails
Disable daemon side digest pinning for new API versions (unclear if this is necessary)

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

This is Gosha, the friendliest cat in Berkeley.

alexmavr · 2017-04-05T22:59:02Z

client/service_create.go

What happens if the Image is already a manifest? It looks like we're going to throw a warning

You mean the image already contains a digest? Then the digest will be used to retrieve the manifest, and platform information added (this is not part of this PR, and will come later in a separate PR). Without the platform information stuff, we could just skip this call altogether.

aaronlehmann · 2017-04-05T23:21:40Z

I'm okay with this approach but I think we should add a flag in ServiceCreateOptions to let API users opt in or opt out. Otherwise it would be impossible to use the client to create a service that isn't pinned to a digest.

nishanttotla · 2017-04-05T23:27:00Z

@aaronlehmann I agree, I'll add that. Are there any cons to this approach, apart from having to move digest pinning away from content-trust?

aaronlehmann · 2017-04-06T00:13:21Z

I think it's problematic for the "service update" case. We should only be messing with the image field or the platform info if the user actually specified --image on the command line - it shouldn't be a side effect of an unrelated change like scaling the service. In the CLI it's easy to gate this stuff with an if flags.Changed("image"), but if it's in the client and it's something we have to explicitly enable or disable, it seems a bit more awkward. Not a dealbreaker but I think it's definitely awkward for the update case if we do it this way.

What if we moved resolveServiceImageDigest from the other PR into client?

nishanttotla · 2017-04-06T17:10:49Z

@aaronlehmann I'll update ServiceUpdateOptions to include a field indicating if flags.Changed("image").

When you say moving resolveServiceImageDigest to the client package, do you also want to move content trust here? It doesn't look difficult to do, and also alleviates some of my concerns with this PR separating digest pinning and content trust into separate places.

aaronlehmann · 2017-04-06T17:15:24Z

When you say moving resolveServiceImageDigest to the client package, do you also want to move content trust here? It doesn't look difficult to do, and also alleviates some of my concerns with this PR separating digest pinning and content trust into separate places.

I'm not sure. It sounds like a good idea if it's possible. But I seem to remember that content trust involves a lot of code that lives in cli.

nishanttotla · 2017-04-06T17:19:33Z

@aaronlehmann let me take a look. If it's easy to refactor, I'll include it in this PR.

nishanttotla · 2017-04-06T17:36:58Z

Update: Trust code is definitely entrenched in the CLI, and while it seems "refactorable", I don't think it should be a part of this PR because it is associated with content trust for running containers as well.

nishanttotla · 2017-04-06T19:25:32Z

@aaronlehmann: updated this PR. Added a field called UpdateImage to ServiceCreateOptions that gets set when the --image flag is provided. In the future, it can also be easily used to disable digest pinning from the client.

I also updated resolveServiceImageDigest to resolveServiceImageDigestContentTrust to indicate that it only resolves digest by content trust. In the future, we should move this function to the client package.

aaronlehmann · 2017-04-06T22:35:52Z

api/types/client.go

Let's name this something like QueryRegistry, since I guess it will be extended to things other than pinning by digest in the future (platform info...).

I think we should support the same flag for ServiceCreate, so that digest pinning is not mandatory for all client users.

Let's not mention the --image flag (except possibly as an example), because the client is independent of the Docker CLI.

aaronlehmann · 2017-04-06T22:36:25Z

client/service_create.go

We must not overwrite the Image field here if it already contains a digest (for example, if digest pinning was already done through content trust).

aaronlehmann · 2017-04-06T22:36:29Z

client/service_update.go

We must not overwrite the Image field here if it already contains a digest (for example, if digest pinning was already done through content trust).

nishanttotla · 2017-04-06T23:31:16Z

@aaronlehmann: addressed your comments.

aaronlehmann · 2017-04-06T23:50:42Z

client/service_create.go

This doesn't seem right. We should be replacing the Image field if the image reference doesn't have a digest, not if it doesn't parse.

The reasoning for this was to not update the image if it parses to a legitimate canonical reference AND a manifest is retrieved. Is that not what this is doing? I wanted to reuse something from the distribution/reference package. Do you recommend something else?

Where does it check that the reference is canonical?

func ParseNamed(s string) (Named, error) { named, err := ParseNormalizedNamed(s) if err != nil { return nil, err } if named.String() != s { return nil, ErrNameNotCanonical } return named, nil }

This check I believed was doing that, but perhaps I'm getting it wrong.

f named.String() != s { return nil, ErrNameNotCanonical }

Adding a type check for reference.Canonical should fix that?

That "canonical" is something else :(

Adding a type check for reference.Canonical should fix that?

Yes

Ah okay, let me add that. I wasn't going by the word "canonical" as much as the string check, but it's embarrassing now because that doesn't make any sense. Fixing it now.

clnperez · 2017-04-13T18:38:06Z

Since @dnephin mentioned that this my conflict with my PR (/pull/27455), @nishanttotla can you clarify whether the imageManifest func you're adding pulls from the registry or expects the image to be pre-pulled?

aaronlehmann · 2017-05-12T23:52:27Z

client/service_create.go

Can be var distErr error (but it doesn't matter).

aaronlehmann · 2017-05-12T23:57:34Z

daemon/cluster/services.go

Is there a good reason to continue supporting DOCKER_SERVICE_PREFER_OFFLINE_IMAGE? This was for the integration tests, but if the integration tests use the latest API version, they shouldn't hit this code path anyway.

That said, it may take some work to get things right with the integration tests after removing this. It may be best to remove it in a followup. Speaking of which, how will we prevent CLI-oriented integration tests from trying to resolve digests for local images on Docker Hub? Should we add a CLI option to prevent digest pinning?

This seems correct, we shouldn't have to keep DOCKER_SERVICE_PREFER_OFFLINE_IMAGE, but it does absolutely no harm right now, and isn't user facing, so we should just wait on removing it. I'll add a comment indicating this.

If the CLI-oriented tests will purely run CLI commands, then we will have to add a flag, there is no other way. Given that we know of scenarios where disabling is required, we should add the flag.

aaronlehmann · 2017-05-13T00:00:45Z

Mostly LGTM, but a little concerned that moving digest pinning to the client side will break the DOCKER_SERVICE_PREFER_OFFLINE_IMAGE workaround, and cause integration tests that use docker service create or docker service update --image in the CLI to interact with the hub when they really should not. I think we either need to add a flag to disable digest pinning in the CLI, or rewrite the applicable integration tests to avoid the CLI.

nishanttotla · 2017-05-13T04:39:55Z

@aluzzardi what do you think about adding a flag? It shouldn't be hard to do, given that this PR already has an API-level option to disable the registry lookup.

aluzzardi · 2017-05-15T18:54:10Z

@nishanttotla @aaronlehmann A flag sounds great, I think it was one of the goals of moving resolution client side.

May I suggest --no-resolve-image or something like that?

nishanttotla · 2017-05-15T18:58:28Z

@aluzzardi @aaronlehmann I'll open a follow-up PR for that, since most changes will be in docker/cli, and we might need some time to agree on the flag name and behavior.

aaronlehmann · 2017-05-15T19:51:37Z

That sounds okay. Hopefully the circular dependency between moby and cli doesn't put us in a state where integration tests are trying to perform digest pinning (which makes them flaky). Ideally the commit that updates moby to point to a revision of cli that does the digest pinning will also update the integration tests to use the new flag that disables it.

nishanttotla · 2017-05-15T20:40:32Z

I'll add the flag to the same PR (docker/cli#30) that enables the QueryRegistry option in the CLI.

Signed-off-by: Nishant Totla <[email protected]>

aaronlehmann · 2017-05-15T23:53:04Z

LGTM

tiborvass · 2017-05-16T21:45:26Z

client/service_create.go

+// image name that could not be pinned by digest. The formatting
+// is hardcoded, but could me made smarter in the future
+func digestWarning(image string) string {
+	return fmt.Sprintf("image %s could not be accessed on a registry to record\nits digest. Each node will access %s independently,\npossibly leading to different nodes running different\nversions of the image.\n", image, image)


the \n in the middle like that is kinda unusual

@tiborvass this is to add line breaks, since the text is big. This is how it is also in the daemon right now.

tiborvass · 2017-05-16T21:47:53Z

api/types/client.go

+	// contacting a registry. A registry may be contacted to retrieve
+	// the image digest and manifest, which in turn can be used to update
+	// platform or other information about the service.
+	QueryRegistry bool


@nishanttotla I'm wondering if the default shouldn't be the current behavior. SkipRegistryQuery ?

@tiborvass the corresponding CLI PR is docker/cli#30.

The idea is that QueryRegistry should be set on all service create commands unless the user explicitly provides a flag to prevent it, and for all service update commands that update the image, using the --image flag (also unless the user explicitly asks to not do it with the flag).

@tiborvass: The current behavior is that the client does not perform this query.

@aaronlehmann my bad i thought this was server side.

tiborvass · 2017-05-16T22:23:08Z

LGTM

nishanttotla · 2017-05-17T01:43:00Z

Adding link back to #31348

GordonTheTurtle added the status/0-triage label Apr 5, 2017

alexmavr reviewed Apr 5, 2017

View reviewed changes

nishanttotla force-pushed the pin-by-digest-on-client-alternative branch 3 times, most recently from f0467f7 to 0726fea Compare April 6, 2017 19:20

nishanttotla force-pushed the pin-by-digest-on-client-alternative branch from 0726fea to 1160815 Compare April 6, 2017 19:25

nishanttotla mentioned this pull request Apr 6, 2017

[WIP] Moving docker service digest pinning to client side #32384

Closed

3 tasks

aaronlehmann reviewed Apr 6, 2017

View reviewed changes

nishanttotla force-pushed the pin-by-digest-on-client-alternative branch 2 times, most recently from 286eb0e to 514baad Compare April 6, 2017 23:30

aaronlehmann reviewed Apr 6, 2017

View reviewed changes

nishanttotla force-pushed the pin-by-digest-on-client-alternative branch from 514baad to 39a9b94 Compare April 7, 2017 01:08

vdemeester added status/1-design-review and removed status/0-triage labels Apr 7, 2017

nishanttotla mentioned this pull request Apr 7, 2017

Adding Docker API endpoint to inspect image manifest #32061

Merged

dnephin mentioned this pull request Apr 7, 2017

Add manifest command to docker cli #27455

Closed

6 tasks

vieux added this to the 17.06 milestone Apr 7, 2017

nishanttotla changed the title ~~[WIP][Alternative] Moving docker service digest pinning to client side~~ [WIP] Moving docker service digest pinning to client side Apr 11, 2017

aaronlehmann reviewed May 12, 2017

View reviewed changes

client/service_create.go Outdated

Copy link

aaronlehmann May 12, 2017

Choose a reason for hiding this comment

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

Can be var distErr error (but it doesn't matter).

aaronlehmann reviewed May 12, 2017

View reviewed changes

nishanttotla force-pushed the pin-by-digest-on-client-alternative branch from b2368aa to 6a43422 Compare May 13, 2017 04:38

nishanttotla added 2 commits May 15, 2017 16:42

Moving docker service digest pinning to client side

c1635c1

Signed-off-by: Nishant Totla <[email protected]>

Disabling digest pinning for API versions < 1.30

c0afd9c

Signed-off-by: Nishant Totla <[email protected]>

nishanttotla force-pushed the pin-by-digest-on-client-alternative branch from 6a43422 to c0afd9c Compare May 15, 2017 23:43

aaronlehmann added status/2-code-review and removed status/1-design-review labels May 15, 2017

nishanttotla mentioned this pull request May 16, 2017

api: Use OCI types for Digest and Platform #33237

Merged

tiborvass reviewed May 16, 2017

View reviewed changes

tiborvass added the impact/changelog label May 16, 2017

tiborvass merged commit d6f4fe9 into moby:master May 16, 2017

nishanttotla deleted the pin-by-digest-on-client-alternative branch May 16, 2017 22:24

nishanttotla mentioned this pull request May 17, 2017

Adding compatible platforms to service spec #33239

Merged

mlaventure mentioned this pull request Jun 6, 2017

Update 17.06 CHANGELOG with rc2 changes docker-archive/docker-ce#40

Merged

dnephin mentioned this pull request Jul 26, 2017

Remove application logic from client/ package #34268

Open

matthanley mentioned this pull request Jul 31, 2017

Regression: Service image no longer pinned by hash in API v1.26 #34324

Open

deviantony mentioned this pull request Jun 20, 2018

feat(services): add the ability to pull latest image when updating a … portainer/portainer#1984

Merged

thaJeztah mentioned this pull request Apr 14, 2020

swarm: fix API version detection #40816

Merged

thaJeztah mentioned this pull request Jul 10, 2023

client: remove custom "headers" type (use http.Header), and omit "version" header on API >= 1.30 #45923

Merged

Conversation

nishanttotla commented Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexmavr Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Apr 5, 2017

Uh oh!

nishanttotla commented Apr 5, 2017

Uh oh!

aaronlehmann commented Apr 6, 2017

Uh oh!

nishanttotla commented Apr 6, 2017

Uh oh!

aaronlehmann commented Apr 6, 2017

Uh oh!

nishanttotla commented Apr 6, 2017

Uh oh!

nishanttotla commented Apr 6, 2017

Uh oh!

nishanttotla commented Apr 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nishanttotla commented Apr 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clnperez commented Apr 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented May 13, 2017

Uh oh!

nishanttotla commented May 13, 2017

Uh oh!

aluzzardi commented May 15, 2017

Uh oh!

nishanttotla commented May 15, 2017

Uh oh!

aaronlehmann commented May 15, 2017

Uh oh!

nishanttotla commented May 15, 2017

Uh oh!

aaronlehmann commented May 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nishanttotla commented Apr 5, 2017 •

edited

Loading

alexmavr Apr 5, 2017 •

edited

Loading