chihsuan commented on code in PR #10916:
URL: https://github.com/apache/ozone/pull/10916#discussion_r3714155614
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMSecurityProtocolServer.java:
##########
@@ -326,7 +334,29 @@ public String getSCMCertificate(ScmNodeDetailsProto
scmNodeDetails,
LOGGER.info("Processing CSR for scm {}, nodeId: {}",
scmNodeDetails.getHostName(), scmNodeDetails.getScmNodeId());
- return getEncodedCertToString(certSignReq, NodeType.SCM);
+ boolean leaderless = !storageContainerManager.checkLeader()
+ && isLeaderlessPrimaryScmSigner(storageContainerManager,
+
storageContainerManager.getScmHAManager().getRatisServer().triggerNotLeaderException(),
+ isRenew);
+
+ return getEncodedCertToString(certSignReq, NodeType.SCM, leaderless,
scmNodeDetails.getScmNodeId());
+ }
+
+ /**
+ * Single source of truth for whether this SCM should sign its own
leaderless bootstrap SCM
+ * certificate: no Ratis leader is known cluster-wide (not merely that this
node isn't leader),
+ * this is not a renewal, and this SCM hosts the primary root CA.
+ *
+ * @param scm - the serving StorageContainerManager.
+ * @param nle - the NotLeaderException produced by the local Ratis
server, or null.
+ * @param isRenew - whether this request is a certificate renewal.
+ * @return true iff the leaderless SCM-certificate signing path should be
used.
+ */
+ @VisibleForTesting
+ public static boolean isLeaderlessPrimaryScmSigner(StorageContainerManager
scm, NotLeaderException nle,
+ boolean isRenew) {
+ return !isRenew && nle != null && nle.getSuggestedLeader() == null
+ && scm.getRootCertificateServer() != null;
Review Comment:
I just noticed that when the primary ID is absent, the upgrade branch
initializes one without an identity check:
https://github.com/apache/ozone/blob/8e3d5e6ae9bf3f74ab4a99a05b631298c2460068/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java#L940-L962
Could a non-primary SCM therefore enter this bypass? If it did, it wouldn't
have any root CA material on disk, so I think it would end up generating a
fresh self-signed one that no other node trusts.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java:
##########
@@ -180,6 +180,44 @@ public void reinitialize(Table<SequenceIdType, Long>
sequenceIdTable)
}
}
+ /**
+ * Allocate the next CertificateId directly against the CertificateId row
+ * of {@link SCMMetadataStore#getSequenceIdTable()}, bypassing Ratis. This
+ * is used for leaderless bootstrap certificate signing, where no Ratis
+ * leader is available yet.
+ *
+ * The live StateManager cache and any un-exhausted batch are refreshed
+ * under the same lock, so that a later Ratis-based allocation of
+ * CertificateId (e.g. after a leader election) cannot CAS a stale cached
+ * lastId and reissue the value handed out here.
+ *
+ * @param scmMetadataStore : the SCMMetadataStore to allocate against.
+ * @return the newly allocated CertificateId.
+ */
+ public long getNextCertificateIdWithoutRatis(SCMMetadataStore
scmMetadataStore)
+ throws IOException {
+ lock.lock();
+ try {
+ // Re-derive the CertificateId row from existing certificates if missing.
+ upgradeToCertificateSequenceId(scmMetadataStore, false);
+
+ Table<SequenceIdType, Long> sequenceIdTable =
scmMetadataStore.getSequenceIdTable();
+ Long lastId = sequenceIdTable.get(SequenceIdType.CertificateId);
+ long newId = (lastId != null ? lastId : INVALID_SEQUENCE_ID) + 1;
+ sequenceIdTable.put(SequenceIdType.CertificateId, newId);
Review Comment:
One more question: if a leader checkpoint later replaces the local DB, would
these updates be lost?
The primary's own certs seem to survive that because they're re-created on
every start, and followers pull anything missing. A cert issued to another node
doesn't look like it has either of those. Would a reconciliation step be needed
once a leader becomes available?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMSecurityProtocolServer.java:
##########
@@ -326,7 +334,29 @@ public String getSCMCertificate(ScmNodeDetailsProto
scmNodeDetails,
LOGGER.info("Processing CSR for scm {}, nodeId: {}",
scmNodeDetails.getHostName(), scmNodeDetails.getScmNodeId());
- return getEncodedCertToString(certSignReq, NodeType.SCM);
+ boolean leaderless = !storageContainerManager.checkLeader()
+ && isLeaderlessPrimaryScmSigner(storageContainerManager,
+
storageContainerManager.getScmHAManager().getRatisServer().triggerNotLeaderException(),
+ isRenew);
+
+ return getEncodedCertToString(certSignReq, NodeType.SCM, leaderless,
scmNodeDetails.getScmNodeId());
+ }
+
+ /**
+ * Single source of truth for whether this SCM should sign its own
leaderless bootstrap SCM
+ * certificate: no Ratis leader is known cluster-wide (not merely that this
node isn't leader),
+ * this is not a renewal, and this SCM hosts the primary root CA.
+ *
+ * @param scm - the serving StorageContainerManager.
+ * @param nle - the NotLeaderException produced by the local Ratis
server, or null.
+ * @param isRenew - whether this request is a certificate renewal.
+ * @return true iff the leaderless SCM-certificate signing path should be
used.
+ */
+ @VisibleForTesting
+ public static boolean isLeaderlessPrimaryScmSigner(StorageContainerManager
scm, NotLeaderException nle,
+ boolean isRenew) {
+ return !isRenew && nle != null && nle.getSuggestedLeader() == null
Review Comment:
Your three checks sound right to me, and I think "has never had a
certificate" may be the strongest one, since it bounds the bypass without
depending on correct leader detection. For the membership check, would the
existing peer lookup on `StorageContainerManager` work here? It reads the live
Raft configuration rather than a local snapshot.
On disjoint ID ranges, I'm not sure it's cheap. The counter gets re-derived
from the highest serial across both cert tables on every primary start, so a
reserved high value would just become the new floor. Clients also pick their
current CA by highest serial, so such a cert might get pinned everywhere. Did
you have a different layout in mind?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMSecurityProtocolServer.java:
##########
@@ -326,7 +334,29 @@ public String getSCMCertificate(ScmNodeDetailsProto
scmNodeDetails,
LOGGER.info("Processing CSR for scm {}, nodeId: {}",
scmNodeDetails.getHostName(), scmNodeDetails.getScmNodeId());
- return getEncodedCertToString(certSignReq, NodeType.SCM);
+ boolean leaderless = !storageContainerManager.checkLeader()
+ && isLeaderlessPrimaryScmSigner(storageContainerManager,
+
storageContainerManager.getScmHAManager().getRatisServer().triggerNotLeaderException(),
+ isRenew);
+
+ return getEncodedCertToString(certSignReq, NodeType.SCM, leaderless,
scmNodeDetails.getScmNodeId());
+ }
+
+ /**
+ * Single source of truth for whether this SCM should sign its own
leaderless bootstrap SCM
+ * certificate: no Ratis leader is known cluster-wide (not merely that this
node isn't leader),
+ * this is not a renewal, and this SCM hosts the primary root CA.
+ *
+ * @param scm - the serving StorageContainerManager.
+ * @param nle - the NotLeaderException produced by the local Ratis
server, or null.
+ * @param isRenew - whether this request is a certificate renewal.
+ * @return true iff the leaderless SCM-certificate signing path should be
used.
+ */
+ @VisibleForTesting
+ public static boolean isLeaderlessPrimaryScmSigner(StorageContainerManager
scm, NotLeaderException nle,
+ boolean isRenew) {
+ return !isRenew && nle != null && nle.getSuggestedLeader() == null
Review Comment:
Thanks for the explanation. That helps. 🙏 I took another look and noticed
that `checkLeader()` checks `isLeaderReady()`, while the suggested leader is
read from `followerInfo`.
https://github.com/apache/ozone/blob/8e3d5e6ae9bf3f74ab4a99a05b631298c2460068/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java#L1962-L1966
https://github.com/apache/ozone/blob/8e3d5e6ae9bf3f74ab4a99a05b631298c2460068/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisServerImpl.java#L309-L310
Ratis seems to populate only the field matching the current role, so this
could also be null for a candidate, or for a leader that hasn't become ready
yet. Since the bootstrap client retries indefinitely, it would keep hitting
this node right through an election. I wonder how often it lands in that
window. Perhaps the Javadoc could also say "no leader is known locally" rather
than "cluster-wide"?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java:
##########
@@ -180,6 +180,44 @@ public void reinitialize(Table<SequenceIdType, Long>
sequenceIdTable)
}
}
+ /**
+ * Allocate the next CertificateId directly against the CertificateId row
+ * of {@link SCMMetadataStore#getSequenceIdTable()}, bypassing Ratis. This
+ * is used for leaderless bootstrap certificate signing, where no Ratis
+ * leader is available yet.
+ *
+ * The live StateManager cache and any un-exhausted batch are refreshed
+ * under the same lock, so that a later Ratis-based allocation of
+ * CertificateId (e.g. after a leader election) cannot CAS a stale cached
+ * lastId and reissue the value handed out here.
+ *
+ * @param scmMetadataStore : the SCMMetadataStore to allocate against.
+ * @return the newly allocated CertificateId.
+ */
+ public long getNextCertificateIdWithoutRatis(SCMMetadataStore
scmMetadataStore)
+ throws IOException {
+ lock.lock();
+ try {
+ // Re-derive the CertificateId row from existing certificates if missing.
+ upgradeToCertificateSequenceId(scmMetadataStore, false);
+
+ Table<SequenceIdType, Long> sequenceIdTable =
scmMetadataStore.getSequenceIdTable();
+ Long lastId = sequenceIdTable.get(SequenceIdType.CertificateId);
Review Comment:
Thanks, that makes sense for the initial conversion. I was wondering about
the disaster-recovery case, though, where an existing secure cluster loses a
node and reboots it. There, the counter has already advanced, and the other
SCMs still hold their own cached value. Is DR meant to be in scope 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: [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]
