Allow symbolic modes in COPY/ADD --chmod - #324
Conversation
checkChmodConversion rejected anything that was not an octal number, so Containerfiles using symbolic --chmod clauses (+x, u+x, a+rX,go-w, u+rX-w, ...) failed at parse time with "Error parsing chmod". Numeric and symbolic --chmod are both part of the dockerfile frontend spec since Dockerfile syntax 1.14, so these forms are mainstream now. Accept symbolic modes by validating them with mode.Parse from github.com/tonistiigi/dchapes-mode, which owns this grammar (it is what buildah uses to resolve these values and what BuildKit's frontend uses), rather than restating the grammar here: chmod(1) allows repeated op-perms groups within a clause (u+r-w), which a hand-rolled subset validator would keep getting wrong. Parsing only checks the syntax; the clauses are resolved against each copied file's current mode by the executor, where conditional bits such as the capital X in a+rX are meaningful. The dependency is already in the module graph via moby/buildkit and is stdlib-only. The dockerclient executor resolved Chmod with ParseInt octal-only, which would have turned these newly-accepted values into a later, fuzzier error; resolve them with mode.Parse there too. The tar-header rewrite (applyChmod) goes through h.FileInfo().Mode() so symbolic clauses see a correct os.FileMode (directory type, special bits) and maps setuid, setgid, and sticky back onto their unix bit positions; a raw cast of the tar mode to os.FileMode would write those bits in the wrong places and silently drop them. applyChmod has unit coverage for numeric, symbolic, special-bit, and directory-X cases, which the conformance suite only gates behind a build tag and a live daemon. Two deliberate behavior alignments, both matching chmod(1) and BuildKit, are worth calling out: numeric (absolute) modes now clear pre-existing setuid/setgid/sticky bits instead of preserving them, and octal values above 07777 (e.g. 17777) are rejected at parse time instead of being masked down at execution. Octal behavior is otherwise unchanged, including rejecting non-octal digits, 0o-prefixed values, and ls-style mode strings like rwxrwxrwx.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: paoloantinori The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @paoloantinori. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
| return nil | ||
| } | ||
| if _, err := mode.Parse(chmod); err != nil { | ||
| return fmt.Errorf("Error parsing chmod %s", chmod) |
There was a problem hiding this comment.
Would be good to add the err here
| return fmt.Errorf("Error parsing chmod %s", chmod) | |
| return fmt.Errorf("Error parsing chmod %s: %w", chmod, err) |
| parsed, err := mode.Parse(c.Chmod) | ||
| if err != nil { | ||
| return err | ||
| return fmt.Errorf("invalid chmod %q", c.Chmod) |
There was a problem hiding this comment.
ditto error add here
| return fmt.Errorf("invalid chmod %q", c.Chmod) | |
| return fmt.Errorf("invalid chmod %q : %w", c.Chmod, err) |
|
Nice change overall. LGTM |
|
Both nits addressed in 2ca5e05 (error now wraps the underlying parse error in both spots). Tests re-run: root, dockerclient, parser and imageprogress packages all ok. |
|
Changes LGTM |
|
Might be related to github's issues yesterday. /ok-to-test I restarted the Travis tests, and they appear to be failing in a conformance test that needs TestConformanceExternal/copy and env interaction: update test context from #325 to catch it up with some changes in the repository that it tests building from. @paoloantinori, if you want to cherry-pick that to get the conformance tests to pass here to not be blocked on that one getting reviewed and merged, that should sort this one out. |
openshift-ci
Bot
added
ok-to-test
Update the context directory used for a conformance test that references an external git repository to keep up with changes on the main branch. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
|
@paoloantinori: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Thanks both for the reviews and the CI restarts. I'll confess I'm not fully familiar with this project's merge process, so I hope I'm not being pushy or bothering anyone with the following - please just ignore this if there's a queue I should be waiting in. Since the text LGTMs don't seem to set the label on their own: @TomSweeneyRedHat, would you mind dropping a formal /assign @nalind |
3 participants
Fixes the Containerfile
COPY/ADD --chmod=instruction path to accept symbolic modes, restoring parity with the dockerfile frontend spec (numeric and symbolic--chmodare both part of the spec since Dockerfile syntax 1.14) and withbuildah copy/add --chmod=, which already resolve symbolic values.Problem
checkChmodConversionvalidated--chmod=withstrconv.ParseUint(octal only) at dispatch time, so every symbolic form (+x,u+x,a+rX,go-w, ...) failed withError parsing chmod <mode>before the executor ever saw it. This is the root cause of podman-container-tools/buildah#6938: buildah's execution layer is already symbolic-aware (PR buildah#6778), but the parser vendored from here rejected the instruction form. That issue includes a repro on released buildah 1.45.0 showing the CLI subcommand and the Containerfile instruction disagreeing on the very same binary.Change
checkChmodConversionaccepts octal (now range-checked to 0-07777, as BuildKit's frontend does) and validates symbolic modes by delegating tomode.Parsefromgithub.com/tonistiigi/dchapes-mode. Delegation rather than a local grammar re-implementation: chmod(1) allows repeated op-perms groups within a clause (u+r-w), which a hand-rolled subset validator gets wrong; dchapes-mode is the library buildah's executor uses to resolve these values and what BuildKit's frontend uses.Parseonly builds the command list (no base mode needed); resolution against each file's current mode, where the conditionalXis meaningful, stays with the executor.dockerclientexecutor resolvedCopy.Chmodwithstrconv.ParseIntoctal-only; with the parser accepting symbolic values it would have failed mid-execution with a fuzzier error. It now parses once withmode.Parseand applies per file via the newapplyChmodhelper, which goes throughh.FileInfo().Mode()(so symbolic clauses see the directory type and special bits correctly) and maps setuid/setgid/sticky back onto their unix bit positions in the tar header. A raw cast of the tar mode toos.FileModewould write those bits in the wrong positions and silently drop them.applyChmodhas unit coverage (numeric, symbolic, special bits, directory-X); the conformance suite only exercises this behind a build tag plus a live daemon.Copy.Chmodfield contract (octal 0-07777 or chmod(1) symbolic clauses, passed to the executor for resolution), mirroringChown.The new dependency is stdlib-only, two files, already in the module graph at this exact pseudo-version:
moby/buildkitv0.29.0 (a direct dependency) requires it, so buildah/podman consumers see no new modules.Behavior
Copy.Chmodfor executors to resolve.Deliberate behavior changes (reviewers please sanity-check):
17777) are now rejected at parse time instead of being masked down at execution.dockerclientexecutor, numeric (absolute) modes now clear pre-existing setuid/setgid/sticky bits instead of preserving them, matchingchmod(1)absolute-mode semantics.Verified end-to-end against buildah
main(replace directive to this branch;FROM scratchbuilds; modes read back from the built image layer):--chmod=a+rX,go-w+xu+x,go-wu+rX-wg+r=rxThe conditional
Xresolves per-file as expected (a+rXon a 0700 script yields 0755). Octal builds are unchanged, and888,rwxrwxrwx,0o755, and17777are rejected.Fixes podman-container-tools/buildah#6938 (see also podman-container-tools/podman#28293). Once this lands, buildah needs only a revendor (the
Copy.ChmodtoAddAndCopyOptions.Chmodwiring already exists on itsmain); I can send that PR with abudtest as soon as there is a revendorable commit.