Add FileActionSymlink and `llb.Symlink` #5519

pmengelbert · 2024-11-13T23:00:42Z

Currently, there is no way to create a symlink directly using llb. This PR adds a new FileAction which handles creating symlinks. Without this change, users have to run a command with llb.State.Run(...), which requires a shell or some other program to create the link.

This PR also adds a test for the new feature.

@cpuguy83

cpuguy83 · 2024-11-13T23:50:17Z

client/llb/fileop.go

+	SetSymlinkOption(*SymlinkInfo)
+}
+
+func Symlink(target, linkpath string) *FileAction {


Looks like the function needs to be modified to accept ...SymlinkOption.

jedevc

Hey nice, I've also wanted something like this 🎉

solver/pb/ops.proto

jedevc · 2024-11-14T00:44:01Z

solver/pb/ops.proto

+		// FileActionSymlink creates a symlink
+		FileActionSymlink symlink = 8;


We should also add a capability https://github.com/moby/buildkit/blob/master/solver/pb/caps.go.

Can you be more specific? What exactly needs to be added and why?

This is done

I checked with @cpuguy83 but you may want to double-check the changes to make sure everything is there

tonistiigi

There are certain file attributes that can be set separately from the target file and should be configurable (as they are with regular files).

/tmp/a # ls -l
total 12
-rw-r--r--    1 root     root            21 Nov 14 01:28 dummy_file.txt
lrwxrwxrwx    1 root     nobody          14 Nov 14 02:28 dummy_symlink.txt -> dummy_file.txt

tonistiigi · 2024-11-14T02:15:48Z

solver/pb/ops.proto

+	// destination path for the new file representing the link
+	string target = 1;
+	// source path for the link
+	string linkPath = 2;


just name it src.

Alternatively, in Go&unix these are newpath and oldpath.

I just pushed up some updates, but I think I'll modify these to oldpath and newpath, I think it gives the clearest indication of which is which.

solver/pb/ops.proto

tonistiigi · 2024-11-14T02:21:13Z

solver/llbsolver/vertex.go

@@ -391,6 +391,8 @@ func fileOpName(actions []*pb.FileAction) string {
 			names = append(names, fmt.Sprintf("mkdir %s", a.Mkdir.Path))
 		case *pb.FileAction_Mkfile:
 			names = append(names, fmt.Sprintf("mkfile %s", a.Mkfile.Path))
+		case *pb.FileAction_Symlink:
+			names = append(names, fmt.Sprintf("symlink %s -> %s", a.Symlink.LinkPath, a.Symlink.Target))


I think this arrow makes more sense with arguments in reverse order. Eg. how it is in ls -l.

Clarifying, you mean you'd like to see symlink newpath -> oldpath (the version displayed by ls -l), rather than symlink oldpath -> newpath (the order of the arguments to ln -s and the symlink syscall)

If the above is correct, this is done

tonistiigi · 2024-11-14T02:21:42Z

solver/llbsolver/ops/file_test.go

@@ -643,6 +644,13 @@ func (b *testFileBackend) Mkfile(_ context.Context, m, user, group fileoptypes.M
 	return nil
 }

+func (b *testFileBackend) Symlink(_ context.Context, m fileoptypes.Mount, a *pb.FileActionSymlink) error {
+	mm := m.(*testMount)
+	mm.id += "-mkfile"


Thanks, missed this one.

pmengelbert · 2024-11-19T12:34:48Z

There are certain file attributes that can be set separately from the target file and should be configurable (as they are with regular files).
/tmp/a # ls -l
total 12
-rw-r--r--    1 root     root            21 Nov 14 01:28 dummy_file.txt
lrwxrwxrwx    1 root     nobody          14 Nov 14 02:28 dummy_symlink.txt -> dummy_file.txt

TIL about symlink ownership and timestamps. Thanks! I just pushed up some updates with symlink timestamps and ownership added -- feel free to review while I make the rest of the changes.

I've tested them manually, but I'll need to add some tests that verify they both work.

tonistiigi · 2024-11-19T18:35:35Z

client/llb/fileop.go

@@ -336,6 +347,51 @@ func Mkfile(p string, m os.FileMode, dt []byte, opts ...MkfileOption) *FileActio
 	}
 }

+type SymlinkInfo struct {
+	ChownOpt    *ChownOpt


Mode as well

According to the man pages, symlink permissions are irrelevant. Is the mode for the SUID, SGID, and sticky bits? What effect do those have on symlinks?

tonistiigi · 2024-11-19T18:38:49Z

solver/llbsolver/file/backend.go

+		return errors.WithStack(err)
+	}
+
+	if err := copy.Chown(src, nil, ch); err != nil {


@profnandaa What's the wcow implications of this. Not sure which of these functions work correctly there (or if they work on symlink or the target).

Let me check and revert.

My preliminary findings

When I run the test on Windows (commenting out // requiresLinux(t)), I get this:

=== NAME TestIntegration/TestFileOpSymlink/worker=containerd client_test.go:2482: Error Trace: C:/dev/buildkit/client/client_test.go:2482 C:/dev/buildkit/util/testutil/integration/run.go:97 C:/dev/buildkit/util/testutil/integration/run.go:211 Error: Not equal: expected: 0 actual : 7777

Then the platform time differences:

=== NAME TestIntegration/TestFileOpSymlink/worker=containerd client_test.go:2495: Error Trace: C:/dev/buildkit/client/client_test.go:2495 C:/dev/buildkit/util/testutil/integration/run.go:97 C:/dev/buildkit/util/testutil/integration/run.go:211 Error: Not equal: expected: time.Date(2024, time.November, 19, 23, 58, 26, 0, time.Local) actual : time.Date(1969, time.December, 31, 16, 0, 42, 0, time.Local) Diff: --- Expected +++ Actual @@ -1,2 +1,2 @@ -(time.Time) 2024-11-19 23:58:26 -0800 PST +(time.Time) 1969-12-31 16:00:42 -0800 PST

Passes when I comment out those:

diff --git a/client/client_test.go b/client/client_test.go index 840135094..667c4de01 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -2411,7 +2411,7 @@ func testOCILayoutPlatformSource(t *testing.T, sb integration.Sandbox) { } func testFileOpSymlink(t *testing.T, sb integration.Sandbox) { - requiresLinux(t) + // requiresLinux(t) const ( fileOwner = 7777 @@ -2479,8 +2479,8 @@ func testFileOpSymlink(t *testing.T, sb integration.Sandbox) { require.NoError(t, err) require.Equal(t, []byte("contents"), dt) - require.Equal(t, header.Uid, fileOwner) - require.Equal(t, header.Gid, fileGroup) + // require.Equal(t, header.Uid, fileOwner) + // require.Equal(t, header.Gid, fileGroup) entry, ok = m["baz"] dt = entry.Data @@ -2489,10 +2489,10 @@ func testFileOpSymlink(t *testing.T, sb integration.Sandbox) { // make sure it was chowned properly require.Equal(t, true, ok) - require.Equal(t, header.Uid, linkOwner) - require.Equal(t, header.Gid, linkGroup) + // require.Equal(t, header.Uid, linkOwner) + // require.Equal(t, header.Gid, linkGroup) - require.Equal(t, header.ModTime, dummyTime) + // require.Equal(t, header.ModTime, dummyTime) }

@profnandaa What's the wcow implications of this. Not sure which of these functions work correctly there (or if they work on symlink or the target).

For context, what's WCOW?

Sorry, Windows Containers on Windows (WCOW).

Sorry, with my diff, I didn't mean that you change your tests. Please go ahead and return the requiresLinux and leave your previous tests as-is. I was just testing on how it could run on Windows ... we'll still need to figure out permission stuff on Windows, that's a separate issue.

I'll revert and open an issue

profnandaa · 2024-11-20T06:54:04Z

client/llb/fileop.go

+	SetSymlinkOption(*SymlinkInfo)
+}
+
+func Symlink(target, src string, opts ...SymlinkOption) *FileAction {


re: #5534 -- will be nice to add some inline documentation for all the exports. We're working with @castrombithisamm to address the other missing ones.

this is done

* Add file.symlink.create capability and wire it up * Run codegen for new FileActionSymlink Message * Add Symlink test * Add user/group ownership and timestamps to symlink ** Symlinks have user/group ownership that are independent of those of the target file; in linux, the ownership of the symlink itself is only checked when the link resides in a directory with the sticky bit set and the link is the subject of removal or renaming. The sticky bit prevents files in the directory from being deleted or renamed by non-owners (members of the group that owns the file may not delete the file; the user must own the file). In addition to user/group restrictions, linux symlinks have timestamps that are independent of the timestamps on the target file. * Expose symlink options to `llb` package * Add symlink integration test * Use tar exporter for tests ** Using the local exporter causes the files to be exported with the permissions of the user who does the exporting, instead of retaining their file permissions from within the container. Using the tar exporter instead preserves the permissions until they can be checked. * Change symlink fields to `oldpath` and `newpath` ** Also run `make generated-files` * Fix typo * Add doc strings to exported `llb` identifiers * Remove `requiresLinux` from integration test * Revert "Remove `requiresLinux` from integration test" * Add fixes to please the linter Signed-off-by: Peter Engelbert <[email protected]>

Copilot reviewed 7 out of 13 changed files in this pull request and generated no suggestions.

Files not reviewed (6)

client/client_test.go: Evaluated as low risk
client/llb/fileop.go: Evaluated as low risk
client/llb/fileop_test.go: Evaluated as low risk
cmd/buildctl/debug/dumpllb.go: Evaluated as low risk
solver/llbsolver/file/backend.go: Evaluated as low risk
solver/llbsolver/ops/file.go: Evaluated as low risk

Comments skipped due to low confidence (1)

solver/llbsolver/ops/file_test.go:647

[nitpick] The parameter names oldpath and newpath might be confusing. Consider renaming them to target and link to avoid confusion.

func (b *testFileBackend) Symlink(_ context.Context, m, user, group fileoptypes.Mount, a *pb.FileActionSymlink) error {

tonistiigi

Looks good. Comments about tests.

tonistiigi · 2024-12-13T00:37:59Z

client/llb/fileop_test.go

+
+	st := Scratch().
+		File(Mkfile("/foo", 0700, []byte("data"))).
+		File(Symlink("foo", "/bar"))


Can we have some tests that chain symlink together with other actions. Atm it is unclear what the point of Mkfile is in this test as it is in a different FileOp that is never checked after marshaling.

Probably the result of copying and pasting from another test. To be clear about your intent, you want to be sure that llb.Symlink plays nicely with other file actions when chained together, i.e. they happen in the right order?

Correct. The actions are all linked together so that the input points to either the input of the fileop or the offset of another action that needs to run before it

tonistiigi · 2024-12-13T00:41:33Z

client/client_test.go

+	require.Equal(t, fileGroup, header.Gid)
+
+	entry, ok = m["baz"]
+	header = entry.Header


Where does this check that it is a symlink and that it points to the correct location?

Noted, will fix this by checking header.Linkname and verifying its destination.

tonistiigi

^

Signed-off-by: Peter Engelbert <[email protected]>

github-actions bot added area/llb area/testing area/client area/solver area/buildctl labels Nov 13, 2024

cpuguy83 requested changes Nov 13, 2024

View reviewed changes

jedevc reviewed Nov 14, 2024

View reviewed changes

tonistiigi reviewed Nov 14, 2024

View reviewed changes

thompson-shaun added this to the v0.18.0 milestone Nov 14, 2024

pmengelbert force-pushed the pmengelbert/add_llb_symlinks/1 branch from 44cbc45 to 6b18ff2 Compare November 19, 2024 12:27

pmengelbert force-pushed the pmengelbert/add_llb_symlinks/1 branch from 6b18ff2 to 61b3374 Compare November 19, 2024 15:08

tonistiigi reviewed Nov 19, 2024

View reviewed changes

profnandaa reviewed Nov 20, 2024

View reviewed changes

pmengelbert force-pushed the pmengelbert/add_llb_symlinks/1 branch from 952a185 to 35fb128 Compare November 20, 2024 15:22

pmengelbert requested a review from cpuguy83 November 20, 2024 17:02

thompson-shaun modified the milestones: v0.18.0, v0.19.0 Nov 20, 2024

pmengelbert force-pushed the pmengelbert/add_llb_symlinks/1 branch 5 times, most recently from f2575bb to ac0ccbf Compare November 21, 2024 20:50

pmengelbert force-pushed the pmengelbert/add_llb_symlinks/1 branch from ac0ccbf to 45eab8c Compare November 21, 2024 21:40

cpuguy83 approved these changes Nov 21, 2024

View reviewed changes

thompson-shaun added the kind/enhancement label Nov 25, 2024

profnandaa approved these changes Nov 26, 2024

View reviewed changes

cpuguy83 requested a review from tonistiigi December 11, 2024 21:50

cpuguy83 requested a review from jedevc December 11, 2024 21:50

tonistiigi requested a review from Copilot December 11, 2024 22:10

Copilot AI reviewed Dec 11, 2024

View reviewed changes

tonistiigi reviewed Dec 13, 2024

View reviewed changes

tonistiigi requested changes Dec 18, 2024

View reviewed changes

testFileOpSymlink: check that symlink is created

171eaaa

Signed-off-by: Peter Engelbert <[email protected]>

		// FileActionSymlink creates a symlink
		FileActionSymlink symlink = 8;

Add FileActionSymlink and llb.Symlink #5519

Are you sure you want to change the base?

Add FileActionSymlink and llb.Symlink #5519

Conversation

pmengelbert commented Nov 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedevc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonistiigi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmengelbert commented Nov 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

My preliminary findings

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonistiigi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonistiigi left a comment

Choose a reason for hiding this comment

Add FileActionSymlink and `llb.Symlink` #5519

Add FileActionSymlink and `llb.Symlink` #5519

tonistiigi left a comment •

edited

Loading