-
Notifications
You must be signed in to change notification settings - Fork 598
build: create parent directories for tar output in lazy writer #3478
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: CrazyMax <[email protected]>
27c1582 to
28b6750
Compare
| if sb.DockerAddress() == "" { | ||
| // there is no Docker atm to load the image | ||
| outFlag += ",dest=" + dirDest + "/image.tar" | ||
| envs = append(envs, "BUILDX_BAKE_ENTITLEMENTS_FS=0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to make changes in this test to disable fs entitlements otherwise it fails: https://github.com/docker/buildx/actions/runs/18711570130/job/53361047820?pr=3478#step:7:481
=== RUN TestIntegration/TestBuildMetadataProvenance/worker=remote/default
bake.go:1375:
Error Trace: /src/tests/bake.go:1375
/src/tests/bake.go:1334
Error: Received unexpected error:
exit status 1
Test: TestIntegration/TestBakeMetadataProvenance/worker=remote/max
Messages: #0 building with "integration-remote-44bdm3ecqo4ofzttczc4bg3yy" instance using remote driver
#1 [internal] load local bake definitions
#1 reading docker-bake.hcl 22B / 22B done
#1 DONE 0.0s
Your build is requesting privileges for following possibly insecure capabilities:
- Write access to path ../002
Pass "--allow=fs.write=../002" to grant requested privileges.
Your full command with requested privileges:
buildx bake --allow=fs.write=../002 --metadata-file /tmp/TestIntegrationTestBakeMetadataProvenanceworker=remotemax18445594/002/md.json --set default.output=type=docker,dest=/tmp/TestIntegrationTestBakeMetadataProvenanceworker=remotemax18445594/002/image.tar
To disable filesystem entitlements checks, you can set BUILDX_BAKE_ENTITLEMENTS_FS=0 .
ERROR: additional privileges requested
Seems fs entitlements were not correctly applied for tar output before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fs entitlements were not correctly applied for tar output before.
As we lazily create directories now for tar output then it goes through
Lines 595 to 603 in abf6ab4
| // If the component doesn't exist, return the last valid path | |
| if os.IsNotExist(err) { | |
| for r := len(dest) - 1; r >= volLen; r-- { | |
| if os.IsPathSeparator(dest[r]) { | |
| return dest[:r], in[start:], nil | |
| } | |
| } | |
| return vol, in[start:], nil | |
| } |
Before the tar output file was already created before entitlement check:
Line 879 in f478741
| f, err := os.Create(entry.Destination) |
Line 1542 in f478741
| bo.Exports, bo.ExportsLocalPathsTemporary, err = build.CreateExports(t.Outputs) |
Lines 595 to 603 in abf6ab4
| // If the component doesn't exist, return the last valid path | |
| if os.IsNotExist(err) { | |
| for r := len(dest) - 1; r >= volLen; r-- { | |
| if os.IsPathSeparator(dest[r]) { | |
| return dest[:r], in[start:], nil | |
| } | |
| } | |
| return vol, in[start:], nil | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum actually this is not correct, this test should have failed before. I'm taking another look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I think I get it now, it seems it does not collect overrides for checking entitlements:
target "default" {}$ docker buildx bake --set *.output=type=docker,dest=../dist/result.tar
#0 building with "default" instance using docker driver
#1 [internal] load local bake definitions
#1 reading docker-bake.hcl 21B / 21B done
#1 DONE 0.0s
#2 [internal] load build definition from Dockerfile
#2 transferring dockerfile: 2B done
#2 DONE 0.1s
Which makes sense as override is a user input but don't think we support this atm. cc @tonistiigi
So before my change the tar file was already created before the entitlement check and didn't request privilege which is wrong.
Now with my change we create the tar file lazily and therefore entitlement checks the parent dir and requests privilege which is good but if I create the file before:
$ touch ../dist/result.tar
$ docker buildx bake --set *.output=type=docker,dest=../dist/result.tar
#0 building with "default" instance using docker driver
#1 [internal] load local bake definitions
#1 reading docker-bake.hcl 21B / 21B done
#1 DONE 0.0s
#2 [internal] load build definition from Dockerfile
#2 DONE 0.0s
Then it builds and does not request privilege which is wrong. So we need to fix this.
This change updates the tar output logic for exporters to ensure parent directories are created automatically when writing to a file destination.
Previously, file creation would fail if the target directory did not exist, which would fail builds that rely on dynamic directory creation:
I have created a lazy file writer that calls
os.MkdirAllbefore creating the file so we are consistent with thelocaloutput that already creates subdirectories if they don't exist.With bake:
Before:
After:
This also fixes another issue where there is left over build result on client side before the build even starts like:
cc @maxcleme