Fix labels copying value from environment variables #1671
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #1671 +/- ##
==========================================
+ Coverage 56.27% 56.27% +<.01%
==========================================
Files 307 307
Lines 21137 21141 +4
==========================================
+ Hits 11894 11898 +4
Misses 8378 8378
Partials 865 865 |
|
@silvin-lubecki @vdemeester ptal partially thinking if we should also move more of this validation to the daemon 🤔 |
|
Were you an archeologist in a previous life @thaJeztah 😆 ? The description is impressive! |
What do you mean? Which part do you want to move? I think the validation is simple enough on the client side 🤔 |
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`; 2b17f4c, 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 moby/moby@abb5e9a#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 31dc5c0), 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 adds validation to `docker container run` / `docker container create`; Validation of labels provided through flags was removed in 31dc5c0, after the validation was changed to fix labels without values, and to prevent labels from being expanded with environment variables in 2b17f4c 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>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
fix_labels_expanding_env_vars
branch
from
b2662e4
to
e5702e0
Compare
|
updated PTAL |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch fixes a bug where labels use the same behavior as
--env, resultingin 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;2b17f4c, 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
ValidateEnvfunction for labels was added in the original implementation of the labels feature in
moby/moby@abb5e9a#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
--envflags, or a misunderstanding (the nameValidateEnvdoesnot communicate that it also expands environment variables), and the existing
ValidateLabelwas designed for engine labels (which required a value tobe 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:
ValidateLabelto match the expected validationrules (this function is no longer used since 31dc5c0),
and the daemon has its own implementation)
ValidateEnvwas used instead ofValidateLabel.Before this patch:
After this patch:
Add back validation for invalid label values on containers
This adds validation to
docker container run/docker container create;Validation of labels provided through flags was removed in 31dc5c0,
after the validation was changed to fix labels without values, and to prevent
labels from being expanded with environment variables in 2b17f4c
However, now empty label names from files (
--label-file) followed differentvalidation rules than labels passed through
--label.This patch adds back minimal validation for labels passed through the command-line
Before this patch:
{ "": "with-leading-equal-sign", " ": " ", "somelabel": "somevalue", "with\"quotes\"in-key": "test", "with-quotes-in-value": "{\"foo\"}", "without-value": "" }After this patch: