prune commands : make sure label filters are considered #21

aduermael · 2017-05-02T23:51:09Z

Fixes #17

Checking server API version is at least 1.29 to accept prune commands with label filters.

Signed-off-by: Adrien Duermael adrien@duermael.com

vdemeester · 2017-05-03T10:32:10Z

cli/command/utils.go

@@ -115,5 +117,23 @@ func PruneFilters(dockerCli Cli, pruneFilters filters.Args) filters.Args {
 		pruneFilters.Add(parts[0], parts[1])
 	}

+	// Server APIs older than 1.29 will just ignore label filters.


I feel we should do that server side though (the cli should be as dumb as possible)
cc @thaJeztah @dnephin

Yes, that would be more correct, but the problem is that the issue is with old servers, so it's hard to fix now.

The server should actually already error with it receives invalid filters. I'm not sure why it isn't.

We already do something like this on the client to hide flags based on version. This is just a little more fine grained. If we do need this we should find a way to do that is easily extend as more fields are added to labels.

@vdemeester @dnephin is right. This client-side fix fixes it for older daemons. I have a moby/moby PR that is almost ready, fixing it server side (returning error for unknown prune filters).
It is gonna be part of 17.06 as 17.05 is about to get released, and we still have to design and discuss a part of this server-side fix. I am having troubles with docker system prune --filter ... as the CLI, for system prune, calls container prune, volume prune, image prune and network prune passing the same filters to all of them... so my to-be PR is currently braking because of this.
Thanks for reading.

@gdevillele yep, I did mis-understand the fix at first 😅.

We might (or should have) validates filters by version (for any api endpoint that supports them) in docker/moby 😓

dnephin · 2017-05-03T15:34:51Z

cli/command/utils.go

@@ -2,6 +2,7 @@ package command

 import (
 	"bufio"
+	"context"


I think we are still using golang.org/x/net/context

Depending on if we support go 1.6 for docker/cli or not, it's no big deal in the usage of it (as the interface is the same)

dnephin · 2017-05-03T15:35:31Z

cli/command/utils.go

+	// would just remove all stopped containers without considering the label
+	// The remote API should reject requests using unknown filters.
+	if pruneFilters.Get("label") != nil {
+		serverVersion, err := dockerCli.Client().ServerVersion(context.Background())


The API version should already be available as dockerCli.Client().ClientVersion() which is set by querying the server API version.

@dnephin

// ClientVersion returns the version string associated with this instance of the Client

Are you sure? What's important is to detect the version server side.

Yup,

cli/cli/command/cli.go

Lines 200 to 203 in 0a61397

// if server version is lower than the current cli, downgrade

if versions.LessThan(ping.APIVersion, cli.client.ClientVersion()) {

cli.client.UpdateClientVersion(ping.APIVersion)

}

sets the client version to the server version if the server version is lower.

@dnephin oh! good to know! thanks, I'll update this part

gdevillele

LGTM

thaJeztah · 2017-05-16T15:02:44Z

Do we still want this change? Daemon side validation was added in moby/moby#33023. "label" filtering was added in docker 17.05 through moby/moby#30740 (which is an edge release, so not supported after 17.06 is released), and the --filter option for until was added in 17.04 (moby/moby#29226), also an edge release, and now EOL. So situations where the filter would be ignored would only be if someone ran an 17.04 or 17.05 client against an older daemon

aduermael · 2017-05-16T15:34:39Z

So situations where the filter would be ignored would only be if someone ran an 17.04 or 17.05 client against an older daemon

That happens to me sometimes, I still have a few older daemons here and there... I guess it won't happen very often, and I would be ok to drop this if the command was not prune. But in that case consequences can be quite bad...

thaJeztah · 2017-05-16T15:47:30Z

Actually (we were discussing on slack); it looks like the --filter flag itself does not have an API version annotation. That should added, so that when talking to an older (17.03, which is "stable") daemon version, an error is shown

Hold on, I'll add an example

thaJeztah

left an example (double check, but I think docker 17.04 was API version 1.28)

thaJeztah · 2017-05-16T15:51:52Z

cli/command/system/prune.go

@@ -34,7 +34,7 @@ func NewPruneCommand(dockerCli *command.DockerCli) *cobra.Command {
 	flags := cmd.Flags()
 	flags.BoolVarP(&opts.force, "force", "f", false, "Do not prompt for confirmation")
 	flags.BoolVarP(&opts.all, "all", "a", false, "Remove all unused images not just dangling ones")
-	flags.Var(&opts.filter, "filter", "Provide filter values (e.g. 'until=<timestamp>')")
+	flags.Var(&opts.filter, "filter", "Provide filter values (e.g. 'label=<key>=<value>')")


This should have an annotation (like in cli/command/stack/deploy.go#L46)

flags.SetAnnotation("filter", "version", []string{"1.28"})

Same for the other prune --filter flags

Oh! I did't know about this!
I'll update the PR then, it's much simpler this way.

Also see see moby/moby#30187 and moby/moby#30186 for some more info about that :)

thaJeztah · 2017-05-17T12:07:41Z

ping @aduermael looks like this needs a rebase (also can you address my comment, so that we can get this in for 17.06?)

thaJeztah

changes LGTM; can you squash the commits?

…i v1.28) and newer Signed-off-by: Gaetan de Villele <gdevillele@gmail.com>

tiborvass · 2017-05-17T23:23:21Z

LGTM

vdemeester reviewed May 3, 2017

View reviewed changes

dnephin reviewed May 3, 2017

View reviewed changes

gdevillele force-pushed the prune-security branch from 29b1d62 to 9f2c431 Compare May 4, 2017 18:57

aduermael force-pushed the prune-security branch from 9f2c431 to e8aef0d Compare May 4, 2017 19:15

gdevillele approved these changes May 8, 2017

View reviewed changes

thaJeztah requested changes May 16, 2017

View reviewed changes

thaJeztah added this to the 17.06.0 milestone May 16, 2017

gdevillele force-pushed the prune-security branch 2 times, most recently from dfa4003 to 86d9eac Compare May 17, 2017 19:06

thaJeztah approved these changes May 17, 2017

View reviewed changes

make system prune “--filter” flag available only for docker 17.04 (ap…

1cc1f54

…i v1.28) and newer Signed-off-by: Gaetan de Villele <gdevillele@gmail.com>

gdevillele force-pushed the prune-security branch from 86d9eac to 1cc1f54 Compare May 17, 2017 19:36

tiborvass merged commit 63ecaf1 into docker:master May 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prune commands : make sure label filters are considered #21

prune commands : make sure label filters are considered #21

aduermael commented May 2, 2017 •

edited by gdevillele

Loading

vdemeester May 3, 2017

dnephin May 3, 2017 •

edited

Loading

gdevillele May 3, 2017

vdemeester May 3, 2017

dnephin May 3, 2017

vdemeester May 3, 2017

dnephin May 3, 2017

aduermael May 3, 2017

dnephin May 3, 2017

aduermael May 3, 2017

gdevillele left a comment

thaJeztah commented May 16, 2017

aduermael commented May 16, 2017

thaJeztah commented May 16, 2017

thaJeztah left a comment

thaJeztah May 16, 2017

aduermael May 16, 2017

thaJeztah May 16, 2017

thaJeztah commented May 17, 2017

thaJeztah left a comment

tiborvass commented May 17, 2017

	// if server version is lower than the current cli, downgrade
	if versions.LessThan(ping.APIVersion, cli.client.ClientVersion()) {
	cli.client.UpdateClientVersion(ping.APIVersion)
	}

prune commands : make sure label filters are considered #21

prune commands : make sure label filters are considered #21

Conversation

aduermael commented May 2, 2017 • edited by gdevillele Loading

Choose a reason for hiding this comment

dnephin May 3, 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

gdevillele left a comment

Choose a reason for hiding this comment

thaJeztah commented May 16, 2017

aduermael commented May 16, 2017

thaJeztah commented May 16, 2017

thaJeztah left a 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

thaJeztah commented May 17, 2017

thaJeztah left a comment

Choose a reason for hiding this comment

tiborvass commented May 17, 2017

aduermael commented May 2, 2017 •

edited by gdevillele

Loading

dnephin May 3, 2017 •

edited

Loading