EncryptionRequestHandler supports encryption requests distribution. #115

bruno-roustant · 2025-01-06T15:48:23Z

The client can call EncryptionRequestHandler and provide the additional parameter "distrib" = "true". In this case, this handler will distribute the encryption request to all the leader replicas of all the shards of the collection, ensuring they all encrypt their index shard. The response "encryptionState" will be "complete" only when all the shards return "complete". So the caller may repeatedly send the same encryption request with "distrib" = "true" to know when the whole collection encryption is complete. In addition, the caller can set the "timeAllowed" parameter to define a timeout for the request distribution, in milliseconds. This timeout is for the distribution itself, not the encryption process.

dsmiley · 2025-01-07T06:22:30Z

encryption/src/main/java/org/apache/solr/encryption/EncryptionRequestHandler.java

    EncryptionDirectoryFactory.getFactory(req.getCore());
+
+    if (req.getParams().getBool(DISTRIB, false)) {


It'd be nice if we could default "distrib" based on whether the request was addressed to the collection or not. Maybe that metadata is there?

I don't see this metadata. The collection name is present in the CloudSolrClient.request call, but not passed to the inner LBSolrClient.request call.

SolrQueryRequest.getHttpSolrCall().getCoreOrColName() -- but never mind my thought on this. Since SearchHandler/CloudReplicaSource doesn't toggle its scope based on looking at this, we shouldn't either. Consistency.

dsmiley · 2025-01-07T06:30:48Z

encryption/src/main/java/org/apache/solr/encryption/EncryptionRequestHandler.java

+      }
+      try (SolrClientHolder solrClient = getHttpSolrClient(req)) {
+        ModifiableSolrParams params = createDistributedRequestParams(req, rsp, keyId);
+        for (Slice slice : docCollection.getActiveSlices()) {


Hmm; sends in series vs in parallel. We could chat about that tomorrow.

Good point. I modified to use the ExecutorService to send the request in parallel.

dsmiley · 2025-01-07T06:32:00Z

encryption/src/main/java/org/apache/solr/encryption/EncryptionRequestHandler.java

+      }
+      if (log.isInfoEnabled()) {
+        long timeMs = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startTimeNs);
+        log.info("Responding encryption distributed state={} success={} for keyId={} collection={} timeMs={}",


FYI collection is redundant with MDC.
And you might want to put state and/or success into rsp.addToLog where it may be seen in structured logs

How MDC knows the collection if it is not passed to the inner LBSolrClient.request by the CloudSolrClient?

Solr sets MDC at key spots; do a find-usages of org.apache.solr.logging.MDCLoggingContext. SolrCore.open() is a subtle one. CloudSolrClient/LBSolrClient isn't pertinent at all.

encryption/src/main/java/org/apache/solr/encryption/EncryptionRequestHandler.java

dsmiley · 2025-01-07T06:38:46Z

encryption/src/main/java/org/apache/solr/encryption/EncryptionRequestHandler.java

+  private NamedList<Object> sendEncryptionRequest(Replica replica, ModifiableSolrParams params, Http2SolrClient httpSolrClient)
+      throws SolrServerException, IOException {
+    GenericSolrRequest request = new GenericSolrRequest(SolrRequest.METHOD.POST, getPath(), params);
+    request.setBasePath(replica.getCoreUrl());


setBasePath is now gone in Solr main. Instead, you should use httpSolrClient.requestWithBaseUrl

It is not available in Solr 9.6, the Solr version used here. I propose to let this code for now, and replace it when I upgrade to 9.8.

Finally, Solr 9.8.0 was released during this long lasting PR, so I updated.

encryption/src/test/java/org/apache/solr/encryption/EncryptionTestUtil.java

dsmiley

I think you'll find use of the Solr TimeOut class to be helpful here.

dsmiley · 2025-01-10T23:08:08Z

encryption/src/test/java/org/apache/solr/encryption/EncryptionTestUtil.java

    });
    return encryptionStatus;
  }

+  private EncryptionStatus encryptDistrib(SolrParams params) throws SolrServerException, IOException {
+    params = SolrParams.wrapDefaults(params, new ModifiableSolrParams().set(DISTRIB, "true"));


Nice :-)
BTW set's 2nd arg value needn't be a string; it's overloaded to take the primitive boolean.

dsmiley · 2025-01-11T03:48:09Z

encryption/src/main/java/org/apache/solr/encryption/EncryptionRequestHandler.java

+    String collectionName = null;
+    State collectionState = null;
+    long timeAllowedMs = req.getParams().getLong(TIME_ALLOWED, 0);
+    long maxTimeNs = timeAllowedMs <= 0 ? Long.MAX_VALUE : startTimeNs + timeAllowedMs;


you are adding milliseconds and nanoseconds. Maybe convert timeAllowsMs on fetch so that you don't have any millisecond vars

dsmiley · 2025-01-11T04:00:41Z

encryption/src/main/java/org/apache/solr/encryption/EncryptionRequestHandler.java

+            try {
+              state = response.get();
+            } catch (ExecutionException e) {
+              collectionState = State.ERROR;


should log.error(e)

dsmiley · 2025-01-11T04:01:42Z

encryption/src/main/java/org/apache/solr/encryption/EncryptionRequestHandler.java

+        } catch (InterruptedException e) {
+          collectionState = State.INTERRUPTED;
+        }
+        success = collectionState == null || collectionState.isSuccess();


why is null a success?

null only happens if there is no distribution because there are no slices. I don't know if it can happen actually, but the request to encrypt all shards is complete.

dsmiley · 2025-01-11T04:05:27Z

encryption/src/main/java/org/apache/solr/encryption/EncryptionRequestHandler.java

+    ModifiableSolrParams params = new ModifiableSolrParams();
+    params.set(PARAM_KEY_ID, keyId == null ? NO_KEY_ID : keyId);
+    return params;


minor: this can be exactly one line since set returns itself

dsmiley · 2025-01-11T04:08:35Z

encryption/src/main/java/org/apache/solr/encryption/EncryptionRequestHandler.java

+    return params;
+  }
+
+  boolean isTimeout(long maxTimeNs) {


this method named confused me a little. If you name it "hasTimedOut", it'd be a bit clearer. Also "maxTimeNs" reads looks like it's probably a duration, but it's an instant in time. "timeoutAtNs" would be clearer.

Following your advice, I replaced the code by using TimeOut.
Doing so I discovered a long overflow bug in TimeOut if we try to create a TimeOut with interval = Long.MAX_VALUE, or any large interval which overflows when added to TimeSource.getTimeNs(). I'll create an issue.

dsmiley · 2025-01-11T04:12:54Z

encryption/src/main/java/org/apache/solr/encryption/EncryptionRequestHandler.java

+  }
+
+  protected String getPath() {
+    return "/admin/encrypt";


instead of hard-coding this, should probably use the path of the current request (as we are calling ourselves), which is request.getPath()

dsmiley · 2025-01-11T04:23:56Z

encryption/src/main/java/org/apache/solr/encryption/EncryptionRequestHandler.java

+    CloudSolrClient cloudSolrClient = coreContainer.getSolrClientCache().getCloudSolrClient(coreContainer.getZkController().getZkClient().getZkServerAddress());
+    if (cloudSolrClient instanceof CloudHttp2SolrClient) {
+      return new SolrClientHolder(((CloudHttp2SolrClient) cloudSolrClient).getHttpClient(), false);
+    }
+    return new SolrClientHolder(new Http2SolrClient.Builder().build(), true);


I don't think this is needed, nor is SolrClientHolder. Just use an existing client.
You can use coreContainer.getUpdateShardHandler().getUpdateOnlyHttpClient().
It's mildly debatable if it's the perfect client since an encryption request is sorta kinda an "update" but it's really not a big deal.
You shouldn't create new clients (which was your fallback here) as it's tricky to do it correctly inside Solr as there are some instrumentation & authentication matters to attend to that UpdateShardHandler works out for you.

…8. Fix EncryptionDirectoryTest.

bruno-roustant · 2025-02-07T14:18:43Z

Thank you for your thorough review David. I have updated to integrate your remarks.

dsmiley · 2025-02-07T17:01:49Z

encryption/src/main/java/org/apache/solr/encryption/EncryptionRequestHandler.java

-  protected String getPath() {
-    return "/admin/encrypt";
+    GenericSolrRequest distributedRequest = new GenericSolrRequest(SolrRequest.METHOD.POST, requestPath, params);
+    return httpSolrClient.requestWithBaseUrl(replica.getCoreUrl(), replica.getCollection(), distributedRequest);


if you use getCoreUrl, it's complete (solr (aka "base url") + core). Passing collection here would be a problem; concatenating that after the core url which doesn't make sense. A test had better fail here.

dsmiley · 2025-02-07T17:03:00Z

encryption/src/main/java/org/apache/solr/encryption/EncryptionRequestHandler.java

        CommitUpdateCommand commitCmd = new CommitUpdateCommand(req, true);
        // Trigger EncryptionMergePolicy.findForcedMerges() to re-encrypt
        // each segment which is not encrypted with the latest active key id.
        commitCmd.maxOptimizeSegments = Integer.MAX_VALUE;
        req.getCore().getUpdateHandler().commit(commitCmd);
-        log.info("Successfully triggered index encryption with commit in {}", elapsedTime(startTimeNs));
+        log.info("Successfully triggered index encryption with commit in {} ms", elapsedTimeMs(startTimeNs));


Looks confusing / error-prone. Seeing elapsedTimeMs take a parameter with "Ns" suffix smells of a bug. I wonder if we should embrace Instant where we can and thus avoid time units in many places?

dsmiley · 2025-02-07T17:09:58Z

encryption/src/main/java/org/apache/solr/encryption/EncryptionRequestHandler.java

+      // Use the update-only http client, considering encryption is an update as we indeed create new Lucene segments.
+      Http2SolrClient solrClient = req.getCoreContainer().getUpdateShardHandler().getUpdateOnlyHttpClient();


Seems you could move this to inside sendEncryptionRequestWithRetry, thus one fewer param as well. It doesn't matter but sendEncryptionRequestWithRetry has lots of params so maybe this move would be nicer. Up to you.

EncryptionRequestHandler supports encryption requests distribution.

85204b0

bruno-roustant linked an issue Jan 6, 2025 that may be closed by this pull request

EncryptionRequestHandler support for collection with multiple shards #110

Open

bruno-roustant mentioned this pull request Jan 6, 2025

EncryptionRequestHandler support for collection with multiple shards #110

Open

dsmiley reviewed Jan 7, 2025

View reviewed changes

EncryptionRequestHandler distributes encryption request in parallel.

fb49fd7

dsmiley reviewed Jan 11, 2025

View reviewed changes

Use update-only httpclient and TimeOut. Upgrade dependency to Solr 9.…

5b9a0e5

…8. Fix EncryptionDirectoryTest.

dsmiley reviewed Feb 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EncryptionRequestHandler supports encryption requests distribution. #115

EncryptionRequestHandler supports encryption requests distribution. #115

bruno-roustant commented Jan 6, 2025

dsmiley Jan 7, 2025

bruno-roustant Jan 9, 2025

dsmiley Jan 10, 2025

dsmiley Jan 7, 2025

bruno-roustant Jan 9, 2025

dsmiley Jan 7, 2025

bruno-roustant Jan 9, 2025

dsmiley Jan 10, 2025

dsmiley Jan 7, 2025

bruno-roustant Jan 9, 2025

bruno-roustant Feb 6, 2025

dsmiley left a comment

dsmiley Jan 10, 2025

dsmiley Jan 11, 2025

dsmiley Jan 11, 2025

dsmiley Jan 11, 2025

bruno-roustant Feb 6, 2025

dsmiley Jan 11, 2025

dsmiley Jan 11, 2025

bruno-roustant Feb 6, 2025

dsmiley Jan 11, 2025

dsmiley Jan 11, 2025

bruno-roustant commented Feb 7, 2025

dsmiley Feb 7, 2025

dsmiley Feb 7, 2025

dsmiley Feb 7, 2025

		EncryptionDirectoryFactory.getFactory(req.getCore());

		if (req.getParams().getBool(DISTRIB, false)) {

		// Use the update-only http client, considering encryption is an update as we indeed create new Lucene segments.
		Http2SolrClient solrClient = req.getCoreContainer().getUpdateShardHandler().getUpdateOnlyHttpClient();

EncryptionRequestHandler supports encryption requests distribution. #115

Are you sure you want to change the base?

EncryptionRequestHandler supports encryption requests distribution. #115

Conversation

bruno-roustant commented Jan 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsmiley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruno-roustant commented Feb 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment