New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moving docker service digest pinning to client side #32388

Merged
merged 2 commits into from May 16, 2017

Conversation

@nishanttotla
Contributor

nishanttotla commented Apr 5, 2017

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 #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)
goshi

This is Gosha, the friendliest cat in Berkeley.

Show outdated Hide outdated client/service_create.go Outdated
@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 5, 2017

Contributor

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.

Contributor

aaronlehmann commented Apr 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla Apr 5, 2017

Contributor

@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?

Contributor

nishanttotla commented Apr 5, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 6, 2017

Contributor

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?

Contributor

aaronlehmann commented Apr 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla Apr 6, 2017

Contributor

@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.

Contributor

nishanttotla commented Apr 6, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 6, 2017

Contributor

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.

Contributor

aaronlehmann commented Apr 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla Apr 6, 2017

Contributor

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

Contributor

nishanttotla commented Apr 6, 2017

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

@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla Apr 6, 2017

Contributor

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.

Contributor

nishanttotla commented Apr 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla Apr 6, 2017

Contributor

@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.

Contributor

nishanttotla commented Apr 6, 2017

@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.

Show outdated Hide outdated api/types/client.go Outdated
Show outdated Hide outdated client/service_create.go Outdated
Show outdated Hide outdated client/service_update.go Outdated
@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla Apr 6, 2017

Contributor

@aaronlehmann: addressed your comments.

Contributor

nishanttotla commented Apr 6, 2017

@aaronlehmann: addressed your comments.

Show outdated Hide outdated client/service_create.go Outdated

@dnephin dnephin referenced this pull request Apr 7, 2017

Closed

Add manifest command to docker cli #27455

3 of 6 tasks complete

@vieux vieux added this to the 17.06 milestone Apr 7, 2017

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

@clnperez

This comment has been minimized.

Show comment
Hide comment
@clnperez

clnperez Apr 13, 2017

Contributor

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?

Contributor

clnperez commented Apr 13, 2017

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?

@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla Apr 13, 2017

Contributor

@clnperez it talks to the registry directly, does not look at local images at all.

Contributor

nishanttotla commented Apr 13, 2017

@clnperez it talks to the registry directly, does not look at local images at all.

@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla May 1, 2017

Contributor

Resolved the merge conflict, and updated the PR to reflect changes in #32061. CI still fails of course.

Contributor

nishanttotla commented May 1, 2017

Resolved the merge conflict, and updated the PR to reflect changes in #32061. CI still fails of course.

@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla May 6, 2017

Contributor

Moving CLI changes to docker/cli#30 and rebasing this PR.

Contributor

nishanttotla commented May 6, 2017

Moving CLI changes to docker/cli#30 and rebasing this PR.

@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla May 12, 2017

Contributor

Disabled digest pinning for client versions < 1.30.

Contributor

nishanttotla commented May 12, 2017

Disabled digest pinning for client versions < 1.30.

Show outdated Hide outdated client/service_create.go Outdated
Show outdated Hide outdated client/service_update.go Outdated
Show outdated Hide outdated client/service_create.go Outdated
@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 13, 2017

Contributor

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.

Contributor

aaronlehmann commented May 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla May 13, 2017

Contributor

@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.

Contributor

nishanttotla commented May 13, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@aluzzardi

aluzzardi May 15, 2017

Member

@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?

Member

aluzzardi commented May 15, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla May 15, 2017

Contributor

@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.

Contributor

nishanttotla commented May 15, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 15, 2017

Contributor

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.

Contributor

aaronlehmann commented May 15, 2017

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

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla May 15, 2017

Contributor

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

Contributor

nishanttotla commented May 15, 2017

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

nishanttotla added some commits Apr 5, 2017

Moving docker service digest pinning to client side
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
Disabling digest pinning for API versions < 1.30
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 15, 2017

Contributor

LGTM

Contributor

aaronlehmann commented May 15, 2017

LGTM

// 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)

This comment has been minimized.

@tiborvass

tiborvass May 16, 2017

Collaborator

the \n in the middle like that is kinda unusual

@tiborvass

tiborvass May 16, 2017

Collaborator

the \n in the middle like that is kinda unusual

This comment has been minimized.

@nishanttotla

nishanttotla May 16, 2017

Contributor

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

@nishanttotla

nishanttotla May 16, 2017

Contributor

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

// 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

This comment has been minimized.

@tiborvass

tiborvass May 16, 2017

Collaborator

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

@tiborvass

tiborvass May 16, 2017

Collaborator

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

This comment has been minimized.

@nishanttotla

nishanttotla May 16, 2017

Contributor

@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).

@nishanttotla

nishanttotla May 16, 2017

Contributor

@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).

This comment has been minimized.

@aaronlehmann

aaronlehmann May 16, 2017

Contributor

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

@aaronlehmann

aaronlehmann May 16, 2017

Contributor

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

This comment has been minimized.

@tiborvass

tiborvass May 16, 2017

Collaborator

@aaronlehmann my bad i thought this was server side.

@tiborvass

tiborvass May 16, 2017

Collaborator

@aaronlehmann my bad i thought this was server side.

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass May 16, 2017

Collaborator

LGTM

Collaborator

tiborvass commented May 16, 2017

LGTM

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

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 34156 has succeeded
Details
janky Jenkins build Docker-PRs 42755 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 3140 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 13990 has succeeded
Details
z Jenkins build Docker-PRs-s390x 2856 has succeeded
Details

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

@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla May 17, 2017

Contributor

Adding link back to #31348

Contributor

nishanttotla commented May 17, 2017

Adding link back to #31348

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment