fapifta commented on code in PR #5163:
URL: https://github.com/apache/ozone/pull/5163#discussion_r1360424616
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/utils/SelfSignedCertificate.java:
##########
@@ -70,7 +70,7 @@
* provided.
*/
public final class SelfSignedCertificate {
- private static final String NAME_FORMAT = "CN=%s,OU=%s,O=%s";
+ private static final String NAME_FORMAT = "CN=%s,OU=%s,O=%s,SERIALNUMBER=%s";
Review Comment:
Is there a particular reason to use a separate constant for calculating the
DN here? Can we use the one from CertificateSignRequest instead?
Looking at this class a bit deeper, probably we should leave this as it is
for now, and work on the CertificateSignRequest and SelfSignedCertificate
classes later to pull up the common parts into a common base class for these
two. I filed [HDDS-9467](https://issues.apache.org/jira/browse/HDDS-9467) to
follow up on this one.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/protocolPB/SCMSecurityProtocolClientSideTranslatorPB.java:
##########
@@ -207,8 +207,10 @@ public String getSCMCertificate(ScmNodeDetailsProto
scmNodeDetails,
* @return String - pem encoded SCM signed
* certificate.
*/
+ @Override
public String getSCMCertificate(ScmNodeDetailsProto scmNodeDetails,
- String certSignReq, boolean renew) throws IOException {
+ String certSignReq, boolean renew)
+ throws IOException {
Review Comment:
Whitspace change.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/protocolPB/SCMSecurityProtocolClientSideTranslatorPB.java:
##########
@@ -222,8 +224,8 @@ public String getSCMCertificate(ScmNodeDetailsProto
scmNodeDetails,
* signed certificate and root CA certificate.
*/
public SCMGetCertResponseProto getSCMCertChain(
- ScmNodeDetailsProto scmNodeDetails, String certSignReq, boolean isRenew)
- throws IOException {
+ ScmNodeDetailsProto scmNodeDetails, String certSignReq,
+ boolean isRenew) throws IOException {
Review Comment:
Whitspace change.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMSecurityProtocolServer.java:
##########
@@ -341,14 +356,15 @@ public String getSCMCertificate(ScmNodeDetailsProto
scmNodeDetails,
* @throws IOException
*/
private synchronized String getEncodedCertToString(String certSignReq,
- NodeType nodeType) throws IOException {
+ NodeType nodeType, String certSerialId) throws IOException {
Review Comment:
Wouldn't it be easier to just call the getNextCertificateId() here instead
of passing on the evaluation of the method from every places from where we call
into this method?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/protocol/SCMSecurityProtocolServerSideTranslatorPB.java:
##########
@@ -159,7 +159,6 @@ public SCMSecurityResponse
processRequest(SCMSecurityRequest request)
.setRemoveExpiredCertificatesResponseProto(
removeExpiredCertificates())
.build();
-
Review Comment:
Whitespace change.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java:
##########
@@ -69,7 +69,12 @@ public class SequenceIdGenerator {
public static final String LOCAL_ID = "localId";
public static final String DEL_TXN_ID = "delTxnId";
public static final String CONTAINER_ID = "containerId";
+
+ @Deprecated
Review Comment:
I am not sure, so let me ask, do we need to keep this one in the code? I
know it is in the database, but this one is a value in the DB that we never
read back in the current code, so intuition of mine suggests that we can remove
it, however I might have a blind spot here.
Probably the only problem is that we might have a value in some RocksDB,
that lingers there, I guess if this is the only problem, then we should file a
JIRA to cleanup the DB in an UpgradeAction after a metadata layout version
change and get rid of this constant at the same time. If there are no other
reasons, let's create that JIRA under
[HDDS-7335](https://issues.apache.org/jira/browse/HDDS-7335).
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/protocol/SCMSecurityProtocol.java:
##########
@@ -87,7 +87,8 @@ String getSCMCertificate(ScmNodeDetailsProto scmNodeDetails,
* certificate.
*/
String getSCMCertificate(ScmNodeDetailsProto scmNodeDetails,
- String certSignReq, boolean isRenew) throws IOException;
+ String certSignReq, boolean isRenew)
Review Comment:
Can we avoid these whitespace changes from this PR? There are a few more,
and I guess it comes from some refactoring where first a parameter was added,
then removed as the implementation evolved over time.
I don't necessarily mind if these stay, but it would make the review easier.
I tag the places with "Whitspace change.".
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/utils/CertificateSignRequest.java:
##########
@@ -73,8 +73,11 @@
* PKCS10CertificationRequest to CertificateServer.
*/
public final class CertificateSignRequest {
- // Ozone Certificate distinguished format: (CN=Subject,OU=ScmID,O=ClusterID).
+ // Ozone Certificate distinguished format:
+ // (CN=Subject,OU=ScmID,O=ClusterID,SERIALNUMBER=SerialID).
private static final String DISTINGUISHED_NAME_FORMAT = "CN=%s,OU=%s,O=%s";
Review Comment:
The DISTINGUISHED_NAME_FORMAT constant is used onlu in the
getDistingiuishedName(String, String, String) method as of now, and that method
is not used anywhere in the code as far as I can see, so I believe we should
remove the constant and the method as well, however it is even better if we
just do not introduce the new constant and new method, but used the old names
with the new value.
What do you think? Do I miss something here? Actually... I am not sure,
but... Isn't this name format needed for handling previously issued
certificates somewhere in the code?
--
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]
