-
Notifications
You must be signed in to change notification settings - Fork 588
HDDS-6686. Do Leadship check before SASL token verification. #3382
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -20,6 +20,8 @@ | |||||
| import com.google.protobuf.RpcController; | ||||||
| import com.google.protobuf.ServiceException; | ||||||
| import java.io.IOException; | ||||||
|
|
||||||
| import org.apache.hadoop.ozone.om.OzoneManager; | ||||||
| import org.apache.hadoop.ozone.om.helpers.OMNodeDetails; | ||||||
| import org.apache.hadoop.ozone.om.protocolPB.OMInterServiceProtocolPB; | ||||||
| import org.apache.hadoop.ozone.om.ratis.OzoneManagerRatisServer; | ||||||
|
|
@@ -38,9 +40,11 @@ public class OMInterServiceProtocolServerSideImpl implements | |||||
|
|
||||||
| private final OzoneManagerRatisServer omRatisServer; | ||||||
| private final boolean isRatisEnabled; | ||||||
| private final OzoneManager ozoneManager; | ||||||
|
|
||||||
| public OMInterServiceProtocolServerSideImpl( | ||||||
| public OMInterServiceProtocolServerSideImpl(OzoneManager ozoneMgr, | ||||||
| OzoneManagerRatisServer ratisServer, boolean enableRatis) { | ||||||
| this.ozoneManager = ozoneMgr; | ||||||
| this.omRatisServer = ratisServer; | ||||||
| this.isRatisEnabled = enableRatis; | ||||||
| } | ||||||
|
|
@@ -60,7 +64,7 @@ public BootstrapOMResponse bootstrap(RpcController controller, | |||||
| .build(); | ||||||
| } | ||||||
|
|
||||||
| checkLeaderStatus(); | ||||||
| OzoneManagerRatisUtils.checkLeaderStatus(ozoneManager); | ||||||
|
||||||
| OzoneManagerRatisUtils.checkLeaderStatus(ozoneManager); | |
| ozoneManager.checkLeaderStatus(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@neils-dev, could you help to take another look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
- For the case that
isRatisEnabled == trueands3Auth == false, that's what happens with your leader checkOzoneManagerRatisUtils.checkLeaderStatus. It is handled. - For the case that
isRatisEnabled == trueandrequest.hasS3Authentication == trueit needs to be handled so that theServiceExceptionis thrown in the event of a leadership check exception.This looks to be currently wrapping the leadership exception in an IOException (It is handled by the validateS3Credential (missed it on initial read). Thanks.InvalidToken) and returned to the caller through the errorOmResponse.
The javadoc for validateS3Credential can be updated to include the ServiceException that may be thrown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@neils-dev , the javadoc is updated. Would you like to take another look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @neils-dev . Do you think the PR can be merged now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (I am unable to merge.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -160,7 +160,6 @@ private OMResponse processRequest(OMRequest request) throws | |
| // if current OM is leader and then proceed with processing the request. | ||
| if (request.hasS3Authentication()) { | ||
| s3Auth = true; | ||
| checkLeaderStatus(); | ||
| S3SecurityUtil.validateS3Credential(request, ozoneManager); | ||
| } | ||
|
||
| } catch (IOException ex) { | ||
|
|
@@ -181,7 +180,7 @@ private OMResponse processRequest(OMRequest request) throws | |
| // To validate credentials we have already verified leader status. | ||
| // This will skip of checking leader status again if request has S3Auth. | ||
| if (!s3Auth) { | ||
| checkLeaderStatus(); | ||
| OzoneManagerRatisUtils.checkLeaderStatus(ozoneManager); | ||
| } | ||
| try { | ||
| omClientRequest = | ||
|
|
@@ -267,7 +266,8 @@ private ServiceException createLeaderNotReadyException() { | |
| /** | ||
| * Submits request directly to OM. | ||
| */ | ||
| private OMResponse submitRequestDirectlyToOM(OMRequest request) { | ||
| private OMResponse submitRequestDirectlyToOM(OMRequest request) throws | ||
| ServiceException { | ||
| OMClientResponse omClientResponse = null; | ||
| long index = 0L; | ||
| try { | ||
|
|
@@ -312,11 +312,6 @@ private OMResponse submitRequestDirectlyToOM(OMRequest request) { | |
| return omClientResponse.getOMResponse(); | ||
| } | ||
|
|
||
| private void checkLeaderStatus() throws ServiceException { | ||
| OzoneManagerRatisUtils.checkLeaderStatus(omRatisServer.checkLeaderStatus(), | ||
| omRatisServer.getRaftPeerId()); | ||
| } | ||
|
|
||
| /** | ||
| * Create OMResponse from the specified OMRequest and exception. | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,9 +36,12 @@ | |
| import org.apache.hadoop.hdds.security.x509.exceptions.CertificateException; | ||
| import org.apache.hadoop.io.Text; | ||
| import org.apache.hadoop.ozone.om.OMConfigKeys; | ||
| import org.apache.hadoop.ozone.om.OzoneManager; | ||
| import org.apache.hadoop.ozone.om.S3SecretManager; | ||
| import org.apache.hadoop.ozone.om.S3SecretManagerImpl; | ||
| import org.apache.hadoop.ozone.om.exceptions.OMException; | ||
| import org.apache.hadoop.ozone.om.exceptions.OMLeaderNotReadyException; | ||
| import org.apache.hadoop.ozone.om.exceptions.OMNotLeaderException; | ||
| import org.apache.hadoop.ozone.security.OzoneSecretStore.OzoneManagerSecretState; | ||
| import org.apache.hadoop.ozone.security.OzoneTokenIdentifier.TokenInfo; | ||
| import org.apache.hadoop.security.AccessControlException; | ||
|
|
@@ -71,6 +74,7 @@ public class OzoneDelegationTokenSecretManager | |
| private final long tokenRemoverScanInterval; | ||
| private String omCertificateSerialId; | ||
| private String omServiceId; | ||
| private final OzoneManager ozoneManager; | ||
|
|
||
| /** | ||
| * If the delegation token update thread holds this lock, it will not get | ||
|
|
@@ -94,6 +98,7 @@ public OzoneDelegationTokenSecretManager(Builder b) throws IOException { | |
| this.s3SecretManager = (S3SecretManagerImpl) b.s3SecretManager; | ||
| this.store = new OzoneSecretStore(b.ozoneConf, | ||
| this.s3SecretManager.getOmMetadataManager()); | ||
| this.ozoneManager = b.ozoneManager; | ||
| isRatisEnabled = b.ozoneConf.getBoolean( | ||
| OMConfigKeys.OZONE_OM_RATIS_ENABLE_KEY, | ||
| OMConfigKeys.OZONE_OM_RATIS_ENABLE_DEFAULT); | ||
|
|
@@ -113,6 +118,7 @@ public static class Builder { | |
| private S3SecretManager s3SecretManager; | ||
| private CertificateClient certClient; | ||
| private String omServiceId; | ||
| private OzoneManager ozoneManager; | ||
|
|
||
| public OzoneDelegationTokenSecretManager build() throws IOException { | ||
| return new OzoneDelegationTokenSecretManager(this); | ||
|
|
@@ -157,6 +163,11 @@ public Builder setOmServiceId(String serviceId) { | |
| this.omServiceId = serviceId; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setOzoneManager(OzoneManager ozoneMgr) { | ||
| this.ozoneManager = ozoneMgr; | ||
| return this; | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -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; | ||
| } | ||
|
||
|
|
||
| if (identifier.getTokenType().equals(S3AUTHINFO)) { | ||
| return validateS3AuthInfo(identifier); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can drop this method.