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

Fix `docker build --label` when the label includes single quotes and a space #31750

Merged
merged 3 commits into from Apr 4, 2017

Conversation

@dnephin
Member

dnephin commented Mar 10, 2017

Fixes #31697

Previously labels passed with --labels were converted into a quoted string, and then parsed to create Node objects. This PR changes the behaviour to create Node objects directly from the map[string]string, and avoids the parsing entirely. The parsing was was failing because the value was being quoted with single quotes.

This PR also includes some refactoring, and additional unit test.
Some single-use functions were moved out of utils.go next to their caller.

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Mar 14, 2017

Member

ping @tonistiigi since this is about the builder

Member

dnephin commented Mar 14, 2017

ping @tonistiigi since this is about the builder

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi
Member

tonistiigi commented Mar 14, 2017

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Mar 14, 2017

Member

I had to add single quotes back to the value stored in the node because of #27174.

I'm thinking a better solution might be to add a new field to parser.Node, something like

AllowEnvReplacement bool

The node would then be the source of truth instead of builder/dockerfile/evaluator.go:replaceEnvAllowed.

As it is right now, we still strip off any quotes, so there is still an inconsistency with docker run labels, as described in #26027.

What do you think?

Member

dnephin commented Mar 14, 2017

I had to add single quotes back to the value stored in the node because of #27174.

I'm thinking a better solution might be to add a new field to parser.Node, something like

AllowEnvReplacement bool

The node would then be the source of truth instead of builder/dockerfile/evaluator.go:replaceEnvAllowed.

As it is right now, we still strip off any quotes, so there is still an inconsistency with docker run labels, as described in #26027.

What do you think?

Next: &Node{
Value: "weird",
Next: &Node{
Value: "'first' second'",

This comment has been minimized.

@duglin

duglin Mar 15, 2017

Contributor

you should this is right? I would have expected second' to be part of the 3rd label, not part of the 2nd label's value.

@duglin

duglin Mar 15, 2017

Contributor

you should this is right? I would have expected second' to be part of the 3rd label, not part of the 2nd label's value.

This comment has been minimized.

@dnephin

dnephin Mar 15, 2017

Member

There are only two labels, there is no third.

Yes I'm pretty sure it's correct.

@dnephin

dnephin Mar 15, 2017

Member

There are only two labels, there is no third.

Yes I'm pretty sure it's correct.

This comment has been minimized.

@duglin

duglin Mar 15, 2017

Contributor
LABEL "foo"='bar' fox'=

Will produce 2 labels, not one. Now, this is a different case than yours because of the trailing "=", but w/o it I get an error about a missing "=". But, that tells me that it treated fox' as a 2nd label, not part of the first.

@duglin

duglin Mar 15, 2017

Contributor
LABEL "foo"='bar' fox'=

Will produce 2 labels, not one. Now, this is a different case than yours because of the trailing "=", but w/o it I get an error about a missing "=". But, that tells me that it treated fox' as a 2nd label, not part of the first.

This comment has been minimized.

@dnephin

dnephin Mar 15, 2017

Member

The input is not a line in the Dockerfile. The input is --label foo=bar --label weird="first' second". It seems pretty clear to me that this is two labels, not 3.

@dnephin

dnephin Mar 15, 2017

Member

The input is not a line in the Dockerfile. The input is --label foo=bar --label weird="first' second". It seems pretty clear to me that this is two labels, not 3.

This comment has been minimized.

@duglin

duglin Mar 15, 2017

Contributor

ah ok I was thinking this was from a Dockerfile. Yes, with that quoting its 2.

@duglin

duglin Mar 15, 2017

Contributor

ah ok I was thinking this was from a Dockerfile. Yes, with that quoting its 2.

This comment has been minimized.

@duglin

duglin Mar 15, 2017

Contributor
@duglin

duglin Mar 15, 2017

Contributor

This comment has been minimized.

@duglin

duglin Mar 15, 2017

Contributor

I guess the issue here is that the "expected" Dockerfile text is misleading, at least to me

@duglin

duglin Mar 15, 2017

Contributor

I guess the issue here is that the "expected" Dockerfile text is misleading, at least to me

This comment has been minimized.

@dnephin

dnephin Mar 15, 2017

Member

Ah true, yes it does. All the nodes include the "original" input, but in the case of labels passed as command line flags we have always constructed a fake line, so I preserved that behaviour.

@dnephin

dnephin Mar 15, 2017

Member

Ah true, yes it does. All the nodes include the "original" input, but in the case of labels passed as command line flags we have always constructed a fake line, so I preserved that behaviour.

This comment has been minimized.

@dnephin

dnephin Mar 15, 2017

Member

Unfortunately I don't think there is any way to construct a line that provides the correct behaviour.

Maybe this would get us closer?

@dnephin

dnephin Mar 15, 2017

Member

Unfortunately I don't think there is any way to construct a line that provides the correct behaviour.

Maybe this would get us closer?

dnephin added some commits Mar 10, 2017

Refactor some builder code
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Fix --label on docker build when a single quote is used in the value
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Fix --label being env var expanded.
Signed-off-by: Daniel Nephin <dnephin@docker.com>
@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Mar 24, 2017

Member

rebased

Member

dnephin commented Mar 24, 2017

rebased

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Mar 24, 2017

Member

Too bad we have to do so much work for this thing. I'm all for AllowEnvReplacement or even just calling the evaluator directly. The other thing we should do is evaluate the AST before execution.

Until then this seems to fix the issue. LGTM

Member

tonistiigi commented Mar 24, 2017

Too bad we have to do so much work for this thing. I'm all for AllowEnvReplacement or even just calling the evaluator directly. The other thing we should do is evaluate the AST before execution.

Until then this seems to fix the issue. LGTM

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Mar 29, 2017

Member

@duglin What do you think? Can we get this merged?

Member

dnephin commented Mar 29, 2017

@duglin What do you think? Can we get this merged?

@vdemeester vdemeester requested review from tonistiigi, simonferquel and duglin Apr 3, 2017

@vdemeester

LGTM 👼

@tonistiigi tonistiigi merged commit 6abbc93 into moby:master Apr 4, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 32055 has succeeded
Details
janky Jenkins build Docker-PRs 40675 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 809 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 11752 has succeeded
Details
z Jenkins build Docker-PRs-s390x 662 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 17.05.0 milestone Apr 4, 2017

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