Skip to content
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

bake: add wildcard to fs entitlements to allow any paths #2812

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

crazy-max
Copy link
Member

follow-up #2796 (comment)

@crazy-max
Copy link
Member Author

Removed long path name handling in our tests 619fa8a but it fails: https://github.com/docker/buildx/actions/runs/11972131393/job/33378391055#step:6:1330

 === Failed
=== FAIL: bake TestValidateEntitlements/ExportLocal (0.00s)
    entitlements_test.go:352: 
        	Error Trace:	D:/a/buildx/buildx/bake/entitlements_test.go:352
        	Error:      	Not equal: 
        	            	expected: bake.EntitlementConf{NetworkHost:false, SecurityInsecure:false, FSRead:[]string{"D:\\a\\buildx\\buildx\\bake"}, FSWrite:[]string{"C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\TestValidateEntitlements4101399922\\001", "C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\TestValidateEntitlements4101399922\\002"}, ImagePush:[]string(nil), ImageLoad:[]string(nil), SSH:false}
        	            	actual  : bake.EntitlementConf{NetworkHost:false, SecurityInsecure:false, FSRead:[]string{"D:\\a\\buildx\\buildx\\bake"}, FSWrite:[]string{"C:\\Users\\runneradmin\\AppData\\Local\\Temp\\TestValidateEntitlements4101399922\\001", "C:\\Users\\runneradmin\\AppData\\Local\\Temp\\TestValidateEntitlements4101399922\\002"}, ImagePush:[]string(nil), ImageLoad:[]string(nil), SSH:false}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -7,4 +7,4 @@
        	            	  FSWrite: ([]string) (len=2) {
        	            	-  (string) (len=75) "C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\TestValidateEntitlements4101399922\\001",
        	            	-  (string) (len=75) "C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\TestValidateEntitlements4101399922\\002"
        	            	+  (string) (len=78) "C:\\Users\\runneradmin\\AppData\\Local\\Temp\\TestValidateEntitlements4101399922\\001",
        	            	+  (string) (len=78) "C:\\Users\\runneradmin\\AppData\\Local\\Temp\\TestValidateEntitlements4101399922\\002"
        	            	  },
        	Test:       	TestValidateEntitlements/ExportLocal
    --- FAIL: TestValidateEntitlements/ExportLocal (0.00s)

=== FAIL: bake TestValidateEntitlements/SecretFromEscapeLink (0.00s)
    entitlements_test.go:352: 
        	Error Trace:	D:/a/buildx/buildx/bake/entitlements_test.go:352
        	Error:      	Not equal: 
        	            	expected: bake.EntitlementConf{NetworkHost:false, SecurityInsecure:false, FSRead:[]string{"C:\\Users\\RUNNER~1\\AppData\\Local\\Temp"}, FSWrite:[]string(nil), ImagePush:[]string(nil), ImageLoad:[]string(nil), SSH:false}
        	            	actual  : bake.EntitlementConf{NetworkHost:false, SecurityInsecure:false, FSRead:[]string{"C:\\Users\\runneradmin\\AppData\\Local\\Temp"}, FSWrite:[]string(nil), ImagePush:[]string(nil), ImageLoad:[]string(nil), SSH:false}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -4,3 +4,3 @@
        	            	  FSRead: ([]string) (len=1) {
        	            	-  (string) (len=36) "C:\\Users\\RUNNER~1\\AppData\\Local\\Temp"
        	            	+  (string) (len=39) "C:\\Users\\runneradmin\\AppData\\Local\\Temp"
        	            	  },
        	Test:       	TestValidateEntitlements/SecretFromEscapeLink
    --- FAIL: TestValidateEntitlements/SecretFromEscapeLink (0.00s)

=== FAIL: bake TestValidateEntitlements (1.07s)

It makes sense as the expected paths don't match anymore like

"dest": dir1,

So as filepath.EvalSymlinks evaluates itself to a long path name in entitlements implementation we don't have the choice to handle it in our tests as well.

Here is a repro of the behavior: https://github.com/crazy-max/go-longpath-win/blob/435f1ae22f324f213b7c7b49be7165aa9eafbd3d/main.go

tmpDir := os.TempDir()
wd, _ := os.Getwd()
fmt.Printf("TempDir: %s\n", tmpDir)
fmt.Printf("WorkingDir: %s\n", wd)

tmpDirSym, _ := filepath.EvalSymlinks(tmpDir)
wdSym, _ := filepath.EvalSymlinks(wd)
fmt.Printf("TempDirEvalSymlinks: %s\n", tmpDirSym)
fmt.Printf("WorkingDirEvalSymlinks: %s\n", wdSym)

Output on GHA: https://github.com/crazy-max/go-longpath-win/actions/runs/11972415653/job/33379286983#step:4:7

TempDir: C:\Users\RUNNER~1\AppData\Local\Temp
WorkingDir: D:\a\go-longpath-win\go-longpath-win
TempDirEvalSymlinks: C:\Users\runneradmin\AppData\Local\Temp
WorkingDirEvalSymlinks: D:\a\go-longpath-win\go-longpath-win

@crazy-max crazy-max changed the title bake: filesystem entitlements fixes for windows bake: add wildcard to fs entitlements to allow any volumes on windows Nov 22, 2024
Comment on lines 346 to 366
expected: EntitlementConf{
FSRead: func() []string {
// on windows root (/) is only allowed if it is the same volume as wd
if filepath.VolumeName(wd) == filepath.VolumeName(escapeLink) {
return nil
}
// if not, then escapeLink is not allowed
p, err := evaluateToExistingPath(escapeLink)
require.NoError(t, err)
return []string{p}
}(),
},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert the change to allow any volumes for / on windows and handle that case in our tests instead.

@crazy-max crazy-max added this to the v0.19.0 milestone Nov 22, 2024
@crazy-max crazy-max marked this pull request as ready for review November 22, 2024 12:47
@crazy-max crazy-max requested a review from tonistiigi November 22, 2024 12:47
@crazy-max crazy-max changed the title bake: add wildcard to fs entitlements to allow any volumes on windows bake: add wildcard to fs entitlements to allow any paths Nov 25, 2024
bake/entitlements.go Outdated Show resolved Hide resolved
@crazy-max crazy-max merged commit 5ce6597 into docker:master Nov 25, 2024
122 checks passed
@crazy-max crazy-max deleted the bake-win-fs-ent branch November 25, 2024 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants