dombizita commented on code in PR #6725:
URL: https://github.com/apache/ozone/pull/6725#discussion_r1630913991
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/authority/DefaultCAServer.java:
##########
@@ -128,10 +124,8 @@ public class DefaultCAServer implements CertificateServer {
*/
private PKIProfile profile;
private CertificateApprover approver;
- private CRLApprover crlApprover;
private CertificateStore store;
private Lock lock;
- private static boolean testSecureFlag;
Review Comment:
This was used only in the `TestDelegationToken` and in the
`TestSecureOzoneCluster` test classes, but nothing happened based on them, if I
understand it correctly, right? That's why we can remove it?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMSecurityProtocolServer.java:
##########
@@ -439,28 +424,23 @@ public String getCACertificate() throws IOException {
}
/**
- *
- * @param role - node role: OM/SCM/DN.
- * @param startSerialId - start certificate serial id.
- * @param count - max number of certificates returned in a batch.
- * @param isRevoked - whether list for revoked certs only.
+ * @param role - node role: OM/SCM/DN.
+ * @param startSerialId - start certificate serial id.
+ * @param count - max number of certificates returned in a batch.
* @return
* @throws IOException
*/
@Override
public List<String> listCertificate(NodeType role,
- long startSerialId, int count, boolean isRevoked) throws IOException {
- List<X509Certificate> certificates =
- scmCertificateServer.listCertificate(role, startSerialId, count,
- isRevoked);
+ long startSerialId, int count) throws IOException {
+ List<X509Certificate> certificates =
scmCertificateServer.listCertificate(role, startSerialId, count);
List<String> results = new ArrayList<>(certificates.size());
for (X509Certificate cert : certificates) {
try {
String certStr = getPEMEncodedString(cert);
results.add(certStr);
} catch (SCMSecurityException e) {
- throw new SCMSecurityException("listCertificate operation failed.",
- e, e.getErrorCode());
+ throw new SCMSecurityException("listCertificate operation failed.", e,
e.getErrorCode());
Review Comment:
NIT: unnecessary change
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/protocol/SCMSecurityProtocol.java:
##########
@@ -110,14 +109,13 @@ String getSCMCertificate(ScmNodeDetailsProto
scmNodeDetails,
/**
* Get list of certificates meet the query criteria.
*
- * @param type - node type: OM/SCM/DN.
- * @param startSerialId - start certificate serial id.
- * @param count - max number of certificates returned in a batch.
- * @param isRevoked - whether list for revoked certs only.
+ * @param type - node type: OM/SCM/DN.
+ * @param startSerialId - start certificate serial id.
+ * @param count - max number of certificates returned in a batch.
Review Comment:
NIT: maybe remove the tab changes here (and similarly later in the code,
where this same javadoc is there).
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/authority/CertificateStore.java:
##########
@@ -74,40 +66,8 @@ void storeValidScmCertificate(BigInteger serialID,
*/
void checkValidCertID(BigInteger serialID) throws IOException;
-
- /**
- * Adds the certificates to be revoked to a new CRL and moves all the
- * certificates in a transactional manner from valid certificate to
- * revoked certificate state. Returns an empty {@code Optional} instance if
- * the certificates were invalid / not found / already revoked and no CRL
- * was generated. Otherwise, returns the newly generated CRL sequence ID.
- * @param serialIDs - List of Serial IDs of Certificates to be revoked.
- * @param caCertificateHolder - X509 Certificate Holder of the CA.
- * @param reason - CRLReason for revocation.
- * @param revocationTime - Revocation Time for the certificates.
- * @param approver - CRL approver to sign the CRL.
- * @return An empty {@code Optional} instance if no CRL was generated.
- * Otherwise, returns the newly generated CRL sequence ID.
- * @throws IOException - on failure.
- */
- @Replicate
- Optional<Long> revokeCertificates(List<BigInteger> serialIDs,
- X509CertificateHolder caCertificateHolder,
- CRLReason reason,
- Date revocationTime,
- CRLApprover approver)
- throws IOException;
-
- /**
- * Deletes an expired certificate from the store. Please note: We don't
- * remove revoked certificates, we need that information to generate the
- * CRLs.
- * @param serialID - Certificate ID.
- */
- void removeExpiredCertificate(BigInteger serialID) throws IOException;
Review Comment:
Why did you remove this too? Also you didn't remove
`removeAllExpiredCertificates` below.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/TestHddsDatanodeService.java:
##########
@@ -106,22 +103,6 @@ public void setUp() throws IOException {
conf.set(DFSConfigKeysLegacy.DFS_DATANODE_DATA_DIR_KEY, volumeDir);
}
- @Test
- public void testStartup() {
- service.start(conf);
-
- assertNotNull(service.getDatanodeDetails());
- assertNotNull(service.getDatanodeDetails().getHostName());
- assertFalse(service.getDatanodeStateMachine().isDaemonStopped());
- assertNotNull(service.getCRLStore());
-
- service.stop();
- // CRL store must be stopped when the service stops
- assertNull(service.getCRLStore().getStore());
Review Comment:
Why did you remove this whole test case, not just this part?
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/report/TestReportPublisher.java:
##########
@@ -182,66 +170,4 @@ public void testCommandStatusPublisher() throws
InterruptedException {
"Should publish report with 2 status objects");
executorService.shutdown();
}
-
- @Test
- public void testCRLStatusReportPublisher() throws IOException {
- StateContext dummyContext = mock(StateContext.class);
- DatanodeStateMachine dummyStateMachine =
- mock(DatanodeStateMachine.class);
- ReportPublisher publisher = new CRLStatusReportPublisher();
- DatanodeCRLStore dnCrlStore = mock(DatanodeCRLStore.class);
- when(dnCrlStore.getLatestCRLSequenceID()).thenReturn(3L);
- List<CRLInfo> pendingCRLs = new ArrayList<>();
- pendingCRLs.add(mock(CRLInfo.class));
- pendingCRLs.add(mock(CRLInfo.class));
- when(dnCrlStore.getPendingCRLs()).thenReturn(pendingCRLs);
- when(dummyStateMachine.getDnCRLStore()).thenReturn(dnCrlStore);
- when(dummyContext.getParent()).thenReturn(dummyStateMachine);
- publisher.setConf(config);
-
- ScheduledExecutorService executorService = HadoopExecutors
- .newScheduledThreadPool(1,
- new ThreadFactoryBuilder().setDaemon(true)
- .setNameFormat("TestReportManagerThread-%d").build());
- publisher.init(dummyContext, executorService);
- Message report =
- ((CRLStatusReportPublisher) publisher).getReport();
- assertNotNull(report);
- for (Descriptors.FieldDescriptor descriptor :
- report.getDescriptorForType().getFields()) {
- if (descriptor.getNumber() ==
- CRLStatusReport.RECEIVEDCRLID_FIELD_NUMBER) {
- assertEquals(3L, report.getField(descriptor));
- }
- }
- executorService.shutdown();
- }
-
- /**
- * Get a datanode details.
- *
- * @return DatanodeDetails
- */
- private static DatanodeDetails getDatanodeDetails() {
Review Comment:
This was just simply an unused method, right? It wasn't related to CRL 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]
