You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "Galsza (via GitHub)" <gi...@apache.org> on 2023/07/19 16:35:15 UTC
[GitHub] [ozone] Galsza opened a new pull request, #5089: HDDS-8764. Remove expired certificates from SCM DB
Galsza opened a new pull request, #5089:
URL: https://github.com/apache/ozone/pull/5089
## What changes were proposed in this pull request?
Periodically remove expired certificates from scm rocksdb just as housekeeping
[HDDS-7380](https://issues.apache.org/jira/browse/HDDS-7380)
## How was this patch tested?
Added unit test and robot test and also observed it in a local cluster
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #5089: HDDS-7380. Remove expired certificates from SCM DB
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #5089:
URL: https://github.com/apache/ozone/pull/5089#discussion_r1282630452
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMCertStore.java:
##########
@@ -220,6 +222,37 @@ public void removeExpiredCertificate(BigInteger serialID)
// TODO: Later this allows removal of expired certificates from the system.
}
+ @Override
+ public void removeAllExpiredCertificates() {
+ lock.lock();
+ try (BatchOperation batchOperation =
+ scmMetadataStore.getBatchHandler().initBatchOperation()) {
+ addExpiredCertsToBeRemoved(batchOperation,
+ scmMetadataStore.getValidCertsTable());
+ addExpiredCertsToBeRemoved(batchOperation,
+ scmMetadataStore.getValidSCMCertsTable());
+ scmMetadataStore.getStore().commitBatchOperation(batchOperation);
+ } catch (IOException e) {
+ LOG.error("Error while trying to remove expired certificate.", e);
Review Comment:
Please throw out the exception here.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] Galsza commented on pull request #5089: HDDS-7380. Remove expired certificates from SCM DB
Posted by "Galsza (via GitHub)" <gi...@apache.org>.
Galsza commented on PR #5089:
URL: https://github.com/apache/ozone/pull/5089#issuecomment-1663896521
@ChenSammi thank you Sammi for the review comments, I've addressed them, PTAL
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #5089: HDDS-7380. Remove expired certificates from SCM DB
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #5089:
URL: https://github.com/apache/ozone/pull/5089#discussion_r1282623490
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsConfigKeys.java:
##########
@@ -230,6 +230,11 @@ public final class HddsConfigKeys {
"hdds.x509.ca.rotation.enabled";
public static final boolean HDDS_X509_CA_ROTATION_ENABLED_DEFAULT = false;
+ public static final String HDDS_X509_EXPIRED_CERTIFICATE_REMOVAL_INTERVAL =
+ "hdds.x509.expired.certificate.removal.interval";
Review Comment:
hdds.x509.expired.certificate.removal.interval -> hdds.x509.expired.certificate.check.interval
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #5089: HDDS-7380. Remove expired certificates from SCM DB
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on PR #5089:
URL: https://github.com/apache/ozone/pull/5089#issuecomment-1664916258
Thanks @Galsza , the last patch LGTM +1.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #5089: HDDS-7380. Remove expired certificates from SCM DB
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #5089:
URL: https://github.com/apache/ozone/pull/5089#discussion_r1282631760
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/authority/CertificateStore.java:
##########
@@ -106,6 +106,13 @@ Optional<Long> revokeCertificates(List<BigInteger> serialIDs,
*/
void removeExpiredCertificate(BigInteger serialID) throws IOException;
+ /**
+ * Deletes all non-revoked expired certificates from the store.
+ *
+ * @throws IOException
+ */
+ void removeAllExpiredCertificates();
Review Comment:
This need go through ratis. All APIs go through ratis requires annotation "@Replicate".
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi merged pull request #5089: HDDS-7380. Remove expired certificates from SCM DB
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi merged PR #5089:
URL: https://github.com/apache/ozone/pull/5089
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #5089: HDDS-7380. Remove expired certificates from SCM DB
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #5089:
URL: https://github.com/apache/ozone/pull/5089#discussion_r1282627826
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMCertStore.java:
##########
@@ -220,6 +222,37 @@ public void removeExpiredCertificate(BigInteger serialID)
// TODO: Later this allows removal of expired certificates from the system.
}
+ @Override
+ public void removeAllExpiredCertificates() {
+ lock.lock();
+ try (BatchOperation batchOperation =
+ scmMetadataStore.getBatchHandler().initBatchOperation()) {
+ addExpiredCertsToBeRemoved(batchOperation,
+ scmMetadataStore.getValidCertsTable());
+ addExpiredCertsToBeRemoved(batchOperation,
+ scmMetadataStore.getValidSCMCertsTable());
+ scmMetadataStore.getStore().commitBatchOperation(batchOperation);
+ } catch (IOException e) {
+ LOG.error("Error while trying to remove expired certificate.", e);
+ } finally {
+ lock.unlock();
+ }
+ }
+
+ private void addExpiredCertsToBeRemoved(BatchOperation batchOperation,
+ Table<BigInteger, X509Certificate> certTable) throws IOException {
+ TableIterator<BigInteger, ? extends Table.KeyValue<BigInteger,
+ X509Certificate>> certsIterator = certTable.iterator();
Review Comment:
Please use try() with this iterator to make sure it will be closed in any cases.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #5089: HDDS-7380. Remove expired certificates from SCM DB
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #5089:
URL: https://github.com/apache/ozone/pull/5089#discussion_r1282629122
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMCertStore.java:
##########
@@ -220,6 +222,37 @@ public void removeExpiredCertificate(BigInteger serialID)
// TODO: Later this allows removal of expired certificates from the system.
}
+ @Override
+ public void removeAllExpiredCertificates() {
+ lock.lock();
+ try (BatchOperation batchOperation =
+ scmMetadataStore.getBatchHandler().initBatchOperation()) {
+ addExpiredCertsToBeRemoved(batchOperation,
+ scmMetadataStore.getValidCertsTable());
+ addExpiredCertsToBeRemoved(batchOperation,
+ scmMetadataStore.getValidSCMCertsTable());
+ scmMetadataStore.getStore().commitBatchOperation(batchOperation);
+ } catch (IOException e) {
+ LOG.error("Error while trying to remove expired certificate.", e);
+ } finally {
+ lock.unlock();
+ }
+ }
+
+ private void addExpiredCertsToBeRemoved(BatchOperation batchOperation,
+ Table<BigInteger, X509Certificate> certTable) throws IOException {
+ TableIterator<BigInteger, ? extends Table.KeyValue<BigInteger,
+ X509Certificate>> certsIterator = certTable.iterator();
+ while (certsIterator.hasNext()) {
+ Table.KeyValue<BigInteger, X509Certificate> certEntry =
+ certsIterator.next();
+ if (certEntry.getValue().getNotAfter().toInstant().isBefore(
+ Instant.now())) {
Review Comment:
Please move this "Instant.now()" out, create a variable for it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #5089: HDDS-7380. Remove expired certificates from SCM DB
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #5089:
URL: https://github.com/apache/ozone/pull/5089#discussion_r1282623490
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsConfigKeys.java:
##########
@@ -230,6 +230,11 @@ public final class HddsConfigKeys {
"hdds.x509.ca.rotation.enabled";
public static final boolean HDDS_X509_CA_ROTATION_ENABLED_DEFAULT = false;
+ public static final String HDDS_X509_EXPIRED_CERTIFICATE_REMOVAL_INTERVAL =
+ "hdds.x509.expired.certificate.removal.interval";
Review Comment:
I would suggest use the word "check" instead of "removal" in the property.
hdds.x509.expired.certificate.removal.interval -> hdds.x509.expired.certificate.check.interval
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #5089: HDDS-7380. Remove expired certificates from SCM DB
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #5089:
URL: https://github.com/apache/ozone/pull/5089#discussion_r1282748445
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/RootCARotationManager.java:
##########
@@ -219,6 +219,17 @@ public void start() throws SCMServiceException {
LOG.info("Monitor task for root certificate {} is started with " +
"interval {}.", scmCertClient.getCACertificate().getSerialNumber(),
checkInterval);
+ executorService.scheduleAtFixedRate(this::removeExpiredCertTask, 0,
+ secConf.getExpiredCertificateRemovalInterval().toMillis(),
+ TimeUnit.MILLISECONDS);
+ LOG.info("Scheduling expired certificate removal with interval {}s",
+ secConf.getExpiredCertificateRemovalInterval().getSeconds());
+ }
+
+ private void removeExpiredCertTask() {
+ if (scm.getCertificateStore() != null) {
Review Comment:
Please add the isRunning check.
if (!isRunning.get()) {
return;
}
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org