Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-9604

Pooled SSL connections are not being reused with client authentication

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.3, 7.0
    • Component/s: None
    • Security Level: Public (Default Security Level. Issues are Public)
    • Labels:
      None

      Description

      Solr isn't setting user tokens on any of its HttpClientContext objects when requested new http connections. This means that when SSL + client authentication is used, HttpClient is creating a new connection on every request, to ensure that authentication tokens aren't shared between different users. We end up with lots of unused open connections in the connection pool, leading to slowness and out-of-memory errors.

      1. fails with null.png
        760 kB
        Mikhail Khludnev
      2. SOLR-9604.patch
        22 kB
        Alan Woodward
      3. SOLR-9604.patch
        21 kB
        Alan Woodward
      4. SOLR-9604-6x.patch
        27 kB
        Mikhail Khludnev

        Issue Links

          Activity

          Hide
          romseygeek Alan Woodward added a comment -

          Patch, based on Mikhail Khludnev's last patch on SOLR-9182.

          There are a few other places in addition to HttpSolrClient that need an HttpClientContext, so I added a cacheKey object to the context factory. HttpSolrClient can pass itself, as can ConcurrentUpdateSolrClient. The UpdateHandler client uses its parent CoreContainer as a key.

          It may make sense to allow users to pass in cache keys as well, particularly for CUSC, which is used by DistributedUpdateProcessor, and can share connection pools with other clients in a container.

          Show
          romseygeek Alan Woodward added a comment - Patch, based on Mikhail Khludnev 's last patch on SOLR-9182 . There are a few other places in addition to HttpSolrClient that need an HttpClientContext, so I added a cacheKey object to the context factory. HttpSolrClient can pass itself, as can ConcurrentUpdateSolrClient. The UpdateHandler client uses its parent CoreContainer as a key. It may make sense to allow users to pass in cache keys as well, particularly for CUSC, which is used by DistributedUpdateProcessor, and can share connection pools with other clients in a container.
          Hide
          romseygeek Alan Woodward added a comment -

          The concurrent client in DistributedUpdateProcessor actually already uses the HttpClient object from UpdateHandler, so I think this is ready to go.

          Show
          romseygeek Alan Woodward added a comment - The concurrent client in DistributedUpdateProcessor actually already uses the HttpClient object from UpdateHandler, so I think this is ready to go.
          Hide
          mkhludnev Mikhail Khludnev added a comment -

          seems nice. but I couldn't find a method definition for

          HttpClientUtil.createNewHttpClientRequestContext(null) 
          

          couldn't you forget to git add ?

          Show
          mkhludnev Mikhail Khludnev added a comment - seems nice. but I couldn't find a method definition for HttpClientUtil.createNewHttpClientRequestContext( null ) couldn't you forget to git add ?
          Hide
          romseygeek Alan Woodward added a comment -

          It's there in the patch:

          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 b9580b8..0f738c2 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
          @@ -198,7 +198,12 @@ public class HttpClientUtil {
              *          configuration (no additional configuration) is created. 
              */
             public static CloseableHttpClient createClient(SolrParams params) {
          -    return createClient(params, new PoolingHttpClientConnectionManager(schemaRegistryProvider.getSchemaRegistry()));
          +    return createClient(params, createPoolingConnectionManager());
          +  }
          +
          +  /** test usage subject to change @lucene.experimental */ 
          +  static PoolingHttpClientConnectionManager createPoolingConnectionManager() {
          +    return new PoolingHttpClientConnectionManager(schemaRegistryProvider.getSchemaRegistry());
             }
             
             public static CloseableHttpClient createClient(SolrParams params, PoolingHttpClientConnectionManager cm) {
          @@ -396,10 +401,17 @@ public class HttpClientUtil {
             }
           
             /**
          -   * 
          +   * Create a HttpClientContext object
          +   *
          +   * If the client is going to be re-used, then you should pass in an object that
          +   * can be used by internal connection pools as a cache key.  This is particularly
          +   * important if client authentication is enabled, as SSL connections will not
          +   * be re-used if no cache key is provided.
          +   *
          +   * @param cacheKey an Object to be used as a cache key for pooling connections
              */
          -  public static HttpClientContext createNewHttpClientRequestContext() {
          -    return httpClientRequestContextBuilder.createContext();
          +  public static HttpClientContext createNewHttpClientRequestContext(Object cacheKey) {
          +    return httpClientRequestContextBuilder.createContext(cacheKey);
             }
          
          Show
          romseygeek Alan Woodward added a comment - It's there in the patch: 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 b9580b8..0f738c2 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 @@ -198,7 +198,12 @@ public class HttpClientUtil { * configuration (no additional configuration) is created. */ public static CloseableHttpClient createClient(SolrParams params) { - return createClient(params, new PoolingHttpClientConnectionManager(schemaRegistryProvider.getSchemaRegistry())); + return createClient(params, createPoolingConnectionManager()); + } + + /** test usage subject to change @lucene.experimental */ + static PoolingHttpClientConnectionManager createPoolingConnectionManager() { + return new PoolingHttpClientConnectionManager(schemaRegistryProvider.getSchemaRegistry()); } public static CloseableHttpClient createClient(SolrParams params, PoolingHttpClientConnectionManager cm) { @@ -396,10 +401,17 @@ public class HttpClientUtil { } /** - * + * Create a HttpClientContext object + * + * If the client is going to be re-used, then you should pass in an object that + * can be used by internal connection pools as a cache key. This is particularly + * important if client authentication is enabled, as SSL connections will not + * be re-used if no cache key is provided. + * + * @param cacheKey an Object to be used as a cache key for pooling connections */ - public static HttpClientContext createNewHttpClientRequestContext() { - return httpClientRequestContextBuilder.createContext(); + public static HttpClientContext createNewHttpClientRequestContext( Object cacheKey) { + return httpClientRequestContextBuilder.createContext(cacheKey); }
          Hide
          mkhludnev Mikhail Khludnev added a comment -

          ok. my fault. Now I see. The patch amends arguments of public methods, what's about API compatibility?

          Show
          mkhludnev Mikhail Khludnev added a comment - ok. my fault. Now I see. The patch amends arguments of public methods, what's about API compatibility?
          Hide
          mkhludnev Mikhail Khludnev added a comment -

          also, it's worth to remove @RandomSSL from HttpSolrClientSSLAuthConPoolTest to make it really random.

          Show
          mkhludnev Mikhail Khludnev added a comment - also, it's worth to remove @RandomSSL from HttpSolrClientSSLAuthConPoolTest to make it really random.
          Hide
          romseygeek Alan Woodward added a comment -

          Updated with the old methods left in with deprecations.

          Mikhail Khludnev, can you ensure that the test fails if you change HttpSolrClient so that it passes 'null' instead of 'this' to the context builder? It still passes for me locally...

          Show
          romseygeek Alan Woodward added a comment - Updated with the old methods left in with deprecations. Mikhail Khludnev , can you ensure that the test fails if you change HttpSolrClient so that it passes 'null' instead of 'this' to the context builder? It still passes for me locally...
          Hide
          mkhludnev Mikhail Khludnev added a comment - - edited

          Mikhail Khludnev, can you ensure that the test fails if you change HttpSolrClient so that it passes 'null' instead of 'this' to the context builder?

          absolutely

          Show
          mkhludnev Mikhail Khludnev added a comment - - edited Mikhail Khludnev, can you ensure that the test fails if you change HttpSolrClient so that it passes 'null' instead of 'this' to the context builder? absolutely
          Hide
          romseygeek Alan Woodward added a comment -

          Спасибо!

          I'll commit this shortly, and then we can try removing the SuppressSSL annotation from some tests and see if they pass. I think it's worth keeping the RandomizeSSL annotation on this test though, as it's only really applicable to SSL+clientAuth situations.

          Show
          romseygeek Alan Woodward added a comment - Спасибо! I'll commit this shortly, and then we can try removing the SuppressSSL annotation from some tests and see if they pass. I think it's worth keeping the RandomizeSSL annotation on this test though, as it's only really applicable to SSL+clientAuth situations.
          Hide
          romseygeek Alan Woodward added a comment -

          Ah, of course, 6.x is using a completely different way of configuring HttpClients... I'll put up a separate 6.x patch shortly.

          Show
          romseygeek Alan Woodward added a comment - Ah, of course, 6.x is using a completely different way of configuring HttpClients... I'll put up a separate 6.x patch shortly.
          Hide
          romseygeek Alan Woodward added a comment -

          OK, not being able to reproduce this behaviour at all locally is making this quite tricky to write - Mikhail Khludnev do you think you'll have any time to backport the above patch to 6.x?

          Show
          romseygeek Alan Woodward added a comment - OK, not being able to reproduce this behaviour at all locally is making this quite tricky to write - Mikhail Khludnev do you think you'll have any time to backport the above patch to 6.x?
          Hide
          mkhludnev Mikhail Khludnev added a comment -

          No problem. Feel free to push in master what you have, I'll take care about porting to branch_6x

          Show
          mkhludnev Mikhail Khludnev added a comment - No problem. Feel free to push in master what you have, I'll take care about porting to branch_6x
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 0eb6b1c823d347319cc0894b5fea95f085d4c8d4 in lucene-solr's branch refs/heads/master from Alan Woodward
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=0eb6b1c ]

          SOLR-9604: Ensure SSL connections are re-used

          Show
          jira-bot ASF subversion and git services added a comment - Commit 0eb6b1c823d347319cc0894b5fea95f085d4c8d4 in lucene-solr's branch refs/heads/master from Alan Woodward [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=0eb6b1c ] SOLR-9604 : Ensure SSL connections are re-used
          Hide
          hossman Hoss Man added a comment -

          also, it's worth to remove @RandomSSL from HttpSolrClientSSLAuthConPoolTest to make it really random.

          I think it's worth keeping the RandomizeSSL annotation on this test though, as it's only really applicable to SSL+clientAuth situations.

          Agreed, but I think I also see Mikhail's point: from a coverage standpoint, it would be nice to have a test that helps ensure we are re-using connection pools regardless of whether SSL+clientAuth is used or not (ie: as the test stands now, we only ensure they are re-used with SSL, some future bug could break connection re-use for non-ssl connections and we'd never know – heck: in theory the changes in this fix for SSL connection reuse could be breaking connection reuse for non-SSL requests, and we wouldn't know it.)

          So perhaps refactor the current HttpSolrClientSSLAuthConPoolTest class into a HttpSolrClientConPoolTest that uses the default SSL randomization, and then make HttpSolrClientSSLAuthConPoolTest a subclass that forces @RandomizeSSL(1.0) ?
          (along with a "test the test" sanity assertion that the urls start with "https")

          Show
          hossman Hoss Man added a comment - also, it's worth to remove @RandomSSL from HttpSolrClientSSLAuthConPoolTest to make it really random. I think it's worth keeping the RandomizeSSL annotation on this test though, as it's only really applicable to SSL+clientAuth situations. Agreed, but I think I also see Mikhail's point: from a coverage standpoint, it would be nice to have a test that helps ensure we are re-using connection pools regardless of whether SSL+clientAuth is used or not (ie: as the test stands now, we only ensure they are re-used with SSL, some future bug could break connection re-use for non-ssl connections and we'd never know – heck: in theory the changes in this fix for SSL connection reuse could be breaking connection reuse for non-SSL requests, and we wouldn't know it.) So perhaps refactor the current HttpSolrClientSSLAuthConPoolTest class into a HttpSolrClientConPoolTest that uses the default SSL randomization, and then make HttpSolrClientSSLAuthConPoolTest a subclass that forces @RandomizeSSL(1.0) ? (along with a "test the test" sanity assertion that the urls start with "https")
          Hide
          mkhludnev Mikhail Khludnev added a comment -

          Sigh..

          Show
          mkhludnev Mikhail Khludnev added a comment - Sigh..
          Hide
          mkhludnev Mikhail Khludnev added a comment -

          OK, not being able to reproduce this behaviour at all locally

          And I probably know why

          Show
          mkhludnev Mikhail Khludnev added a comment - OK, not being able to reproduce this behaviour at all locally And I probably know why
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 36b3b0884a8f745bc137db1b1dc9890a59fa0895 in lucene-solr's branch refs/heads/master from Alan Woodward
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=36b3b08 ]

          Revert "SOLR-9604: Ensure SSL connections are re-used"

          This reverts commit 0eb6b1c823d347319cc0894b5fea95f085d4c8d4, which was
          causing test failures in ConnectionReuseTest - see SOLR-9608

          Show
          jira-bot ASF subversion and git services added a comment - Commit 36b3b0884a8f745bc137db1b1dc9890a59fa0895 in lucene-solr's branch refs/heads/master from Alan Woodward [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=36b3b08 ] Revert " SOLR-9604 : Ensure SSL connections are re-used" This reverts commit 0eb6b1c823d347319cc0894b5fea95f085d4c8d4, which was causing test failures in ConnectionReuseTest - see SOLR-9608
          Hide
          romseygeek Alan Woodward added a comment -

          My commit didn't take into account CloudSolrClient, which also needs fixing here, and broke TestConnectionReuse (except when it selected CSC to test, which it obviously did when I ran things locally), so I've backed it out.

          Show
          romseygeek Alan Woodward added a comment - My commit didn't take into account CloudSolrClient, which also needs fixing here, and broke TestConnectionReuse (except when it selected CSC to test, which it obviously did when I ran things locally), so I've backed it out.
          Hide
          mkhludnev Mikhail Khludnev added a comment - - edited

          attaching SOLR-9604-6x.patch
          HttpSolrClient*ConPoolTest works (and fails) as expected.
          ConnectionReuseTest asserts makes me wonder (I swapped two lines to fix one failure for null metrics). It fails so far SOLR-9608.

          Show
          mkhludnev Mikhail Khludnev added a comment - - edited attaching SOLR-9604-6x.patch HttpSolrClient*ConPoolTest works (and fails) as expected. ConnectionReuseTest asserts makes me wonder (I swapped two lines to fix one failure for null metrics). It fails so far SOLR-9608 .
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit b58ccc3906014fb13ecffe17ae989ea7d07b814e in lucene-solr's branch refs/heads/branch_6x from Alan Woodward
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b58ccc3 ]

          SOLR-9604,SOLR-9608: Ensure SSL connections are re-used.

          Fix ConnectionReuseTest. Add coverage for all SolrClients.
          Remove explicit cacheKey for HttpRequestContext, make it singleton.

          Show
          jira-bot ASF subversion and git services added a comment - Commit b58ccc3906014fb13ecffe17ae989ea7d07b814e in lucene-solr's branch refs/heads/branch_6x from Alan Woodward [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b58ccc3 ] SOLR-9604 , SOLR-9608 : Ensure SSL connections are re-used. Fix ConnectionReuseTest. Add coverage for all SolrClients. Remove explicit cacheKey for HttpRequestContext, make it singleton.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit f22b1da261b93f60687431b161828e2fb27fdc8f in lucene-solr's branch refs/heads/master from Alan Woodward
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=f22b1da ]

          SOLR-9604,SOLR-9608: Ensure SSL connections are re-used

          Fix ConnectionReuseTest. Add coverage for all SolrClients.
          Remove explicit cacheKey for HttpRequestContext, make it singleton.

          Show
          jira-bot ASF subversion and git services added a comment - Commit f22b1da261b93f60687431b161828e2fb27fdc8f in lucene-solr's branch refs/heads/master from Alan Woodward [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=f22b1da ] SOLR-9604 , SOLR-9608 : Ensure SSL connections are re-used Fix ConnectionReuseTest. Add coverage for all SolrClients. Remove explicit cacheKey for HttpRequestContext, make it singleton.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit d8bb56d4ad3177b34349fa106cdb3edfe9c711df in lucene-solr's branch refs/heads/master from Mikhail Khludnev
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d8bb56d ]

          SOLR-9604,SOLR-9608: fix HttpSolrClientConPoolTest.testPoolSize()

          Show
          jira-bot ASF subversion and git services added a comment - Commit d8bb56d4ad3177b34349fa106cdb3edfe9c711df in lucene-solr's branch refs/heads/master from Mikhail Khludnev [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d8bb56d ] SOLR-9604 , SOLR-9608 : fix HttpSolrClientConPoolTest.testPoolSize()
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 2b5fc01a5d6b1bb2245fa0d5cf854637020f0c97 in lucene-solr's branch refs/heads/branch_6x from Mikhail Khludnev
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2b5fc01 ]

          SOLR-9604,SOLR-9608: fix HttpSolrClientConPoolTest.testPoolSize()

          Show
          jira-bot ASF subversion and git services added a comment - Commit 2b5fc01a5d6b1bb2245fa0d5cf854637020f0c97 in lucene-solr's branch refs/heads/branch_6x from Mikhail Khludnev [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2b5fc01 ] SOLR-9604 , SOLR-9608 : fix HttpSolrClientConPoolTest.testPoolSize()
          Hide
          shalinmangar Shalin Shekhar Mangar added a comment -

          Closing after 6.3.0 release.

          Show
          shalinmangar Shalin Shekhar Mangar added a comment - Closing after 6.3.0 release.

            People

            • Assignee:
              romseygeek Alan Woodward
              Reporter:
              romseygeek Alan Woodward
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development