You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2021/03/12 08:56:13 UTC
[GitHub] [ozone] bharatviswa504 opened a new pull request #2034: HDDS-4953. Make CertStore DB updates for StoreValidateCertificate go via Ratis
bharatviswa504 opened a new pull request #2034:
URL: https://github.com/apache/ozone/pull/2034
## What changes were proposed in this pull request?
This Jira is to make DB updates of CertStore go via ratis, so that all SCM's can be in sync.
In this way, OM/DN/SCM Certs will be in sync across Ratis.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4953
## How was this patch tested?
Added tests for codec.
----------------------------------------------------------------
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.
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] bharatviswa504 commented on pull request #2034: HDDS-4953. [SCM HA Security] Make CertStore DB updates for StoreValidateCertificate go via Ratis
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #2034:
URL: https://github.com/apache/ozone/pull/2034#issuecomment-799143560
> > > Thanks @bharatviswa504 for the patch. The changes look good. Can you please add a miniOzoneHACluster based test and verify the storeCertificate behaviour with ratis replication?
> >
> >
> > @bshashikant I think We don't have a MiniOzoneSecure cluster, due to the limitation of not able to login multiple Kerberos credential in a single JVM (I think this was the reason, but not 100% sure cc @xiaoyuyao ). Once we have complete Security work I can try to add tests and verify this via Robot tests.
>
> @bharatviswa504 , can't we directly use server API's i MiniOzoneHACuster to verify the behaviour of storeValidCertificate bypassing the other security checks?
We don't start the security protocol Server in non-secure. So, not sure what do you mean by directly use server API 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.
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] bharatviswa504 commented on a change in pull request #2034: HDDS-4953. [SCM HA Security] Make CertStore DB updates for StoreValidateCertificate go via Ratis
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2034:
URL: https://github.com/apache/ozone/pull/2034#discussion_r594156310
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMCertStore.java
##########
@@ -56,20 +61,21 @@
/**
* A Certificate Store class that persists certificates issued by SCM CA.
*/
-public class SCMCertStore implements CertificateStore {
+public final class SCMCertStore implements CertificateStore {
private static final Logger LOG =
LoggerFactory.getLogger(SCMCertStore.class);
private SCMMetadataStore scmMetadataStore;
private final Lock lock;
private AtomicLong crlSequenceId;
- public SCMCertStore(SCMMetadataStore dbStore, long sequenceId) {
+ private SCMCertStore(SCMMetadataStore dbStore, long sequenceId) {
this.scmMetadataStore = dbStore;
lock = new ReentrantLock();
crlSequenceId = new AtomicLong(sequenceId);
}
@Override
+ @Replicate
Review comment:
TestSCMCertStore is not constructing RatisServer that is the reason, we don't see the log.
I have updated TestReplicateAnnotation itself, to call storeAndValidateCertificate.
----------------------------------------------------------------
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.
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] bharatviswa504 commented on a change in pull request #2034: HDDS-4953. [SCM HA Security] Make CertStore DB updates for StoreValidateCertificate go via Ratis
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2034:
URL: https://github.com/apache/ozone/pull/2034#discussion_r595697759
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMCertStore.java
##########
@@ -56,14 +60,14 @@
/**
* A Certificate Store class that persists certificates issued by SCM CA.
*/
-public class SCMCertStore implements CertificateStore {
+public final class SCMCertStore implements CertificateStore {
private static final Logger LOG =
LoggerFactory.getLogger(SCMCertStore.class);
private SCMMetadataStore scmMetadataStore;
private final Lock lock;
private AtomicLong crlSequenceId;
- public SCMCertStore(SCMMetadataStore dbStore, long sequenceId) {
+ private SCMCertStore(SCMMetadataStore dbStore, long sequenceId) {
Review comment:
This was an intentional choice not to make CertStore DB updates use transactionalBuffer. The reason is, there is no in-memory map(Like how containerstateManager has), to get immediate reads work (Read operation after a write). (Or we should use table cache and add to table cache, and then listCerts need to be modified for it). As the request for certs in a cluster is a number of nodes in the cluster, so made this choice of directly persisting to DB.
When I am about to type it is idempotent, I see there is a problem, as for replay transaction because of below check looks
if ((getCertificateByID(serialID, VALID_CERTS) != null) ||
(getCertificateByID(serialID, CertType.REVOKED_CERTS) != null)) {
throw new SCMSecurityException("Conflicting certificate ID");
}
If we move this check to DefaultCAServer, then storeValidCertificate will be an idempotent operation, and we will not have a replay issue. Let me know if this makes sense, I will open a Jira to fix this.
----------------------------------------------------------------
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.
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] bharatviswa504 edited a comment on pull request #2034: HDDS-4953. [SCM HA Security] Make CertStore DB updates for StoreValidateCertificate go via Ratis
Posted by GitBox <gi...@apache.org>.
bharatviswa504 edited a comment on pull request #2034:
URL: https://github.com/apache/ozone/pull/2034#issuecomment-799137227
> Thanks @bharatviswa504 for the patch. The changes look good. Can you please add a miniOzoneHACluster based test and verify the storeCertificate behaviour with ratis replication?
@bshashikant I think We don't have a MiniOzoneSecure cluster, due to the limitation of not able to login multiple Kerberos credential in a single JVM (I think this was the reason, but not 100% sure cc @xiaoyuyao ). Once we have complete Security work I can try to add tests and verify this via Robot tests.
----------------------------------------------------------------
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.
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] GlenGeng edited a comment on pull request #2034: HDDS-4953. [SCM HA Security] Make CertStore DB updates for StoreValidateCertificate go via Ratis
Posted by GitBox <gi...@apache.org>.
GlenGeng edited a comment on pull request #2034:
URL: https://github.com/apache/ozone/pull/2034#issuecomment-799151598
> **Just a side question**
>
> Do we have any specific tests to test the behavior of replicate annotation(looks like no) if those work we should be good right, as this is using existing Replicate annotation, and the codec serialization/deserialization tests are already added. Do you think we need to add tests?
We have one `TestReplicationAnnotation`, yet it is a unit test.
Beside test, you can check from the trace to see if the replication annotation is triggered as expected.
```
private Object invokeRatis(Method method, Object[] args)
throws Exception {
long startTime = Time.monotonicNowNanos();
Preconditions.checkNotNull(ratisHandler);
final SCMRatisResponse response = ratisHandler.submitRequest(
SCMRatisRequest.of(requestType, method.getName(), args));
LOG.info("Invoking method {} on target {}, cost {}us",
method, ratisHandler, (Time.monotonicNowNanos() - startTime) / 1000.0);
if (response.isSuccess()) {
return response.getResult();
}
// Should we unwrap and throw proper exception from here?
throw response.getException();
}
```
----------------------------------------------------------------
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.
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] GlenGeng commented on a change in pull request #2034: HDDS-4953. [SCM HA Security] Make CertStore DB updates for StoreValidateCertificate go via Ratis
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #2034:
URL: https://github.com/apache/ozone/pull/2034#discussion_r594084855
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMCertStore.java
##########
@@ -56,20 +61,21 @@
/**
* A Certificate Store class that persists certificates issued by SCM CA.
*/
-public class SCMCertStore implements CertificateStore {
+public final class SCMCertStore implements CertificateStore {
private static final Logger LOG =
LoggerFactory.getLogger(SCMCertStore.class);
private SCMMetadataStore scmMetadataStore;
private final Lock lock;
private AtomicLong crlSequenceId;
- public SCMCertStore(SCMMetadataStore dbStore, long sequenceId) {
+ private SCMCertStore(SCMMetadataStore dbStore, long sequenceId) {
this.scmMetadataStore = dbStore;
lock = new ReentrantLock();
crlSequenceId = new AtomicLong(sequenceId);
}
@Override
+ @Replicate
Review comment:
I just run `TestSCMCertStore` locally, and not seen the trace
```
LOG.info("Invoking method {} on target {}, cost {}us",
method, ratisHandler, (Time.monotonicNowNanos() - startTime) / 1000.0);
```
seems the annotation does not work if placing in subclass.
@bshashikant Above trace is sufficient to indicate whether annotated method goes through ratis, even in unit test.
----------------------------------------------------------------
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.
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] bshashikant commented on pull request #2034: HDDS-4953. [SCM HA Security] Make CertStore DB updates for StoreValidateCertificate go via Ratis
Posted by GitBox <gi...@apache.org>.
bshashikant commented on pull request #2034:
URL: https://github.com/apache/ozone/pull/2034#issuecomment-799141158
> > Thanks @bharatviswa504 for the patch. The changes look good. Can you please add a miniOzoneHACluster based test and verify the storeCertificate behaviour with ratis replication?
>
> @bshashikant I think We don't have a MiniOzoneSecure cluster, due to the limitation of not able to login multiple Kerberos credential in a single JVM (I think this was the reason, but not 100% sure cc @xiaoyuyao ). Once we have complete Security work I can try to add tests and verify this via Robot tests.
@bharatviswa504 , can't we directly use server API's i MiniOzoneHACuster to verify the behaviour of storeValidCertificate bypassing the other security checks?
----------------------------------------------------------------
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.
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] GlenGeng commented on a change in pull request #2034: HDDS-4953. [SCM HA Security] Make CertStore DB updates for StoreValidateCertificate go via Ratis
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #2034:
URL: https://github.com/apache/ozone/pull/2034#discussion_r594076855
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMCertStore.java
##########
@@ -56,20 +61,21 @@
/**
* A Certificate Store class that persists certificates issued by SCM CA.
*/
-public class SCMCertStore implements CertificateStore {
+public final class SCMCertStore implements CertificateStore {
private static final Logger LOG =
LoggerFactory.getLogger(SCMCertStore.class);
private SCMMetadataStore scmMetadataStore;
private final Lock lock;
private AtomicLong crlSequenceId;
- public SCMCertStore(SCMMetadataStore dbStore, long sequenceId) {
+ private SCMCertStore(SCMMetadataStore dbStore, long sequenceId) {
this.scmMetadataStore = dbStore;
lock = new ReentrantLock();
crlSequenceId = new AtomicLong(sequenceId);
}
@Override
+ @Replicate
Review comment:
You'd better place `@Replicate` inside the interface `CertificateStore`. Please check `ContainerStateManagerV2` and `ContainerStateManagerImpl` as an example.
----------------------------------------------------------------
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.
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] bharatviswa504 commented on a change in pull request #2034: HDDS-4953. [SCM HA Security] Make CertStore DB updates for StoreValidateCertificate go via Ratis
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2034:
URL: https://github.com/apache/ozone/pull/2034#discussion_r594079027
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMCertStore.java
##########
@@ -56,20 +61,21 @@
/**
* A Certificate Store class that persists certificates issued by SCM CA.
*/
-public class SCMCertStore implements CertificateStore {
+public final class SCMCertStore implements CertificateStore {
private static final Logger LOG =
LoggerFactory.getLogger(SCMCertStore.class);
private SCMMetadataStore scmMetadataStore;
private final Lock lock;
private AtomicLong crlSequenceId;
- public SCMCertStore(SCMMetadataStore dbStore, long sequenceId) {
+ private SCMCertStore(SCMMetadataStore dbStore, long sequenceId) {
this.scmMetadataStore = dbStore;
lock = new ReentrantLock();
crlSequenceId = new AtomicLong(sequenceId);
}
@Override
+ @Replicate
Review comment:
Replicate class in hadoop-hdds-server-scm and CertificateStore in hdds-server-framework.
hadoop-hdds-server-scm depends on framework. So, taken a simple approach and placed annotation in SCMCertSttore.
So, to move @Replication to CertificateStore I need to move the Replicate Class to hdds-server-framework. Let me do 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.
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] bharatviswa504 edited a comment on pull request #2034: HDDS-4953. [SCM HA Security] Make CertStore DB updates for StoreValidateCertificate go via Ratis
Posted by GitBox <gi...@apache.org>.
bharatviswa504 edited a comment on pull request #2034:
URL: https://github.com/apache/ozone/pull/2034#issuecomment-799144645
**Just a side question**
Do we have any specific tests to test the behavior of replicate annotation(looks like no) if those work we should be good right, as this is using existing Replicate annotation, and the codec serialization/deserialization tests are already added.
----------------------------------------------------------------
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.
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] bshashikant commented on a change in pull request #2034: HDDS-4953. [SCM HA Security] Make CertStore DB updates for StoreValidateCertificate go via Ratis
Posted by GitBox <gi...@apache.org>.
bshashikant commented on a change in pull request #2034:
URL: https://github.com/apache/ozone/pull/2034#discussion_r594093068
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMCertStore.java
##########
@@ -56,20 +61,21 @@
/**
* A Certificate Store class that persists certificates issued by SCM CA.
*/
-public class SCMCertStore implements CertificateStore {
+public final class SCMCertStore implements CertificateStore {
private static final Logger LOG =
LoggerFactory.getLogger(SCMCertStore.class);
private SCMMetadataStore scmMetadataStore;
private final Lock lock;
private AtomicLong crlSequenceId;
- public SCMCertStore(SCMMetadataStore dbStore, long sequenceId) {
+ private SCMCertStore(SCMMetadataStore dbStore, long sequenceId) {
this.scmMetadataStore = dbStore;
lock = new ReentrantLock();
crlSequenceId = new AtomicLong(sequenceId);
}
@Override
+ @Replicate
Review comment:
Its not preferrable solution i feel, to check for specific log messages in a test. That's why , i thought replicating over ratis and verifying the db is a better choice. In this case, i am ok with any of the approaches mentioned.
@GlenGeng , thanks for testing out replicate annotation over the sub class and interface.
----------------------------------------------------------------
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.
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] bharatviswa504 commented on pull request #2034: HDDS-4953. [SCM HA Security] Make CertStore DB updates for StoreValidateCertificate go via Ratis
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #2034:
URL: https://github.com/apache/ozone/pull/2034#issuecomment-799144645
**Just a side question**
Do we have any specific tests to test the behavior of replicate annotation(looks like no) if those work we should be good right, as this is using existing Replicate annotation.
----------------------------------------------------------------
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.
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] bshashikant commented on a change in pull request #2034: HDDS-4953. [SCM HA Security] Make CertStore DB updates for StoreValidateCertificate go via Ratis
Posted by GitBox <gi...@apache.org>.
bshashikant commented on a change in pull request #2034:
URL: https://github.com/apache/ozone/pull/2034#discussion_r594093068
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMCertStore.java
##########
@@ -56,20 +61,21 @@
/**
* A Certificate Store class that persists certificates issued by SCM CA.
*/
-public class SCMCertStore implements CertificateStore {
+public final class SCMCertStore implements CertificateStore {
private static final Logger LOG =
LoggerFactory.getLogger(SCMCertStore.class);
private SCMMetadataStore scmMetadataStore;
private final Lock lock;
private AtomicLong crlSequenceId;
- public SCMCertStore(SCMMetadataStore dbStore, long sequenceId) {
+ private SCMCertStore(SCMMetadataStore dbStore, long sequenceId) {
this.scmMetadataStore = dbStore;
lock = new ReentrantLock();
crlSequenceId = new AtomicLong(sequenceId);
}
@Override
+ @Replicate
Review comment:
Its not preferrable solution i feel (unless there seems no easy way), to check for specific log messages in a test. That's why , i thought replicating over ratis and verifying the db is a better choice. In this case, i am ok with any of the approaches mentioned.
@GlenGeng , thanks for testing out replicate annotation over the sub class and interface.
----------------------------------------------------------------
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.
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] bharatviswa504 commented on pull request #2034: HDDS-4953. [SCM HA Security] Make CertStore DB updates for StoreValidateCertificate go via Ratis
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #2034:
URL: https://github.com/apache/ozone/pull/2034#issuecomment-799137227
> Thanks @bharatviswa504 for the patch. The changes look good. Can you please add a miniOzoneHACluster based test and verify the storeCertificate behaviour with ratis replication?
@bshashikant I think We don't have a MiniOzoneSecure cluster, due to the limitation of not able to login Kerberos credential in a single JVM (I think this was the reason, but not 100% sure cc @xiaoyuyao ). Once we have complete Security work I can try to add tests and verify this via Robot tests.
----------------------------------------------------------------
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.
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] bharatviswa504 removed a comment on pull request #2034: HDDS-4953. [SCM HA Security] Make CertStore DB updates for StoreValidateCertificate go via Ratis
Posted by GitBox <gi...@apache.org>.
bharatviswa504 removed a comment on pull request #2034:
URL: https://github.com/apache/ozone/pull/2034#issuecomment-799143435
> storeValidCertificate
We don't start the security protocol Server in non-secure. So, not sure what do you mean by directly use server API 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.
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] GlenGeng commented on pull request #2034: HDDS-4953. [SCM HA Security] Make CertStore DB updates for StoreValidateCertificate go via Ratis
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on pull request #2034:
URL: https://github.com/apache/ozone/pull/2034#issuecomment-797360433
Please hold on the merge util we finish merging master back to 2823. Thanks!
----------------------------------------------------------------
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.
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] bharatviswa504 merged pull request #2034: HDDS-4953. [SCM HA Security] Make CertStore DB updates for StoreValidateCertificate go via Ratis
Posted by GitBox <gi...@apache.org>.
bharatviswa504 merged pull request #2034:
URL: https://github.com/apache/ozone/pull/2034
----------------------------------------------------------------
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.
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] GlenGeng commented on pull request #2034: HDDS-4953. [SCM HA Security] Make CertStore DB updates for StoreValidateCertificate go via Ratis
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on pull request #2034:
URL: https://github.com/apache/ozone/pull/2034#issuecomment-799321873
Thanks for the verification. LGTM. Please rebase with the latest 2823.
----------------------------------------------------------------
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.
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] GlenGeng commented on a change in pull request #2034: HDDS-4953. [SCM HA Security] Make CertStore DB updates for StoreValidateCertificate go via Ratis
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #2034:
URL: https://github.com/apache/ozone/pull/2034#discussion_r595664647
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMCertStore.java
##########
@@ -56,14 +60,14 @@
/**
* A Certificate Store class that persists certificates issued by SCM CA.
*/
-public class SCMCertStore implements CertificateStore {
+public final class SCMCertStore implements CertificateStore {
private static final Logger LOG =
LoggerFactory.getLogger(SCMCertStore.class);
private SCMMetadataStore scmMetadataStore;
private final Lock lock;
private AtomicLong crlSequenceId;
- public SCMCertStore(SCMMetadataStore dbStore, long sequenceId) {
+ private SCMCertStore(SCMMetadataStore dbStore, long sequenceId) {
Review comment:
@bharatviswa504 After integrate with Ratis, the db writes should go to `transactionBuffer` and async flush to db by `takeSnapshot`, seems this part is missing.
----------------------------------------------------------------
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.
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] bharatviswa504 commented on a change in pull request #2034: HDDS-4953. [SCM HA Security] Make CertStore DB updates for StoreValidateCertificate go via Ratis
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2034:
URL: https://github.com/apache/ozone/pull/2034#discussion_r594156310
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMCertStore.java
##########
@@ -56,20 +61,21 @@
/**
* A Certificate Store class that persists certificates issued by SCM CA.
*/
-public class SCMCertStore implements CertificateStore {
+public final class SCMCertStore implements CertificateStore {
private static final Logger LOG =
LoggerFactory.getLogger(SCMCertStore.class);
private SCMMetadataStore scmMetadataStore;
private final Lock lock;
private AtomicLong crlSequenceId;
- public SCMCertStore(SCMMetadataStore dbStore, long sequenceId) {
+ private SCMCertStore(SCMMetadataStore dbStore, long sequenceId) {
this.scmMetadataStore = dbStore;
lock = new ReentrantLock();
crlSequenceId = new AtomicLong(sequenceId);
}
@Override
+ @Replicate
Review comment:
TestSCMCertStore is not constructing RatisServer that is the reason, we don't see the log. As it will call invokeLocal
I have updated TestReplicateAnnotation itself, to call storeAndValidateCertificate.
----------------------------------------------------------------
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.
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] bharatviswa504 commented on pull request #2034: HDDS-4953. [SCM HA Security] Make CertStore DB updates for StoreValidateCertificate go via Ratis
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #2034:
URL: https://github.com/apache/ozone/pull/2034#issuecomment-799143435
> storeValidCertificate
We don't start the security protocol Server in non-secure. So, not sure what do you mean by directly use server API 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.
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] GlenGeng commented on a change in pull request #2034: HDDS-4953. [SCM HA Security] Make CertStore DB updates for StoreValidateCertificate go via Ratis
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #2034:
URL: https://github.com/apache/ozone/pull/2034#discussion_r595718218
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMCertStore.java
##########
@@ -56,14 +60,14 @@
/**
* A Certificate Store class that persists certificates issued by SCM CA.
*/
-public class SCMCertStore implements CertificateStore {
+public final class SCMCertStore implements CertificateStore {
private static final Logger LOG =
LoggerFactory.getLogger(SCMCertStore.class);
private SCMMetadataStore scmMetadataStore;
private final Lock lock;
private AtomicLong crlSequenceId;
- public SCMCertStore(SCMMetadataStore dbStore, long sequenceId) {
+ private SCMCertStore(SCMMetadataStore dbStore, long sequenceId) {
Review comment:
Thanks for the verification! Will inform you if find any issue.
----------------------------------------------------------------
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.
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] bharatviswa504 edited a comment on pull request #2034: HDDS-4953. [SCM HA Security] Make CertStore DB updates for StoreValidateCertificate go via Ratis
Posted by GitBox <gi...@apache.org>.
bharatviswa504 edited a comment on pull request #2034:
URL: https://github.com/apache/ozone/pull/2034#issuecomment-799144645
**Just a side question**
Do we have any specific tests to test the behavior of replicate annotation(looks like no) if those work we should be good right, as this is using existing Replicate annotation, and the codec serialization/deserialization tests are already added. Do you think we need to add tests?
----------------------------------------------------------------
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.
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] bharatviswa504 commented on pull request #2034: HDDS-4953. [SCM HA Security] Make CertStore DB updates for StoreValidateCertificate go via Ratis
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #2034:
URL: https://github.com/apache/ozone/pull/2034#issuecomment-799337302
Thank You @bshashikant and @GlenGeng for the review.
----------------------------------------------------------------
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.
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] GlenGeng commented on pull request #2034: HDDS-4953. [SCM HA Security] Make CertStore DB updates for StoreValidateCertificate go via Ratis
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on pull request #2034:
URL: https://github.com/apache/ozone/pull/2034#issuecomment-799151598
> **Just a side question**
>
> Do we have any specific tests to test the behavior of replicate annotation(looks like no) if those work we should be good right, as this is using existing Replicate annotation, and the codec serialization/deserialization tests are already added. Do you think we need to add tests?
We have one `TestReplicationAnnotation`, yet it is a unit test.
----------------------------------------------------------------
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.
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