duongkame commented on code in PR #4747:
URL: https://github.com/apache/ozone/pull/4747#discussion_r1203459147
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HddsServerUtil.java:
##########
@@ -511,6 +512,18 @@ public static SCMSecurityProtocol getScmSecurityClient(
SCMSecurityProtocol.class, conf);
}
+ public static SecretKeyProtocolScm getScmSecretClient(
Review Comment:
Add a javadoc to mention it's intended to be used by clients under SCM
identify. Maybe rename it to `getSecretKeyClientForSCM` to make the intention
clear.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/client/ScmClient.java:
##########
@@ -362,6 +363,15 @@ StartContainerBalancerResponseProto startContainerBalancer(
*/
List<String> getScmRatisRoles() throws IOException;
+ /**
+ * Force generates new secret keys (rotate).
+ *
+ * @param force boolean flag that forcefully rotates the key on demand
+ * @return
+ * @throws TimeoutException
+ */
+ boolean checkAndRotate(boolean force) throws TimeoutException, IOException;
Review Comment:
nit: maybe name it `rotateSecretKeys`. Note that this class name has no
SecretKey context, so the method name needs it.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/protocol/SecretKeyProtocolServerSideTranslatorPB.java:
##########
@@ -102,6 +104,16 @@ public SCMSecretKeyResponse
processRequest(SCMSecretKeyRequest request)
.setSecretKeysListResponseProto(getAllSecretKeys())
.build();
+ case GetCheckAndRotate:
+ try {
+ return scmSecurityResponse
+ .setCheckAndRotateResponseProto(
+
checkAndRotate(request.getCheckAndRotateRequest().getForce()))
+ .build();
+ } catch (TimeoutException e) {
+ LOG.error("Timeout occurred while executing checkAndRotate.", e);
Review Comment:
See the exception handling several lines down. I think this timeout error
can be considered an internal error and get transferred to client side.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestBlockTokens.java:
##########
@@ -377,6 +386,23 @@ private static void setSecureConfig() throws IOException {
ozoneKeytab.getAbsolutePath());
}
+ @Test
+ public void testRotateKeySCMAdminCommand()
Review Comment:
I think we need to move this test to a new test class. `TestBlockToken` is
intended for block token test-cases and configure a relatively short secret key
lifetime. This CLI test should be done with a longer rotation duration, e.g.
1h, so that the CLI and the automatic rotation don't collide.
##########
hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerOperationClient.java:
##########
@@ -124,6 +129,16 @@ public static StorageContainerLocationProtocol
newContainerRpcClient(
return HAUtils.getScmContainerClient(configSource);
}
+ public static SecretKeyProtocolScm newSecretKeyClient(
+ ConfigurationSource configSource) {
+ try {
+ return HddsServerUtil.getScmSecretClient(configSource);
+ } catch (IOException e) {
+ LOG.error("Error while getting the SCM secret client.", e);
Review Comment:
Just throw, let the client code (the CLI catch and log it).
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMSecurityProtocolServer.java:
##########
@@ -232,6 +233,16 @@ private void validateSecretKeyStatus() throws
SCMSecretKeyException {
}
}
+ @Override
+ public boolean checkAndRotate(boolean force) throws TimeoutException {
+ try {
+ validateSecretKeyStatus();
+ } catch (SCMSecretKeyException e) {
+ LOGGER.error("Error while validating the secret key status.", e);
Review Comment:
Don't catch these validation exceptions on server side. We should let the
translator to translate them to error code and transfer to client side.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
