Enhancement: print warnings when falling back from JSON to "shell" syntax (and other warnings)

[thaJeztah]

Background

Some Dockerfile instructions support both JSON and "shell" form syntax. Dockerfile parser handles these by checking if the command contains a valid JSON, and otherwise considers it to be a "shell form". When falling back to "shell form", the command is used "as-is". Reason for this is that [ is a valid command, and therefore the command could be valid;

$ which [
/bin/[

Writing valid JSON can be tricky, and user-mistakes, such as using single quotes instead of double quotes are easy to make;

RUN ['/bin/sh', '-c', 'echo hello']

While this behavior may be "by design", and technically correct, it's often confusing users. In many cases, running [ is not the result the user was expecting, and many users are not aware of JSON requiring double-quotes, making it difficult for them to parse the error that's produced, and to locate the mistake;

DOCKER_BUILDKIT=0 docker build -<<'EOF'
FROM busybox
RUN ['/bin/sh', '-c', 'echo hello']
EOF

...
Step 2/2 : RUN ['/bin/sh', '-c', 'echo hello']
 ---> Running in 72589aa870ee
/bin/sh: [/bin/sh,: not found
The command '/bin/sh -c ['/bin/sh', '-c', 'echo hello']' returned a non-zero code: 127

DOCKER_BUILDKIT=1 docker build --progress=plain -<<'EOF'
FROM busybox
RUN ['/bin/sh', '-c', 'echo hello']
EOF

...
#5 [2/2] RUN ['/bin/sh', '-c', 'echo hello']
#5 sha256:aa03118cdf661629e56fc294f774524be8d0cc42e069264efce120aa9e203ae2
#5 0.298 /bin/sh: [/bin/sh,: not found
#5 ERROR: executor failed running [/bin/sh -c ['/bin/sh', '-c', 'echo hello']]: exit code: 127
------
 > [2/2] RUN ['/bin/sh', '-c', 'echo hello']:
------
executor failed running [/bin/sh -c ['/bin/sh', '-c', 'echo hello']]: exit code: 127

Suggested change

I suggest that we print an informational message to notify the user that we attempted to parse the Dockerfile instruction as JSON, but failed to parse it as valid JSON, and thus fell back to treating it as a shell instruction.

Implementation / changes needed

I had a quick look at what changes would be needed to implement this. The parseMaybeJSON() and parseMaybeJSONToList() functions parse Dockerfile instructions that support either a JSON array format ("exec form"), or a string format ("shell" form);

buildkit/frontend/dockerfile/parser/line_parsers.go

Line 307 in 7bdb659

func parseMaybeJSON(rest string, d *directives) (*Node, map[string]bool, error) {

buildkit/frontend/dockerfile/parser/line_parsers.go

Line 329 in 7bdb659

func parseMaybeJSONToList(rest string, d *directives) (*Node, map[string]bool, error) {

Looking at that code, parsers currently can only return an error, but don't have a way to return non-fatal "warnings" or "informational" messages;

buildkit/frontend/dockerfile/parser/parser.go

Lines 176 to 182 in 6b1c950

	// Dispatch Table. see line_parsers.go for the parse functions.
	// The command is parsed and mapped to the line parser. The line parser
	// receives the arguments but not the command, and returns an AST after
	// reformulating the arguments according to the rules in the parser
	// functions. Errors are propagated up by Parse() and the resulting AST can
	// be incorporated directly into the existing AST as a next.
	dispatch = map[string]func(string, *directives) (*Node, map[string]bool, error){

buildkit/frontend/dockerfile/parser/parser.go

Lines 218 to 230 in 6b1c950

	next, attrs, err := fn(args, d)
	if err != nil {
	return nil, err
	}
	return &Node{
	Value: cmd,
	Original: line,
	Flags: flags,
	Next: next,
	Attributes: attrs,
	PrevComment: comments,
	}, nil

Nodes are collected in a Result, which does have a Warnings property, but is currently only used to warn about empty continuation lines;

buildkit/frontend/dockerfile/parser/parser.go

Lines 298 to 311 in 6b1c950

	hasEmptyContinuationLine = true
	continue
	}
	continuationLine := string(bytesRead)
	continuationLine, isEndOfLine = trimContinuationCharacter(continuationLine, d)
	line += continuationLine
	}
	if hasEmptyContinuationLine {
	warnings = append(warnings, "[WARNING]: Empty continuation line found in:\n "+line)
	}
	child, err := newNodeFromLine(line, d, comments)

Looking at the above, I see two possible implementations;

1. Add an extra return value to the line parsers (such as parseMaybeJSON()), and newNodeFromLine()
2. Add a Warnings property to the Node struct

The warning would be added to Result.Warnings, together with the line number

2. would be consistent with Result.Warnings, but I don't know if changing the Node struct would have side-effects, so opening this ticket first to discuss options.

[thaJeztah]

@tonistiigi happy to work on this, if you could provide some input on what approach would work best (1. or 2., or perhaps another approach)

[tonistiigi]

There is no support for sending warnings to the progressbar from a frontend. That would need to happen first.

[thaJeztah]

@tonistiigi wouldn't this do that?

buildkit/frontend/dockerfile/parser/parser.go

Lines 240 to 246 in 6b1c950

	// PrintWarnings to the writer
	func (r *Result) PrintWarnings(out io.Writer) {
	if len(r.Warnings) == 0 {
	return
	}
	fmt.Fprintf(out, strings.Join(r.Warnings, "\n")+"\n")
	}

[thaJeztah]

Related; cdalvaro/docker-salt-master#78

[thaJeztah]

• other example from Older Docker accepted inline comments in some places in spite of documentation otherwise, leading to broken builds now that it's fixed, and opaque error messaging interferes with troubleshooting and repair. #5625 (comment)

BuildKit;

echo -e 'FROM scratch\nCOPY ["foo", "bar", "."' | docker build -
...
------
 > [internal] load build context:
------
ERROR: failed to solve: rpc error: code = Unknown desc = invalid includepatterns: []: syntax error in pattern

Compared to;

echo -e 'FROM scratch\nCOPY ["foo", "bar", "."' | DOCKER_BUILDKIT=0 docker build -
...
Step 2/2 : COPY ["foo", "bar", "."
COPY failed: file not found in build context or excluded by .dockerignore: stat bar,: file does not exist

I wonder if we have ways to surface more what happened; i.e., “we fell back to handling this as non-JSON”. or even “WARNING: did you mean xxx here?” because as a human, this LOOKS perfectly valid;

COPY ["./foo", "."] # copy the foo dir

Overall, I think we can be pretty confident that (specifically with COPY, ADD, VOLUME) if the line starts with [ that the intent was “should be JSON”. RUN is the only case where I think it’s less confident ([ being used as part of the command), so I wonder if we could add warnings for those.