buildctl: read a valueless build-arg from the environment

[ravi-arnan]

Closes #7028

--opt build-arg:FOO failed the whole build with invalid opt: invalid value build-arg:FOO instead of taking FOO from the client environment, so callers had to spell out --opt build-arg:FOO=$FOO.

Why this is a consistency fix rather than a new feature

buildctl already sources a build arg from the environment: loadOptEnv (cmd/buildctl/build/util.go) propagates SOURCE_DATE_EPOCH into build-arg:SOURCE_DATE_EPOCH. docker buildx build --build-arg FOO reads the environment as well. The valueless --opt build-arg:FOO was the only form that did not.

Scope

The change is in ParseOpt, not in attrMap. attrMap is shared with ParseLocal, where --local name without a value should keep failing, so relaxing it there would be too broad. There is a second, unrelated copy of attrMap at cmd/buildkitd/main.go:1003 that parses daemon flags; it is deliberately untouched.

Only the build-arg: prefix is affected. A bare --opt target still errors, and so does --opt build-arg: with no name.

A variable that is not present in the environment is left out of the frontend attributes entirely rather than sent as an empty string, so a default in the Dockerfile still applies. That matches buildx.

Verification

New unit tests in cmd/buildctl/build/opt_test.go, 8 cases. The 3 that cover the new behaviour fail on master with exactly the reported error; the 5 regression cases pass on both.

End to end against a real buildkitd (moby/buildkit:latest in a container, reached over docker-container://), using the reporter's Dockerfile:

$ printf 'ARG IMAGE\nFROM $IMAGE\nRUN echo working\n' > Dockerfile
$ export IMAGE=alpine

$ buildctl-master build --frontend dockerfile.v0 --local dockerfile=. --local context=. --opt build-arg:IMAGE
error: invalid opt: invalid value build-arg:IMAGE

$ buildctl-fixed build --frontend dockerfile.v0 --local dockerfile=. --local context=. --opt build-arg:IMAGE
#5 [2/2] RUN echo working
#5 0.118 working
#5 DONE 0.1s

And the unset case, confirming the Dockerfile default is what applies rather than an empty value:

$ printf 'ARG IMAGE=busybox\nFROM $IMAGE\nRUN echo default-applied\n' > Dockerfile
$ unset IMAGE

$ buildctl-fixed build --frontend dockerfile.v0 --local dockerfile=. --local context=. --opt build-arg:IMAGE
#5 [2/2] RUN echo default-applied
#5 0.141 default-applied
#5 DONE 0.2s

gofmt -s, go vet and go test ./cmd/buildctl/build/ are clean.

Docs updated in docs/reference/buildctl.md.

[tonistiigi]

This can mess up the order of the what opt wins on duplicates. Should still add to rest.

[tonistiigi]

nit: this reads better if conditions are reversed.

[ravi-arnan]

Both fixed, thanks.

The ordering one was a real bug rather than just a style issue: with --opt build-arg:FOO=bar --opt build-arg:FOO the valueless form came last but lost, because it was written straight into the result map before attrMap ran over the rest. Resolved args now keep their position in the list and go through attrMap like everything else, so last-wins holds in both directions. Added cases for both orders.

Reversing the conditions fell out of the same rewrite.

One case I had to pick a behaviour for, happy to change it: --opt build-arg:FOO=bar --opt build-arg:FOO where FOO is not in the environment. The unset valueless arg is skipped rather than clearing the earlier explicit value, so the result is bar. That keeps "not in the environment means leave it out entirely", but it does mean an unset valueless arg is not a way to unset a previous one. Pinned it with a test so it is at least explicit.

[tonistiigi]

just to be safe these test should clear IMAGE or use some env that is specific enough to never be accidentally defined by env

[ravi-arnan]

Good point, that was a real hole. With IMAGE set in the ambient environment two
cases failed: "a build arg missing from the environment is dropped" picked up the
ambient value, and "a valueless build arg missing from the environment does not
clear an earlier value" returned it instead of busybox.

Extended the existing SOURCE_DATE_EPOCH guard to clear IMAGE as well, so the
cases are hermetic regardless of what the caller has exported. Went with clearing
rather than a more specific variable name so it stays correct if the name is ever
changed.