Skip to content

Commit

Permalink
Fix posix permissions directories diff (#7950)
Browse files Browse the repository at this point in the history 
* Fix posix permissions directories diff

* CR Fixes

* CR Fixes
  • Loading branch information
@N-o-Z
N-o-Z authored Jul 3, 2024
1 parent b49429b commit 17fd557
Show file tree
Hide file tree
Showing 5 changed files with 47 additions and 26 deletions.
5 changes: 2 additions & 3 deletions cmd/lakectl/cmd/local.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,12 @@ func withForceFlag(cmd *cobra.Command, usage string) {
func localDiff(ctx context.Context, client apigen.ClientWithResponsesInterface, remote *uri.URI, path string) local.Changes {
fmt.Printf("\ndiff 'local://%s' <--> '%s'...\n", path, remote)
currentRemoteState := make(chan apigen.ObjectStats, maxDiffPageSize)
includePOSIXPermissions := cfg.Experimental.Local.POSIXPerm.Enabled
var wg errgroup.Group
wg.Go(func() error {
return local.ListRemote(ctx, client, remote, currentRemoteState)
return local.ListRemote(ctx, client, remote, currentRemoteState, includePOSIXPermissions)
})

includePOSIXPermissions := cfg.Experimental.Local.POSIXPerm.Enabled

changes, err := local.DiffLocalWithHead(currentRemoteState, path, includePOSIXPermissions, includePOSIXPermissions)
if err != nil {
DieErr(err)
Expand Down
4 changes: 2 additions & 2 deletions pkg/local/diff.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ func DiffLocalWithHead(left <-chan apigen.ObjectStats, rightPath string, include
}

// ListRemote - Lists objects from a remote uri and inserts them into the objects channel
func ListRemote(ctx context.Context, client apigen.ClientWithResponsesInterface, loc *uri.URI, objects chan<- apigen.ObjectStats) error {
func ListRemote(ctx context.Context, client apigen.ClientWithResponsesInterface, loc *uri.URI, objects chan<- apigen.ObjectStats, includeDirs bool) error {
hasMore := true
var after string
defer func() {
Expand All @@ -362,7 +362,7 @@ func ListRemote(ctx context.Context, client apigen.ClientWithResponsesInterface,
for _, o := range listResp.JSON200.Results {
p := strings.TrimPrefix(o.Path, loc.GetPath())
// skip directory markers
if p == "" || (strings.HasSuffix(p, uri.PathSeparator) && swag.Int64Value(o.SizeBytes) == 0) {
if !includeDirs && (p == "" || (strings.HasSuffix(p, uri.PathSeparator) && swag.Int64Value(o.SizeBytes) == 0)) {
continue
}
p = strings.TrimPrefix(p, uri.PathSeparator)
Expand Down
47 changes: 32 additions & 15 deletions pkg/local/diff_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"slices"
"sort"
"strings"
"syscall"
"testing"
"time"

Expand All @@ -17,15 +18,13 @@ import (
"github.com/treeverse/lakefs/pkg/local"
)

const (
diffTestCorrectTime = 1691570412
filePermModeFile = 0o100644
localPermModeFolder = 0o40755
)
const diffTestCorrectTime = 1691570412

func TestDiffLocal(t *testing.T) {
osUid := os.Getuid()
osGid := os.Getgid()
umask := syscall.Umask(0)
syscall.Umask(umask)

cases := []struct {
Name string
Expand Down Expand Up @@ -63,27 +62,27 @@ func TestDiffLocal(t *testing.T) {
Path: ".hidden-file",
SizeBytes: swag.Int64(64),
Mtime: diffTestCorrectTime,
Metadata: getPermissionsMetadata(osUid, osGid, filePermModeFile),
Metadata: getPermissionsMetadata(osUid, osGid, local.DefaultFilePermissions-umask),
}, {
Path: "sub/",
SizeBytes: swag.Int64(1),
Mtime: diffTestCorrectTime,
Metadata: getPermissionsMetadata(osUid, osGid, localPermModeFolder),
Metadata: getPermissionsMetadata(osUid, osGid, local.DefaultDirectoryPermissions-umask),
}, {
Path: "sub/f.txt",
SizeBytes: swag.Int64(3),
Mtime: diffTestCorrectTime,
Metadata: getPermissionsMetadata(osUid, osGid, filePermModeFile),
Metadata: getPermissionsMetadata(osUid, osGid, local.DefaultFilePermissions-umask),
}, {
Path: "sub/folder/",
SizeBytes: swag.Int64(1),
Mtime: diffTestCorrectTime,
Metadata: getPermissionsMetadata(osUid, osGid, localPermModeFolder),
Metadata: getPermissionsMetadata(osUid, osGid, local.DefaultDirectoryPermissions-umask),
}, {
Path: "sub/folder/f.txt",
SizeBytes: swag.Int64(6),
Mtime: diffTestCorrectTime,
Metadata: getPermissionsMetadata(osUid, osGid, filePermModeFile),
Metadata: getPermissionsMetadata(osUid, osGid, local.DefaultFilePermissions-umask),
},
},
Expected: []*local.Change{},
Expand All @@ -110,7 +109,8 @@ func TestDiffLocal(t *testing.T) {
{
Path: "sub/f.txt",
Type: local.ChangeTypeModified,
}, {
},
{
Path: "sub/folder/f.txt",
Type: local.ChangeTypeModified,
},
Expand All @@ -124,7 +124,8 @@ func TestDiffLocal(t *testing.T) {
Path: ".hidden-file",
SizeBytes: swag.Int64(64),
Mtime: diffTestCorrectTime,
}, {
},
{
Path: "sub/folder/f.txt",
SizeBytes: swag.Int64(6),
Mtime: diffTestCorrectTime,
Expand Down Expand Up @@ -210,6 +211,18 @@ func TestDiffLocal(t *testing.T) {
},
},
Expected: []*local.Change{
{
Path: ".hidden-file",
Type: local.ChangeTypeModified,
},
{
Path: "sub/",
Type: local.ChangeTypeModified,
},
{
Path: "sub/f.txt",
Type: local.ChangeTypeModified,
},
{
Path: "sub/folder/",
Type: local.ChangeTypeAdded,
Expand All @@ -234,12 +247,12 @@ func TestDiffLocal(t *testing.T) {
Path: "folder/",
SizeBytes: swag.Int64(1),
Mtime: diffTestCorrectTime,
Metadata: getPermissionsMetadata(osUid+1, osGid, localPermModeFolder),
Metadata: getPermissionsMetadata(osUid+1, osGid, local.DefaultDirectoryPermissions-umask),
}, {
Path: "folder/f.txt",
SizeBytes: swag.Int64(6),
Mtime: diffTestCorrectTime,
Metadata: getPermissionsMetadata(osUid, osGid, filePermModeFile),
Metadata: getPermissionsMetadata(osUid, osGid, local.DefaultFilePermissions-umask),
},
},
Expected: []*local.Change{
Expand Down Expand Up @@ -310,7 +323,11 @@ func fixTime(t *testing.T, localPath string, includeDirs bool) {

func fixUnixPermissions(t *testing.T, localPath string) {
err := filepath.WalkDir(localPath, func(path string, d fs.DirEntry, err error) error {
return os.Chown(path, os.Getuid(), os.Getgid())
perm := local.GetDefaultPermissions(d.IsDir())
if err = os.Chown(path, os.Getuid(), os.Getgid()); err != nil {
return err
}
return os.Chmod(path, perm.Mode)
})
require.NoError(t, err)
}
Expand Down
11 changes: 8 additions & 3 deletions pkg/local/posix_permissions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,13 @@ func GetDefaultPermissions(isDir bool) POSIXPermissions {
}

// getPermissionFromStats - Get POSIX mode and ownership from object metadata, fallback to default permissions in case metadata doesn't exist
func getPermissionFromStats(stats apigen.ObjectStats) (*POSIXPermissions, error) {
permissions := GetDefaultPermissions(strings.HasSuffix(stats.Path, uri.PathSeparator))
// and withDefault is true
func getPermissionFromStats(stats apigen.ObjectStats, withDefault bool) (*POSIXPermissions, error) {
permissions := POSIXPermissions{}
if withDefault {
permissions = GetDefaultPermissions(strings.HasSuffix(stats.Path, uri.PathSeparator))
}

if stats.Metadata != nil {
posixPermissions, ok := stats.Metadata.Get(POSIXPermissionsMetadataKey)
if ok {
Expand Down Expand Up @@ -101,7 +106,7 @@ func isPermissionsChanged(localFileInfo os.FileInfo, remoteFileStats apigen.Obje
return true
}

remote, err := getPermissionFromStats(remoteFileStats)
remote, err := getPermissionFromStats(remoteFileStats, false)
if err != nil {
return true
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/local/sync.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ func (s *SyncManager) download(ctx context.Context, rootPath string, remote *uri
if err := fileutil.VerifyRelPath(strings.TrimPrefix(path, uri.PathSeparator), rootPath); err != nil {
return err
}
destination := fmt.Sprintf("%s%c%s", rootPath, os.PathSeparator, path)
destination := filepath.ToSlash(fmt.Sprintf("%s%s%s", rootPath, uri.PathSeparator, path))
destinationDirectory := filepath.Dir(destination)

if err := os.MkdirAll(destinationDirectory, os.FileMode(DefaultDirectoryPermissions)); err != nil {
Expand Down Expand Up @@ -243,7 +243,7 @@ func (s *SyncManager) download(ctx context.Context, rootPath string, remote *uri
var perm *POSIXPermissions
isDir := strings.HasSuffix(path, uri.PathSeparator)
if s.includePerm { // Optimization - fail on to get permissions from metadata before having to download the entire file
if perm, err = getPermissionFromStats(objStat); err != nil {
if perm, err = getPermissionFromStats(objStat, true); err != nil {
return err
}
} else if isDir {
Expand Down Expand Up @@ -277,7 +277,7 @@ func (s *SyncManager) upload(ctx context.Context, rootPath string, remote *uri.U
if err := fileutil.VerifySafeFilename(source); err != nil {
return err
}
dest := filepath.ToSlash(filepath.Join(remote.GetPath(), path))
dest := strings.TrimPrefix(filepath.ToSlash(fmt.Sprintf("%s%s%s", remote.GetPath(), uri.PathSeparator, path)), uri.PathSeparator)

f, err := os.Open(source)
if err != nil {
Expand Down

0 comments on commit 17fd557

Please sign in to comment.