Enable client side digest pinning for stack deploy #121

nishanttotla · 2017-05-22T21:44:02Z

This PR is a follow-up to #30.

It also adds a --resolve-image flag to stack deploy to provide digest pinning options.

aaronlehmann · 2017-05-24T23:41:16Z

I think we need this for 17.06 because if we didn't have it, we'd lose digest pinning from stack deploy, and that would be a regression.

nishanttotla · 2017-05-25T00:08:43Z

@dnephin @tiborvass what can we do to make sure this makes it into 17.06?

Also opened moby/moby#33386 to ensure tests don't contact registry because of this.

aaronlehmann · 2017-05-26T17:20:17Z

cli/command/stack/deploy_composefile.go

@@ -320,6 +322,13 @@ func deployServices(
 			if sendAuth {
 				updateOpts.EncodedRegistryAuth = encodedAuth
 			}
+
+			if image != service.Spec.TaskTemplate.ContainerSpec.Image {


I'm not sure this check makes sense. It means that if the digest wasn't pinned before, we won't pin the digest (or add platform information) going forward. Something like a temporary registry error would cause pinning to be turned off for future runs as well.

@aaronlehmann you're right, that would be a problem.

For service update, it's pretty simple to judge because we have a --image flag, but I couldn't find a way of deciding whether an image was updated in an updated stack file. Is there a way of discovering this at the CLI level? cc @dnephin

I think it would be best to just remove the check, so the digest and platform information are always updated.

I'm a little uncomfortable with that idea, because if the stack file contains multiple images, then it's possibly too much overhead each time any update is done.

Although it's easy to avoid that given the --no-resolve-image flag, but the some might find that frustrating UI. Anyhow, if there's no other way, then I agree that removing the check is the right thing to do.

I don't see a better way, unless we want to change the UX to make digest pinning a manual operation, rather than getting the latest digest on every deploy.

That sounds reasonable to me.

We would need to make sure there is a way to enable digest pinning if the service was originally deployed with --no-resolve-image, or vice versa.

Do we expect --no-resolve-image to persist? I expect that it would only apply to the current deploy.

For any service with a missing label we can treat that as an image name mismatch. The first deploy will cause an update to include the service image label.

Otherwise I wouldn't expect --no-resolve-image to change the label value (it will always contain the string from the config, not a digest), so the flag acts as an override.

I wouldn't expect it to persist, but just saying that storing the original name in a label and comparing with that when deciding whether to update the digest could cause us not to add a digest if there wasn't one originally. We just need to make sure to take that account in the logic.

It isn't very clear to me how having a label solves the problem. If the label contains the image that was originally deployed (without a digest because say pinning failed), then later when the stack file is updated (without --no-resolve-image flag, but the same image), how do we decide to contact the registry?

To me, the --image flag in service create and service update also conveys "user intent", something that I'm not able to grasp in stack files. Please correct me if I've misunderstood.

The label can give us the same signal as the --image flag on service commands.

Here's how it works:

in convert.Services() we add a label to each service with a key of com.docker.stack.image (or something like this) and a value of the literal string from the compose file (composetypes.Service.Image)

when we enter this deployServices() code we compare the existing label value against the new value from the compose file

image := serviceSpec.TaskTemplate.ContainerSpec.Image // has the image value changed in the compose file? if service.Spec.Labels["com.docker.stack.image"] != image { if !noResolveImage { updateOpts.QueryRegistry = true } }

This is different from the current code because we don't know if service.Spec.TaskTemplate.ContainerSpec.Image was modified or not, but we do know the label is never modified by other code.

Does that make sense?

aaronlehmann · 2017-05-27T00:35:47Z

cli/command/stack/deploy_composefile.go

@@ -320,6 +322,13 @@ func deployServices(
 			if sendAuth {
 				updateOpts.EncodedRegistryAuth = encodedAuth
 			}
+
+			if image != service.Spec.TaskTemplate.ContainerSpec.Image {


I don't see a better way, unless we want to change the UX to make digest pinning a manual operation, rather than getting the latest digest on every deploy.

dnephin · 2017-05-30T15:57:49Z

cli/command/stack/deploy_composefile.go

@@ -320,6 +322,13 @@ func deployServices(
 			if sendAuth {
 				updateOpts.EncodedRegistryAuth = encodedAuth
 			}
+
+			if image != service.Spec.TaskTemplate.ContainerSpec.Image {


If I'm understanding the issue correctly, I think the right way to handle this is by setting a label on each service with the original image name.

If the label of the existing service doesn't match the string from the config, then we know the image has changed and we should set QueryRegistry = true.

I don't think we should force update the digest on every deploy.

dnephin · 2017-05-30T16:00:23Z

cli/command/stack/deploy_composefile.go

+			// query registry if flag disabling it was not set
+			if !noResolveImage && versions.GreaterThanOrEqualTo(apiClient.ClientVersion(), "1.30") {
+				createOpts.QueryRegistry = true
+			}


Instead of checking the version twice I think we should handle the version check in runDeploy() by setting the option accordingly (set the option to true for older versions). That way noResolveImage will always be correct inside deployServices() and we won't need to check version twice.

@dnephin I addressed this comment, and the one about refactoring.

dnephin · 2017-05-30T16:01:26Z

cli/command/stack/opts.go

+	var flagNoResolveImage = "no-resolve-image"
+	flags.BoolVar(opt, flagNoResolveImage, false, "Do not query the registry to resolve image digest and supported platforms")
+	flags.SetAnnotation(flagNoResolveImage, "version", []string{"1.30"})
+}


A minor nit: typically we only use these functions for flags that are shared between commands. I think it's fine to keep this as a separate function, but maybe move it to the single file where it's used?

dnephin · 2017-06-02T02:54:38Z

cli/command/stack/deploy.go

 	return cmd
 }

 func runDeploy(dockerCli command.Cli, opts deployOptions) error {
 	ctx := context.Background()

+	// image resolution should not happen for clients older than v1.30


Even though this says "ClientVersion()" it's important to note it actually reflects the supported version of the server. The comment might be a bit misleading in that sense, but I guess it's fine.

Yes, I agree with changing this comment.

aaronlehmann · 2017-06-02T06:55:58Z

The label can give us the same signal as the --image flag on service commands. Here's how it works: in convert.Services() we add a label to each service with a key of com.docker.stack.image (or something like this) and a value of the literal string from the compose file (composetypes.Service.Image) when we enter this deployServices() code we compare the existing label value against the new value from the compose file

What about the case where digest pinning fails, and I want to retry it? I would expect rerunning docker stack deploy without --no-resolve-image to do this, but if I understand the proposal correctly, it would detect that the image has not changed in the compose file, and avoid digest pinning.

dnephin · 2017-06-02T13:39:20Z

What about the case where digest pinning fails,

If a service create/update is performed with with digest pinning enabled, but it failed to pin, I would expect the service create/update call to fail, not for the command to return success anyway. Is that not the case?

aaronlehmann · 2017-06-02T13:55:29Z

It completes successfully but warns the user that warns the user that the image is referenced by name, not by digest.

Otherwise, you would need to use a special flag to create a service using a local or pre-pulled image, which would break backward compat.

dnephin · 2017-06-02T14:17:45Z

I see. In that case I guess we need a way to say "force pinning". We should still add the label for the default case. For the force case I think we should change the flag to --resolve-image=<value> where values can be (something like): resolve (the default), never (equivalent to current flag), force set QueryRegistry even if we can't detect any changes.

aaronlehmann · 2017-06-02T14:37:45Z

SGTM

nishanttotla · 2017-06-02T23:02:52Z

I've added the label part, and adding the --resolve-image=<value> flag now.

As far as labels go though, consider this example.

service A created with image alpine:latest. Label is set to alpine:latest, and digest pinning succeeds, so spec.TaskTemplate.ContainerSpec.Image is set to alpine:latest@sha256:xxx.
alpine:latest has been updated, and now has a new digest alpine:latest@sha256:yyy.
on a redeploy, service A continues to use alpine:latest, and because the older label is the same, the digest will not be updated, unless we did --resolve-image=force.

Is this the expected UI?

aaronlehmann · 2017-06-03T20:48:23Z

As far as labels go though, consider this example.

This is why I think it's much better to (at least by default) always query the registry. Doing so matches the old behavior when this was done daemon-side, assuming stack deploy set the Image field in ContainerSpec to the value from the compose file instead of leaving the old, pinned value.

dnephin · 2017-06-05T15:22:40Z

I'm really not sure what someone would expect when they use latest. It's not pinned, so some people might expect it to update, and others might not.

Maybe the default should be force (rename to always ?), and resolve (rename to changed?) could be used when you only way to query if the image name is changed in the file?

nishanttotla · 2017-06-05T22:55:43Z

@dnephin I'm possibly restating what's been said above, but just for clarity: changing the default to force (or always, which sounds more appropriate) sounds equivalent to what @aaronlehmann is suggesting. Then the resolve (or changed) and 'never' flags will give users more control over that behavior.

With this proposal, digest pinning always happens, unless the user chooses resolve in which case it'll happen only when the image provided in the compose file does not match that which is actually running. In this scenario, if digest pinning failed to begin with, it will never be tried again unless forced. If digest pinning succeeded with resolve in the first place, then it'll still happen if the user provided only a tag.

I'd be satisfied with this sort of behavior, if we all agree on this. (I'll change the option names too)

dnephin · 2017-06-06T15:00:58Z

Yes, I think that sounds right.

In this scenario, if digest pinning failed to begin with, it will never be tried again unless forced

I don't understand this part. Why would this be the case? I think using a label fixes this problem?

nishanttotla · 2017-06-06T18:14:00Z

@dnephin my comment above explains in more detail what I mean: #121 (comment)

nishanttotla · 2017-06-07T01:59:00Z

@dnephin added validation for the CLI option, and moved label addition to convertService().

aaronlehmann · 2017-06-07T06:38:03Z

cli/command/stack/deploy.go

@@ -62,6 +71,18 @@ func runDeploy(dockerCli command.Cli, opts deployOptions) error {
 	}
 }

+// checkImageResolutionAllowed validates the opts.resolveImage command line option
+// and also turns image resolution off if the version is older than 1.30
+func checkImageResolutionAllowed(dockerCli command.Cli, opts *deployOptions) error {


validateResolveImageFlag?

aaronlehmann · 2017-06-07T06:38:19Z

cli/command/stack/deploy.go

+// checkImageResolutionAllowed validates the opts.resolveImage command line option
+// and also turns image resolution off if the version is older than 1.30
+func checkImageResolutionAllowed(dockerCli command.Cli, opts *deployOptions) error {
+	if opts.resolveImage != "always" && opts.resolveImage != "changed" && opts.resolveImage != "never" {


This could be a switch, but it's fine this way too.

I'd suggest declaring constants for the possible values, so it is harder for this to be out of sync with the code that interprets the flag values, or for typos to creep into the string literals in some places. Again, not a big deal either way.

aaronlehmann

Overall LGTM

It would be nice to have test coverage, but I'm not immediately familiar with how tests work for this portion of the code, so I'll let Daniel advise on what makes sense.

dnephin

You can write unit tests for these using a fakeClient.

dnephin · 2017-06-07T14:29:08Z

cli/compose/convert/service.go

+	// add an image label to serviceSpec
+	if serviceSpec.Labels == nil {
+		serviceSpec.Labels = make(map[string]string)
+	}


This condition is already handled by AddStackLabel().

Signed-off-by: Nishant Totla <nishanttotla@gmail.com>

codecov-io · 2017-06-07T19:41:56Z

Codecov Report

Merging #121 into master will decrease coverage by 0.06%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
- Coverage   44.79%   44.72%   -0.07%     
==========================================
  Files         171      171              
  Lines       11454    11467      +13     
==========================================
- Hits         5131     5129       -2     
- Misses       6027     6043      +16     
+ Partials      296      295       -1

nishanttotla · 2017-06-07T21:29:55Z

I've addressed comments and fixed the merge conflict.

For the test, it seems like deployServices is the function that would be ideal to test, but fakeClient seems to be an API client, and deployServices needs to use a command.Cli object. Is there one that exists for this purpose?

Alternatively, @dnephin would you suggest a different unit test? Could we make this a follow-up?

dnephin

LGTM

There is a FakeCli in cli/internal/test that is used for testing things that depend on command.Cli.

If this is time sensitive tests in a follow up are fine with me.

nishanttotla · 2017-06-08T19:41:03Z

@dnephin it is time sensitive since it must be cherry picked for rc3, so I can add the test later.

nishanttotla · 2017-06-08T23:06:05Z

I've confirmed these changes work as desired in local (manual) testing, and opened a cherry-pick PR here: docker-archive/docker-ce#44

GordonTheTurtle added the status/0-triage label May 22, 2017

nishanttotla force-pushed the digest-pinning-stack-deploy branch 2 times, most recently from 305a611 to 9b9ae49 Compare May 22, 2017 22:11

nishanttotla changed the title ~~Enable client side digest pinning for stack deploy (composefile)~~ Enable client side digest pinning for stack deploy May 22, 2017

dnephin added status/1-design-review and removed status/0-triage labels May 23, 2017

nishanttotla mentioned this pull request May 25, 2017

Don't resolve image by contacting registry for docker stack deploy tests moby/moby#33386

Closed

tiborvass added this to the 17.06.0 milestone May 25, 2017

aaronlehmann mentioned this pull request May 25, 2017

17.06.0 RC2 tracker docker/for-linux#2

Closed

23 tasks

aaronlehmann reviewed May 26, 2017

View reviewed changes

aaronlehmann suggested changes May 27, 2017

View reviewed changes

thaJeztah added the process/cherry-pick label May 30, 2017

dnephin reviewed May 30, 2017

View reviewed changes

nishanttotla force-pushed the digest-pinning-stack-deploy branch 2 times, most recently from 543b0a6 to 0db4de3 Compare June 2, 2017 00:21

dnephin reviewed Jun 2, 2017

View reviewed changes

nishanttotla force-pushed the digest-pinning-stack-deploy branch from 0db4de3 to f158368 Compare June 2, 2017 23:04

andrewhsu mentioned this pull request Jun 6, 2017

17.06.0 RC3 tracker docker/for-linux#8

Closed

40 tasks

nishanttotla force-pushed the digest-pinning-stack-deploy branch 3 times, most recently from 4ad7796 to 29b22c4 Compare June 7, 2017 01:44

aaronlehmann added status/2-code-review and removed status/1-design-review labels Jun 7, 2017

aaronlehmann reviewed Jun 7, 2017

View reviewed changes

aaronlehmann approved these changes Jun 7, 2017

View reviewed changes

dnephin reviewed Jun 7, 2017

View reviewed changes

nishanttotla force-pushed the digest-pinning-stack-deploy branch 4 times, most recently from 2c53999 to fb37e1e Compare June 7, 2017 18:52

nishanttotla added 2 commits June 7, 2017 12:30

Enable client side digest pinning for stack deploy

9f1bea2

Signed-off-by: Nishant Totla <nishanttotla@gmail.com>

Change --no-resolve-image flag to --resolve-image string flag

f790e83

Signed-off-by: Nishant Totla <nishanttotla@gmail.com>

nishanttotla force-pushed the digest-pinning-stack-deploy branch from fb37e1e to f790e83 Compare June 7, 2017 19:30

dnephin approved these changes Jun 8, 2017

View reviewed changes

aaronlehmann merged commit 4d98088 into docker:master Jun 8, 2017

nishanttotla deleted the digest-pinning-stack-deploy branch June 8, 2017 20:33

nishanttotla mentioned this pull request Jun 8, 2017

[cherry-pick] Enable client side digest pinning for stack deploy docker-archive/docker-ce#44

Merged

thaJeztah added process/cherry-picked and removed process/cherry-pick labels Jun 10, 2017

andrewhsu mentioned this pull request Jul 26, 2017

WIP [17.07] fix tests docker-archive/docker-ce#140

Closed

albers mentioned this pull request Oct 9, 2017

Add bash completion for stack deploy --resolve-image #611

Merged

Enable client side digest pinning for stack deploy #121

Enable client side digest pinning for stack deploy #121

Conversation

nishanttotla commented May 22, 2017 • edited Loading

aaronlehmann commented May 24, 2017

nishanttotla commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nishanttotla May 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronlehmann commented Jun 2, 2017 via email

dnephin commented Jun 2, 2017

aaronlehmann commented Jun 2, 2017

dnephin commented Jun 2, 2017

aaronlehmann commented Jun 2, 2017 via email

nishanttotla commented Jun 2, 2017 • edited Loading

aaronlehmann commented Jun 3, 2017

dnephin commented Jun 5, 2017

nishanttotla commented Jun 5, 2017

dnephin commented Jun 6, 2017

nishanttotla commented Jun 6, 2017

nishanttotla commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronlehmann left a comment

Choose a reason for hiding this comment

dnephin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 7, 2017 • edited Loading

Codecov Report

nishanttotla commented Jun 7, 2017

dnephin left a comment

Choose a reason for hiding this comment

nishanttotla commented Jun 8, 2017

nishanttotla commented Jun 8, 2017

nishanttotla commented May 22, 2017 •

edited

Loading

nishanttotla May 26, 2017 •

edited

Loading

nishanttotla commented Jun 2, 2017 •

edited

Loading

codecov-io commented Jun 7, 2017 •

edited

Loading