Conversation
jainakanksha-msft
requested review from
ashruti-msft,
souravgupta-msft,
syeleti-msft and
vibhansa-msft
as code owners
There was a problem hiding this comment.
Pull Request Overview
This pull request adds chown (change ownership) handling for ADLS accounts, building on existing framework to support file ownership operations. The implementation includes proper error handling, metadata management, and comprehensive test coverage.
- Implements chown functionality for both ADLS and blob storage backends with proper owner/group metadata handling
- Adds owner and group information tracking in file attributes with corresponding flags for detection
- Extends libfuse handlers to properly route chown operations through the component chain with appropriate error mapping
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/attribute.go | Adds owner/group fields and detection flags to ObjAttr structure |
| component/azstorage/datalake.go | Implements actual chown functionality for ADLS using SetAccessControl API |
| component/azstorage/block_blob.go | Implements chown for blob storage using metadata approach |
| component/libfuse/*.go | Updates FUSE handlers to route chown operations and handle errors |
| test files | Adds comprehensive test coverage for chown scenarios and error cases |
Comments suppressed due to low confidence (2)
test/e2e_tests/file_test.go:677
- The test function
TestFileChownandTestFileChownADLShave identical implementations. Consider renaming one to clarify the difference or consolidating if they serve the same purpose.
func (suite *fileTestSuite) TestFileChown() {
| stat := info.Sys().(*syscall.Stat_t) | ||
| suite.Equal(nil, err) | ||
| suite.Equal(1000, stat.Uid) | ||
| suite.Equal(1000, stat.Gid) |
There was a problem hiding this comment.
The type assertion (*syscall.Stat_t) could panic if info.Sys() returns a different type. Consider using a safe type assertion with the comma ok idiom: stat, ok := info.Sys().(*syscall.Stat_t); if !ok { ... }
| stat := info.Sys().(*syscall.Stat_t) | |
| suite.Equal(nil, err) | |
| suite.Equal(1000, stat.Uid) | |
| suite.Equal(1000, stat.Gid) | |
| stat, ok := info.Sys().(*syscall.Stat_t) | |
| suite.True(ok, "info.Sys() did not return *syscall.Stat_t") | |
| suite.Equal(nil, err) | |
| if ok { | |
| suite.Equal(1000, stat.Uid) | |
| suite.Equal(1000, stat.Gid) | |
| } |
| stat := info.Sys().(*syscall.Stat_t) | ||
| suite.Equal(nil, err) |
There was a problem hiding this comment.
The type assertion (*syscall.Stat_t) could panic if info.Sys() returns a different type. Consider using a safe type assertion with the comma ok idiom: stat, ok := info.Sys().(*syscall.Stat_t); if !ok { ... }
| stat := info.Sys().(*syscall.Stat_t) | |
| suite.Equal(nil, err) | |
| stat, ok := info.Sys().(*syscall.Stat_t) | |
| suite.Equal(nil, err) | |
| if !ok { | |
| suite.Fail("Failed to assert type *syscall.Stat_t for file info.Sys()") | |
| return | |
| } |
| return attr.Flags.IsSet(PropFlagModeDefault) | ||
| } | ||
|
|
||
| // OwnerDetected : |
There was a problem hiding this comment.
The comment // OwnerDetected : is incomplete and doesn't describe what the function does. Consider updating it to: // OwnerInfoFound returns true if owner information was found in the object attributes
| // OwnerDetected : | |
| // OwnerInfoFound returns true if owner information was found in the object attributes. |
| return attr.Flags.IsSet(PropFlagOwnerInfoFound) | ||
| } | ||
|
|
||
| // GroupDetected : |
There was a problem hiding this comment.
The comment // GroupDetected : is incomplete and doesn't describe what the function does. Consider updating it to: // GroupInfoFound returns true if group information was found in the object attributes
| // GroupDetected : | |
| // GroupInfoFound returns true if group information was found in the object attributes |
component/azstorage/datalake.go
Outdated
| var uidStr, gidStr string | ||
| if uid != -1 { | ||
| uidStr = fmt.Sprintf("%d", uid) | ||
| opts.Owner = &uidStr | ||
| } | ||
|
|
||
| if gid != -1 { | ||
| gidStr = fmt.Sprintf("%d", gid) |
There was a problem hiding this comment.
[nitpick] The variables uidStr and gidStr are declared but gidStr is only used conditionally. Consider declaring them closer to their usage or within the conditional blocks to improve code clarity.
| var uidStr, gidStr string | |
| if uid != -1 { | |
| uidStr = fmt.Sprintf("%d", uid) | |
| opts.Owner = &uidStr | |
| } | |
| if gid != -1 { | |
| gidStr = fmt.Sprintf("%d", gid) | |
| var uidStr string | |
| if uid != -1 { | |
| uidStr = fmt.Sprintf("%d", uid) | |
| opts.Owner = &uidStr | |
| } | |
| if gid != -1 { | |
| gidStr := fmt.Sprintf("%d", gid) |
| if uid != 0xffffffff { | ||
| // Update only if user has explicitly set the uid | ||
| updatedOwner = AddMetadata(prop.Metadata, common.POSIXOwnerMeta, strconv.FormatUint(uint64(uid), 10)) | ||
| } | ||
| if gid != 0xffffffff { |
There was a problem hiding this comment.
The magic number 0xffffffff is used to check for invalid UID/GID. Consider defining a constant like InvalidUID = 0xffffffff to make the code more readable and maintainable.
| if uid != 0xffffffff { | |
| // Update only if user has explicitly set the uid | |
| updatedOwner = AddMetadata(prop.Metadata, common.POSIXOwnerMeta, strconv.FormatUint(uint64(uid), 10)) | |
| } | |
| if gid != 0xffffffff { | |
| if uid != InvalidUID { | |
| // Update only if user has explicitly set the uid | |
| updatedOwner = AddMetadata(prop.Metadata, common.POSIXOwnerMeta, strconv.FormatUint(uint64(uid), 10)) | |
| } | |
| if gid != InvalidUID { |
| if uid != 0xffffffff { | ||
| // Update only if user has explicitly set the uid | ||
| updatedOwner = AddMetadata(prop.Metadata, common.POSIXOwnerMeta, strconv.FormatUint(uint64(uid), 10)) | ||
| } | ||
| if gid != 0xffffffff { |
There was a problem hiding this comment.
The magic number 0xffffffff is used to check for invalid UID/GID. Consider defining a constant like InvalidGID = 0xffffffff to make the code more readable and maintainable.
| if uid != 0xffffffff { | |
| // Update only if user has explicitly set the uid | |
| updatedOwner = AddMetadata(prop.Metadata, common.POSIXOwnerMeta, strconv.FormatUint(uint64(uid), 10)) | |
| } | |
| if gid != 0xffffffff { | |
| if uid != InvalidUIDGID { | |
| // Update only if user has explicitly set the uid | |
| updatedOwner = AddMetadata(prop.Metadata, common.POSIXOwnerMeta, strconv.FormatUint(uint64(uid), 10)) | |
| } | |
| if gid != InvalidUIDGID { |
| if owner != 0xffffffff { | ||
| value.attr.Metadata[common.POSIXOwnerMeta] = &ownerStr | ||
| } | ||
|
|
||
| if group != 0xffffffff { |
There was a problem hiding this comment.
The magic number 0xffffffff is used to check for invalid owner. Consider defining a constant like InvalidOwner = 0xffffffff to make the code more readable and maintainable.
| if owner != 0xffffffff { | |
| value.attr.Metadata[common.POSIXOwnerMeta] = &ownerStr | |
| } | |
| if group != 0xffffffff { | |
| if owner != InvalidOwner { | |
| value.attr.Metadata[common.POSIXOwnerMeta] = &ownerStr | |
| } | |
| if group != InvalidOwner { |
| if owner != 0xffffffff { | ||
| value.attr.Metadata[common.POSIXOwnerMeta] = &ownerStr | ||
| } | ||
|
|
||
| if group != 0xffffffff { |
There was a problem hiding this comment.
The magic number 0xffffffff is used to check for invalid group. Consider defining a constant like InvalidGroup = 0xffffffff to make the code more readable and maintainable.
| if owner != 0xffffffff { | |
| value.attr.Metadata[common.POSIXOwnerMeta] = &ownerStr | |
| } | |
| if group != 0xffffffff { | |
| if owner != InvalidGroup { | |
| value.attr.Metadata[common.POSIXOwnerMeta] = &ownerStr | |
| } | |
| if group != InvalidGroup { |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if err != nil { | ||
| return 0 | ||
| } | ||
| return int(val) |
Check failure
Code scanning / CodeQL
Incorrect conversion between integer types High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
General Fix:
Ensure that the converted value from ParseUint fits in the range of int (which is at least [0, math.MaxInt32] on 32-bit systems; could be up to math.MaxInt64 on 64 bit but safest is to restrict to math.MaxInt).
Detailed Fix:
- After parsing, check whether
valis less than or equal tomath.MaxInt.- If so, safely convert to
int(val)and return. - If not, return 0 or another sentinel value.
- If so, safely convert to
- As
math.MaxIntis available via themathpackage, ensure it's imported. - The code should look like:
val, err := strconv.ParseUint(s, 10, 32)
if err != nil {
return 0
}
if val > uint64(math.MaxInt) {
return 0
}
return int(val)Region/lines to change:
Edit within the function ParseInt in common/util.go (lines 527–536).
| @@ -44,6 +44,7 @@ | ||
| "fmt" | ||
| "hash/crc64" | ||
| "io" | ||
| "math" | ||
| "os" | ||
| "os/exec" | ||
| "os/user" | ||
| @@ -532,6 +533,9 @@ | ||
| if err != nil { | ||
| return 0 | ||
| } | ||
| if val > uint64(math.MaxInt) { | ||
| return 0 | ||
| } | ||
| return int(val) | ||
| } | ||
|
|
4 participants
Type of Change
Description
This pull request introduces changes to add chown handling handling for adls account, and includes corresponding tests.
How Has This Been Tested?
Tested locally and with unit testing.
Still have to cover file_cache handling
Checklist