Adjust builder to validate that JSON in Dockerfiles are arrays of strings and nothing else to match how we describe them to people (and what all our existing tests already assumed) #9881

tianon · 2015-01-03T05:51:38Z

This also adds more tests to help verify this, including unicode and nonprintable characters (hence the commit switching to strconv.Quote).

As a bonus, this fixes a subtle bug where [] was turned into [""] and then turned back into [] (and thus [""] was impossible to actually round-trip correctly in a Dockerfile).

In the process, I replaced the custom QuoteString function with the equivalent (but with more special cased characters) strconv.Quote in Node.Dump(), and simplified the builder TestTestData a little by using ioutil.ReadFile directly instead of os.Open followed immediately by ioutil.ReadAll.

…QuoteString function (the only change to existing files is literal tabs becoming \t, but future files may use nonprintable characters and the like now) Signed-off-by: Andrew "Tianon" Page <admwiggin@gmail.com>

…ad of os.Open+ioutil.ReadAll Signed-off-by: Andrew "Tianon" Page <admwiggin@gmail.com>

LK4D4 · 2015-01-03T05:58:06Z

builder/parser/line_parsers.go

 	for _, str := range myJson {
 		switch str.(type) {
 		case string:
-		case float64:
-			str = strconv.FormatFloat(str.(float64), 'G', -1, 64)


THIS IS BREAKING CHANGE!!!111!!

No tests for it, no guarantee of non breakage.

Also, we never describe it this way. Can you find me Dockerfiles in the wild that use it legitimately? Is there actually a case where a float64 is preferred to a string? For reals?

it really makes no sense for a cmd or entrypoint

or a RUN line either 😉

Everywhere in docs this is just "JSON Array". Also I am +100 on this breaking change. As I was +100 on breaking change for USER+COPY, so don't take my words too serious.

If the docs maintainers want me to, I can go on a docs dive replacing JSON array with JSON array of strings... 👍 😉

so you mean you've broken RUN ["echo", 3.14] ? perhaps mention it in the release notes, someone will trip on it, but I too would expect it to be very few.

We don't have a place where we currently aggregate "next version" release notes, right?

erikh · 2015-01-03T15:21:02Z

You should probably make Node values take string only now instead of interface. Should speed the builder up.

tianon · 2015-01-03T17:09:18Z

I'm not sure what you mean?
https://github.com/docker/docker/blob/b2ab733c993ee6124c164defceac6996206d69fa/builder/parser/parser.go#L25

Aren't they already?

erikh · 2015-01-04T04:17:55Z

sorry I misread some of the above code, it's been a while since I was in here.

icecrime · 2015-01-04T20:54:53Z

builder/parser/line_parsers.go

 	if err := json.Unmarshal([]byte(rest), &myJson); err != nil {
 		return nil, nil, err
 	}

+	var top, prev *Node
 	for _, str := range myJson {
 		switch str.(type) {


Do we need a type switch rather than a single type assertion? I don't think it changes anything besides readability, but I'd find if s, ok := str.(string); !ok { easier (and avoids casting twice).

jessfraz · 2015-01-05T19:35:40Z

LGTM but maybe just a single type assertion like @icecrime suggested :)

…ings and nothing else to match how we describe them to people (and what all our existing tests already assumed) This also adds more tests to help verify this, including unicode and nonprintable characters (hence the earlier commit switching to strconv.Quote). As a bonus, this fixes a subtle bug where [] was turned into [""] and then turned back into [] (and thus [""] was impossible to actually round-trip correctly in a Dockerfile). Signed-off-by: Andrew "Tianon" Page <admwiggin@gmail.com>

tianon · 2015-01-05T21:31:49Z

Updated type switch to be just a type assertion instead. 👍

erikh · 2015-01-05T22:22:12Z

lgtm

jessfraz · 2015-01-06T01:39:57Z

I think we are just waiting on @tiborvass

jessfraz · 2015-01-09T01:10:44Z

@duglin feel free to review and merge at your convenience :) and obviously only if you like it

duglin · 2015-01-09T01:59:46Z

What should: RUN [ "-e foo" ] result in? I get different results from the shell vs docker(this pr).
Should this PR have fixed this ambiguity?

tiborvass · 2015-01-09T02:06:35Z

builder/parser/parser.go

-		node.Next = sexp
-	}
-
+	node.Next = sexp


@tianon why this change?

This if was here to check for the case of [] turning into [""] and turning it back into []. By fixing parseJSON to return nil (the linked-list representation of an empty list), this if is no longer required, so now [] and [""] can both round-trip successfully.

tianon · 2015-01-09T04:29:09Z

@duglin that's a valid JSON array of strings, and there's no way for us to detect that it should be parsed by the shell as-is instead, so that becomes the literal command -e foo (which very likely won't exist in PATH)

tianon · 2015-01-09T04:29:54Z

Which is the current behavior without this PR, too:

$ docker build -
FROM busybox
RUN [ "-e foo" ]
Sending build context to Docker daemon 2.048 kB
Sending build context to Docker daemon 
Step 0 : FROM busybox
 ---> 4986bf8c1536
Step 1 : RUN -e foo
 ---> Running in 439e68f47660
/bin/sh: illegal option - 
INFO[0006] The command [/bin/sh -c -e foo] returned a non-zero code: 2

tianon · 2015-01-09T04:30:14Z

Although it's strange that we're getting /bin/sh there when we shouldn't be...

duglin · 2015-01-09T11:38:36Z

Yes that's what we get today too - I was just wondering if this PR was attempting to fix that issue as well. Its not, and probably can't, but was just hoping you found some magic.

jessfraz · 2015-01-14T23:08:01Z

what was the conclusion here?

LK4D4 · 2015-01-14T23:10:00Z

@jfrazelle that json sucks, but this PR has 2 l.g.t.m.s already :)

thaJeztah · 2015-01-14T23:10:17Z

Nice JSON related issue: #10097

jessfraz · 2015-01-14T23:12:17Z

i dont know eriks was not all caps

Adjust builder to validate that JSON in Dockerfiles are arrays of strings and nothing else to match how we describe them to people (and what all our existing tests already assumed)

erikh · 2015-01-15T03:38:58Z

IHERDAT

tianon added 2 commits January 2, 2015 22:36

Simplify builder ast.Dump by using strconv.Quote instead of a custom …

1edf16e

…QuoteString function (the only change to existing files is literal tabs becoming \t, but future files may use nonprintable characters and the like now) Signed-off-by: Andrew "Tianon" Page <admwiggin@gmail.com>

Simplify builder TestTestData slightly by using ioutil.ReadFile inste…

f6cb1ea

…ad of os.Open+ioutil.ReadAll Signed-off-by: Andrew "Tianon" Page <admwiggin@gmail.com>

LK4D4 reviewed Jan 3, 2015
View reviewed changes

icecrime reviewed Jan 4, 2015
View reviewed changes

tianon force-pushed the json-array-of-strings branch from e9cf38d to 05c2d2d Compare January 5, 2015 21:31

tiborvass reviewed Jan 9, 2015
View reviewed changes

jessfraz merged commit 640e0fc into moby:master Jan 15, 2015

tianon deleted the json-array-of-strings branch January 15, 2015 01:14

SvenDowideit mentioned this pull request Jan 27, 2015

Post 1.4.1 docs update 2 #9739

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust builder to validate that JSON in Dockerfiles are arrays of strings and nothing else to match how we describe them to people (and what all our existing tests already assumed) #9881

Adjust builder to validate that JSON in Dockerfiles are arrays of strings and nothing else to match how we describe them to people (and what all our existing tests already assumed) #9881

tianon commented Jan 3, 2015

LK4D4 Jan 3, 2015

tianon Jan 3, 2015

tianon Jan 3, 2015

jessfraz Jan 3, 2015

tianon Jan 3, 2015

LK4D4 Jan 3, 2015

tianon Jan 3, 2015

SvenDowideit Jan 4, 2015

tianon Jan 5, 2015

erikh commented Jan 3, 2015

tianon commented Jan 3, 2015

erikh commented Jan 4, 2015

icecrime Jan 4, 2015

erikh Jan 5, 2015

jessfraz commented Jan 5, 2015

tianon commented Jan 5, 2015

erikh commented Jan 5, 2015

jessfraz commented Jan 6, 2015

jessfraz commented Jan 9, 2015

duglin commented Jan 9, 2015

tiborvass Jan 9, 2015

tianon Jan 9, 2015

tianon commented Jan 9, 2015

tianon commented Jan 9, 2015

tianon commented Jan 9, 2015

duglin commented Jan 9, 2015

jessfraz commented Jan 14, 2015

LK4D4 commented Jan 14, 2015

thaJeztah commented Jan 14, 2015

jessfraz commented Jan 14, 2015

erikh commented Jan 15, 2015

Adjust builder to validate that JSON in Dockerfiles are arrays of strings and nothing else to match how we describe them to people (and what all our existing tests already assumed) #9881

Adjust builder to validate that JSON in Dockerfiles are arrays of strings and nothing else to match how we describe them to people (and what all our existing tests already assumed) #9881

Conversation

tianon commented Jan 3, 2015

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

erikh commented Jan 3, 2015

tianon commented Jan 3, 2015

erikh commented Jan 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessfraz commented Jan 5, 2015

tianon commented Jan 5, 2015

erikh commented Jan 5, 2015

jessfraz commented Jan 6, 2015

jessfraz commented Jan 9, 2015

duglin commented Jan 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tianon commented Jan 9, 2015

tianon commented Jan 9, 2015

tianon commented Jan 9, 2015

duglin commented Jan 9, 2015

jessfraz commented Jan 14, 2015

LK4D4 commented Jan 14, 2015

thaJeztah commented Jan 14, 2015

jessfraz commented Jan 14, 2015

erikh commented Jan 15, 2015