SOLR-17194: Fix bug - CloudHttp2SolrClient doesn't preserve the passed httpClient to Http2ClusterStateProvider when using the internalClientBuilder (#2412)
Co-authored-by: Lamine Idjeraoui <lidjeraoui@apple.com>
Co-authored-by: Eric Pugh <epugh@opensourceconnections.com>
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index 1ce3bd0..fc4b687 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -191,9 +191,10 @@
* SOLR-16866: Reorder nested directory deletions to avoid logging spurious NoSuchFileException (Michael Gibney)
+* SOLR-17194: Fix CloudHttp2SolrClient doesn't preserve the passed httpClient to Http2ClusterStateProvider when using the internalClientBuilder (Eric Pugh, Lamine Idjeraoui, Houston Putman, Anshum Gupta)
+
Dependency Upgrades
---------------------
-
* SOLR-17157: Upgrade Lucene to 9.10.0 (hossman, Christine Poerschke)
* SOLR-17214: Update forbiddenapis to 3.7 (hossman)
@@ -202,6 +203,7 @@
Other Changes
---------------------
+
* SOLR-17126: Cut over System.getProperty to EnvUtils.getProperty (janhoy)
* SOLR-17066: GenericSolrRequest now has a `setRequiresCollection` setter that allows it to specify whether
diff --git a/solr/core/src/test/org/apache/solr/cli/TestExportTool.java b/solr/core/src/test/org/apache/solr/cli/TestExportTool.java
index 0aa9bfb..88b4e4a 100644
--- a/solr/core/src/test/org/apache/solr/cli/TestExportTool.java
+++ b/solr/core/src/test/org/apache/solr/cli/TestExportTool.java
@@ -17,6 +17,12 @@
package org.apache.solr.cli;
+import static java.util.Collections.singletonList;
+import static java.util.Collections.singletonMap;
+import static org.apache.solr.cli.SolrCLI.findTool;
+import static org.apache.solr.cli.SolrCLI.parseCmdLine;
+import static org.apache.solr.security.Sha256AuthenticationProvider.getSaltedHashedValue;
+
import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
@@ -27,6 +33,7 @@
import java.util.List;
import java.util.Map;
import java.util.function.Predicate;
+import org.apache.commons.cli.CommandLine;
import org.apache.lucene.tests.util.TestUtil;
import org.apache.solr.SolrTestCaseJ4;
import org.apache.solr.client.solrj.SolrClient;
@@ -43,10 +50,15 @@
import org.apache.solr.common.cloud.Slice;
import org.apache.solr.common.util.FastInputStream;
import org.apache.solr.common.util.JsonRecordReader;
+import org.apache.solr.common.util.Utils;
+import org.apache.solr.security.BasicAuthPlugin;
+import org.apache.solr.security.RuleBasedAuthorizationPlugin;
+import org.junit.Test;
@SolrTestCaseJ4.SuppressSSL
public class TestExportTool extends SolrCloudTestCase {
+ @Test
public void testBasic() throws Exception {
String COLLECTION_NAME = "globalLoaderColl";
configureCluster(4).addConfig("conf", configset("cloud-dynamic")).configure();
@@ -218,6 +230,68 @@
}
}
+ @Test
+ public void testWithBasicAuth() throws Exception {
+ String COLLECTION_NAME = "secureCollection";
+ String USER = "solr";
+ String PASS = "SolrRocksAgain";
+ final String SECURITY_JSON =
+ Utils.toJSONString(
+ Map.of(
+ "authorization",
+ Map.of(
+ "class",
+ RuleBasedAuthorizationPlugin.class.getName(),
+ "user-role",
+ singletonMap(USER, "admin"),
+ "permissions",
+ singletonList(Map.of("name", "all", "role", "admin"))),
+ "authentication",
+ Map.of(
+ "class",
+ BasicAuthPlugin.class.getName(),
+ "blockUnknown",
+ true,
+ "credentials",
+ singletonMap(USER, getSaltedHashedValue(PASS)))));
+
+ configureCluster(2)
+ .addConfig("conf", configset("cloud-minimal"))
+ .withSecurityJson(SECURITY_JSON)
+ .configure();
+
+ try {
+ CollectionAdminRequest.createCollection(COLLECTION_NAME, "conf", 2, 1)
+ .setBasicAuthCredentials(USER, PASS)
+ .process(cluster.getSolrClient());
+ cluster.waitForActiveCollection(COLLECTION_NAME, 2, 2);
+
+ File outFile = File.createTempFile("output", ".json");
+
+ String[] args = {
+ "export",
+ "-url",
+ cluster.getJettySolrRunner(0).getBaseUrl() + "/" + COLLECTION_NAME,
+ "-credentials",
+ USER + ":" + PASS,
+ "-out",
+ outFile.getAbsolutePath(),
+ "-verbose"
+ };
+
+ assertEquals(0, runTool(args));
+ } finally {
+ cluster.shutdown();
+ }
+ }
+
+ private int runTool(String[] args) throws Exception {
+ Tool tool = findTool(args);
+ assertTrue(tool instanceof ExportTool);
+ CommandLine cli = parseCmdLine(tool.getName(), args, tool.getOptions());
+ return tool.runTool(cli);
+ }
+
private void assertJavabinDocsCount(ExportTool.Info info, int expected) throws IOException {
assertTrue(
"" + info.docsWritten.get() + " expected " + expected, info.docsWritten.get() >= expected);
diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java
index 9e4bdad..ffd481f 100644
--- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java
+++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java
@@ -54,17 +54,12 @@
*/
protected CloudHttp2SolrClient(Builder builder) {
super(builder.shardLeadersOnly, builder.parallelUpdates, builder.directUpdatesToLeadersOnly);
- if (builder.httpClient == null) {
- this.clientIsInternal = true;
- if (builder.internalClientBuilder == null) {
- this.myClient = new Http2SolrClient.Builder().build();
- } else {
- this.myClient = builder.internalClientBuilder.build();
- }
- } else {
- this.clientIsInternal = false;
- this.myClient = builder.httpClient;
- }
+ this.clientIsInternal = builder.httpClient == null;
+ this.myClient = createOrGetHttpClientFromBuilder(builder);
+ this.stateProvider =
+ builder.zkHosts.isEmpty()
+ ? createHttp2ClusterStateProvider(builder.solrUrls, myClient)
+ : createZkClusterStateProvider(builder);
this.retryExpiryTimeNano = builder.retryExpiryTimeNano;
this.defaultCollection = builder.defaultCollection;
if (builder.requestWriter != null) {
@@ -73,7 +68,6 @@
if (builder.responseParser != null) {
this.myClient.setParser(builder.responseParser);
}
- this.stateProvider = builder.stateProvider;
this.collectionStateCache.timeToLiveMs =
TimeUnit.MILLISECONDS.convert(builder.timeToLiveSeconds, TimeUnit.SECONDS);
@@ -85,14 +79,64 @@
this.lbClient = new LBHttp2SolrClient.Builder(myClient).build();
}
+ private Http2SolrClient createOrGetHttpClientFromBuilder(Builder builder) {
+ if (builder.httpClient != null) {
+ return builder.httpClient;
+ } else if (builder.internalClientBuilder != null) {
+ return builder.internalClientBuilder.build();
+ } else {
+ return new Http2SolrClient.Builder().build();
+ }
+ }
+
+ private ClusterStateProvider createZkClusterStateProvider(Builder builder) {
+ try {
+ ClusterStateProvider stateProvider =
+ ClusterStateProvider.newZkClusterStateProvider(
+ builder.zkHosts, builder.zkChroot, builder.canUseZkACLs);
+ if (stateProvider instanceof SolrZkClientTimeoutAware) {
+ var timeoutAware = (SolrZkClientTimeoutAware) stateProvider;
+ timeoutAware.setZkClientTimeout(builder.zkClientTimeout);
+ timeoutAware.setZkConnectTimeout(builder.zkConnectTimeout);
+ }
+ return stateProvider;
+ } catch (Exception e) {
+ closeMyClientIfNeeded();
+ throw (e);
+ }
+ }
+
+ private ClusterStateProvider createHttp2ClusterStateProvider(
+ List<String> solrUrls, Http2SolrClient httpClient) {
+ try {
+ return new Http2ClusterStateProvider(solrUrls, httpClient);
+ } catch (Exception e) {
+ closeMyClientIfNeeded();
+ throw new RuntimeException(
+ "Couldn't initialize a HttpClusterStateProvider (is/are the "
+ + "Solr server(s), "
+ + solrUrls
+ + ", down?)",
+ e);
+ }
+ }
+
+ private void closeMyClientIfNeeded() {
+ try {
+ if (clientIsInternal && myClient != null) {
+ myClient.close();
+ }
+ } catch (Exception e) {
+ throw new RuntimeException("Exception on closing myClient", e);
+ }
+ }
+
@Override
public void close() throws IOException {
stateProvider.close();
lbClient.close();
- if (clientIsInternal && myClient != null) {
- myClient.close();
- }
+ closeMyClientIfNeeded();
super.close();
}
@@ -407,32 +451,11 @@
/** Create a {@link CloudHttp2SolrClient} based on the provided configuration. */
public CloudHttp2SolrClient build() {
- if (stateProvider == null) {
- if (!zkHosts.isEmpty() && !solrUrls.isEmpty()) {
- throw new IllegalArgumentException(
- "Both zkHost(s) & solrUrl(s) have been specified. Only specify one.");
- } else if (!zkHosts.isEmpty()) {
- stateProvider =
- ClusterStateProvider.newZkClusterStateProvider(zkHosts, zkChroot, canUseZkACLs);
- if (stateProvider instanceof SolrZkClientTimeoutAware) {
- var timeoutAware = (SolrZkClientTimeoutAware) stateProvider;
- timeoutAware.setZkClientTimeout(zkClientTimeout);
- timeoutAware.setZkConnectTimeout(zkConnectTimeout);
- }
- } else if (!solrUrls.isEmpty()) {
- try {
- stateProvider = new Http2ClusterStateProvider(solrUrls, httpClient);
- } catch (Exception e) {
- throw new RuntimeException(
- "Couldn't initialize a HttpClusterStateProvider (is/are the "
- + "Solr server(s), "
- + solrUrls
- + ", down?)",
- e);
- }
- } else {
- throw new IllegalArgumentException("Both zkHosts and solrUrl cannot be null.");
- }
+ if (!zkHosts.isEmpty() && !solrUrls.isEmpty()) {
+ throw new IllegalArgumentException(
+ "Both zkHost(s) & solrUrl(s) have been specified. Only specify one.");
+ } else if (zkHosts.isEmpty() && solrUrls.isEmpty()) {
+ throw new IllegalArgumentException("Both zkHosts and solrUrl cannot be null.");
}
return new CloudHttp2SolrClient(this);
}
diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2ClusterStateProvider.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2ClusterStateProvider.java
index f46fe1a..3bb222a 100644
--- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2ClusterStateProvider.java
+++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2ClusterStateProvider.java
@@ -43,4 +43,8 @@
protected SolrClient getSolrClient(String baseUrl) {
return new Http2SolrClient.Builder(baseUrl).withHttpClient(httpClient).build();
}
+
+ public Http2SolrClient getHttpClient() {
+ return httpClient;
+ }
}
diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientBuilderTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientBuilderTest.java
index b776755..19c32d9 100644
--- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientBuilderTest.java
+++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientBuilderTest.java
@@ -17,7 +17,6 @@
package org.apache.solr.client.solrj.impl;
-import static org.apache.solr.SolrTestCaseJ4.assumeWorkingMockito;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
@@ -28,15 +27,31 @@
import java.util.Collections;
import java.util.List;
import java.util.Optional;
-import org.apache.solr.SolrTestCase;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.junit.BeforeClass;
import org.junit.Test;
import org.mockito.Mockito;
-public class CloudHttp2SolrClientBuilderTest extends SolrTestCase {
+public class CloudHttp2SolrClientBuilderTest extends SolrCloudTestCase {
private static final String ANY_CHROOT = "/ANY_CHROOT";
private static final String ANY_ZK_HOST = "ANY_ZK_HOST";
private static final String ANY_OTHER_ZK_HOST = "ANY_OTHER_ZK_HOST";
+ @BeforeClass
+ public static void setupCluster() throws Exception {
+ assumeWorkingMockito();
+ configureCluster(1)
+ .addConfig(
+ "conf",
+ getFile("solrj")
+ .toPath()
+ .resolve("solr")
+ .resolve("configsets")
+ .resolve("configset-1")
+ .resolve("conf"))
+ .configure();
+ }
+
@Test
public void testSingleZkHostSpecified() throws IOException {
try (CloudSolrClient createdClient =
@@ -108,7 +123,6 @@
@Test
public void testExternalClientAndInternalBuilderTogether() {
- assumeWorkingMockito();
expectThrows(
IllegalStateException.class,
() ->
@@ -129,7 +143,6 @@
@Test
public void testProvideInternalBuilder() throws IOException {
- assumeWorkingMockito();
Http2SolrClient http2Client = Mockito.mock(Http2SolrClient.class);
Http2SolrClient.Builder http2ClientBuilder = Mockito.mock(Http2SolrClient.Builder.class);
when(http2ClientBuilder.build()).thenReturn(http2Client);
@@ -149,7 +162,6 @@
@Test
public void testProvideExternalClient() throws IOException {
- assumeWorkingMockito();
Http2SolrClient http2Client = Mockito.mock(Http2SolrClient.class);
CloudHttp2SolrClient.Builder clientBuilder =
new CloudHttp2SolrClient.Builder(
@@ -172,4 +184,48 @@
assertEquals("aCollection", createdClient.getDefaultCollection());
}
}
+
+ /**
+ * Tests the consistency between the HTTP client used by {@link CloudHttp2SolrClient} and the one
+ * used by its associated {@link Http2ClusterStateProvider}. This method ensures that whether a
+ * {@link CloudHttp2SolrClient} is created with a specific HTTP client, an internal client
+ * builder, or with no specific HTTP client at all, the same HTTP client instance is used both by
+ * the {@link CloudHttp2SolrClient} and by its {@link Http2ClusterStateProvider}.
+ */
+ @Test
+ public void testHttpClientPreservedInHttp2ClusterStateProvider() throws IOException {
+ List<String> solrUrls = List.of(cluster.getJettySolrRunner(0).getBaseUrl().toString());
+
+ // No httpClient - No internalClientBuilder
+ testHttpClientConsistency(solrUrls, null, null);
+
+ // httpClient - No internalClientBuilder
+ try (Http2SolrClient httpClient = new Http2SolrClient.Builder().build()) {
+ testHttpClientConsistency(solrUrls, httpClient, null);
+ }
+
+ // No httpClient - internalClientBuilder
+ Http2SolrClient.Builder internalClientBuilder = new Http2SolrClient.Builder();
+ testHttpClientConsistency(solrUrls, null, internalClientBuilder);
+ }
+
+ private void testHttpClientConsistency(
+ List<String> solrUrls,
+ Http2SolrClient httpClient,
+ Http2SolrClient.Builder internalClientBuilder)
+ throws IOException {
+ CloudHttp2SolrClient.Builder clientBuilder = new CloudHttp2SolrClient.Builder(solrUrls);
+
+ if (httpClient != null) {
+ clientBuilder.withHttpClient(httpClient);
+ } else if (internalClientBuilder != null) {
+ clientBuilder.withInternalClientBuilder(internalClientBuilder);
+ }
+
+ try (CloudHttp2SolrClient client = clientBuilder.build()) {
+ assertEquals(
+ client.getHttpClient(),
+ ((Http2ClusterStateProvider) client.getClusterStateProvider()).getHttpClient());
+ }
+ }
}
