Fix #2439 (COPY with Heredocs eats quotes)

[jedevc]

Woops 😄

From the commit message:

In the contents of COPY/ADD, we perform expansion on variables using a lexer. However, this lexer, by default, removes quotes as well expanding variables - this isn't really the kind of behavior we're after, as it feels quite unintuitive.

To fix this, we introduce a new ExpandRaw function, which commands can implement that implement an alternative expansion that preserves quotes (and possibly other characters/features in the future).

Additionally, we introduce new tests to more clearly define the behavior. One major note is that backslashes are not passed and are processed, following normal escape rules (so that we can use $ symbols directly).

This fixes #2439 but does also leave backslashes in COPY not working quite as one might expect - I think this can probably be fixed with some documentation, happy to patch that if needed. Additionally, users can get "raw" content by quoting the heredoc to prevent expansion, i.e. <<"EOF".

[tonistiigi]

Why isn't this done for the other commands as well? Where does it break in these cases?

[jedevc]

It shouldn't cause any problems in the RUN command - because with those, we leave those alone completely until container runtime, so they're not expanded at all by buildkit, but by the native shell in whatever container image is being built.

COPY/ADD are special, because we do some expansion at build, specifically doing environment variable replacement in the file contents. However, since we use the Lex type for that, by default it strips quotes in processing words, which is why they get removed.

This fix is pretty much an elaborate "don't remove those quotes for heredocs" - we do still want to remove them for all other commands though, preserving the behaviour we had before, which this fix does.

[tonistiigi]

Can you give me an example where the old behavior is preferred for COPY/ADD (outside heredocs). When you use quotes in path?

The expansion we do just for display in progress bar would be better with quotes kept I think? Eg. RUN ./myapp "foo $bar" baz

FYI @thaJeztah

[jedevc]

Of course 🎉

ENV foo="bar"

Currently, this sets the variable foo to the contents bar - choosing not to strip quotes like this in Expand would instead set foo to "bar".

[thaJeztah]

choosing not to strip quotes like this in Expand would instead set foo to "bar".

Ah, hm, yes, that would be problematic. Originally that was the behaviour (anything after = would be treated as value, including quotes (if any)), but this confused users, so support for stripping quotes was added at some point (long time ago though, in the classic builder)

[jedevc]

Is there anything left blocking this being merged? Let me know if there's still anything missing 👍

[tonistiigi]

Could we have a case with <<'EOF' as well to verify that \ is retained in that case. If these cases already exist in other tests then comment pointing to these cases from here would do.

[thaJeztah]

Ah, I think I wanted to compare the behavior with doing the equivalent in a shell. Here's doing so:

No quotes (plain EOF)

export world=world

cat > myfile <<EOF
\
One
\\
Two
\
Hello "$world"
Hello '$world'
\$
EOF

cat myfile
One
\
Two
Hello "world"
Hello 'world'
$

Single quotes ('EOF')

cat > myfile <<'EOF'
\
One
\\
Two
\
Hello "$world"
Hello '$world'
\$
EOF

cat myfile
\
One
\\
Two
\
Hello "$world"
Hello '$world'
\$

Double quotes ("EOF")

cat > myfile <<"EOF"
\
One
\\
Two
\
Hello "$world"
Hello '$world'
\$
EOF

\
One
\\
Two
\
Hello "$world"
Hello '$world'
\$

(edit: added \$ and single quotes within the here doc)

[thaJeztah]

I agree with @tonistiigi #2442 (comment) - it would make sense to verify both the "plain" EOF and the quoted variant(s); perhaps something along the cases I wrote above

[jedevc]

Have taken the test cases from @thaJeztah's comment and split them up across a couple tests - I've put the backslashes and dollars and such into testCopyHeredocSpecialSymbols and the quotes related ones into testHeredocVarSubstitution.

Everything seems ok to me, except for the case:

COPY <<EOF /dest/q1
Hello '${name}'!
EOF

For this, we were producing the wrong result, and not expanding ${name} - this is because POSIX treats the whole heredoc as one double-quoted word. So, to achieve the same results, I've added a new property to the lexer which will do the same thing by ignoring and passing over quotes entirely - then we can use this for the ExpandRaw function.

[jedevc]

Not quite sure what the test failure is - doesn't look to me like anything that should've broken? I also can't seem to reproduce it.

[crazy-max]

Not quite sure what the test failure is - doesn't look to me like anything that should've broken? I also can't seem to reproduce it.

Yeah that's a flaky one. I re-run the workflow.

[thaJeztah]

SGTM, but deferring to @tonistiigi and @crazy-max, who are more familiar with the code

[LecrisUT]

I wonder if this PR introduced an issue like containers/buildah#5422

[jedevc]

@LecrisUT this is an old PR - the linked issue is in buildah, which is unrelated to this project, so it seems unlikely to have been caused here.

[LecrisUT]

Oh, sorry about that. I had a tab open with a similar PR in buildah