adoroszlai commented on a change in pull request #2236:
URL: https://github.com/apache/ozone/pull/2236#discussion_r629978932
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
##########
@@ -404,13 +403,12 @@ private boolean hasRequiredReplicas(ContainerInfo
contInfo) {
@Override
public void deleteContainer(long containerID) throws IOException {
- String remoteUser = getRpcRemoteUsername();
boolean auditSuccess = true;
Map<String, String> auditMap = Maps.newHashMap();
auditMap.put("containerID", String.valueOf(containerID));
- auditMap.put("remoteUser", remoteUser);
+ auditMap.put("remoteUser", getRpcRemoteUsername());
Review comment:
Can we store result of `Server.getRemoteUser()` and reuse it for both
audit and check?
```suggestion
UserGroupInformation remoteUser = Server.getRemoteUser();
auditMap.put("remoteUser", remoteUser.getUserName());
```
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
##########
@@ -404,13 +403,12 @@ private boolean hasRequiredReplicas(ContainerInfo
contInfo) {
@Override
public void deleteContainer(long containerID) throws IOException {
- String remoteUser = getRpcRemoteUsername();
boolean auditSuccess = true;
Map<String, String> auditMap = Maps.newHashMap();
auditMap.put("containerID", String.valueOf(containerID));
- auditMap.put("remoteUser", remoteUser);
+ auditMap.put("remoteUser", getRpcRemoteUsername());
try {
- getScm().checkAdminAccess(remoteUser);
+ getScm().checkAdminAccess(Server.getRemoteUser());
Review comment:
```suggestion
getScm().checkAdminAccess(remoteUser);
```
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
##########
@@ -502,7 +497,7 @@ public void closeContainer(long containerID) throws
IOException {
auditMap.put("containerID", String.valueOf(containerID));
auditMap.put("remoteUser", remoteUser);
try {
- scm.checkAdminAccess(remoteUser);
+ scm.checkAdminAccess(Server.getRemoteUser());
Review comment:
Same here. If both of these are updated, `getRpcRemoteUsername()` can
be deleted.
##########
File path: hadoop-ozone/dist/src/main/compose/ozonesecure-ha/test.sh
##########
@@ -34,6 +34,9 @@ execute_robot_test ${SCM} freon
execute_robot_test ${SCM} basic/links.robot
execute_robot_test ${SCM} s3
+
+export SCM=scm2.org
+execute_robot_test ${SCM} security-ha
Review comment:
Instead of introducing a new test suite (`security-ha`) with only a
single test case that is not even HA-specific, I think we should run the
complete `admincli` test suite (probably for both scm1 and scm2).
```suggestion
execute_robot_test ${SCM} admincli
export SCM=scm2.org
execute_robot_test ${SCM} admincli
```
Only one test case in each `admincli` test file needs to be changed for this
to work: `... on unknown host` fails because `--scm` parameter is ignored for
SCM HA cluster. I think it is a bug that should be fixed in the long term.
But for now these test cases can be disabled (commented out), as they are not
too important. I think it is more important to have full `admincli` test
coverage for HA than to run this marginal test case for non-HA.
https://github.com/apache/ozone/blob/49538aaa11f41bf80f565f9ce4f2c49ebac5fd51/hadoop-ozone/dist/src/main/smoketest/admincli/container.robot#L75-L77
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
##########
@@ -404,13 +403,12 @@ private boolean hasRequiredReplicas(ContainerInfo
contInfo) {
@Override
public void deleteContainer(long containerID) throws IOException {
- String remoteUser = getRpcRemoteUsername();
boolean auditSuccess = true;
Map<String, String> auditMap = Maps.newHashMap();
auditMap.put("containerID", String.valueOf(containerID));
- auditMap.put("remoteUser", remoteUser);
+ auditMap.put("remoteUser", getRpcRemoteUsername());
try {
- getScm().checkAdminAccess(remoteUser);
+ getScm().checkAdminAccess(Server.getRemoteUser());
Review comment:
Also note that `TestStorageContainerManager` integration test is
failing, because `Server.getRemoteUser()` returns `null` (no mock set up for
this static method, as opposed to
`SCMClientProtocolServer.getRpcRemoteUsername()`), so access is granted.
https://github.com/apache/ozone/pull/2236/checks?check_run_id=2553193751#step:4:4223
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
