Galsza commented on code in PR #3930:
URL: https://github.com/apache/ozone/pull/3930#discussion_r1013953345
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsConfigKeys.java:
##########
@@ -184,6 +184,16 @@ public final class HddsConfigKeys {
// Default Certificate duration to one year.
public static final String HDDS_X509_DEFAULT_DURATION_DEFAULT = "P365D";
+ /**
+ * Duration in days within which before the certificate expiration date the
Review Comment:
This description is hard to understand (at least to me), I'd prefer
something like this: "Duration of the grace period in days in which a
certificate should be removed before the current one expires."
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
@@ -647,7 +650,8 @@ protected enum InitCase {
PRIVATE_KEY,
PRIVATEKEY_CERT,
PUBLICKEY_PRIVATEKEY,
- ALL
+ ALL,
+ EXPIRES
Review Comment:
Please change the documentation of this method to match the current state.
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/x509/certificate/client/TestDefaultCertificateClient.java:
##########
@@ -474,4 +484,46 @@ public void testInitCertAndKeypairValidationFailures()
throws Exception {
omClientLog.clearOutput();
}
+ @SuppressWarnings("checkstyle:LeftCurly")
Review Comment:
Is this still needed?
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/SCMCertificateClient.java:
##########
@@ -60,6 +60,11 @@ public SCMCertificateClient(SecurityConfig securityConfig,
super(securityConfig, LOG, certSerialId, component);
}
+ @Override
+ protected boolean handleExpiration() {
Review Comment:
I would add a comment here or to DefaultCertificateClient to explain that we
don't want to reinit SCM certs because they are the CA.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
@@ -705,13 +708,28 @@ public synchronized InitResponse init() throws
CertificateException {
if (certificate != null) {
initCase = initCase | 1;
}
+
+ Calendar shouldRenewAfter = Calendar.getInstance();
+ shouldRenewAfter
+ .add(Calendar.DAY_OF_YEAR, securityConfig.getRenewalGraceDays());
+ if (initCase == InitCase.ALL.ordinal() &&
Review Comment:
Could we use an enumset for initcase instead of a bitfield? The current
solution is very fragile if the InitCase enum ever changes and the code is hard
to read as well.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
@@ -774,6 +797,19 @@ protected InitResponse handleCase(InitCase init)
}
}
+ protected void removeMaterial() throws CertificateException {
+ try {
+ FileUtils.deleteDirectory(
+ securityConfig.getKeyLocation(component).toFile());
Review Comment:
Are these directories only containing only exactly the expired
public/private key and the certificate itself?
--
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]
