fapifta commented on code in PR #6802:
URL: https://github.com/apache/ozone/pull/6802#discussion_r1637294330
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/authority/DefaultCAServer.java:
##########
@@ -223,70 +219,50 @@ public Future<CertPath> requestCertificate(
CertificateApprover.ApprovalType approverType, NodeType role,
String certSerialId) {
LocalDateTime beginDate = LocalDateTime.now();
- LocalDateTime endDate;
- // When issuing certificates for sub-ca use the max certificate duration
- // similar to self signed root certificate.
- if (role == NodeType.SCM) {
- endDate = beginDate.plus(config.getMaxCertificateDuration());
- } else {
- endDate = beginDate.plus(config.getDefaultCertDuration());
- }
-
- CompletableFuture<X509CertificateHolder> xcertHolder =
- approver.inspectCSR(csr);
+ LocalDateTime endDate = expiryFor(beginDate, role);
- CompletableFuture<CertPath> xCertHolders
- = new CompletableFuture<>();
-
- if (xcertHolder.isCompletedExceptionally()) {
- // This means that approver told us there are things which it disagrees
- // with in this Certificate Request. Since the first set of sanity
- // checks failed, we just return the future object right here.
Review Comment:
I don't really a fan of this comment, as it does not really tell anything
new or useful. If you look at what the code does before (if something completed
exceptionally we complete exceptionally), and look at this description than it
is easier to figure out what failure causes us to fail than just from the code,
however after renaming the variables, it is fairly visible that if the
inspection of the CSR is not successful, we complete exceptionally here.
Let me know if you agree, if you still think it is useful to have a comment
here about what is happening, I can be open to it, however if you look at the
DefaultApprover APIDoc, it pretty much describes in detail what it does, though
the doc is outdated when you look at to the code (at least for me it seems).
On the other hand, we swallow a useful exception here... I will take a look
tomorrow to the Approver API docs and I will provide a change here that
propagates the currently swallowed exception together with that (I don't know
how I did that) extra space removal.
--
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]
