NC | Multi Protocol Access | List object with conflicting ownership #8751
Conversation
naveenpaul1
force-pushed
the
list_object_conflicting_files
branch
from
9d96e8d to
a8b2e99
Compare
naveenpaul1
requested review from
romayalon,
shirady,
jackyalbo and
dannyzaken
Signed-off-by: naveenpaul1 <[email protected]>
naveenpaul1
force-pushed
the
list_object_conflicting_files
branch
from
a8b2e99 to
816485f
Compare
| @@ -924,6 +919,24 @@ class NamespaceFS { | |||
| } | |||
| } | |||
|
|
|||
| async validate_results(results, fs_context) { | |||
There was a problem hiding this comment.
if you remove from the list right before it's being returned to the user, you might be left with less than 1000 objects, even if you have access to more than 1000 objects, can you please check that?
| dbg.warn('NamespaceFS: validate_results : couldnt access file entry_path', entry_path, ", skipping..."); | ||
| const index = results.indexOf(r); | ||
| if (index > -1) { | ||
| results.splice(index, 1); |
There was a problem hiding this comment.
what about bucket owner, do we always want to remove this entry from the list? this should be compatible to S3
There was a problem hiding this comment.
@romayalon When removing an item from the list, there is a chance the list object returns with less than 1000 items One solution would be to check the access while adding the item to result in an array and return 1000 items
| const use_lstat = !(await this._is_path_in_bucket_boundaries(fs_context, entry_path)); | ||
| r.stat = await nb_native().fs.stat(fs_context, entry_path, { use_lstat }); | ||
| })); | ||
| await this.validate_results(results, fs_context); |
There was a problem hiding this comment.
I am not sure this is a good name for this function. there is no much validation going on here. maybe change it to something like _get_stats_for_list_object. also, in my opinion, if you like to move it to a function it would be better to move the lambda function to a desperate function, instead of the whole Promise.all. WDYT?
| const use_lstat = !(await this._is_path_in_bucket_boundaries(fs_context, entry_path)); | ||
| try { | ||
| r.stat = await nb_native().fs.stat(fs_context, entry_path, { use_lstat }); | ||
| } catch (err) { |
There was a problem hiding this comment.
notice that you throw for all errors and not just EACCES( permission denied) is this intentional? maybe it would be better to create a separate function that doesn't throw for selected errors. like we do in unlink_ignore_enoent. that way we can reuse in case we would like to handle permission issues in other places. see stat for possible error list
There was a problem hiding this comment.
also that way we will be able to handle what happens in case of EACCES outside the catch. something like:
const stat = stat_ignore_eaccess();
if (!stat) {
....
}
|
@naveenpaul1 note that this PR will only handle ListObject. its worth checking if this also works for ListObjectVersions. what about other commands, it worth checking if we have other issues. for example bucket owner will not be able to override keys made by other users, is this acceptable? |
Explain the changes
Issues: Fixed #xxx / Gap #xxx
Testing Instructions: