Skip to content

Set ownership on symlinks created by build actions during their execution #211

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.

Sign up for GitHub

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

Open
moroten wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Set ownership on symlinks created by build actions during their execution #211

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 13 additions & 9 deletions cmd/bb_worker/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,16 +219,16 @@ func main() {
normalizer = virtual.CaseInsensitiveComponentNormalizer
}

defaultAttributesSetter := func(requested virtual.AttributesMask, attributes *virtual.Attributes) {
// No need to set ownership attributes
// on the top-level directory.
}
symlinkFactory = virtual.NewHandleAllocatingSymlinkFactory(
virtual.BaseSymlinkFactory,
virtual.NewBaseSymlinkFactory(defaultAttributesSetter),
Copy link
Member

@EdSchouten EdSchouten Feb 6, 2026

Choose a reason for hiding this comment

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

If we are going to give the top level directory a SymlinkFactory that differs from the one that's used per build directory, what are your thoughts on giving it an instance that always returns an error? There should be no need for anyone to ever place symlinks in there. This does require changing SymlinkFactory.LookupSymlink() to return an error.

handleAllocator.New())
characterDeviceFactory = virtual.NewHandleAllocatingCharacterDeviceFactory(
virtual.BaseCharacterDeviceFactory,
handleAllocator.New())
defaultAttributesSetter := func(requested virtual.AttributesMask, attributes *virtual.Attributes) {
// No need to set ownership attributes
// on the top-level directory.
}
virtualBuildDirectory = virtual.NewInMemoryPrepopulatedDirectory(
virtual.NewHandleAllocatingFileAllocator(
virtual.NewPoolBackedFileAllocator(
Expand Down Expand Up @@ -343,6 +343,13 @@ func main() {
}
runnerClient := runner_pb.NewRunnerClient(runnerConnection)

defaultAttributesSetter := func(requested virtual.AttributesMask, attributes *virtual.Attributes) {
attributes.SetOwnerUserID(runnerConfiguration.BuildDirectoryOwnerUserId)
attributes.SetOwnerGroupID(runnerConfiguration.BuildDirectoryOwnerGroupId)
}
symlinkFactory := virtual.NewHandleAllocatingSymlinkFactory(
Copy link
Member

@EdSchouten EdSchouten Feb 6, 2026

Choose a reason for hiding this comment

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

Given that defaultAttributesSetter and handleAllocator are both already provided to NewVirtualBuildDirectory(), I think it's cleaner if we move construction of symlinkFactory into NewVirtualBuildDirectory(). This is also a bit more consistent with the creation of FileAllocator, which already lives inside virtualBuildDirectory.

virtual.NewBaseSymlinkFactory(defaultAttributesSetter),
handleAllocator.New())
for threadID := uint64(0); threadID < runnerConfiguration.Concurrency; threadID++ {
// Per-worker separate writer of the Content
// Addressable Storage that batches writes after
Expand Down Expand Up @@ -391,10 +398,7 @@ func main() {
symlinkFactory,
characterDeviceFactory,
handleAllocator,
/* defaultAttributesSetter = */ func(requested virtual.AttributesMask, attributes *virtual.Attributes) {
attributes.SetOwnerUserID(runnerConfiguration.BuildDirectoryOwnerUserId)
attributes.SetOwnerGroupID(runnerConfiguration.BuildDirectoryOwnerGroupId)
},
defaultAttributesSetter,
clock.SystemClock,
)
} else {
Expand Down
1 change: 1 addition & 0 deletions pkg/builder/virtual_build_directory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ func (d *virtualBuildDirectory) InstallHooks(filePool pool.FilePool, errorLogger
),
do.handleAllocator,
),
do.symlinkFactory,
errorLogger,
do.defaultAttributesSetter,
namedAttributesFactory,
Expand Down
24 changes: 18 additions & 6 deletions pkg/filesystem/virtual/base_symlink_factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,30 @@ import (
"google.golang.org/grpc/status"
)

type symlinkFactory struct{}
type symlinkFactory struct {
Copy link
Member

@EdSchouten EdSchouten Feb 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

While there, can we rename this to baseSymlinkFactory? That's more consistent with the constructor function. I guess I forgot to rename this when I added the SymlinkFactory interface. It used to be the case that symlinks were constructed directly, without using a factory type.

defaultAttributesSetter DefaultAttributesSetter
}

func (symlinkFactory) LookupSymlink(target []byte) LinkableLeaf {
return symlink{target: target}
func (f *symlinkFactory) LookupSymlink(target []byte) LinkableLeaf {
return symlink{
defaultAttributesSetter: f.defaultAttributesSetter,
target: target,
}
}

// BaseSymlinkFactory can be used to create simple immutable symlink nodes.
var BaseSymlinkFactory SymlinkFactory = symlinkFactory{}
// NewBaseSymlinkFactory creates a SymlinkFactory that can be used to create
// simple immutable symlink nodes.
func NewBaseSymlinkFactory(defaultAttributesSetter DefaultAttributesSetter) SymlinkFactory {
return &symlinkFactory{
defaultAttributesSetter: defaultAttributesSetter,
}
}

type symlink struct {
placeholderFile

target []byte
defaultAttributesSetter DefaultAttributesSetter
target []byte
}

func (f symlink) readlinkParser() (path.Parser, error) {
Expand All @@ -48,6 +59,7 @@ func (f symlink) readlinkString() (string, error) {
}

func (f symlink) VirtualGetAttributes(ctx context.Context, requested AttributesMask, attributes *Attributes) {
f.defaultAttributesSetter(requested, attributes)
attributes.SetChangeID(0)
attributes.SetFileType(filesystem.FileTypeSymlink)
attributes.SetHasNamedAttributes(false)
Expand Down
9 changes: 5 additions & 4 deletions pkg/filesystem/virtual/in_memory_prepopulated_directory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ type StringMatcher func(s string) bool
// inMemoryFilesystem contains state that is shared across all
// inMemoryPrepopulatedDirectory objects that form a single hierarchy.
type inMemoryFilesystem struct {
symlinkFactory SymlinkFactory
statefulHandleAllocator StatefulHandleAllocator
initialContentsSorter Sorter
hiddenFilesMatcher StringMatcher
Expand All @@ -44,6 +43,7 @@ type inMemoryFilesystem struct {
type inMemorySubtree struct {
filesystem *inMemoryFilesystem
fileAllocator FileAllocator
symlinkFactory SymlinkFactory
errorLogger util.ErrorLogger
defaultAttributesSetter DefaultAttributesSetter
namedAttributesFactory NamedAttributesFactory
Expand All @@ -52,14 +52,14 @@ type inMemorySubtree struct {
func newInMemorySubtree(fileAllocator FileAllocator, symlinkFactory SymlinkFactory, errorLogger util.ErrorLogger, handleAllocator StatefulHandleAllocator, initialContentsSorter Sorter, hiddenFilesMatcher StringMatcher, clock clock.Clock, normalizer ComponentNormalizer, defaultAttributesSetter DefaultAttributesSetter, namedAttributesFactory NamedAttributesFactory) *inMemorySubtree {
return &inMemorySubtree{
filesystem: &inMemoryFilesystem{
symlinkFactory: symlinkFactory,
statefulHandleAllocator: handleAllocator,
initialContentsSorter: initialContentsSorter,
hiddenFilesMatcher: hiddenFilesMatcher,
normalizer: normalizer,
clock: clock,
},
fileAllocator: fileAllocator,
symlinkFactory: symlinkFactory,
errorLogger: errorLogger,
defaultAttributesSetter: defaultAttributesSetter,
namedAttributesFactory: namedAttributesFactory,
Expand Down Expand Up @@ -529,13 +529,14 @@ func (i *inMemoryPrepopulatedDirectory) postRemoveChildren(entries *inMemoryDire
}
}

func (i *inMemoryPrepopulatedDirectory) InstallHooks(fileAllocator FileAllocator, errorLogger util.ErrorLogger, defaultAttributesSetter DefaultAttributesSetter, namedAttributesFactory NamedAttributesFactory) {
func (i *inMemoryPrepopulatedDirectory) InstallHooks(fileAllocator FileAllocator, symlinkFactory SymlinkFactory, errorLogger util.ErrorLogger, defaultAttributesSetter DefaultAttributesSetter, namedAttributesFactory NamedAttributesFactory) {
i.lock.Lock()
defer i.lock.Unlock()

i.subtree = &inMemorySubtree{
filesystem: i.subtree.filesystem,
fileAllocator: fileAllocator,
symlinkFactory: symlinkFactory,
errorLogger: errorLogger,
defaultAttributesSetter: defaultAttributesSetter,
namedAttributesFactory: namedAttributesFactory,
Expand Down Expand Up @@ -1121,7 +1122,7 @@ func (i *inMemoryPrepopulatedDirectory) VirtualSymlink(ctx context.Context, poin
if s := contents.virtualMayAttach(normalizedLinkName); s != StatusOK {
return nil, ChangeInfo{}, s
}
child := i.subtree.filesystem.symlinkFactory.LookupSymlink(pointedTo)
child := i.subtree.symlinkFactory.LookupSymlink(pointedTo)
changeIDBefore := contents.changeID
contents.attach(i.subtree, linkName, normalizedLinkName, inMemoryDirectoryChild{}.FromLeaf(child))

Expand Down
24 changes: 23 additions & 1 deletion pkg/filesystem/virtual/in_memory_prepopulated_directory_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,9 +404,10 @@ func TestInMemoryPrepopulatedDirectoryInstallHooks(t *testing.T) {
defaultAttributesSetter1 := mock.NewMockDefaultAttributesSetter(ctrl)
d := virtual.NewInMemoryPrepopulatedDirectory(fileAllocator1, symlinkFactory1, errorLogger1, handleAllocator, sort.Sort, hiddenFilesPatternForTesting.MatchString, clock.SystemClock, virtual.CaseSensitiveComponentNormalizer, defaultAttributesSetter1.Call, virtual.NoNamedAttributesFactory)
fileAllocator2 := mock.NewMockFileAllocator(ctrl)
symlinkFactory2 := mock.NewMockSymlinkFactory(ctrl)
errorLogger2 := mock.NewMockErrorLogger(ctrl)
defaultAttributesSetter2 := mock.NewMockDefaultAttributesSetter(ctrl)
d.InstallHooks(fileAllocator2, errorLogger2, defaultAttributesSetter2.Call, virtual.NoNamedAttributesFactory)
d.InstallHooks(fileAllocator2, symlinkFactory2, errorLogger2, defaultAttributesSetter2.Call, virtual.NoNamedAttributesFactory)

// Validate that the top-level directory uses both the new file
// allocator and error logger.
Expand All @@ -424,6 +425,27 @@ func TestInMemoryPrepopulatedDirectoryInstallHooks(t *testing.T) {
&attr)
require.Equal(t, virtual.StatusErrIO, s)

// Validate that symlinks uses the new symlink allocator
// and error logger as well.
symlinkLeaf := mock.NewMockLinkableLeaf(ctrl)
symlinkFactory2.EXPECT().LookupSymlink([]byte("target")).Return(symlinkLeaf)
symlinkLeaf.EXPECT().VirtualGetAttributes(
ctx,
virtual.AttributesMaskInodeNumber,
gomock.Any(),
).Do(func(ctx context.Context, requested virtual.AttributesMask, attributes *virtual.Attributes) {
attributes.SetInodeNumber(3)
})
var out virtual.Attributes
actualLeaf, changeInfo, s := d.VirtualSymlink(ctx, []byte("target"), path.MustNewComponent("symlink"), virtual.AttributesMaskInodeNumber, &out)
require.Equal(t, virtual.StatusOK, s)
require.NotNil(t, actualLeaf)
require.Equal(t, virtual.ChangeInfo{
Before: 0,
After: 1,
}, changeInfo)
require.Equal(t, (&virtual.Attributes{}).SetInodeNumber(3), &out)

// Validate that a subdirectory uses the new file allocator
// and error logger as well.
inMemoryPrepopulatedDirectoryExpectMkdir(ctrl, handleAllocator)
Expand Down
2 changes: 1 addition & 1 deletion pkg/filesystem/virtual/prepopulated_directory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ type PrepopulatedDirectory interface {
// This function is identical to BuildDirectory.InstallHooks(),
// except that it uses the FUSE specific FileAllocator instead
// of FilePool.
InstallHooks(fileAllocator FileAllocator, errorLogger util.ErrorLogger, defaultAttributesSetter DefaultAttributesSetter, namedAttributesFactory NamedAttributesFactory)
InstallHooks(fileAllocator FileAllocator, symlinkFactory SymlinkFactory, errorLogger util.ErrorLogger, defaultAttributesSetter DefaultAttributesSetter, namedAttributesFactory NamedAttributesFactory)
// FilterChildren() can be used to traverse over all of the
// InitialContentsFetcher and LinkableLeaf objects stored in this
// directory hierarchy. For each of the objects, a callback is
Expand Down
2 changes: 1 addition & 1 deletion pkg/filesystem/virtual/winfsp/file_system_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ func createWinFSPMountForTest(t *testing.T, terminationGroup program.Group, case
handleAllocator,
),
virtual.NewHandleAllocatingSymlinkFactory(
virtual.BaseSymlinkFactory,
virtual.NewBaseSymlinkFactory(defaultAttributesSetter),
handleAllocator.New(),
),
util.DefaultErrorLogger,
Expand Down