[
https://issues.apache.org/jira/browse/HADOOP-14913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16197731#comment-16197731
]
Steve Loughran commented on HADOOP-14913:
-----------------------------------------
-1. This would change the semantics of calling rename() if the source could not
be found, downgrading it from an FNFE to a false.
rename() is the operation we fear. In POSIX it has mild ambiguity, in native
file:// implementations much more (windows ::MoveFile downgrades to a copy
across volumes, posix fails); return codes ambiguous. At the same time, it's an
essential operation for protocols trying to use the fs to commit operations.
# I would tread very carefully near rename, get reviews from others.
# And the behaviour must follow that covered in
`hadoop-common-project/hadoop-common/src/site/markdown/filesystem/filesystem.md`,
including how to handle missing source files.
# the tests in {{AbstractContractRenameTest}} validate implementations against
the spec, with
xsupport for variants in semantics; {{ITestAzureNativeContractRename}} being
the azure one. Ideally I'd like to see that suite running with auth enabled,
to make sure that having auth on doesn't interfere with the outcome.
# Return codes are problematic as returning "false" is essentially useless to
people calling it.
Have a look at how apps handle a return value of false to see what I mean:
usally its throwing an exception "rename failed (and we don't know why)"
It's why we've discussed opening up {{void rename(Path src, Path dst, Rename...
options) throws IOException}} as a public call; that one is required to raise
exceptions when there are problems. It's also why S3A has its own
{{RenameFailedException}} which throws up the exit code; a design to consider
copying in future.
h2. NativeAzureFilesystem
L3168. It's a bit confusing here as "false" means the source file doesn't exiost
L3422 isStickyBitCheckViolatedForRenameOperation
The javadocs here are incomplete/misleading. Seems to me that if there's an
auth failure, you get WasbAuthorizationException.This makes the meaning of the
method name confusing too, its not a bool "is the permission violated", more
something saying "fail rename silently". I'd like failures of the check to
raise, as noted.
Part of the problem here is that FNFEs are being caught and downgraded. If all
exception handling in {{isStickyBitCheckViolatedForRenameOperation}} was pulled
out into rename(), it'd have rename() making the decision about what missing
objects meant, not this method.
recommndation: remove all catch IOE logic. rename() should do the catch
something like
{code}
if (azureAuthorization ) {
try {
checkStickyBitCheckViolatedForRenameOperation(
absoluteSrcPath, srcParentFolder)}
} catch (WasbAuthorizationException e) {
throw e;
} catch {FileNotFoundException ex} {
return false;
} catch {IOException ex}
// examine exceptions
if (isFileNotFound(ex) return false;
throw ex;
}
{code}
And {{checkStickyBitCheckViolatedForRenameOperation}} becomes simpler,
something like (uncompiled code) checking the data, with no exception catching,
and
so returning void.
{code}
/**
* Performs sticky bit check on source folder for rename operation.
* @throws WasbAuthorizationException sticky bit check was violated
* @throws FileNotFoundException HEAD requests of file or parent returned null
* @throws IOException other failure: may be nested Azure exception.
*/
private void performStickyBitCheckForRenameOperation(Path srcPath,
Path srcParentPath) throws WasbAuthorizationException,
FileNotFoundException, IOException {
String srcKey = pathToKey(srcPath);
FileMetadata srcMetadata = store.retrieveMetadata(srcKey);
if (srcMetadata == null) {
LOG.debug("Source {} doesn't exist. Failing rename.",
srcPath.toString());
throw new FileNotFoundException(srcPath.toString());
}
String parentkey = pathToKey(srcParentPath);
FileMetadata parentMetadata = null;
parentMetadata = store.retrieveMetadata(parentkey);
if (parentMetadata == null) {
LOG.debug("Path {} doesn't exist, failing rename.",
srcParentPath.toString());
throw new FileNotFoundException(srcPath.toString());
}
if (isStickyBitCheckViolated(srcMetadata, parentMetadata)) {
throw new WasbAuthorizationException(
String.format("Rename operation for %s is not permitted."
+ " Details : Stickybit check failed.", srcPath.toString()));
}
}
{code}
Doing it this way lines things up for rename/3 being opened up in future, as it
keeps all policy of "When should rename() return false" from this code in
rename() itself.
h2. Tests
Look at the tests in {{AbstractContractRenameTest}}, identify those which
aren't being mimiced in {{TestNativeAzureFileSystemAuthorization}}. copy them
over, get them working. Start with the ones with missing source files: the
behaviour with auth on must match that with auth off:
{{testRenameNonexistentFile()}}. I'd also consider a test of renaming a
directory under another directory, and on top of an empty directory.
{{testRenameAccessCheckPositiveOnDstFolder}}
* doesn't close the source file in fs.create. Use {{ContractTestUtils.touch}}
* assert that the return code matches that expected on a normal rename
{{testRenamePositiveWhenDestinationFolderExists}}
same
{{testRenamePositiveWhenDestinationFolderDoesNotExists}}
Test name should be {{testRenamePositiveWhenDestinationFolderDoesNotExist}};
same issues as {{testRenameAccessCheckPositiveOnDstFolder}}
> Sticky bit implementation for Rename operation in Azure fs
> ----------------------------------------------------------
>
> Key: HADOOP-14913
> URL: https://issues.apache.org/jira/browse/HADOOP-14913
> Project: Hadoop Common
> Issue Type: New Feature
> Components: fs, fs/azure
> Reporter: Varada Hemeswari
> Assignee: Varada Hemeswari
> Labels: azure, fs, secure
> Attachments: HADOOP-14193.001.patch, HADOOP-14193.002.patch
>
>
> When authorization is enabled in WASB filesystem, there is a need for
> stickybit in cases where multiple users can create files under a shared
> directory. This additional check for sticky bit is reqired since any user can
> delete/rename another user's file because the parent has WRITE permission for
> all users.
> The purpose of this jira is to implement sticky bit equivalent for 'rename'
> call when authorization is enabled.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
