Restore env replacement process of ARG's variable names

[kariya-mitsuru]

Up to 1.7.1, variable names in ARG command could also use environment replacement, but in commit 47a52a1 remove the replacement process, so the replacement for variable names is no longer available in 1.8.0.
This PR restore this.
In addition, remove the Expand method of ArgCommand because it was no longer used.

[tonistiigi]

What's the example case? Seems to work fine for me in v0.15.0-rc2

0.074 abc=123
0.074 PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
0.074 FOO=bar123
0.074 PWD=/
0.074 /bin/sh: stop: not found
------
WARNING: No output specified with docker-container driver. Build result will only remain in the build cache. To push result image into registry use --push or to load image into docker use --load
Dockerfile:4
--------------------
   2 |     env abc=123
   3 |     arg FOO=bar${abc}
   4 | >>> run env && stop
   5 |
--------------------

If there is a regression then needs tests as well.

[tonistiigi]

Ah, I understand the report now. The replacement is in the key of ARG, not in the value.

0.060 FOO=bar
0.060 rhubarb=123
0.060 PWD=/
0.061 /bin/sh: stop: not found
------
Dockerfile:4
--------------------
   2 |     env FOO=bar
   3 |     arg rhu${FOO}b=123
   4 | >>> run env && stop
   5 |
--------------------

I had no idea this was supported. It doesn't look very useful as you need to have static keys for passing --build-arg anyway and not what you can do in bash. This also allows spaces, for example, that otherwise I think are not possible.

@thaJeztah @dvdksn Do you know if this is documented behavior? Or something that you think is being relied on?

[tonistiigi]

More weird cases (with v0.13)

0.037 rb=123
0.037 rhuba=rb=123
0.037 FOO=ba=r
0.037 /bin/sh: stop: not found
------
--------------------
   2 |     env FOO="ba=r"
   3 |     arg rhu${FOO}b=123
   4 | >>> run echo "$rhuba" && env && stop
   5 |
--------------------

[kariya-mitsuru]

Thanks for the quick response!

Honestly, I have to confess that I originally didn't think this was supported either, but when I looked into it earlier, I noticed that it was available for ENV, LABEL and ARG. At the time, I thought that, aside from ARG, there must surely be a useful usecase for ENV and LABEL, and that ARG was probably supported for the sake of syntactic consistency. (Because it was implemented intentionally)

I stumbled upon these behaviors while looking into environment replacement, and unfortunately, I couldn't find any documentation about it.

Recently, I happened to notice that in version docker/dockerfile:1.8.0, only ARG is no longer supported, so I create this PR. I don't need this feature myself, however I just thought it would be better to restore if it had unexpectedly be modified in the above commit.

Therefore, if ARG shouldn't be supported, feel free to reject this PR.

(In any case, I don't think we need ArgCommand's Expand method, so I will create another PR in that case.)

[dvdksn]

I don't think this is a documented behavior. Personally I don't think interpolation for keys seems like a good idea.

[thaJeztah]

Yes, I don't think this is documented either, but admittedly we've not always been great at documenting some of the semantics in-depths (specifically in the early beginnings of Docker). Worse, in some cases PRs have been accepted that updated the docs to match the behavior, which in some cases led to documenting a bug as "intended", so we must always keep our eyes peeled to see if the behavior is the expected behavior 😅

I do recall that (at least with the classic builder), we had some cases in the past where code was shared between parsing of LABEl, ARG, and ENV (but this may have been in the CLI for parsing corresponding flags); that resulted in substitution happening in places where it shouldn't. Testing on the classic builder, it looks like it does expand, so we may have to look if we can search GitHub (and elsewhere) to see if there's people depending on this 😞

DOCKER_BUILDKIT=0 docker build -t foo -<<'EOF'
FROM alpine

ENV FOO=foo
ENV FOO$FOO=foofoo
LABEL FOO$FOO=foofoo
EOF

docker image inspect foo --format 'ENV: {{json .Config.Env}} LABEL: {{json .Config.Labels}}'
ENV: ["PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin","FOO=foo","FOOfoo=foofoo"] LABEL: {"FOOfoo":"foofoo"}

For this one;

• For ENV and ARG; if it's not documented, I'd consider it a bug if expansion worked before. At least it was not a conscious decision to support it.
• I don't think we should support this feature (for ENV, ARG), unless there's really strong arguments and clear use-cases; but if we do, we must have a close eye on validation (as mentioned: this could allow for env-vars with spaces to be created?); validation is particularly tricky for env-vars, because what's supported is not well-defined in the kernel; we once added validation, but it turned out that there's tools depending on invalid (or "not recommended") env-vars, so we removed validation (or severely limited to only deny null).
• 👉 for LABEL, I think we could support this, as there may be good reasons to include a build-argument as part of label names (LABEL com.foo.$SOME_VAR=$SOME_OTHER_VAR)

[S-Fichtl]

Thank you @tonistiigi for linking my Bug report to this PR.
@kariya-mitsuru @thaJeztah @dvdksn please see my Bug #5178 report for a very practical use-case for this functionality that we have relied on for years in my company, and I can't imagine that we're the only ones who use it this way.