You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2021/05/11 09:30:41 UTC
[GitHub] [ozone] adoroszlai commented on a change in pull request #2236: HDDS-5205. Make admin check work for SCM HA cluster
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org