From 66a51b76b399ac0b018a4f600de3fc349ae7a2ec Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Sat, 10 Sep 2016 22:35:03 +0100 Subject: [PATCH] SOLR-9344: Allow multiple HttpClientConfigurers to be set on HttpClientUtil --- solr/CHANGES.txt | 5 ++ .../org/apache/solr/core/CoreContainer.java | 2 +- .../cloud/TestMiniSolrCloudClusterSSL.java | 53 ++++++++---------- .../client/solrj/impl/HttpClientUtil.java | 44 +++++++++++---- .../client/solrj/impl/HttpClientUtilTest.java | 54 ++++++++++++------- .../java/org/apache/solr/SolrTestCaseJ4.java | 5 +- 6 files changed, 100 insertions(+), 63 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 849d1b948bb..11b845d0cfe 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -88,6 +88,11 @@ Bug Fixes * SOLR-9490: Fixed bugs in BoolField that caused it to erroneously return "false" for all docs depending on usage (Colvin Cowie, Dan Fox, hossman) +* SOLR-9344: HttpClientConfigurers are now chained, rather than only one being + set at a time. This prevents a bug where BasicAuth and SSL could not be + combined on an HttpClient object. (Alan Woodward) + + Optimizations ---------------------- diff --git a/solr/core/src/java/org/apache/solr/core/CoreContainer.java b/solr/core/src/java/org/apache/solr/core/CoreContainer.java index 456775141ed..16dce9c7c3f 100644 --- a/solr/core/src/java/org/apache/solr/core/CoreContainer.java +++ b/solr/core/src/java/org/apache/solr/core/CoreContainer.java @@ -327,7 +327,7 @@ public class CoreContainer { if (authcPlugin instanceof HttpClientInterceptorPlugin) { // Setup HttpClient to use the plugin's configurer for internode communication HttpClientConfigurer configurer = ((HttpClientInterceptorPlugin) authcPlugin).getClientConfigurer(); - HttpClientUtil.setConfigurer(configurer); + HttpClientUtil.addConfigurer(configurer); // The default http client of the core container's shardHandlerFactory has already been created and // configured using the default httpclient configurer. We need to reconfigure it using the plugin's diff --git a/solr/core/src/test/org/apache/solr/cloud/TestMiniSolrCloudClusterSSL.java b/solr/core/src/test/org/apache/solr/cloud/TestMiniSolrCloudClusterSSL.java index a0111621c0d..b79e8f98aa4 100644 --- a/solr/core/src/test/org/apache/solr/cloud/TestMiniSolrCloudClusterSSL.java +++ b/solr/core/src/test/org/apache/solr/cloud/TestMiniSolrCloudClusterSSL.java @@ -16,43 +16,36 @@ */ package org.apache.solr.cloud; -import java.lang.invoke.MethodHandles; - -import java.util.Collections; -import java.util.List; +import javax.net.ssl.SSLContext; import java.io.File; import java.io.IOException; +import java.lang.invoke.MethodHandles; +import java.util.Collections; +import java.util.List; -import javax.net.ssl.SSLContext; - +import org.apache.http.client.HttpClient; +import org.apache.http.client.methods.HttpHead; +import org.apache.http.config.Registry; +import org.apache.http.config.RegistryBuilder; +import org.apache.http.conn.socket.ConnectionSocketFactory; +import org.apache.http.conn.socket.PlainConnectionSocketFactory; +import org.apache.http.conn.ssl.SSLConnectionSocketFactory; +import org.apache.http.impl.client.CloseableHttpClient; +import org.apache.http.impl.client.HttpClientBuilder; +import org.apache.http.impl.conn.PoolingHttpClientConnectionManager; +import org.apache.lucene.util.Constants; import org.apache.solr.SolrTestCaseJ4; import org.apache.solr.client.solrj.embedded.JettyConfig; import org.apache.solr.client.solrj.embedded.JettySolrRunner; import org.apache.solr.client.solrj.impl.CloudSolrClient; -import org.apache.solr.client.solrj.impl.HttpSolrClient; import org.apache.solr.client.solrj.impl.HttpClientUtil; -import org.apache.solr.client.solrj.impl.HttpClientConfigurer; +import org.apache.solr.client.solrj.impl.HttpSolrClient; import org.apache.solr.client.solrj.request.CoreAdminRequest; import org.apache.solr.common.cloud.ZkStateReader; import org.apache.solr.common.params.CoreAdminParams.CoreAdminAction; import org.apache.solr.util.SSLTestConfig; - -import org.apache.http.impl.client.CloseableHttpClient; -import org.apache.http.impl.client.HttpClientBuilder; -import org.apache.http.impl.conn.PoolingHttpClientConnectionManager; -import org.apache.http.client.HttpClient; -import org.apache.http.client.methods.HttpHead; -import org.apache.http.config.RegistryBuilder; -import org.apache.http.config.Registry; -import org.apache.http.conn.socket.ConnectionSocketFactory; -import org.apache.http.conn.socket.PlainConnectionSocketFactory; -import org.apache.http.conn.ssl.SSLConnectionSocketFactory; - -import org.apache.lucene.util.Constants; - import org.junit.After; import org.junit.Before; - import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -83,19 +76,19 @@ public class TestMiniSolrCloudClusterSSL extends SolrTestCaseJ4 { public void before() { // undo the randomization of our super class log.info("NOTE: This Test ignores the randomized SSL & clientAuth settings selected by base class"); - HttpClientUtil.setConfigurer(new HttpClientConfigurer()); + HttpClientUtil.resetConfigurers(); System.clearProperty(ZkStateReader.URL_SCHEME); } @After public void after() { - HttpClientUtil.setConfigurer(new HttpClientConfigurer()); + HttpClientUtil.resetConfigurers(); System.clearProperty(ZkStateReader.URL_SCHEME); SSLContext.setDefault(DEFAULT_SSL_CONTEXT); } public void testNoSsl() throws Exception { final SSLTestConfig sslConfig = new SSLTestConfig(false, false); - HttpClientUtil.setConfigurer(sslConfig.getHttpClientConfigurer()); + HttpClientUtil.addConfigurer(sslConfig.getHttpClientConfigurer()); System.setProperty(ZkStateReader.URL_SCHEME, "http"); checkClusterWithNodeReplacement(sslConfig); } @@ -105,14 +98,14 @@ public class TestMiniSolrCloudClusterSSL extends SolrTestCaseJ4 { // but we test it anyway for completeness of sanity checking the behavior of code that looks at those // options. final SSLTestConfig sslConfig = new SSLTestConfig(false, true); - HttpClientUtil.setConfigurer(sslConfig.getHttpClientConfigurer()); + HttpClientUtil.addConfigurer(sslConfig.getHttpClientConfigurer()); System.setProperty(ZkStateReader.URL_SCHEME, "http"); checkClusterWithNodeReplacement(sslConfig); } public void testSslAndNoClientAuth() throws Exception { final SSLTestConfig sslConfig = new SSLTestConfig(true, false); - HttpClientUtil.setConfigurer(sslConfig.getHttpClientConfigurer()); + HttpClientUtil.addConfigurer(sslConfig.getHttpClientConfigurer()); System.setProperty(ZkStateReader.URL_SCHEME, "https"); checkClusterWithNodeReplacement(sslConfig); } @@ -122,7 +115,7 @@ public class TestMiniSolrCloudClusterSSL extends SolrTestCaseJ4 { final SSLTestConfig sslConfig = new SSLTestConfig(true, true); - HttpClientUtil.setConfigurer(sslConfig.getHttpClientConfigurer()); + HttpClientUtil.addConfigurer(sslConfig.getHttpClientConfigurer()); System.setProperty(ZkStateReader.URL_SCHEME, "https"); checkClusterWithNodeReplacement(sslConfig); } @@ -149,7 +142,7 @@ public class TestMiniSolrCloudClusterSSL extends SolrTestCaseJ4 { // our test config doesn't use SSL, and reset HttpClientUtil to it's defaults so it picks up our // SSLContext that way. SSLContext.setDefault( sslConfig.isSSLMode() ? sslConfig.buildClientSSLContext() : DEFAULT_SSL_CONTEXT); - HttpClientUtil.setConfigurer(new HttpClientConfigurer()); + HttpClientUtil.resetConfigurers(); // recheck that we can communicate with all the jetty instances in our cluster checkClusterJettys(cluster, sslConfig); diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpClientUtil.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpClientUtil.java index 2ad89757e04..96f4d823950 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpClientUtil.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpClientUtil.java @@ -27,6 +27,7 @@ import java.util.List; import java.util.zip.GZIPInputStream; import java.util.zip.InflaterInputStream; +import com.google.common.collect.Lists; import org.apache.http.Header; import org.apache.http.HeaderElement; import org.apache.http.HttpEntity; @@ -93,20 +94,41 @@ public class HttpClientUtil { static final DefaultHttpRequestRetryHandler NO_RETRY = new DefaultHttpRequestRetryHandler( 0, false); - private static HttpClientConfigurer configurer = new HttpClientConfigurer(); + private static final List configurers + = Collections.synchronizedList(Lists.newArrayList(new HttpClientConfigurer())); - private static final List interceptors = Collections.synchronizedList(new ArrayList()); + private static final List interceptors = Collections.synchronizedList(new ArrayList<>()); /** - * Replace the {@link HttpClientConfigurer} class used in configuring the http - * clients with a custom implementation. + * Add a {@link HttpClientConfigurer} class used to configure the http + * clients. + * + * @deprecated use {@link #addConfigurer(HttpClientConfigurer)} */ + @Deprecated public static void setConfigurer(HttpClientConfigurer newConfigurer) { - configurer = newConfigurer; + addConfigurer(newConfigurer); } - - public static HttpClientConfigurer getConfigurer() { - return configurer; + + /** + * Add a {@link HttpClientConfigurer} to the list of configurers used + * to create http clients + */ + public static void addConfigurer(HttpClientConfigurer newConfigurer) { + configurers.add(newConfigurer); + } + + /** + * Remove a {@link HttpClientConfigurer} from the list of configurers used + * to create http clients + */ + public static void removeConfigurer(HttpClientConfigurer configurer) { + configurers.remove(configurer); + } + + public static void resetConfigurers() { + configurers.clear(); + configurers.add(new HttpClientConfigurer()); } /** @@ -146,7 +168,11 @@ public class HttpClientUtil { */ public static void configureClient(final DefaultHttpClient httpClient, SolrParams config) { - configurer.configure(httpClient, config); + synchronized (configurers) { + for (HttpClientConfigurer configurer : configurers) { + configurer.configure(httpClient, config); + } + } synchronized(interceptors) { for(HttpRequestInterceptor interceptor: interceptors) { httpClient.addRequestInterceptor(interceptor); diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpClientUtilTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpClientUtilTest.java index 9fa2b0bd0eb..a882623b0ca 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpClientUtilTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpClientUtilTest.java @@ -47,6 +47,7 @@ public class HttpClientUtilTest extends LuceneTestCase { client.close(); } + @SuppressWarnings("deprecation") @Test public void testSetParams() { ModifiableSolrParams params = new ModifiableSolrParams(); @@ -59,27 +60,25 @@ public class HttpClientUtilTest extends LuceneTestCase { params.set(HttpClientUtil.PROP_MAX_CONNECTIONS_PER_HOST, 32345); params.set(HttpClientUtil.PROP_SO_TIMEOUT, 42345); params.set(HttpClientUtil.PROP_USE_RETRY, false); - DefaultHttpClient client = (DefaultHttpClient) HttpClientUtil.createClient(params); - try { + try (DefaultHttpClient client = (DefaultHttpClient) HttpClientUtil.createClient(params)) { assertEquals(12345, HttpConnectionParams.getConnectionTimeout(client.getParams())); assertEquals(PoolingClientConnectionManager.class, client.getConnectionManager().getClass()); - assertEquals(22345, ((PoolingClientConnectionManager)client.getConnectionManager()).getMaxTotal()); - assertEquals(32345, ((PoolingClientConnectionManager)client.getConnectionManager()).getDefaultMaxPerRoute()); + assertEquals(22345, ((PoolingClientConnectionManager) client.getConnectionManager()).getMaxTotal()); + assertEquals(32345, ((PoolingClientConnectionManager) client.getConnectionManager()).getDefaultMaxPerRoute()); assertEquals(42345, HttpConnectionParams.getSoTimeout(client.getParams())); assertEquals(HttpClientUtil.NO_RETRY, client.getHttpRequestRetryHandler()); assertEquals("pass", client.getCredentialsProvider().getCredentials(new AuthScope("127.0.0.1", 1234)).getPassword()); assertEquals("user", client.getCredentialsProvider().getCredentials(new AuthScope("127.0.0.1", 1234)).getUserPrincipal().getName()); assertEquals(true, client.getParams().getParameter(ClientPNames.HANDLE_REDIRECTS)); - } finally { - client.close(); } } + @SuppressWarnings("deprecation") @Test public void testAuthSchemeConfiguration() { System.setProperty(Krb5HttpClientConfigurer.LOGIN_CONFIG_PROP, "test"); try { - HttpClientUtil.setConfigurer(new Krb5HttpClientConfigurer()); + HttpClientUtil.addConfigurer(new Krb5HttpClientConfigurer()); AbstractHttpClient client = (AbstractHttpClient)HttpClientUtil.createClient(null); assertEquals(1, client.getAuthSchemes().getSchemeNames().size()); assertTrue(AuthSchemes.SPNEGO.equalsIgnoreCase(client.getAuthSchemes().getSchemeNames().get(0))); @@ -89,30 +88,45 @@ public class HttpClientUtilTest extends LuceneTestCase { } } + @SuppressWarnings("deprecation") @Test - public void testReplaceConfigurer() throws IOException{ - - try { - final AtomicInteger counter = new AtomicInteger(); - HttpClientConfigurer custom = new HttpClientConfigurer(){ + public void testMultipleConfigurers() throws IOException { + + final AtomicInteger counter1 = new AtomicInteger(); + final AtomicInteger counter2 = new AtomicInteger(); + + HttpClientConfigurer custom1 = new HttpClientConfigurer() { @Override public void configure(DefaultHttpClient httpClient, SolrParams config) { super.configure(httpClient, config); - counter.set(config.getInt("custom-param", -1)); + counter1.set(config.getInt("custom-param", -1)); + } + }; + HttpClientConfigurer custom2 = new HttpClientConfigurer() { + @Override + public void configure(DefaultHttpClient httpClient, SolrParams config) { + super.configure(httpClient, config); + counter2.set(config.getInt("custom-param", -1)); } - }; - HttpClientUtil.setConfigurer(custom); + HttpClientUtil.addConfigurer(custom1); + HttpClientUtil.addConfigurer(custom2); ModifiableSolrParams params = new ModifiableSolrParams(); params.set("custom-param", 5); HttpClientUtil.createClient(params).close(); - assertEquals(5, counter.get()); - } finally { - //restore default configurer - HttpClientUtil.setConfigurer(new HttpClientConfigurer()); - } + assertEquals(5, counter1.get()); + assertEquals(5, counter2.get()); + + HttpClientUtil.createClient(null).close(); + assertEquals(-1, counter1.get()); + assertEquals(-1, counter2.get()); + + HttpClientUtil.removeConfigurer(custom1); + HttpClientUtil.createClient(params).close(); + assertEquals(-1, counter1.get()); + assertEquals(5, counter2.get()); } diff --git a/solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java b/solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java index 2383a3cc6fd..c15da952a3a 100644 --- a/solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java +++ b/solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java @@ -67,15 +67,14 @@ import org.apache.lucene.util.LuceneTestCase.SuppressFileSystems; import org.apache.lucene.util.LuceneTestCase.SuppressSysoutChecks; import org.apache.lucene.util.QuickPatchThreadsFilter; import org.apache.lucene.util.TestUtil; +import org.apache.solr.client.solrj.ResponseParser; import org.apache.solr.client.solrj.embedded.JettyConfig; -import org.apache.solr.client.solrj.impl.HttpClientConfigurer; import org.apache.solr.client.solrj.impl.CloudSolrClient; import org.apache.solr.client.solrj.impl.ConcurrentUpdateSolrClient; import org.apache.solr.client.solrj.impl.HttpClientUtil; import org.apache.solr.client.solrj.impl.HttpSolrClient; import org.apache.solr.client.solrj.impl.HttpSolrClient.Builder; import org.apache.solr.client.solrj.impl.LBHttpSolrClient; -import org.apache.solr.client.solrj.ResponseParser; import org.apache.solr.client.solrj.util.ClientUtils; import org.apache.solr.cloud.IpTables; import org.apache.solr.common.SolrDocument; @@ -280,7 +279,7 @@ public abstract class SolrTestCaseJ4 extends LuceneTestCase { System.clearProperty("urlScheme"); System.clearProperty("solr.peerSync.useRangeVersions"); - HttpClientUtil.setConfigurer(new HttpClientConfigurer()); + HttpClientUtil.resetConfigurers(); // clean up static sslConfig = null;