Skip to content

HADOOP-16825: Checkaccess testcase fix #1821

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.

Sign up for GitHub

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

Closed
bilaharith wants to merge 6 commits into from
Closed

HADOOP-16825: Checkaccess testcase fix #1821

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
729f44e
HADOOP-16825 Testcase fix
bilaharith Jan 28, 2020
49abe9c
HADOOP-16825 Test case fix
bilaharith Feb 5, 2020
6954c0c
Merge branch 'HADOOP-16825' of https://github.com/bilaharith/hadoop i…
bilaharith Feb 5, 2020
331a203
HADOOP-16825 Testcase fixes
bilaharith Feb 5, 2020
72aea97
HADOOP-16825 Addressed the review comments
bilaharith Feb 6, 2020
195023b
HADOOP-16825 Incorporating review comments
bilaharith Feb 6, 2020
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: 1 addition & 1 deletion ...p-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -946,7 +946,7 @@ public AclStatus getAclStatus(final Path path) throws IOException {
@Override
public void access(final Path path, final FsAction mode) throws IOException {
LOG.debug("AzureBlobFileSystem.access path : {}, mode : {}", path, mode);
Path qualifiedPath = makeQualified(path);
Path qualifiedPath = path == null ? null : makeQualified(path);

@ThomasMarquardt ThomasMarquardt Feb 5, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The original line 949 is correct. If path is null, FileSystem.access should throw IllegalArgumentException and that's what calling makeQualified(path) will do.

@bilaharith bilaharith Feb 6, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Keeping it this way as we don't need to break the existing flow when the checkaccess flag is false.

@ThomasMarquardt ThomasMarquardt Feb 6, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don’t follow. What flow are you referring to? I think you should revert this line because null path should result in IllegalArgumentException. Calling makeQualified is a standard thing done by all the APIs taking a Path.

@bilaharith bilaharith Feb 6, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removing the addition as discussed.

try {
this.abfsStore.access(qualifiedPath, mode);
} catch (AzureBlobFileSystemException ex) {
Expand Down
25 changes: 20 additions & 5 deletions ...zure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCheckAccess.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public class ITestAzureBlobFileSystemCheckAccess

private static final String TEST_FOLDER_PATH = "CheckAccessTestFolder";
private final FileSystem superUserFs;
private final FileSystem testUserFs;
private FileSystem testUserFs;
private final String testUserGuid;
private final boolean isCheckAccessEnabled;
private final boolean isHNSEnabled;
Expand All @@ -63,13 +63,15 @@ public ITestAzureBlobFileSystemCheckAccess() throws Exception {
this.superUserFs = getFileSystem();
testUserGuid = getConfiguration()
.get(FS_AZURE_BLOB_FS_CHECKACCESS_TEST_USER_GUID);
this.testUserFs = getTestUserFs();
this.isCheckAccessEnabled = getConfiguration().isCheckAccessEnabled();
this.isHNSEnabled = getConfiguration()
.getBoolean(FS_AZURE_TEST_NAMESPACE_ENABLED_ACCOUNT, false);
}

private FileSystem getTestUserFs() throws Exception {
private void setTestUserFs() throws Exception {
if(this.testUserFs != null) {

@ThomasMarquardt ThomasMarquardt Feb 5, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: please add a space between if and (.

@bilaharith bilaharith Feb 6, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

return;
}
String orgClientId = getConfiguration().get(FS_AZURE_BLOB_FS_CLIENT_ID);
String orgClientSecret = getConfiguration()
.get(FS_AZURE_BLOB_FS_CLIENT_SECRET);
Expand All @@ -88,11 +90,12 @@ private FileSystem getTestUserFs() throws Exception {
getRawConfiguration()
.setBoolean(AZURE_CREATE_REMOTE_FILESYSTEM_DURING_INITIALIZATION,
orgCreateFileSystemDurungInit);
return fs;
this.testUserFs = fs;
}

@Test(expected = IllegalArgumentException.class)

@ThomasMarquardt ThomasMarquardt Feb 5, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is expected to throw IllegalArgumentException.

@bilaharith bilaharith Feb 6, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@Test
public void testCheckAccessWithNullPath() throws IOException {
assumeHNSAndCheckAccessEnabled();
superUserFs.access(null, FsAction.READ);
}

Expand All @@ -106,6 +109,7 @@ public void testCheckAccessForFileWithNullFsAction() throws Exception {
@Test(expected = FileNotFoundException.class)
public void testCheckAccessForNonExistentFile() throws Exception {
assumeHNSAndCheckAccessEnabled();
setTestUserFs();
Path nonExistentFile = setupTestDirectoryAndUserAccess(
"/nonExistentFile1.txt", FsAction.ALL);
superUserFs.delete(nonExistentFile, true);
Expand All @@ -121,6 +125,7 @@ public void testWhenCheckAccessConfigIsOff() throws Exception {
FileSystem fs = FileSystem.newInstance(conf);
Path testFilePath = setupTestDirectoryAndUserAccess("/test1.txt",
FsAction.NONE);
fs.access(null, FsAction.READ);
fs.access(testFilePath, FsAction.EXECUTE);
fs.access(testFilePath, FsAction.READ);
fs.access(testFilePath, FsAction.WRITE);
Expand Down Expand Up @@ -149,12 +154,16 @@ public void testCheckAccessForAccountWithoutNS() throws Exception {
Assume.assumeFalse(FS_AZURE_TEST_NAMESPACE_ENABLED_ACCOUNT + " is true",
getConfiguration()
.getBoolean(FS_AZURE_TEST_NAMESPACE_ENABLED_ACCOUNT, true));
Assume.assumeTrue(FS_AZURE_ENABLE_CHECK_ACCESS + " is false",
isCheckAccessEnabled);
setTestUserFs();
testUserFs.access(new Path("/"), FsAction.READ);
}

@Test
public void testFsActionNONE() throws Exception {
assumeHNSAndCheckAccessEnabled();
setTestUserFs();
Path testFilePath = setupTestDirectoryAndUserAccess("/test2.txt",
FsAction.NONE);
assertInaccessible(testFilePath, FsAction.EXECUTE);
Expand All @@ -169,6 +178,7 @@ public void testFsActionNONE() throws Exception {
@Test
public void testFsActionEXECUTE() throws Exception {
assumeHNSAndCheckAccessEnabled();
setTestUserFs();
Path testFilePath = setupTestDirectoryAndUserAccess("/test3.txt",
FsAction.EXECUTE);
assertAccessible(testFilePath, FsAction.EXECUTE);
Expand All @@ -184,6 +194,7 @@ public void testFsActionEXECUTE() throws Exception {
@Test
public void testFsActionREAD() throws Exception {
assumeHNSAndCheckAccessEnabled();
setTestUserFs();
Path testFilePath = setupTestDirectoryAndUserAccess("/test4.txt",
FsAction.READ);
assertAccessible(testFilePath, FsAction.READ);
Expand All @@ -199,6 +210,7 @@ public void testFsActionREAD() throws Exception {
@Test
public void testFsActionWRITE() throws Exception {
assumeHNSAndCheckAccessEnabled();
setTestUserFs();
Path testFilePath = setupTestDirectoryAndUserAccess("/test5.txt",
FsAction.WRITE);
assertAccessible(testFilePath, FsAction.WRITE);
Expand All @@ -214,6 +226,7 @@ public void testFsActionWRITE() throws Exception {
@Test
public void testFsActionREADEXECUTE() throws Exception {
assumeHNSAndCheckAccessEnabled();
setTestUserFs();
Path testFilePath = setupTestDirectoryAndUserAccess("/test6.txt",
FsAction.READ_EXECUTE);
assertAccessible(testFilePath, FsAction.EXECUTE);
Expand All @@ -229,6 +242,7 @@ public void testFsActionREADEXECUTE() throws Exception {
@Test
public void testFsActionWRITEEXECUTE() throws Exception {
assumeHNSAndCheckAccessEnabled();
setTestUserFs();
Path testFilePath = setupTestDirectoryAndUserAccess("/test7.txt",
FsAction.WRITE_EXECUTE);
assertAccessible(testFilePath, FsAction.EXECUTE);
Expand All @@ -244,6 +258,7 @@ public void testFsActionWRITEEXECUTE() throws Exception {
@Test
public void testFsActionALL() throws Exception {
assumeHNSAndCheckAccessEnabled();
setTestUserFs();
Path testFilePath = setupTestDirectoryAndUserAccess("/test8.txt",
FsAction.ALL);
assertAccessible(testFilePath, FsAction.EXECUTE);
Expand Down