lakectl local: posix improvements (#8257)

* lakectl local: posix improvements * Fix test * CR Fixes
treeverse · Oct 2, 2024 · c939d71 · c939d71
1 parent aecc37a
commit c939d71
Show file tree

Hide file tree

Showing 11 changed files with 125 additions and 7 deletions.
diff --git a/cmd/lakectl/cmd/local.go b/cmd/lakectl/cmd/local.go
@@ -71,6 +71,8 @@ func localDiff(ctx context.Context, client apigen.ClientWithResponsesInterface,
 	changes, err := local.DiffLocalWithHead(currentRemoteState, path, local.Config{
 		SkipNonRegularFiles: cfg.Local.SkipNonRegularFiles,
 		IncludePerm:         cfg.Experimental.Local.POSIXPerm.Enabled,
+		IncludeUID:          cfg.Experimental.Local.POSIXPerm.IncludeUID,
+		IncludeGID:          cfg.Experimental.Local.POSIXPerm.IncludeGID,
 	})
 	if err != nil {
 		DieErr(err)

diff --git a/cmd/lakectl/cmd/local_checkout.go b/cmd/lakectl/cmd/local_checkout.go
@@ -61,6 +61,8 @@ func localCheckout(cmd *cobra.Command, localPath string, specifiedRef string, co
 		SyncFlags:           syncFlags,
 		SkipNonRegularFiles: cfg.Local.SkipNonRegularFiles,
 		IncludePerm:         cfg.Experimental.Local.POSIXPerm.Enabled,
+		IncludeUID:          cfg.Experimental.Local.POSIXPerm.IncludeUID,
+		IncludeGID:          cfg.Experimental.Local.POSIXPerm.IncludeGID,
 	})
 	// confirm on local changes
 	if confirmByFlag && len(diffs) > 0 {

diff --git a/cmd/lakectl/cmd/local_clone.go b/cmd/lakectl/cmd/local_clone.go
@@ -90,6 +90,8 @@ var localCloneCmd = &cobra.Command{
 			SyncFlags:           syncFlags,
 			SkipNonRegularFiles: cfg.Local.SkipNonRegularFiles,
 			IncludePerm:         cfg.Experimental.Local.POSIXPerm.Enabled,
+			IncludeUID:          cfg.Experimental.Local.POSIXPerm.IncludeUID,
+			IncludeGID:          cfg.Experimental.Local.POSIXPerm.IncludeGID,
 		})
 		err = s.Sync(localPath, stableRemote, ch)
 		if err != nil {

diff --git a/cmd/lakectl/cmd/local_commit.go b/cmd/lakectl/cmd/local_commit.go
@@ -176,6 +176,8 @@ var localCommitCmd = &cobra.Command{
 			SyncFlags:           syncFlags,
 			SkipNonRegularFiles: cfg.Local.SkipNonRegularFiles,
 			IncludePerm:         cfg.Experimental.Local.POSIXPerm.Enabled,
+			IncludeUID:          cfg.Experimental.Local.POSIXPerm.IncludeUID,
+			IncludeGID:          cfg.Experimental.Local.POSIXPerm.IncludeGID,
 		})
 		err = s.Sync(idx.LocalPath(), remote, c)
 		if err != nil {

diff --git a/cmd/lakectl/cmd/local_pull.go b/cmd/lakectl/cmd/local_pull.go
@@ -70,6 +70,8 @@ var localPullCmd = &cobra.Command{
 			SyncFlags:           syncFlags,
 			SkipNonRegularFiles: cfg.Local.SkipNonRegularFiles,
 			IncludePerm:         cfg.Experimental.Local.POSIXPerm.Enabled,
+			IncludeUID:          cfg.Experimental.Local.POSIXPerm.IncludeUID,
+			IncludeGID:          cfg.Experimental.Local.POSIXPerm.IncludeGID,
 		})
 		err = s.Sync(idx.LocalPath(), newBase, c)
 		if err != nil {

diff --git a/cmd/lakectl/cmd/root.go b/cmd/lakectl/cmd/root.go
@@ -103,7 +103,9 @@ type Configuration struct {
 	Experimental struct {
 		Local struct {
 			POSIXPerm struct {
-				Enabled bool `mapstructure:"enabled"`
+				Enabled    bool `mapstructure:"enabled"`
+				IncludeUID bool `mapstructure:"include_uid"`
+				IncludeGID bool `mapstructure:"include_gid"`
 			} `mapstructure:"posix_permissions"`
 		} `mapstructure:"local"`
 	} `mapstructure:"experimental"`

diff --git a/pkg/local/config.go b/pkg/local/config.go
@@ -21,4 +21,6 @@ type Config struct {
 	SkipNonRegularFiles bool
 	// IncludePerm - Experimental: preserve Unix file permissions
 	IncludePerm bool
+	IncludeUID  bool
+	IncludeGID  bool
 }
diff --git a/pkg/local/diff.go b/pkg/local/diff.go
@@ -318,7 +318,7 @@ func DiffLocalWithHead(left <-chan apigen.ObjectStats, rightPath string, cfg Con
 				// dirs might have different sizes on different operating systems
 				sizeChanged := !info.IsDir() && localBytes != swag.Int64Value(currentRemoteFile.SizeBytes)
 				mtimeChanged := localMtime != remoteMtime
-				permissionsChanged := cfg.IncludePerm && isPermissionsChanged(info, currentRemoteFile)
+				permissionsChanged := isPermissionsChanged(info, currentRemoteFile, cfg)
 				if sizeChanged || mtimeChanged || permissionsChanged {
 					// we made a change!
 					changes = append(changes, &Change{ChangeSourceLocal, localPath, ChangeTypeModified})

diff --git a/pkg/local/diff_test.go b/pkg/local/diff_test.go
@@ -29,6 +29,8 @@ func TestDiffLocal(t *testing.T) {
 	cases := []struct {
 		Name                   string
 		IncludeUnixPermissions bool
+		IncludeGID             bool
+		IncludeUID             bool
 		LocalPath              string
 		InitLocalPath          func() string
 		CleanLocalPath         func(localPath string)
@@ -58,6 +60,8 @@ func TestDiffLocal(t *testing.T) {
 		{
 			Name:                   "t1_no_diff_include_folders",
 			IncludeUnixPermissions: true,
+			IncludeGID:             true,
+			IncludeUID:             true,
 			LocalPath:              "testdata/localdiff/t1",
 			RemoteList: []apigen.ObjectStats{
 				{
@@ -196,6 +200,8 @@ func TestDiffLocal(t *testing.T) {
 		{
 			Name:                   "t1_folder_added",
 			IncludeUnixPermissions: true,
+			IncludeGID:             true,
+			IncludeUID:             true,
 			LocalPath:              "testdata/localdiff/t1",
 			RemoteList: []apigen.ObjectStats{
 				{
@@ -238,6 +244,8 @@ func TestDiffLocal(t *testing.T) {
 		{
 			Name:                   "t1_unix_permissions_modified",
 			IncludeUnixPermissions: true,
+			IncludeGID:             true,
+			IncludeUID:             true,
 			LocalPath:              "testdata/localdiff/t1/sub",
 			RemoteList: []apigen.ObjectStats{
 				{
@@ -249,12 +257,50 @@ func TestDiffLocal(t *testing.T) {
 					Path:      "folder/",
 					SizeBytes: swag.Int64(1),
 					Mtime:     diffTestCorrectTime,
-					Metadata:  getPermissionsMetadata(osUid+1, osGid, local.DefaultDirectoryPermissions-umask),
+					Metadata:  getPermissionsMetadata(osUid, osGid+1, local.DefaultDirectoryPermissions-umask),
 				}, {
 					Path:      "folder/f.txt",
 					SizeBytes: swag.Int64(6),
 					Mtime:     diffTestCorrectTime,
-					Metadata:  getPermissionsMetadata(osUid, osGid, local.DefaultFilePermissions-umask),
+					Metadata:  getPermissionsMetadata(osUid+1, osGid, local.DefaultFilePermissions-umask),
+				},
+			},
+			Expected: []*local.Change{
+				{
+					Path: "f.txt",
+					Type: local.ChangeTypeModified,
+				},
+				{
+					Path: "folder/",
+					Type: local.ChangeTypeModified,
+				},
+				{
+					Path: "folder/f.txt",
+					Type: local.ChangeTypeModified,
+				},
+			},
+		},
+		{
+			Name:                   "t1_unix_permissions_modified_only_gid",
+			IncludeUnixPermissions: true,
+			IncludeGID:             true,
+			LocalPath:              "testdata/localdiff/t1/sub",
+			RemoteList: []apigen.ObjectStats{
+				{
+					Path:      "f.txt",
+					SizeBytes: swag.Int64(3),
+					Mtime:     diffTestCorrectTime,
+					Metadata:  getPermissionsMetadata(osUid, osGid, 755),
+				}, {
+					Path:      "folder/",
+					SizeBytes: swag.Int64(1),
+					Mtime:     diffTestCorrectTime,
+					Metadata:  getPermissionsMetadata(osUid, osGid+1, local.DefaultDirectoryPermissions-umask),
+				}, {
+					Path:      "folder/f.txt",
+					SizeBytes: swag.Int64(6),
+					Mtime:     diffTestCorrectTime,
+					Metadata:  getPermissionsMetadata(osUid+1, osGid, local.DefaultFilePermissions-umask),
 				},
 			},
 			Expected: []*local.Change{
@@ -268,9 +314,45 @@ func TestDiffLocal(t *testing.T) {
 				},
 			},
 		},
+		{
+			Name:                   "t1_unix_permissions_modified_only_uid",
+			IncludeUnixPermissions: true,
+			IncludeUID:             true,
+			LocalPath:              "testdata/localdiff/t1/sub",
+			RemoteList: []apigen.ObjectStats{
+				{
+					Path:      "f.txt",
+					SizeBytes: swag.Int64(3),
+					Mtime:     diffTestCorrectTime,
+					Metadata:  getPermissionsMetadata(osUid, osGid, 755),
+				}, {
+					Path:      "folder/",
+					SizeBytes: swag.Int64(1),
+					Mtime:     diffTestCorrectTime,
+					Metadata:  getPermissionsMetadata(osUid, osGid+1, local.DefaultDirectoryPermissions-umask),
+				}, {
+					Path:      "folder/f.txt",
+					SizeBytes: swag.Int64(6),
+					Mtime:     diffTestCorrectTime,
+					Metadata:  getPermissionsMetadata(osUid+1, osGid, local.DefaultFilePermissions-umask),
+				},
+			},
+			Expected: []*local.Change{
+				{
+					Path: "f.txt",
+					Type: local.ChangeTypeModified,
+				},
+				{
+					Path: "folder/f.txt",
+					Type: local.ChangeTypeModified,
+				},
+			},
+		},
 		{
 			Name:                   "t1_empty_folder_removed",
 			IncludeUnixPermissions: true,
+			IncludeGID:             true,
+			IncludeUID:             true,
 			LocalPath:              "testdata/localdiff/t1/sub/folder",
 			RemoteList: []apigen.ObjectStats{
 				{
@@ -314,6 +396,8 @@ func TestDiffLocal(t *testing.T) {
 		{
 			Name:                   "empty_folder_added",
 			IncludeUnixPermissions: true,
+			IncludeGID:             true,
+			IncludeUID:             true,
 			InitLocalPath: func() string {
 				return createTempEmptyFolder(t)
 			},
@@ -360,6 +444,8 @@ func TestDiffLocal(t *testing.T) {
 
 			changes, err := local.DiffLocalWithHead(lc, tt.LocalPath, local.Config{
 				IncludePerm: tt.IncludeUnixPermissions,
+				IncludeUID:  tt.IncludeUID,
+				IncludeGID:  tt.IncludeGID,
 			})
 
 			if tt.CleanLocalPath != nil {

diff --git a/pkg/local/posix_permissions.go b/pkg/local/posix_permissions.go
@@ -83,7 +83,10 @@ func getPermissionFromFileInfo(info os.FileInfo) (*POSIXPermissions, error) {
 	return permissionsFromFileInfo(info)
 }
 
-func isPermissionsChanged(localFileInfo os.FileInfo, remoteFileStats apigen.ObjectStats) bool {
+func isPermissionsChanged(localFileInfo os.FileInfo, remoteFileStats apigen.ObjectStats, cfg Config) bool {
+	if !cfg.IncludePerm {
+		return false
+	}
 	local, err := getPermissionFromFileInfo(localFileInfo)
 	if err != nil {
 		return true
@@ -94,5 +97,12 @@ func isPermissionsChanged(localFileInfo os.FileInfo, remoteFileStats apigen.Obje
 		return true
 	}
 
-	return local.Mode != remote.Mode || local.POSIXOwnership != remote.POSIXOwnership
+	if cfg.IncludeUID && local.UID != remote.UID {
+		return true
+	}
+	if cfg.IncludeGID && local.GID != remote.GID {
+		return true
+	}
+
+	return local.Mode != remote.Mode
 }
diff --git a/pkg/local/sync.go b/pkg/local/sync.go
@@ -262,7 +262,15 @@ func (s *SyncManager) download(ctx context.Context, rootPath string, remote *uri
 
 	// change ownership and permissions
 	if s.cfg.IncludePerm {
-		if err = os.Chown(destination, perm.UID, perm.GID); err != nil {
+		uid := perm.UID
+		gid := perm.GID
+		if !s.cfg.IncludeUID {
+			uid = -1
+		}
+		if !s.cfg.IncludeGID {
+			gid = -1
+		}
+		if err = os.Chown(destination, uid, gid); err != nil {
 			return err
 		}
 		err = syscall.Chmod(destination, uint32(perm.Mode))