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 2022/05/05 06:52:17 UTC
[GitHub] [ozone] ChenSammi opened a new pull request, #3382: HDDS-6686. Do Leadship check before SASL token verification.
ChenSammi opened a new pull request, #3382:
URL: https://github.com/apache/ozone/pull/3382
https://issues.apache.org/jira/browse/HDDS-6686
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] ChenSammi commented on a diff in pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on code in PR #3382:
URL: https://github.com/apache/ozone/pull/3382#discussion_r889870420
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OMInterServiceProtocolServerSideImpl.java:
##########
@@ -60,7 +64,7 @@ public BootstrapOMResponse bootstrap(RpcController controller,
.build();
}
- checkLeaderStatus();
+ OzoneManagerRatisUtils.checkLeaderStatus(ozoneManager);
Review Comment:
Thanks @neils-dev . Do you think the PR can be merged now?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] ChenSammi commented on a diff in pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on code in PR #3382:
URL: https://github.com/apache/ozone/pull/3382#discussion_r886350701
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OMInterServiceProtocolServerSideImpl.java:
##########
@@ -60,7 +64,7 @@ public BootstrapOMResponse bootstrap(RpcController controller,
.build();
}
- checkLeaderStatus();
+ OzoneManagerRatisUtils.checkLeaderStatus(ozoneManager);
Review Comment:
Hey @neils-dev, sorry for the late response. Just want to confirm that the comments left to address is the following, right?
> The javadoc for validateS3Credential can be updated to include the ServiceException that may be thrown.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] neils-dev commented on a diff in pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
neils-dev commented on code in PR #3382:
URL: https://github.com/apache/ozone/pull/3382#discussion_r871879139
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/security/OzoneDelegationTokenSecretManager.java:
##########
@@ -404,6 +415,18 @@ public void removeToken(OzoneTokenIdentifier ozoneTokenIdentifier) {
@Override
public byte[] retrievePassword(OzoneTokenIdentifier identifier)
throws InvalidToken {
+ // Tokens are a bit different in that a follower OM may be behind and
+ // thus not yet know of all tokens issued by the leader OM. the
+ // following check does not allow ANY token auth. In optimistic, it should
+ // allow known tokens in.
+ try {
+ ozoneManager.checkLeaderStatus();
+ } catch (OMNotLeaderException | OMLeaderNotReadyException e) {
+ InvalidToken wrappedStandby = new InvalidToken("IOException");
+ wrappedStandby.initCause(e);
+ throw wrappedStandby;
+ }
Review Comment:
Hi @ChenSammi. I see that `retrievePassword` method now not only throws an exception when the s3 user cannot be authenticated but now also when the OM is not the leader. With this new InvaidToken exception (OMNotLeader / NotReady exception), the users of the retrievePassword may not accurately process the error.
Do we need to change the `S3SecurityUtil.validateS3Credential` handling of an exception thrown by its call to `retrievePassword`?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerProtocolServerSideTranslatorPB.java:
##########
@@ -158,7 +158,7 @@ private OMResponse processRequest(OMRequest request) throws
// if current OM is leader and then proceed with processing the request.
if (request.hasS3Authentication()) {
s3Auth = true;
- checkLeaderStatus();
+ OzoneManagerRatisUtils.checkLeaderStatus(ozoneManager);
S3SecurityUtil.validateS3Credential(request, ozoneManager);
}
Review Comment:
With `validateS3Credential()` checking the leader status when validating the password (S3Token) from the `OMRequest`, is the `OzoneManagerRatisUtils.checkStatus()` in the line above it unnecessary, redundant?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] neils-dev commented on a diff in pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
neils-dev commented on code in PR #3382:
URL: https://github.com/apache/ozone/pull/3382#discussion_r889298428
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OMInterServiceProtocolServerSideImpl.java:
##########
@@ -60,7 +64,7 @@ public BootstrapOMResponse bootstrap(RpcController controller,
.build();
}
- checkLeaderStatus();
+ OzoneManagerRatisUtils.checkLeaderStatus(ozoneManager);
Review Comment:
Thanks @ChenSammi. Looks great. I tested the changes on the secure docker development clusters as well without issues - clients handle OMException raised as expected for authentication errors (S3) and client failovers for raised ServiceException wrapped OMNotALeader exceptions.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] neils-dev commented on a diff in pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
neils-dev commented on code in PR #3382:
URL: https://github.com/apache/ozone/pull/3382#discussion_r890255127
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OMInterServiceProtocolServerSideImpl.java:
##########
@@ -60,7 +64,7 @@ public BootstrapOMResponse bootstrap(RpcController controller,
.build();
}
- checkLeaderStatus();
+ OzoneManagerRatisUtils.checkLeaderStatus(ozoneManager);
Review Comment:
LGTM
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] neils-dev commented on a diff in pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
neils-dev commented on code in PR #3382:
URL: https://github.com/apache/ozone/pull/3382#discussion_r872580918
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerProtocolServerSideTranslatorPB.java:
##########
@@ -158,7 +158,7 @@ private OMResponse processRequest(OMRequest request) throws
// if current OM is leader and then proceed with processing the request.
if (request.hasS3Authentication()) {
s3Auth = true;
- checkLeaderStatus();
+ OzoneManagerRatisUtils.checkLeaderStatus(ozoneManager);
S3SecurityUtil.validateS3Credential(request, ozoneManager);
}
Review Comment:
Hi @ChenSammi, thanks for the comment and for the help you've given me when I started with ozone and ozone development.
> With HDDS-4440 and HDDS-5881, S3 authentication will be carried as part of the OMRequest.
Yes, authentication is done using the `S3Authentication` fields of the `OMRequest`. This` processRequest()` in the `ProtocalServerSideTranslatorPB` is part of the path for processing requests with the authentication through `S3SecurityUtil.validateS3Credential`.
> and these requests will not be processed by SASL. So the leader check is still needed here.
Yes, the leader check is needed here, however isn't the leader check done along with the call to `S3SecurityUtil.validateS3Credential`s as well? So in this code path the leader is checked twice. Once just prior to the call to `S3SecurityUtil.ValidateS3Credential` and once inside the call (within `retrievePassword`). Is the call to `OzoneManagerRatisUtils.checkLeaderStatus(ozoneManager)` still needed?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] adoroszlai commented on pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on PR #3382:
URL: https://github.com/apache/ozone/pull/3382#issuecomment-1118437349
> The failed flaky test is irrelevant.
May be irrelevant here, but please see https://github.com/apache/ozone/pull/3376#issuecomment-1118161635 and below.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] ChenSammi merged pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
ChenSammi merged PR #3382:
URL: https://github.com/apache/ozone/pull/3382
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] neils-dev commented on a diff in pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
neils-dev commented on code in PR #3382:
URL: https://github.com/apache/ozone/pull/3382#discussion_r877482887
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OMInterServiceProtocolServerSideImpl.java:
##########
@@ -60,7 +64,7 @@ public BootstrapOMResponse bootstrap(RpcController controller,
.build();
}
- checkLeaderStatus();
+ OzoneManagerRatisUtils.checkLeaderStatus(ozoneManager);
Review Comment:
Hi @ChenSammi, thanks for your updated comment on handling the leader exceptions.
> All protocol service API need throws ServiceException, while ozoneManager.checkLeaderStatus throws OMNotLeaderException and OMLeaderNotReadyException. So we need OzoneManagerRatisUtils.checkLeaderStatus wrapper to covert exceptions.
Having that, the `OzoneManagerProtocolServerServerSideTranslatorPB.processRequest` needs to throw a `ServiceException` should `isRatisEnabled == true` and it is _not_ a leader. There looks to be two cases where this happens in `processRequest`:
1. For the case that `isRatisEnabled == true` and `s3Auth == false`, that's what happens with your leader check `OzoneManagerRatisUtils.checkLeaderStatus`. It is handled.
2. For the case that `isRatisEnabled == true` and `request.hasS3Authentication == true` it needs to be handled so that the `ServiceException` is thrown in the event of a leadership check exception. This looks to be currently wrapping the leadership exception in an IOException (`InvalidToken`) and returned to the caller through the error `OmResponse`. Does that look to be the case? If so, instead it needs to throw a leader exception wrapped in a `ServiceException` for the caller. If we are not throwing the `ServiceException`, would you like to insert the `OzoneManagerRatisUtils.checkLeaderStatus` to do so? Or have the `IOException` handler for `processRequest` (catch block) wrap the extracted` Leader exception` into a `ServiceException` and throw?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] ChenSammi commented on pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #3382:
URL: https://github.com/apache/ozone/pull/3382#issuecomment-1118433988
Hey @nandakumar131 , would you help to review the patch? The failed flaky test is irrelevant. They can pass locally.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] kerneltime commented on a diff in pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on code in PR #3382:
URL: https://github.com/apache/ozone/pull/3382#discussion_r872594052
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OMInterServiceProtocolServerSideImpl.java:
##########
@@ -60,7 +64,7 @@ public BootstrapOMResponse bootstrap(RpcController controller,
.build();
}
- checkLeaderStatus();
+ OzoneManagerRatisUtils.checkLeaderStatus(ozoneManager);
Review Comment:
Why go through this nesting? alternatively we could `ozoneManager.checkLeaderStatus`
```suggestion
ozoneManager.checkLeaderStatus();
```
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] neils-dev commented on a diff in pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
neils-dev commented on code in PR #3382:
URL: https://github.com/apache/ozone/pull/3382#discussion_r871884747
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerProtocolServerSideTranslatorPB.java:
##########
@@ -158,7 +158,7 @@ private OMResponse processRequest(OMRequest request) throws
// if current OM is leader and then proceed with processing the request.
if (request.hasS3Authentication()) {
s3Auth = true;
- checkLeaderStatus();
+ OzoneManagerRatisUtils.checkLeaderStatus(ozoneManager);
S3SecurityUtil.validateS3Credential(request, ozoneManager);
}
Review Comment:
With `validateS3Credential()` checking the leader status when validating the password (S3Token) from the `OMRequest`, is the `OzoneManagerRatisUtils.checkLeaderStatus()` in the line above it unnecessary, redundant?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] ChenSammi commented on pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #3382:
URL: https://github.com/apache/ozone/pull/3382#issuecomment-1148120899
Thanks @adoroszlai @kerneltime @neils-dev @swagle for the code review.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] ChenSammi commented on a diff in pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on code in PR #3382:
URL: https://github.com/apache/ozone/pull/3382#discussion_r872298893
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/security/OzoneDelegationTokenSecretManager.java:
##########
@@ -404,6 +415,18 @@ public void removeToken(OzoneTokenIdentifier ozoneTokenIdentifier) {
@Override
public byte[] retrievePassword(OzoneTokenIdentifier identifier)
throws InvalidToken {
+ // Tokens are a bit different in that a follower OM may be behind and
+ // thus not yet know of all tokens issued by the leader OM. the
+ // following check does not allow ANY token auth. In optimistic, it should
+ // allow known tokens in.
+ try {
+ ozoneManager.checkLeaderStatus();
+ } catch (OMNotLeaderException | OMLeaderNotReadyException e) {
+ InvalidToken wrappedStandby = new InvalidToken("IOException");
+ wrappedStandby.initCause(e);
+ throw wrappedStandby;
+ }
Review Comment:
Yes. Will take care of that in next patch.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] ChenSammi commented on a diff in pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on code in PR #3382:
URL: https://github.com/apache/ozone/pull/3382#discussion_r873403723
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerProtocolServerSideTranslatorPB.java:
##########
@@ -158,7 +158,7 @@ private OMResponse processRequest(OMRequest request) throws
// if current OM is leader and then proceed with processing the request.
if (request.hasS3Authentication()) {
s3Auth = true;
- checkLeaderStatus();
+ OzoneManagerRatisUtils.checkLeaderStatus(ozoneManager);
S3SecurityUtil.validateS3Credential(request, ozoneManager);
}
Review Comment:
Right. Will remove it.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] ChenSammi commented on a diff in pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on code in PR #3382:
URL: https://github.com/apache/ozone/pull/3382#discussion_r873408778
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OMInterServiceProtocolServerSideImpl.java:
##########
@@ -60,7 +64,7 @@ public BootstrapOMResponse bootstrap(RpcController controller,
.build();
}
- checkLeaderStatus();
+ OzoneManagerRatisUtils.checkLeaderStatus(ozoneManager);
Review Comment:
All protocol service API need throws ServiceException, while ozoneManager.checkLeaderStatus throws OMNotLeaderException and OMLeaderNotReadyException. So we need OzoneManagerRatisUtils.checkLeaderStatus wrapper to covert exceptions.
ozoneManager.checkLeaderStatus is used both in SASL path authentication and protocol level authentication. It throws the core exception. Let the caller wrap the core exception into desired outer exceptions.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] kerneltime commented on a diff in pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on code in PR #3382:
URL: https://github.com/apache/ozone/pull/3382#discussion_r872596745
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/utils/OzoneManagerRatisUtils.java:
##########
@@ -401,43 +399,13 @@ public static String getOMRatisSnapshotDirectory(ConfigurationSource conf) {
return snapshotDir;
}
- public static void checkLeaderStatus(RaftServerStatus raftServerStatus,
- RaftPeerId raftPeerId) throws ServiceException {
- switch (raftServerStatus) {
- case LEADER_AND_READY: return;
-
- case LEADER_AND_NOT_READY: throw createLeaderNotReadyException(raftPeerId);
-
- case NOT_LEADER: throw createNotLeaderException(raftPeerId);
-
- default: throw new IllegalStateException(
- "Unknown Ratis Server state: " + raftServerStatus);
+ public static void checkLeaderStatus(OzoneManager ozoneManager)
Review Comment:
I think we can drop this method.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] kerneltime commented on a diff in pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on code in PR #3382:
URL: https://github.com/apache/ozone/pull/3382#discussion_r872595147
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerProtocolServerSideTranslatorPB.java:
##########
@@ -158,7 +158,7 @@ private OMResponse processRequest(OMRequest request) throws
// if current OM is leader and then proceed with processing the request.
if (request.hasS3Authentication()) {
s3Auth = true;
- checkLeaderStatus();
+ OzoneManagerRatisUtils.checkLeaderStatus(ozoneManager);
Review Comment:
```suggestion
ozoneManager.checkLeaderStatus();
```
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] kerneltime commented on a diff in pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on code in PR #3382:
URL: https://github.com/apache/ozone/pull/3382#discussion_r872593290
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OMInterServiceProtocolServerSideImpl.java:
##########
@@ -60,7 +64,7 @@ public BootstrapOMResponse bootstrap(RpcController controller,
.build();
}
- checkLeaderStatus();
+ OzoneManagerRatisUtils.checkLeaderStatus(ozoneManager);
Review Comment:
Why do through this nesting why not check with `ozoneManager.checkLeaderStatus`
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] neils-dev commented on a diff in pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
neils-dev commented on code in PR #3382:
URL: https://github.com/apache/ozone/pull/3382#discussion_r877482887
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OMInterServiceProtocolServerSideImpl.java:
##########
@@ -60,7 +64,7 @@ public BootstrapOMResponse bootstrap(RpcController controller,
.build();
}
- checkLeaderStatus();
+ OzoneManagerRatisUtils.checkLeaderStatus(ozoneManager);
Review Comment:
Hi @ChenSammi, thanks for your updated comment on handling the leader exceptions.
> All protocol service API need throws ServiceException, while ozoneManager.checkLeaderStatus throws OMNotLeaderException and OMLeaderNotReadyException. So we need OzoneManagerRatisUtils.checkLeaderStatus wrapper to covert exceptions.
Having that, the `OzoneManagerProtocolServerServerSideTranslatorPB.processRequest` needs to throw a `ServiceException` should `isRatisEnabled == true` and it is _not_ a leader. There looks to be two cases where this happens in `processRequest`:
1. For the case that `isRatisEnabled == true` and `s3Auth == false`, that's what happens with your leader check `OzoneManagerRatisUtils.checkLeaderStatus`. It is handled.
2. For the case that `isRatisEnabled == true` and `request.hasS3Authentication == true` it needs to be handled so that the `ServiceException` is thrown in the event of a leadership check exception. ~~This looks to be currently wrapping the leadership exception in an IOException (`InvalidToken`) and returned to the caller through the error `OmResponse`.~~ It is handled by the validateS3Credential (missed it on initial read). Thanks.
The javadoc for` validateS3Credential` can be updated to include the `ServiceException` that may be thrown.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] neils-dev commented on pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
neils-dev commented on PR #3382:
URL: https://github.com/apache/ozone/pull/3382#issuecomment-1126577076
> > The failed flaky test is irrelevant.
>
> May be irrelevant here, but please see [#3376 (comment)](https://github.com/apache/ozone/pull/3376#issuecomment-1118161635) and below.
Applying Ratis patch [RATIS-1481], I've been able to run the `TestOzonePrepare#testPrepareDownedOM` without failure. see comment https://github.com/apache/ozone/pull/3186#issuecomment-1121542574.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] ChenSammi commented on a diff in pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on code in PR #3382:
URL: https://github.com/apache/ozone/pull/3382#discussion_r875627793
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OMInterServiceProtocolServerSideImpl.java:
##########
@@ -60,7 +64,7 @@ public BootstrapOMResponse bootstrap(RpcController controller,
.build();
}
- checkLeaderStatus();
+ OzoneManagerRatisUtils.checkLeaderStatus(ozoneManager);
Review Comment:
@neils-dev, could you help to take another look?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] neils-dev commented on a diff in pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
neils-dev commented on code in PR #3382:
URL: https://github.com/apache/ozone/pull/3382#discussion_r890318520
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OMInterServiceProtocolServerSideImpl.java:
##########
@@ -60,7 +64,7 @@ public BootstrapOMResponse bootstrap(RpcController controller,
.build();
}
- checkLeaderStatus();
+ OzoneManagerRatisUtils.checkLeaderStatus(ozoneManager);
Review Comment:
+1
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] ChenSammi commented on a diff in pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on code in PR #3382:
URL: https://github.com/apache/ozone/pull/3382#discussion_r871980621
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerProtocolServerSideTranslatorPB.java:
##########
@@ -158,7 +158,7 @@ private OMResponse processRequest(OMRequest request) throws
// if current OM is leader and then proceed with processing the request.
if (request.hasS3Authentication()) {
s3Auth = true;
- checkLeaderStatus();
+ OzoneManagerRatisUtils.checkLeaderStatus(ozoneManager);
S3SecurityUtil.validateS3Credential(request, ozoneManager);
}
Review Comment:
Correct me if I'm wrong. With HDDS-4440 and HDDS-5881, S3 authentication will be carried as part of the OMRequest. It doesn't leverage the underline hadoop RPC SASL authentication, and these requests will not be processed by SASL. So the leader check is still needed here.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] neils-dev commented on a diff in pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
neils-dev commented on code in PR #3382:
URL: https://github.com/apache/ozone/pull/3382#discussion_r886926121
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OMInterServiceProtocolServerSideImpl.java:
##########
@@ -60,7 +64,7 @@ public BootstrapOMResponse bootstrap(RpcController controller,
.build();
}
- checkLeaderStatus();
+ OzoneManagerRatisUtils.checkLeaderStatus(ozoneManager);
Review Comment:
Thanks @ChenSammi for following up. Yes, just the javadoc addition for correctness. LGTM. Thanks.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] ChenSammi commented on a diff in pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on code in PR #3382:
URL: https://github.com/apache/ozone/pull/3382#discussion_r888577008
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OMInterServiceProtocolServerSideImpl.java:
##########
@@ -60,7 +64,7 @@ public BootstrapOMResponse bootstrap(RpcController controller,
.build();
}
- checkLeaderStatus();
+ OzoneManagerRatisUtils.checkLeaderStatus(ozoneManager);
Review Comment:
@neils-dev , the javadoc is updated. Would you like to take another look?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] neils-dev commented on a diff in pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
neils-dev commented on code in PR #3382:
URL: https://github.com/apache/ozone/pull/3382#discussion_r890255127
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OMInterServiceProtocolServerSideImpl.java:
##########
@@ -60,7 +64,7 @@ public BootstrapOMResponse bootstrap(RpcController controller,
.build();
}
- checkLeaderStatus();
+ OzoneManagerRatisUtils.checkLeaderStatus(ozoneManager);
Review Comment:
LGTM (I am unable to merge.)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] adoroszlai commented on pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on PR #3382:
URL: https://github.com/apache/ozone/pull/3382#issuecomment-1124698856
@neils-dev if you would like to review
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
[GitHub] [ozone] ChenSammi commented on a diff in pull request #3382: HDDS-6686. Do Leadship check before SASL token verification.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on code in PR #3382:
URL: https://github.com/apache/ozone/pull/3382#discussion_r872298893
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/security/OzoneDelegationTokenSecretManager.java:
##########
@@ -404,6 +415,18 @@ public void removeToken(OzoneTokenIdentifier ozoneTokenIdentifier) {
@Override
public byte[] retrievePassword(OzoneTokenIdentifier identifier)
throws InvalidToken {
+ // Tokens are a bit different in that a follower OM may be behind and
+ // thus not yet know of all tokens issued by the leader OM. the
+ // following check does not allow ANY token auth. In optimistic, it should
+ // allow known tokens in.
+ try {
+ ozoneManager.checkLeaderStatus();
+ } catch (OMNotLeaderException | OMLeaderNotReadyException e) {
+ InvalidToken wrappedStandby = new InvalidToken("IOException");
+ wrappedStandby.initCause(e);
+ throw wrappedStandby;
+ }
Review Comment:
Make sense. Will take care of that in next patch.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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