hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Steve Loughran (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HADOOP-14913) Sticky bit implementation for Rename operation in Azure fs
Date Mon, 09 Oct 2017 21:33:00 GMT

    [ 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-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org


Mime
View raw message