Improve dockerfile directive parsing#27270
Conversation
Signed-off-by: Tonis Tiigi <[email protected]>
There was a problem hiding this comment.
github shows some funky chars at the end - is that due to a missing crlf? Was that on purpose and part of the test? if so, let's add a comment so that people don't edit the file and add it back in by mistake.
There was a problem hiding this comment.
No, this doesn't change the test. Only thing we are testing is if the first line sets escape char or not.
There was a problem hiding this comment.
why was the char removed? Or, why did it work before?
There was a problem hiding this comment.
It's a bug in the previous implementation that allowed garbage data after the directive https://github.com/docker/docker/blob/61753568f5e482c551df6f3693cb7c942261b7de/builder/dockerfile/parser/parser.go#L51 . Only ` and \ are valid escape characters.
There was a problem hiding this comment.
Actually it was kinda by design ;) The escape semantic was # escape = char. So it literally deliberately only interpreted the char bit, and ignored anything after it.
There was a problem hiding this comment.
@jhowardmsft It would be very easy to fix but I don't agree this is correct. Directive syntax should be # key=value, not # key=value+any garbage. Directives in general are not limited to single char.
There was a problem hiding this comment.
I don't disagree with you in the slightest. Just commenting on why it was as it was - I know that bit came up in one of the many conversations when introducing the feature 😇
builder/dockerfile/parser/parser.go
Outdated
There was a problem hiding this comment.
I guess the comment could be updated here for this line.
builder/dockerfile/parser/parser.go
Outdated
There was a problem hiding this comment.
This concerns me causing in panic in the daemon...
There was a problem hiding this comment.
There is no way for any external data to cause this. This can only happen if code change introduces a bug. This is functionally equal to MustCompile of a regexp.
There was a problem hiding this comment.
Ok, but I'd still rather just log something.
builder/dockerfile/parser/parser.go
Outdated
There was a problem hiding this comment.
Is this returned externally? I don't believe we ever returned an error back to the caller on a bad directive, just silently swallowed it (by design).
There was a problem hiding this comment.
This error is for catching the "silent error", if parsing reaches this error it will not be reported but parsing will just continue.
builder/dockerfile/parser/parser.go
Outdated
There was a problem hiding this comment.
I thought this was swallowed (by design) before and just ignored, the whole line being treated as a comment.
There was a problem hiding this comment.
This causes a real error in current version and in this version.
There was a problem hiding this comment.
OK. I'd forgotten that then, but it was a while back 😇
|
Should we wait for #24725 to be merged? That one has been open for a long time |
|
@thaJeztah No need - they're not at all related. |
tonistiigi
force-pushed
the
parse-directives
branch
from
67272b6 to
ec58e16
Compare
Refactor so that new directives can be easily added in the future. Fix bug where directive with whitespace prefix would still be applied. Signed-off-by: Tonis Tiigi <[email protected]>
tonistiigi
force-pushed
the
parse-directives
branch
from
ec58e16 to
936e516
Compare
|
Seems reasonable to me. Moving to code review unless any objection from @duglin or @thaJeztah. Probably worth squashing the commits. My only real thing is the just-in-case |
|
ping @duglin @jhowardmsft PTAL! |
|
|
||
| # Switch back to root and double check that worked exactly as we might expect it to | ||
| USER root | ||
| # Add a "supplementary" group for our dockerio user |
There was a problem hiding this comment.
Should this be in a separate PR? Doesn't seem related...
There was a problem hiding this comment.
It is needed. I thought I left a comment for that in this pr... . It is the same fix as in https://github.com/docker/docker/pull/24725/files#diff-64d1c3a97fd8b1abf170680883e81aaaR3580 . Doesn't matter how we handle empty lines, this Dockerfile should not be parseable.
|
Minor comments, LGTM when those are addressed. |
|
@tonistiigi needs rebase |
6 participants
Refactor so that new directives can be easily added in the future by just adding a new lines in https://github.com/tonistiigi/docker/blob/parse-directives/builder/dockerfile/parser/parser.go#L150-L152 . Simplify so that there is no need to store an instance of directives with the builder or pass it to the
Parse()function.Fix bug where directive with whitespace prefix would still be applied.
In current version, error handling was inconsistent. Specifying invalid escape character would not produce an error but using escape directive twice would. Changed it so that if the directive has correct syntax and valid directive key, it will fail with a proper error.
cc @jhowardmsft