HBASE-26662 User.createUserForTesting should not reset UserProvider.groups every time if hbase.group.service.for.test.only is true

[wchevreuil]

No description provided.

[Apache-HBase]

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 3s	Docker mode activated.
-0 ⚠️	yetus	0m 3s	Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
		_ Prechecks _
		_ master Compile Tests _
+1 💚	mvninstall	4m 51s	master passed
+1 💚	compile	0m 27s	master passed
+1 💚	shadedjars	8m 24s	branch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	0m 28s	master passed
		_ Patch Compile Tests _
+1 💚	mvninstall	4m 36s	the patch passed
+1 💚	compile	0m 28s	the patch passed
+1 💚	javac	0m 28s	the patch passed
+1 💚	shadedjars	8m 27s	patch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	0m 25s	the patch passed
		_ Other Tests _
+1 💚	unit	2m 3s	hbase-common in the patch passed.
		32m 38s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4029/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile
GITHUB PR	#4029
Optional Tests	javac javadoc unit shadedjars compile
uname	Linux fb935207cd35 4.15.0-65-generic #74-Ubuntu SMP Tue Sep 17 17:06:04 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / 3a14cfc
Default Java	AdoptOpenJDK-11.0.10+9
Test Results	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4029/1/testReport/
Max. process+thread count	291 (vs. ulimit of 30000)
modules	C: hbase-common U: hbase-common
Console output	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4029/1/console
versions	git=2.17.1 maven=3.6.3
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

[Apache-HBase]

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 11s	Docker mode activated.
-0 ⚠️	yetus	0m 3s	Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
		_ Prechecks _
		_ master Compile Tests _
+1 💚	mvninstall	4m 42s	master passed
+1 💚	compile	0m 24s	master passed
+1 💚	shadedjars	9m 8s	branch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	0m 23s	master passed
		_ Patch Compile Tests _
+1 💚	mvninstall	4m 16s	the patch passed
+1 💚	compile	0m 23s	the patch passed
+1 💚	javac	0m 23s	the patch passed
+1 💚	shadedjars	9m 2s	patch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	0m 20s	the patch passed
		_ Other Tests _
+1 💚	unit	1m 54s	hbase-common in the patch passed.
		33m 1s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4029/1/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile
GITHUB PR	#4029
Optional Tests	javac javadoc unit shadedjars compile
uname	Linux 0c284ddb29e3 4.15.0-153-generic #160-Ubuntu SMP Thu Jul 29 06:54:29 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / 3a14cfc
Default Java	AdoptOpenJDK-1.8.0_282-b08
Test Results	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4029/1/testReport/
Max. process+thread count	229 (vs. ulimit of 30000)
modules	C: hbase-common U: hbase-common
Console output	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4029/1/console
versions	git=2.17.1 maven=3.6.3
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

[Apache-HBase]

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 33s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	hbaseanti	0m 0s	Patch does not have any anti-patterns.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
		_ master Compile Tests _
+1 💚	mvninstall	4m 28s	master passed
+1 💚	compile	0m 55s	master passed
+1 💚	checkstyle	0m 26s	master passed
+1 💚	spotbugs	0m 51s	master passed
		_ Patch Compile Tests _
+1 💚	mvninstall	4m 8s	the patch passed
+1 💚	compile	0m 52s	the patch passed
+1 💚	javac	0m 52s	the patch passed
+1 💚	checkstyle	0m 25s	the patch passed
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	hadoopcheck	20m 48s	Patch does not cause any errors with Hadoop 3.1.2 3.2.2 3.3.1.
+1 💚	spotbugs	0m 57s	the patch passed
		_ Other Tests _
+1 💚	asflicense	0m 13s	The patch does not generate ASF License warnings.
		43m 11s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4029/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#4029
Optional Tests	dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile
uname	Linux 7f5c5cc73646 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / 3a14cfc
Default Java	AdoptOpenJDK-1.8.0_282-b08
Max. process+thread count	96 (vs. ulimit of 30000)
modules	C: hbase-common U: hbase-common
Console output	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4029/1/console
versions	git=2.17.1 maven=3.6.3 spotbugs=4.2.2
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

[Apache9]

Could you please describe your usage more? I do not fully understand wha's the problem here...

[wchevreuil]

Could you please describe your usage more? I do not fully understand wha's the problem here...

Sure. As mentioned in the jira description, the "if" check below will always unnecessarily reset static var UserProvider.groups to a newly created instance of TestingGroups every time SecureHadoopUser.createUserForTesting is called.

 if (!(UserProvider.groups instanceof TestingGroups) ||
conf.getBoolean(TestingGroups.TEST_CONF, false)) {
UserProvider.groups = new TestingGroups(UserProvider.groups);
}

That will cause all users but the latest one created in a test to be missed in the TestingGroups cache from UserProvider.groups, falling back to the underlying hadoop Groups own group caching. I believe this extra check in SecureHadoopUser.createUserForTesting is redundant. We should only make sure to wrap the underlying Groups implementation once, in the first time we pass through that code, when testing.

I bumped into this when running TestAccessController.testGetUsersPermissions against a hadoop version that includes HADOOP-17079. The changes introduced by HADOOP-17079 around several classes of the hadoop security framework have broken TestAccessController as below:

2022-01-17 17:54:47,648 WARN [RpcServer.default.FPBQ.Fifo.handler=2,queue=0,port=50198] security.ShellBasedUnixGroupsMapping(218): unable to return groups for user globalGroupUser1
PartialGroupNameException The user name globalGroupUser1 is not found. id: globalGroupUser1: no such user
id: globalGroupUser1: no such user
at org.apache.hadoop.security.ShellBasedUnixGroupsMapping.resolvePartialGroupNames(ShellBasedUnixGroupsMapping.java:291)
at org.apache.hadoop.security.ShellBasedUnixGroupsMapping.getUnixGroups(ShellBasedUnixGroupsMapping.java:215)
at org.apache.hadoop.security.ShellBasedUnixGroupsMapping.getGroupsSet(ShellBasedUnixGroupsMapping.java:123)
at org.apache.hadoop.security.Groups$GroupCacheLoader.fetchGroupSet(Groups.java:413)
at org.apache.hadoop.security.Groups$GroupCacheLoader.load(Groups.java:351)
at org.apache.hadoop.security.Groups$GroupCacheLoader.load(Groups.java:300)
at com.google.common.cache.LocalCache$LoadingValueReference.loadFuture(LocalCache.java:3529)
at com.google.common.cache.LocalCache$Segment.loadSync(LocalCache.java:2278)
at com.google.common.cache.LocalCache$Segment.lockedGetOrLoad(LocalCache.java:2155)
at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2045)
at com.google.common.cache.LocalCache.get(LocalCache.java:3953)
at com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:3976)
at com.google.common.cache.LocalCache$LocalLoadingCache.get(LocalCache.java:4960)
at org.apache.hadoop.security.Groups.getGroupInternal(Groups.java:258)
at org.apache.hadoop.security.Groups.getGroups(Groups.java:216)
at org.apache.hadoop.hbase.security.User$TestingGroups.getGroups(User.java:420)
at org.apache.hadoop.hbase.security.access.AccessChecker.getUserGroups(AccessChecker.java:484)
at org.apache.hadoop.hbase.security.access.PermissionStorage.parsePermissionRecord(PermissionStorage.java:642)
at org.apache.hadoop.hbase.security.access.PermissionStorage.parsePermissions(PermissionStorage.java:603)
at org.apache.hadoop.hbase.security.access.PermissionStorage.getPermissions(PermissionStorage.java:540)
at org.apache.hadoop.hbase.security.access.PermissionStorage.getUserPermissions(PermissionStorage.java:585)
at org.apache.hadoop.hbase.master.MasterRpcServices.getUserPermissions(MasterRpcServices.java:2845)

TestAccessController used to pass before this change only because it implements its own ShellBasedUnixGroupsMapping, overriding getGroups method. But after HADOOP-17079, a new method getGroupsSet has been introduced and is the one that gets called by hadoop security Groups class when fetching the groups cache.

[Apache9]

Was wondering why we have the second check but as you said, seems redundant as it will always be true when using TestingGroups.

+1