This adds validation to `docker container run` / `docker container create`;
Validation of labels provided through flags was removed in 31dc5c0a9a,
after the validation was changed to fix labels without values, and to prevent
labels from being expanded with environment variables in 2b17f4c8a8
However, now empty label names from _files_ (`--label-file`) followed different
validation rules than labels passed through `--label`.
This patch adds back minimal validation for labels passed through the command-line
Before this patch:
```bash
docker container create \
--name label \
--label==with-leading-equal-sign \
--label=without-value \
--label=somelabel=somevalue \
--label " = " \
--label=with-quotes-in-value='{"foo"}' \
--label='with"quotes"in-key=test' \
busybox
docker container inspect --format '{{json .Config.Labels}}' label
```
```json
{
"": "with-leading-equal-sign",
" ": " ",
"somelabel": "somevalue",
"with\"quotes\"in-key": "test",
"with-quotes-in-value": "{\"foo\"}",
"without-value": ""
}
```
After this patch:
```bash
docker container create \
--name label \
--label==with-leading-equal-sign \
--label=without-value \
--label=somelabel=somevalue \
--label " = " \
--label=with-quotes-in-value='{"foo"}' \
--label='with"quotes"in-key=test' \
busybox
invalid argument "=with-leading-equal-sign" for "-l, --label" flag: invalid label format: "=with-leading-equal-sign"
```
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This patch fixes a bug where labels use the same behavior as `--env`, resulting
in a value to be copied from environment variables with the same name as the
label if no value is set (i.e. a simple key, no `=` sign, no value).
An earlier pull request addressed similar cases for `docker run`;
2b17f4c8a8, but this did not address the
same situation for (e.g.) `docker service create`.
Digging in history for this bug, I found that use of the `ValidateEnv`
function for labels was added in the original implementation of the labels feature in
abb5e9a077 (diff-ae476143d40e21ac0918630f7365ed3cR34)
However, the design never intended it to expand environment variables,
and use of this function was either due to either a "copy/paste" of the
equivalent `--env` flags, or a misunderstanding (the name `ValidateEnv` does
not communicate that it also expands environment variables), and the existing
`ValidateLabel` was designed for _engine_ labels (which required a value to
be set).
Following the initial implementation, other parts of the code followed
the same (incorrect) approach, therefore leading the bug to be introduced
in services as well.
This patch:
- updates the `ValidateLabel` to match the expected validation
rules (this function is no longer used since 31dc5c0a9a),
and the daemon has its own implementation)
- corrects various locations in the code where `ValidateEnv` was used instead of `ValidateLabel`.
Before this patch:
```bash
export SOME_ENV_VAR=I_AM_SOME_ENV_VAR
docker service create --label SOME_ENV_VAR --tty --name test busybox
docker service inspect --format '{{json .Spec.Labels}}' test
{"SOME_ENV_VAR":"I_AM_SOME_ENV_VAR"}
```
After this patch:
```bash
export SOME_ENV_VAR=I_AM_SOME_ENV_VAR
docker service create --label SOME_ENV_VAR --tty --name test busybox
docker container inspect --format '{{json .Config.Labels}}' test
{"SOME_ENV_VAR":""}
```
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This maps the `--template-driver` flag on secret and config creation.
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
go1.12.1 (released 2019/03/14) includes fixes to cgo, the compiler, the go
command, and the fmt, net/smtp, os, path/filepath, sync, and text/template
packages. See the Go 1.12.1 milestone on our issue tracker for details.
For the relase notes of Go 1.12.0, see: https://golang.org/doc/go1.12
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The warning, printed before running `docker system prune` was printing the
filters in JSON format.
This patch attempts to make the output human readable;
- updating the code, and template to print filters individually
- reducing the indentation (which was quite deep)
Before this patch was applied;
```
docker system prune --filter until=24h --filter label=hello-world --filter label!=foo=bar --filter label=bar=baz
WARNING! This will remove:
- all stopped containers
- all networks not used by at least one container
- all dangling images
- all dangling build cache
- Elements to be pruned will be filtered with:
- label={"label":{"bar=baz":true,"hello-world":true},"label!":{"foo=bar":true},"until":{"24h":true}}
Are you sure you want to continue? [y/N]
```
With this patch applied;
```
WARNING! This will remove:
- all stopped containers
- all networks not used by at least one container
- all dangling images
- all dangling build cache
Items to be pruned will be filtered with:
- label!=foo=bar
- label!=never=remove-me
- label=bar=baz
- label=hello-world
- label=remove=me
- until=24h
Are you sure you want to continue? [y/N]
```
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The warning, printed before runing docker system prune was missing any filter
that was set in the configuration file. In addition, the warning prefixes the
filters with `label=`, which is no longer accurate, now that the prune command
also supports "until" as a filter.
Before this change, only the filters set on the command-line were shown,
and any filter set in the configuration file was missing;
```
mkdir -p ./test-config
echo '{"pruneFilters": ["label!=never=remove-me", "label=remove=me"]}' > test-config/config.json
docker --config=./test-config system prune --filter until=24h --filter label=hello-world --filter label!=foo=bar --filter label=bar=baz
WARNING! This will remove:
- all stopped containers
- all networks not used by at least one container
- all dangling images
- all dangling build cache
- Elements to be pruned will be filtered with:
- label={"label":{"bar=baz":true,"hello-world":true},"label!":{"foo=bar":true},"until":{"24h":true}}
Are you sure you want to continue? [y/N]
```
With this patch applied, both options from the commandline and options set
in the configuration file are shown;
```
mkdir -p ./test-config
echo '{"pruneFilters": ["label!=never=remove-me", "label=remove=me"]}' > test-config/config.json
docker --config=./test-config system prune --filter until=24h --filter label=hello-world --filter label!=foo=bar --filter label=bar=baz
WARNING! This will remove:
- all stopped containers
- all networks not used by at least one container
- all dangling images
- all dangling build cache
- Elements to be pruned will be filtered with:
- filter={"label":{"bar=baz":true,"hello-world":true,"remove=me":true},"label!":{"foo=bar":true,"never=remove-me":true},"until":{"24h":true}}
```
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Vendoring can take some time, depending on network-speed, so
reduce flakiness by increasing the default timeout, to prevent:
make[1]: Entering directory '/go/src/github.com/docker/cli'
rm -rf vendor
bash -c 'vndr |& grep -v -i clone'
2019/03/18 11:38:26 Collecting initial packages
Too long with no output (exceeded 10m0s)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Since #1654 so far we've had problems with it not working on Windows (npipe
lacked the `CloseRead` method) and problems with using tcp with tls (the tls
connection also lacks `CloseRead`). Both of these were workedaround in #1718
which added a nop `CloseRead` method.
However I am now seeing hangs (on Windows) where the `system dial-stdio`
subprocess is not exiting (I'm unsure why so far).
I think the 3rd problem found with this is an indication that `dial-stdio` is
not quite ready for wider use outside of its initial usecase (support for
`ssh://` URLs to connect to remote daemons).
This change simply disables the `dial-stdio` path for all plugins. However
rather than completely reverting 891b3d953e ("cli-plugins: use `docker system
dial-stdio` to call the daemon") I've just disabled the functionality at the
point of use and left in a trap door environment variable so that those who
want to experiment with this mode (and perhaps fully debug it) have an easier
path do doing so.
The e2e test for this case is disabled unless the trap door envvar is set. I
also renamed the test to clarify that it is about cli plugins.
Signed-off-by: Ian Campbell <ijc@docker.com>
- when using "--context default" parameter
- when printing the list of contexts
- when exporting the default context to a tarball
Signed-off-by: Jean-Christophe Sirot <jean-christophe.sirot@docker.com>
(+1 squashed commit)
Squashed commits:
[20670495] Fix CLI initialization for the `docker stack deploy --help` command and ensure that the dockerCli.CurrentContext() always returns a non empty context name (default as a fallback)
Remove now obsolete code handling empty string context name
Minor code cleanup
Signed-off-by: Jean-Christophe Sirot <jean-christophe.sirot@docker.com>
go1.11.6 (released 2019/03/14) includes fixes to cgo, the compiler, linker,
runtime, go command, and the crypto/x509, encoding/json, net, and net/url
packages. See the Go 1.11.6 milestone on our issue tracker for details:
https://github.com/golang/go/issues?q=milestone%3AGo1.11.6
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
I got a bit carried away in d4ced2ef77 ("allow plugins to have argument
which match a top-level flag.") and broke the ability of a plugin to use the
`PersistentPreRun(E)` hook on its top-level command (by unconditionally
overwriting it) and also broke the plugin framework if a plugin's subcommand
used those hooks (because they would shadow the root one). This could result in
either `dockerCli.Client()` returning `nil` or whatever initialisation the
plugin hoped to do not occuring.
This change revert the relevant bits and reinstates the requirement that a
plugin calls `plugin.PersistentPreRunE` if it uses that hook itself.
It is at least a bit nicer now since we avoid the need for the global struct
since the interesting state is now encapsulated in `tcmd` (and the closure).
In principal this could be done even more simply (by calling `tcmd.Initialize`
statically between `tcmd.HandleGlobalFlags` and `cmd.Execute`) however this has
the downside of _always_ initialising the cli (and therefore dialing the
daemon) even for the `docker-cli-plugin-metadata` command but also for the
`help foo` and `foo --help` commands (Cobra short-circuits the hooks in this
case).
Signed-off-by: Ian Campbell <ijc@docker.com>
I regressed this in d4ced2ef77 ("allow plugins to have argument which match a
top-level flag.") by unconditionally overwriting any `PersistentRunE` that the
user may have supplied.
We need to ensure two things:
1. That the user can use `PersistentRunE` (or `PersistentRun`) for their own
purposes.
2. That our initialisation always runs, even if the user has used
`PersistentRun*`, since that will shadow the root.
To do this add a `PersistentRunE` to the helloworld plugin which logs (covers 1
above) and then use it when calling the `apiversion` subcommand (which covers 2
since that uses the client)
Signed-off-by: Ian Campbell <ijc@docker.com>
This makes things more idempotent, rather than relying on undoing the
interspersed settings.
Note that the underlying `Flag`s remain shared, it's just the `FlagSet` which
is duplicated.
Signed-off-by: Ian Campbell <ijc@docker.com>
Previous commits fixed the first issue on #1661, this simply adds a test for
it. Note that this is testing the current behaviour, without regard for the
second issue in #1661 which proposes a different behaviour.
Signed-off-by: Ian Campbell <ijc@docker.com>
The issue with plugin options clashing with globals is that when cobra is
parsing the command line and it comes across an argument which doesn't start
with a `-` it (in the absence of plugins) distinguishes between "argument to
current command" and "new subcommand" based on the list of registered sub
commands.
Plugins breaks that model. When presented with `docker -D plugin -c foo` cobra
parses up to the `plugin`, sees it isn't a registered sub-command of the
top-level docker (because it isn't, it's a plugin) so it accumulates it as an
argument to the top-level `docker` command. Then it sees the `-c`, and thinks
it is the global `-c` (for AKA `--context`) option and tries to treat it as
that, which fails.
In the specific case of the top-level `docker` subcommand we know that it has
no arguments which aren't `--flags` (or `-f` short flags) and so anything which
doesn't start with a `-` must either be a (known) subcommand or an attempt to
execute a plugin.
We could simply scan for and register all installed plugins at start of day, so
that cobra can do the right thing, but we want to avoid that since it would
involve executing each plugin to fetch the metadata, even if the command wasn't
going to end up hitting a plugin.
Instead we can parse the initial set of global arguments separately before
hitting the main cobra `Execute` path, which works here exactly because we know
that the top-level has no non-flag arguments.
One slight wrinkle is that the top-level `PersistentPreRunE` is no longer
called on the plugins path (since it no longer goes via `Execute`), so we
arrange for the initialisation done there (which has to be done after global
flags are parsed to handle e.g. `--config`) to happen explictly after the
global flags are parsed. Rather than make `newDockerCommand` return the
complicated set of results needed to make this happen, instead return a closure
which achieves this.
The new functionality is introduced via a common `TopLevelCommand` abstraction
which lets us adjust the plugin entrypoint to use the same strategy for parsing
the global arguments. This isn't strictly required (in this case the stuff in
cobra's `Execute` works fine) but doing it this way avoids the possibility of
subtle differences in behaviour.
Fixes#1699, and also, as a side-effect, the first item in #1661.
Signed-off-by: Ian Campbell <ijc@docker.com>
These won't pass right now due to https://github.com/docker/cli/issues/1699
("Plugins can't re-use the same flags as cli global flags") and the change in
935d47bbe9 ("Ignore unknown arguments on the top-level command."), but the
intention is to fix them now.
Signed-off-by: Ian Campbell <ijc@docker.com>
This happens on Windows when dialing a named pipe (a path which is used by CLI
plugins), in that case some debugging shows:
DEBU[0000] conn is a *winio.win32MessageBytePipe
DEBU[0000] conn is a halfReadCloser: false
DEBU[0000] conn is a halfWriteCloser: true
the raw stream connection does not implement halfCloser
In such cases we can simply wrap with a nop function since closing for read
isn't too critical.
Signed-off-by: Ian Campbell <ijc@docker.com>
Sebastiaan van Stijn
Stephane Jeandeaux
Sebastiaan van Stijn
Tibor Vass
Vincent Demeester
Tonis Tiigi
Silvin Lubecki
Mason Fish
Silvin Lubecki
Ian Campbell
Jean-Christophe Sirot