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

Handled chown Scenario #1655

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions common/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ const (
UserAgentHeader = "User-Agent"

BlockCacheRWErrMsg = "Notice: The random write flow using block cache is temporarily blocked due to potential data integrity issues. This is a precautionary measure. \nIf you see this message, contact [email protected] or create a GitHub issue. We're working on a fix. More details: https://aka.ms/blobfuse2warnings."
OwnerID = "Uid"
GroupId = "Gid"
)

func FuseIgnoredFlags() []string {
Expand Down
12 changes: 12 additions & 0 deletions common/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -519,3 +519,15 @@ func GetCRC64(data []byte, len int) []byte {

return checksumBytes
}

// parseUint32 converts a *string to uint32
func ParseUint32(s string) uint32 {
if s == "" {
return 0
}
val, err := strconv.ParseUint(s, 10, 32)
if err != nil {
return 0
}
return uint32(val)
}
17 changes: 17 additions & 0 deletions common/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,23 @@ func (suite *utilTestSuite) SetupTest() {
func TestUtil(t *testing.T) {
suite.Run(t, new(utilTestSuite))
}
func (suite *utilTestSuite) TestParseUint32() {
// Test with a valid string
result := ParseUint32("12345")
suite.assert.Equal(uint32(12345), result)

// Test with an empty string
result = ParseUint32("")
suite.assert.Equal(uint32(0), result)

// Test with an invalid string
result = ParseUint32("invalid")
suite.assert.Equal(uint32(0), result)

// Test with a string representing a number larger than uint32
result = ParseUint32("4294967296") // 2^32
suite.assert.Equal(uint32(0), result)
}

func (suite *utilTestSuite) TestIsMountActiveNoMount() {
var out bytes.Buffer
Expand Down
66 changes: 66 additions & 0 deletions component/azstorage/block_blob.go
Original file line number Diff line number Diff line change
Expand Up @@ -666,6 +666,8 @@ func (bb *BlockBlob) getBlobAttr(blobInfo *container.BlobItem) (*internal.ObjAtt
Flags: internal.NewFileBitMap(),
MD5: blobInfo.Properties.ContentMD5,
ETag: sanitizeEtag(blobInfo.Properties.ETag),
GID: blobInfo.Metadata["gid"],
Copy link
Member

Choose a reason for hiding this comment

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

We shall not use random names, check what azcopy uses and we shall be using the same key name in metadata to cross tool compatibility.

UID: blobInfo.Metadata["uid"],
}

parseMetadata(attr, blobInfo.Metadata)
Expand Down Expand Up @@ -1629,3 +1631,67 @@ func (bb *BlockBlob) SetFilter(filter string) error {
bb.Config.filter = &blobfilter.BlobFilter{}
return bb.Config.filter.Configure(filter)
}

func (bb *BlockBlob) updateMetadata(name string, metadata map[string]*string) error {
Copy link
Member

Choose a reason for hiding this comment

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

changeowner method anyways gets the uid and gid so there is no need to implement another method as this could have been done in that method itself,

if len(metadata) == 0 {
log.Info("BlockBlob::updateMetadata: No metadata to update for %s", name)
return nil
}

blobClient := bb.Container.NewBlockBlobClient(filepath.Join(bb.Config.prefixPath, name))

// Fetch existing properties
prop, err := blobClient.GetProperties(context.Background(), &blob.GetPropertiesOptions{
CPKInfo: bb.blobCPKOpt,
})
if err != nil {
log.Err("BlockBlob::updateMetadata: Failed to fetch properties for %s [%s]", name, err.Error())
return err
}

// Update metadata if needed
if prop.Metadata == nil {
prop.Metadata = make(map[string]*string)
}

update := false
for key, value := range metadata {
Copy link
Member

Choose a reason for hiding this comment

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

Why we are doing so much of processing. We can get the metadata and set uid/gid related keys irrespective of whether they are there or not and send the metadata back.

matchedKey := ""
// prop.Metadata having key like Gid and Uid, while I have inserted it gid, uid
for storageKey := range prop.Metadata {
if strings.EqualFold(storageKey, key) {
matchedKey = storageKey
break
}
}
if matchedKey == "" {
// key not found; add it
prop.Metadata[key] = value
update = true
} else {
// key found; compare values
existingValue := prop.Metadata[matchedKey]
if *existingValue != *value {
prop.Metadata[matchedKey] = value
update = true
}
}
}

// Skip update if nothing changed
if !update {
log.Info("BlockBlob::updateMetadata: No changes needed for %s", name)
return nil
}

// Apply metadata update
if _, err := blobClient.SetMetadata(context.Background(), prop.Metadata, &blob.SetMetadataOptions{
CPKInfo: bb.blobCPKOpt,
}); err != nil {
log.Err("BlockBlob::updateMetadata: Failed to set metadata for %s [%s]", name, err.Error())
return err
}

log.Info("BlockBlob::updateMetadata: Metadata updated for %s - %v", name, prop.Metadata)
return nil
}
143 changes: 143 additions & 0 deletions component/azstorage/block_blob_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3582,6 +3582,149 @@ func (s *blockBlobTestSuite) TestList() {
s.assert.EqualValues(base, blobList[0].Path)
}

func (s *blockBlobTestSuite) TestUpdateMetadata() {
defer s.cleanupTest()
// Setup
name := generateFileName()
s.az.CreateFile(internal.CreateFileOptions{Name: name})

// Test updating metadata
metadata := map[string]*string{
common.GroupId: to.Ptr("0"),
common.OwnerID: to.Ptr("0"),
}
err := s.az.storage.(*BlockBlob).updateMetadata(name, metadata)
s.assert.Nil(err)

// Verify metadata
blobClient := s.containerClient.NewBlobClient(name)
props, err := blobClient.GetProperties(ctx, nil)
s.assert.Nil(err)
s.assert.NotNil(props.Metadata)
s.assert.Equal("0", *props.Metadata[common.GroupId])
s.assert.Equal("0", *props.Metadata[common.OwnerID])
}

func (s *blockBlobTestSuite) TestUpdateMetadataNoChanges() {
defer s.cleanupTest()
// Setup
name := generateFileName()
s.az.CreateFile(internal.CreateFileOptions{Name: name})

// Set initial metadata
metadata := map[string]*string{
common.GroupId: to.Ptr("1000"),
common.OwnerID: to.Ptr("1000"),
}
blobClient := s.containerClient.NewBlobClient(name)
_, err := blobClient.SetMetadata(ctx, metadata, nil)
s.assert.Nil(err)
metadata = map[string]*string{}
// Test updating metadata with the same values
err = s.az.storage.(*BlockBlob).updateMetadata(name, metadata)
s.assert.Nil(err)

// Verify metadata remains unchanged
props, err := blobClient.GetProperties(ctx, nil)

s.assert.Nil(err)
s.assert.NotNil(props.Metadata)
s.assert.Equal("1000", *props.Metadata[common.GroupId])
s.assert.Equal("1000", *props.Metadata[common.OwnerID])

resp, err := s.az.GetAttr(internal.GetAttrOptions{Name: name})
s.assert.Nil(err)
s.assert.Equal("1000", *resp.Metadata["gid"])
s.assert.Equal("1000", *resp.Metadata["uid"])
}

func (s *blockBlobTestSuite) TestUpdateMetadataAddNewKey() {
defer s.cleanupTest()
// Setup
name := generateFileName()
s.az.CreateFile(internal.CreateFileOptions{Name: name})

// Set initial metadata
initialMetadata := map[string]*string{
"isDir": to.Ptr("1"),
}
blobClient := s.containerClient.NewBlobClient(name)
_, err := blobClient.SetMetadata(ctx, initialMetadata, nil)
s.assert.Nil(err)

// Test updating metadata with a new key
newMetadata := map[string]*string{
common.GroupId: to.Ptr("1000"),
common.OwnerID: to.Ptr("1000"),
}
err = s.az.storage.(*BlockBlob).updateMetadata(name, newMetadata)
s.assert.Nil(err)

// Verify metadata
props, err := blobClient.GetProperties(ctx, nil)
s.assert.Nil(err)
s.assert.NotNil(props.Metadata)
s.assert.Equal("1000", *props.Metadata[common.OwnerID])
s.assert.Equal("1000", *props.Metadata[common.GroupId])
s.assert.Equal("1", *props.Metadata["Isdir"])
resp, err := s.az.GetAttr(internal.GetAttrOptions{Name: name})
s.assert.Nil(err)
s.assert.Equal("1000", *resp.Metadata["gid"])
s.assert.Equal("1000", *resp.Metadata["uid"])
}

func (s *blockBlobTestSuite) TestUpdateMetadataModifyExistingKey() {
defer s.cleanupTest()
// Setup
name := generateFileName()
s.az.CreateFile(internal.CreateFileOptions{Name: name})

// Set initial metadata
initialMetadata := map[string]*string{
common.GroupId: to.Ptr("1000"),
common.OwnerID: to.Ptr("1000"),
}
blobClient := s.containerClient.NewBlobClient(name)
_, err := blobClient.SetMetadata(ctx, initialMetadata, nil)
s.assert.Nil(err)

// Test updating metadata with a modified value
modifiedMetadata := map[string]*string{
common.GroupId: to.Ptr("0"),
common.OwnerID: to.Ptr("0"),
}
err = s.az.storage.(*BlockBlob).updateMetadata(name, modifiedMetadata)
s.assert.Nil(err)

// Verify metadata
props, err := blobClient.GetProperties(ctx, nil)
s.assert.Nil(err)
s.assert.NotNil(props.Metadata)
s.assert.Equal("0", *props.Metadata[common.OwnerID])
s.assert.Equal("0", *props.Metadata[common.GroupId])
resp, err := s.az.GetAttr(internal.GetAttrOptions{Name: name})
s.assert.Nil(err)
s.assert.Equal("0", *resp.Metadata["gid"])
s.assert.Equal("0", *resp.Metadata["uid"])
}

func (s *blockBlobTestSuite) TestUpdateMetadataEmpty() {
defer s.cleanupTest()
// Setup
name := generateFileName()
s.az.CreateFile(internal.CreateFileOptions{Name: name})

// Test updating metadata with an empty map
err := s.az.storage.(*BlockBlob).updateMetadata(name, map[string]*string{})
s.assert.Nil(err)

// Verify metadata remains unchanged
blobClient := s.containerClient.NewBlobClient(name)
props, err := blobClient.GetProperties(ctx, nil)
s.assert.Nil(err)
s.assert.Nil(props.Metadata)
}

// In order for 'go test' to run this suite, we need to create
// a normal test function and pass our suite to suite.Run
func TestBlockBlob(t *testing.T) {
Expand Down
36 changes: 17 additions & 19 deletions component/azstorage/datalake.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import (
"net/url"
"os"
"path/filepath"
"strconv"
"strings"
"syscall"

Expand Down Expand Up @@ -402,6 +403,8 @@ func (dl *Datalake) GetAttr(name string) (blobAttr *internal.ObjAttr, err error)
Crtime: *prop.LastModified,
Flags: internal.NewFileBitMap(),
ETag: sanitizeEtag(prop.ETag),
UID: prop.Metadata[common.OwnerID],
GID: prop.Metadata[common.GroupId],
}
parseMetadata(blobAttr, prop.Metadata)

Expand Down Expand Up @@ -561,28 +564,23 @@ func (dl *Datalake) ChangeMod(name string, mode os.FileMode) error {
}

// ChangeOwner : Change owner of a path
func (dl *Datalake) ChangeOwner(name string, _ int, _ int) error {
func (dl *Datalake) ChangeOwner(name string, uid int, gid int) error {
log.Trace("Datalake::ChangeOwner : name %s", name)

if dl.Config.ignoreAccessModifiers {
// for operations like git clone where transaction fails if chown is not successful
// return success instead of ENOSYS
return nil
metadata := make(map[string]*string)
Copy link
Member

Choose a reason for hiding this comment

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

just pass the uid/gid as is to blockblob changeOwner method and it can take care of the rest. There is no need to implement this here.

uidStr := strconv.Itoa(uid)
gidStr := strconv.Itoa(gid)
metadata[common.OwnerID] = &uidStr
metadata[common.GroupId] = &gidStr
err := dl.BlockBlob.updateMetadata(name, metadata)
e := storeDatalakeErrToErr(err)
if e == ErrFileNotFound {
return syscall.ENOENT
} else if err != nil {
log.Err("Datalake::ChangeOwner : Failed to change ownership of file %s to [%s]", name, err.Error())
return err
}

// TODO: This is not supported for now.
// fileURL := dl.Filesystem.NewRootDirectoryURL().NewFileURL(filepath.Join(dl.Config.prefixPath, name))
// group := strconv.Itoa(gid)
// owner := strconv.Itoa(uid)
// _, err := fileURL.SetAccessControl(context.Background(), azbfs.BlobFSAccessControl{Group: group, Owner: owner})
// e := storeDatalakeErrToErr(err)
// if e == ErrFileNotFound {
// return syscall.ENOENT
// } else if err != nil {
// log.Err("Datalake::ChangeOwner : Failed to change ownership of file %s to %s [%s]", name, mode, err.Error())
// return err
// }
return syscall.ENOTSUP
return nil
}

// GetCommittedBlockList : Get the list of committed blocks
Expand Down
49 changes: 49 additions & 0 deletions component/azstorage/datalake_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ import (
"io/fs"
"os"
"path/filepath"
"strconv"
"strings"
"syscall"
"testing"
Expand Down Expand Up @@ -2839,6 +2840,54 @@ func (s *datalakeTestSuite) TestList() {
s.assert.NotEqual(0, blobList[0].Mode)
}

func (s *datalakeTestSuite) TestChangeOwner() {
defer s.cleanupTest()
// Setup
name := generateFileName()
s.az.CreateFile(internal.CreateFileOptions{Name: name})

// Test changing owner
uid := 1001
gid := 1002
err := s.az.Chown(internal.ChownOptions{Name: name, Owner: uid, Group: gid})
s.assert.Nil(err)

// Verify the owner has been changed
attr, err := s.az.GetAttr(internal.GetAttrOptions{Name: name})
s.assert.Nil(err)
s.assert.Equal(strconv.Itoa(uid), *attr.Metadata[common.OwnerID])
s.assert.Equal(strconv.Itoa(gid), *attr.Metadata[common.GroupId])
}

// func (s *datalakeTestSuite) TestChangeOwnerNonExistentFile() {
// defer s.cleanupTest()
// // Setup
// name := generateFileName()

// // Test changing owner on a non-existent file
// uid := 1001
// gid := 1002
// err := s.az.Chown(internal.ChownOptions{Name: name, Owner: uid, Group: gid})
// s.assert.NotNil(err)
// s.assert.Equal(syscall.ENOENT, err)
// }

// func (s *datalakeTestSuite) TestChangeOwnerInsuffiecientPermission() {
// defer s.cleanupTest()
// // Setup
// name := generateFileName()
// s.az.CreateFile(internal.CreateFileOptions{Name: name})

// // Simulate insufficient permissions by setting the file to read-only
// s.az.Chmod(internal.ChmodOptions{Name: name, Mode: 0444})

// // Test changing owner with insufficient permissions
// uid := 1001
// gid := 1002
// err := s.az.Chown(internal.ChownOptions{Name: name, Owner: uid, Group: gid})
// s.assert.Nil(err)
// }

// func (s *datalakeTestSuite) TestRAGRS() {
// defer s.cleanupTest()
// // Setup
Expand Down
Loading
Loading