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

daemon returns errors when receiving unsupported prune filters #33023

Merged
merged 1 commit into from May 16, 2017

Conversation

@gdevillele
Contributor

gdevillele commented May 4, 2017

- What I did

  • container prune
  • volume prune
  • image prune
  • network prune

...now return errors if an unsupported filter is received.

container prune only accepts the following filters:

  • label
  • until

volume prune only accepts the following filters:

  • label

image prune only accepts the following filters:

  • dangling
  • label
  • until

network prune only accepts the following filters:

  • label
  • until

- How to verify it

Example:

docker container prune --filter <filter>=<key>=<value>

if is not "label" or "until", you should get a daemon error.

- Description for the changelog

return errors to the client when receiving unsupported prune filters

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

gp-7

cc @thaJeztah @vdemeester

@gdevillele gdevillele changed the title from daemon returns error when receiving unsuppo unknown filters in prune functions to daemon returns error when receiving unsupported prune filters May 4, 2017

@gdevillele gdevillele changed the title from daemon returns error when receiving unsupported prune filters to daemon returns errors when receiving unsupported prune filters May 4, 2017

daemon refuses unknown filters in prune functions
- container prune
- volume prune
- image prune
- network prune

Signed-off-by: Gaetan de Villele <gdevillele@gmail.com>
@@ -25,6 +25,27 @@ var (
// ErrPruneRunning is returned when a prune request is received while
// one is in progress
ErrPruneRunning = fmt.Errorf("a prune operation is already running")

This comment has been minimized.

@allencloud

allencloud May 14, 2017

Contributor

I am also wondering if we could disable the export of error ErrPruneRunning ?
Since it is only used here.
But maybe out of the scope of this PR. 🐱

@allencloud

allencloud May 14, 2017

Contributor

I am also wondering if we could disable the export of error ErrPruneRunning ?
Since it is only used here.
But maybe out of the scope of this PR. 🐱

This comment has been minimized.

@gdevillele

gdevillele May 15, 2017

Contributor

Thanks.
Might be a candidate for another PR.

@gdevillele

gdevillele May 15, 2017

Contributor

Thanks.
Might be a candidate for another PR.

containersAcceptedFilters = map[string]bool{
"label": true,
"label!": true,

This comment has been minimized.

@thaJeztah

thaJeztah May 15, 2017

Member

Should the ! actually be part of the filter name? Not sure how it's handled later on, but looks a bit tricky (i.e. label!=foo.bar would be accepted, but not label != foo.bar, correct?)

@thaJeztah

thaJeztah May 15, 2017

Member

Should the ! actually be part of the filter name? Not sure how it's handled later on, but looks a bit tricky (i.e. label!=foo.bar would be accepted, but not label != foo.bar, correct?)

This comment has been minimized.

@mlaventure

mlaventure May 15, 2017

Contributor

I don't think the later is a valid syntax for filters (e.g. filters="label != foo.bar")

@mlaventure

mlaventure May 15, 2017

Contributor

I don't think the later is a valid syntax for filters (e.g. filters="label != foo.bar")

This comment has been minimized.

@gdevillele

gdevillele May 15, 2017

Contributor

@thaJeztah

  • I agree that this is strange, following our discussion, I will open another PR to rewrite this logic.
  • I don't think filters can contain any space, either.
@gdevillele

gdevillele May 15, 2017

Contributor

@thaJeztah

  • I agree that this is strange, following our discussion, I will open another PR to rewrite this logic.
  • I don't think filters can contain any space, either.

This comment has been minimized.

@thaJeztah

thaJeztah May 16, 2017

Member

Looked up the PR that implemented it, and indeed the format is label!=key=val or label!=key. Negative matches on value alone don't seem to be supported (i.e., no label=key!=val), nor spaces in between (label != key=val etc.)

moby/cli/command/utils.go

Lines 95 to 116 in 7025247

for _, f := range dockerCli.ConfigFile().PruneFilters {
parts := strings.SplitN(f, "=", 2)
if len(parts) != 2 {
continue
}
if parts[0] == "label" {
// CLI label filter supersede config.json.
// If CLI label filter conflict with config.json,
// skip adding label! filter in config.json.
if pruneFilters.Include("label!") && pruneFilters.ExactMatch("label!", parts[1]) {
continue
}
} else if parts[0] == "label!" {
// CLI label! filter supersede config.json.
// If CLI label! filter conflict with config.json,
// skip adding label filter in config.json.
if pruneFilters.Include("label") && pruneFilters.ExactMatch("label", parts[1]) {
continue
}
}
pruneFilters.Add(parts[0], parts[1])
}

@thaJeztah

thaJeztah May 16, 2017

Member

Looked up the PR that implemented it, and indeed the format is label!=key=val or label!=key. Negative matches on value alone don't seem to be supported (i.e., no label=key!=val), nor spaces in between (label != key=val etc.)

moby/cli/command/utils.go

Lines 95 to 116 in 7025247

for _, f := range dockerCli.ConfigFile().PruneFilters {
parts := strings.SplitN(f, "=", 2)
if len(parts) != 2 {
continue
}
if parts[0] == "label" {
// CLI label filter supersede config.json.
// If CLI label filter conflict with config.json,
// skip adding label! filter in config.json.
if pruneFilters.Include("label!") && pruneFilters.ExactMatch("label!", parts[1]) {
continue
}
} else if parts[0] == "label!" {
// CLI label! filter supersede config.json.
// If CLI label! filter conflict with config.json,
// skip adding label filter in config.json.
if pruneFilters.Include("label") && pruneFilters.ExactMatch("label", parts[1]) {
continue
}
}
pruneFilters.Add(parts[0], parts[1])
}

@mlaventure

LGTM

@thaJeztah

LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 16, 2017

Member

@gdevillele can you open an issue for tracking the changes on the way we match the filters? (perhaps write out which formats we want to support, if we want to support whitespace)

Member

thaJeztah commented May 16, 2017

@gdevillele can you open an issue for tracking the changes on the way we match the filters? (perhaps write out which formats we want to support, if we want to support whitespace)

@thaJeztah thaJeztah merged commit f944183 into moby:master May 16, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 33720 has succeeded
Details
janky Jenkins build Docker-PRs 42324 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 2647 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 13580 has succeeded
Details
z Jenkins build Docker-PRs-s390x 2414 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 17.06.0 milestone May 16, 2017

@gdevillele

This comment has been minimized.

Show comment
Hide comment
@gdevillele

gdevillele May 16, 2017

Contributor

@thaJeztah will do. Thanks for merging!

Contributor

gdevillele commented May 16, 2017

@thaJeztah will do. Thanks for merging!

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