Skip to content

HADOOP-18122. ViewFileSystem fails on determining owning group when primary group doesn't exist for user #3987

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
cheyu2022 wants to merge 1 commit into from
Closed

HADOOP-18122. ViewFileSystem fails on determining owning group when primary group doesn't exist for user #3987

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 12 additions & 0 deletions hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/Constants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,4 +139,16 @@ public interface Constants {
String CONFIG_VIEWFS_TRASH_FORCE_INSIDE_MOUNT_POINT =
"fs.viewfs.trash.force-inside-mount-point";
boolean CONFIG_VIEWFS_TRASH_FORCE_INSIDE_MOUNT_POINT_DEFAULT = false;

/**
* Configuration for specifying user name for mount links.
*/
String CONFIG_VIEWFS_MOUNT_LINKS_USER_NAME =
"fs.viewfs.mount.links.user.name";

/**
* Configuration for specifying group name for mount links.
*/
String CONFIG_VIEWFS_MOUNT_LINKS_GROUP_NAME =
"fs.viewfs.mount.links.group.name";
}
51 changes: 39 additions & 12 deletions ...ommon-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,9 @@
import org.apache.hadoop.util.Progressable;
import org.apache.hadoop.util.Time;

import static org.apache.hadoop.fs.viewfs.Constants.CONFIG_VIEWFS_MOUNT_LINKS_USER_NAME;
import static org.apache.hadoop.fs.viewfs.Constants.CONFIG_VIEWFS_MOUNT_LINKS_GROUP_NAME;

/**
* ViewFileSystem (extends the FileSystem interface) implements a client-side
* mount table. Its spec and implementation is identical to {@link ViewFs}.
Expand Down Expand Up @@ -360,7 +363,7 @@ public FileSystem run() throws IOException {

@Override
protected FileSystem getTargetFileSystem(final INodeDir<FileSystem> dir)
throws URISyntaxException {
throws URISyntaxException, IOException {
return new InternalDirOfViewFs(dir, creationTime, ugi, myUri, config,
this);
}
Expand Down Expand Up @@ -1386,10 +1389,12 @@ static class InternalDirOfViewFs extends FileSystem {
final URI myUri;
private final boolean showMountLinksAsSymlinks;
private InodeTree<FileSystem> fsState;

private String mountLinkUserName;
private String mountLinkGroupName;

public InternalDirOfViewFs(final InodeTree.INodeDir<FileSystem> dir,
final long cTime, final UserGroupInformation ugi, URI uri,
Configuration config, InodeTree fsState) throws URISyntaxException {
Configuration config, InodeTree fsState) throws URISyntaxException, IOException {
myUri = uri;
this.fsState = fsState;
try {
Expand All @@ -1403,6 +1408,14 @@ public InternalDirOfViewFs(final InodeTree.INodeDir<FileSystem> dir,
showMountLinksAsSymlinks = config
.getBoolean(CONFIG_VIEWFS_MOUNT_LINKS_AS_SYMLINKS,
CONFIG_VIEWFS_MOUNT_LINKS_AS_SYMLINKS_DEFAULT);
mountLinkUserName = config.get(CONFIG_VIEWFS_MOUNT_LINKS_USER_NAME);
mountLinkGroupName = config.get(CONFIG_VIEWFS_MOUNT_LINKS_GROUP_NAME);
if (mountLinkUserName == null) {
mountLinkUserName = ugi.getShortUserName();
}
if (mountLinkGroupName == null) {
mountLinkGroupName = ugi.getPrimaryGroupName();
}
}

static private void checkPathIsSlash(final Path f) throws IOException {
Expand Down Expand Up @@ -1525,12 +1538,21 @@ public FileChecksum getFileChecksum(final Path f)
public FileStatus getFileStatus(Path f) throws IOException {
checkPathIsSlash(f);
return new FileStatus(0, true, 0, 0, creationTime, creationTime,
PERMISSION_555, ugi.getShortUserName(), ugi.getPrimaryGroupName(),
getMountLinkDefaultPermissions(),
mountLinkUserName,
mountLinkGroupName,
new Path(theInternalDir.fullPath).makeQualified(
myUri, ROOT_PATH));
}


/**
* {@inheritDoc}
*
* Note: listStatus on root("/") considers listing from fallbackLink if
* available. If the same directory name is present in configured mount
* path as well as in fallback link, then only the configured mount path
* will be listed in the returned result.
*/
@Override
public FileStatus[] listStatus(Path f) throws AccessControlException,
FileNotFoundException, IOException {
Expand All @@ -1552,8 +1574,8 @@ public FileStatus[] listStatus(Path f) throws AccessControlException,
// symlink and rest other properties are belongs to mount link only.
linkStatuses.add(
new FileStatus(0, false, 0, 0, creationTime, creationTime,
PERMISSION_555, ugi.getShortUserName(),
ugi.getPrimaryGroupName(), link.getTargetLink(), path));
getMountLinkDefaultPermissions(), mountLinkUserName,
mountLinkGroupName, link.getTargetLink(), path));
continue;
}

Expand Down Expand Up @@ -1584,8 +1606,8 @@ public FileStatus[] listStatus(Path f) throws AccessControlException,
} else {
internalDirStatuses.add(
new FileStatus(0, true, 0, 0, creationTime, creationTime,
PERMISSION_555, ugi.getShortUserName(),
ugi.getPrimaryGroupName(), path));
getMountLinkDefaultPermissions(), mountLinkUserName,
mountLinkGroupName, path));
}
}
FileStatus[] internalDirStatusesMergedWithFallBack = internalDirStatuses
Expand Down Expand Up @@ -1812,9 +1834,10 @@ public void setAcl(Path path, List<AclEntry> aclSpec) throws IOException {
@Override
public AclStatus getAclStatus(Path path) throws IOException {
checkPathIsSlash(path);
return new AclStatus.Builder().owner(ugi.getShortUserName())
.group(ugi.getPrimaryGroupName())
.addEntries(AclUtil.getMinimalAcl(PERMISSION_555))
return new AclStatus.Builder().owner(mountLinkUserName)
.group(mountLinkGroupName)
.setPermission(getMountLinkDefaultPermissions())
.addEntries(AclUtil.getMinimalAcl(getMountLinkDefaultPermissions()))
.stickyBit(false).build();
}

Expand Down Expand Up @@ -1917,6 +1940,10 @@ public Collection<? extends BlockStoragePolicySpi> getAllStoragePolicies()
}
return allPolicies;
}

private FsPermission getMountLinkDefaultPermissions() {
return PERMISSION_555;
Copy link
Contributor

@virajith virajith Apr 20, 2022

Choose a reason for hiding this comment

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

Do you want to make this configurable as well?

}
}

enum RenameStrategy {
Expand Down
55 changes: 37 additions & 18 deletions hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFs.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,9 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import static org.apache.hadoop.fs.viewfs.Constants.CONFIG_VIEWFS_MOUNT_LINKS_USER_NAME;
import static org.apache.hadoop.fs.viewfs.Constants.CONFIG_VIEWFS_MOUNT_LINKS_GROUP_NAME;

/**
* ViewFs (extends the AbstractFileSystem interface) implements a client-side
* mount table. The viewFs file system is implemented completely in memory on
Expand Down Expand Up @@ -271,7 +274,7 @@ public AbstractFileSystem run() throws IOException {

@Override
protected AbstractFileSystem getTargetFileSystem(
final INodeDir<AbstractFileSystem> dir) throws URISyntaxException {
final INodeDir<AbstractFileSystem> dir) throws URISyntaxException, IOException {
return new InternalDirOfViewFs(dir, creationTime, ugi, getUri(), this,
config);
}
Expand Down Expand Up @@ -973,18 +976,27 @@ static class InternalDirOfViewFs extends AbstractFileSystem {
final URI myUri; // the URI of the outer ViewFs
private InodeTree<AbstractFileSystem> fsState;
private Configuration conf;
private String mountLinkUserName;
private String mountLinkGroupName;

public InternalDirOfViewFs(final InodeTree.INodeDir<AbstractFileSystem> dir,
final long cTime, final UserGroupInformation ugi, final URI uri,
InodeTree fsState, Configuration conf)
throws URISyntaxException {
InodeTree fsState, Configuration conf) throws URISyntaxException, IOException {
super(FsConstants.VIEWFS_URI, FsConstants.VIEWFS_SCHEME, false, -1);
theInternalDir = dir;
creationTime = cTime;
this.ugi = ugi;
myUri = uri;
this.fsState = fsState;
this.conf = conf;
mountLinkUserName = conf.get(CONFIG_VIEWFS_MOUNT_LINKS_USER_NAME);
Copy link
Contributor

@virajith virajith Apr 20, 2022

Choose a reason for hiding this comment

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

may be write this as conf.get(CONFIG_VIEWFS_MOUNT_LINKS_USER_NAME) == null ? ugi.getShortUserName() : conf.get(CONFIG_VIEWFS_MOUNT_LINKS_USER_NAME) and explain why you are not using conf.get(CONFIG_VIEWFS_MOUNT_LINKS_USER_NAME, ugi.getShortUserName()) in a comment? Otherwise, it's unclear why you aren't using that method directly. Same for the other places where you are using this template

mountLinkGroupName = conf.get(CONFIG_VIEWFS_MOUNT_LINKS_GROUP_NAME);
if (mountLinkUserName == null) {
mountLinkUserName = ugi.getShortUserName();
}
if (mountLinkGroupName == null) {
mountLinkGroupName = ugi.getPrimaryGroupName();
}
}

static private void checkPathIsSlash(final Path f) throws IOException {
Expand Down Expand Up @@ -1081,7 +1093,8 @@ public FileChecksum getFileChecksum(final Path f)
public FileStatus getFileStatus(final Path f) throws IOException {
checkPathIsSlash(f);
return new FileStatus(0, true, 0, 0, creationTime, creationTime,
PERMISSION_555, ugi.getShortUserName(), ugi.getPrimaryGroupName(),
getMountLinkDefaultPermissions(), mountLinkUserName,
mountLinkGroupName,
new Path(theInternalDir.fullPath).makeQualified(
myUri, null));
}
Expand Down Expand Up @@ -1114,16 +1127,18 @@ public FileStatus getFileLinkStatus(final Path f)
myUri, null));
} catch (FileNotFoundException ex) {
result = new FileStatus(0, false, 0, 0, creationTime, creationTime,
PERMISSION_555, ugi.getShortUserName(), ugi.getPrimaryGroupName(),
inodelink.getTargetLink(),
new Path(inode.fullPath).makeQualified(
myUri, null));
getMountLinkDefaultPermissions(), mountLinkUserName,
mountLinkGroupName,
inodelink.getTargetLink(),
new Path(inode.fullPath).makeQualified(
myUri, null));
}
} else {
result = new FileStatus(0, true, 0, 0, creationTime, creationTime,
PERMISSION_555, ugi.getShortUserName(), ugi.getPrimaryGroupName(),
new Path(inode.fullPath).makeQualified(
myUri, null));
getMountLinkDefaultPermissions(), mountLinkUserName,
mountLinkGroupName,
new Path(inode.fullPath).makeQualified(
myUri, null));
}
return result;
}
Expand Down Expand Up @@ -1178,8 +1193,8 @@ public FileStatus[] listStatus(final Path f) throws IOException {
// symlink and rest other properties are belongs to mount link only.
linkStatuses.add(
new FileStatus(0, false, 0, 0, creationTime, creationTime,
PERMISSION_555, ugi.getShortUserName(),
ugi.getPrimaryGroupName(), link.getTargetLink(), path));
getMountLinkDefaultPermissions(), mountLinkUserName,
mountLinkGroupName, link.getTargetLink(), path));
continue;
}

Expand Down Expand Up @@ -1210,8 +1225,8 @@ public FileStatus[] listStatus(final Path f) throws IOException {
} else {
internalDirStatuses.add(
new FileStatus(0, true, 0, 0, creationTime, creationTime,
PERMISSION_555, ugi.getShortUserName(),
ugi.getPrimaryGroupName(), path));
getMountLinkDefaultPermissions(), mountLinkUserName,
mountLinkGroupName, path));
}
}

Expand Down Expand Up @@ -1408,9 +1423,9 @@ public void setAcl(Path path, List<AclEntry> aclSpec) throws IOException {
@Override
public AclStatus getAclStatus(Path path) throws IOException {
checkPathIsSlash(path);
return new AclStatus.Builder().owner(ugi.getShortUserName())
.group(ugi.getPrimaryGroupName())
.addEntries(AclUtil.getMinimalAcl(PERMISSION_555))
return new AclStatus.Builder().owner(mountLinkUserName)
.group(mountLinkGroupName)
.addEntries(AclUtil.getMinimalAcl(getMountLinkDefaultPermissions()))
.stickyBit(false).build();
}

Expand Down Expand Up @@ -1479,5 +1494,9 @@ public void setStoragePolicy(Path path, String policyName)
throws IOException {
throw readOnlyMountTable("setStoragePolicy", path);
}

private FsPermission getMountLinkDefaultPermissions() {
return PERMISSION_555;
}
}
}
60 changes: 60 additions & 0 deletions ...oject/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/ViewFileSystemBaseTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,12 @@
import static org.apache.hadoop.fs.viewfs.Constants.CONFIG_VIEWFS_ENABLE_INNER_CACHE;
import static org.apache.hadoop.fs.viewfs.Constants.PERMISSION_555;
import static org.apache.hadoop.fs.viewfs.Constants.CONFIG_VIEWFS_TRASH_FORCE_INSIDE_MOUNT_POINT;
import static org.apache.hadoop.fs.viewfs.Constants.CONFIG_VIEWFS_MOUNT_LINKS_USER_NAME;
import static org.apache.hadoop.fs.viewfs.Constants.CONFIG_VIEWFS_MOUNT_LINKS_GROUP_NAME;
import static org.apache.hadoop.fs.FileSystem.TRASH_PREFIX;
import static org.apache.hadoop.test.LambdaTestUtils.intercept;
import static org.mockito.Mockito.when;
import static org.mockito.Mockito.spy;

import org.junit.After;
import org.junit.Assert;
Expand Down Expand Up @@ -1705,4 +1710,59 @@ public void testInvalidMountPoints() throws Exception {
ex.getMessage().startsWith("URISyntax exception"));
}
}

@Test
public void testInternalDirectoryOwnership() throws Exception {
Configuration localConf = new Configuration(conf);
FileSystem fs = FileSystem.get(FsConstants.VIEWFS_URI, localConf);

// Check default owner/group.
final UserGroupInformation currentUser =
UserGroupInformation.getCurrentUser();
FileStatus status = fs.getFileStatus(new Path("/internalDir"));
assertEquals(currentUser.getUserName(), status.getOwner());
Copy link
Contributor

@virajith virajith Apr 20, 2022

Choose a reason for hiding this comment

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

currentUser.getShortUserName()?

assertEquals(currentUser.getGroupNames()[0], status.getGroup());
Copy link
Contributor

@virajith virajith Apr 20, 2022

Choose a reason for hiding this comment

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

currentUser.getPrimaryGroupName()?

assertEquals(PERMISSION_555, status.getPermission());

UserGroupInformation currUgi = UserGroupInformation.getCurrentUser();
try {
// Force exception when currUgi.getPrimaryGroupName() is called. This will
// not be triggered when viewfs mount link configs are defined.
UserGroupInformation spyUgi = spy(currUgi);
String failureMessage = "Fail on group check";
when(spyUgi.getPrimaryGroupName()).thenThrow(
new IOException(failureMessage));
UserGroupInformation.setLoginUser(spyUgi);

fs = FileSystem.get(FsConstants.VIEWFS_URI, localConf);
final FileSystem finalFs = fs;
intercept(IOException.class, () -> {
finalFs.getFileStatus(new Path("/internalDir"));
});

// Set owner and group configs for internal directories.
String fakeUser = "abc";
String fakeGroup = "def";
localConf.set(CONFIG_VIEWFS_MOUNT_LINKS_USER_NAME, fakeUser);
localConf.set(CONFIG_VIEWFS_MOUNT_LINKS_GROUP_NAME, fakeGroup);

// Check that internal directory owner/group relects what's set.
fs = FileSystem.get(FsConstants.VIEWFS_URI, localConf);
status = fs.getFileStatus(new Path("/internalDir"));
assertEquals(fakeUser, status.getOwner());
assertEquals(fakeGroup, status.getGroup());
assertEquals(PERMISSION_555, status.getPermission());

// Check that ACL status reflects what's set as well.
AclStatus aclStatus = fs.getAclStatus(new Path("/internalDir"));
assertEquals(fakeUser, aclStatus.getOwner());
assertEquals(fakeGroup, aclStatus.getGroup());
assertEquals(AclUtil.getMinimalAcl(PERMISSION_555),
aclStatus.getEntries());
assertEquals(PERMISSION_555, aclStatus.getPermission());
} finally {
// Set user back to the original currUgi object.
UserGroupInformation.setLoginUser(currUgi);
}
}
}
36 changes: 36 additions & 0 deletions ...ommon-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/ViewFsBaseTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
import static org.apache.hadoop.fs.FileContextTestHelper.isDir;
import static org.apache.hadoop.fs.FileContextTestHelper.isFile;
import static org.apache.hadoop.fs.viewfs.Constants.PERMISSION_555;
import static org.apache.hadoop.fs.viewfs.Constants.CONFIG_VIEWFS_MOUNT_LINKS_USER_NAME;
import static org.apache.hadoop.fs.viewfs.Constants.CONFIG_VIEWFS_MOUNT_LINKS_GROUP_NAME;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.mockito.ArgumentMatchers.any;
Expand Down Expand Up @@ -1022,4 +1024,38 @@ public Object run() throws Exception {
});
}

public void testInternalDirectoryOwnership() throws IOException {
Configuration localConf = new Configuration(conf);
FileContext fc = FileContext.getFileContext(
FsConstants.VIEWFS_URI, localConf);

// check default owner/group
final UserGroupInformation currentUser =
UserGroupInformation.getCurrentUser();
FileStatus status = fc.getFileStatus(new Path("/internalDir"));
assertEquals(currentUser.getUserName(), status.getOwner());
Copy link
Contributor

@virajith virajith Apr 20, 2022

Choose a reason for hiding this comment

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

currentUser.getShortUserName()?

assertEquals(currentUser.getGroupNames()[0], status.getGroup());
Copy link
Contributor

@virajith virajith Apr 20, 2022

Choose a reason for hiding this comment

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

currentUser.getPrimaryGroupName()?

assertEquals(PERMISSION_555, status.getPermission());

// set owner and group configs for internal directories.
String fakeUser = "abc";
String fakeGroup = "def";
localConf.set(CONFIG_VIEWFS_MOUNT_LINKS_USER_NAME, fakeUser);
localConf.set(CONFIG_VIEWFS_MOUNT_LINKS_GROUP_NAME, fakeGroup);

// check that internal directory owner/group relects what's set.
fc = FileContext.getFileContext(
FsConstants.VIEWFS_URI, localConf);
status = fc.getFileStatus(new Path("/internalDir"));
assertEquals(fakeUser, status.getOwner());
assertEquals(fakeGroup, status.getGroup());
assertEquals(PERMISSION_555, status.getPermission());

// check that ACL status reflects what's set as well.
AclStatus aclStatus = fc.getAclStatus(new Path("/internalDir"));
assertEquals(fakeUser, aclStatus.getOwner());
assertEquals(fakeGroup, aclStatus.getGroup());
assertEquals(aclStatus.getEntries(),
AclUtil.getMinimalAcl(PERMISSION_555));
}
}