SOLR-15217: Use shardsWhitelist in ReplicationHandler.

apache · Apr 6, 2021 · 3132a6f · 3132a6f
1 parent 4454064
commit 3132a6f
Show file tree

Hide file tree

Showing 6 changed files with 79 additions and 8 deletions.
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
@@ -17,6 +17,8 @@ Bug Fixes
 
 * SOLR-15233: Set doAs param in ConfigurableInternodeAuthHadoopPlugin (Geza Nagy, Jason Gerlowski, Mike Drob)
 
+* SOLR-15217: Use shardsWhitelist in ReplicationHandler. (Bruno Roustant)
+
 ==================  8.8.1 ==================
 
 Bug Fixes

diff --git a/solr/core/src/java/org/apache/solr/handler/IndexFetcher.java b/solr/core/src/java/org/apache/solr/handler/IndexFetcher.java
@@ -78,6 +78,7 @@
 import org.apache.solr.cloud.ZkController;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.SolrException.ErrorCode;
+import org.apache.solr.common.cloud.ClusterState;
 import org.apache.solr.common.cloud.Replica;
 import org.apache.solr.common.params.CommonParams;
 import org.apache.solr.common.params.ModifiableSolrParams;
@@ -90,6 +91,8 @@
 import org.apache.solr.core.DirectoryFactory.DirContext;
 import org.apache.solr.core.IndexDeletionPolicyWrapper;
 import org.apache.solr.core.SolrCore;
+import org.apache.solr.handler.component.HttpShardHandlerFactory;
+import org.apache.solr.handler.component.ShardHandlerFactory;
 import org.apache.solr.request.LocalSolrQueryRequest;
 import org.apache.solr.request.SolrQueryRequest;
 import org.apache.solr.search.SolrIndexSearcher;
@@ -248,7 +251,7 @@ public IndexFetcher(@SuppressWarnings({"rawtypes"})final NamedList initArgs, fin
       leaderUrl = leaderUrl.substring(0, leaderUrl.length()-12);
       log.warn("'leaderUrl' must be specified without the {} suffix", ReplicationHandler.PATH);
     }
-    this.leaderUrl = leaderUrl;
+    setLeaderUrl(leaderUrl);
 
     this.replicationHandler = handler;
     String compress = (String) initArgs.get(COMPRESSION);
@@ -271,7 +274,34 @@ public IndexFetcher(@SuppressWarnings({"rawtypes"})final NamedList initArgs, fin
     String httpBasicAuthPassword = (String) initArgs.get(HttpClientUtil.PROP_BASIC_AUTH_PASS);
     myHttpClient = createHttpClient(solrCore, httpBasicAuthUser, httpBasicAuthPassword, useExternalCompression);
   }
-
+
+  private void setLeaderUrl(String leaderUrl) {
+    if (leaderUrl != null) {
+      ShardHandlerFactory shardHandlerFactory = solrCore.getCoreContainer().getShardHandlerFactory();
+      if (shardHandlerFactory instanceof HttpShardHandlerFactory) {
+        ZkController zkController = solrCore.getCoreContainer().getZkController();
+        ClusterState clusterState = zkController == null ? null : zkController.getClusterState();
+        try {
+          ((HttpShardHandlerFactory) shardHandlerFactory).getWhitelistHostChecker()
+              .checkWhitelist(clusterState, null, Collections.singletonList(leaderUrl));
+        } catch (SolrException e) {
+          // Replace the exception because the message is about the 'shard' parameter, which is not right here.
+          // This code is refactored and cleaned up in 9.x and above.
+          if (e.code() == ErrorCode.BAD_REQUEST.code) {
+            throw new SolrException(ErrorCode.BAD_REQUEST,
+                "Invalid URL syntax in '" + LEADER_URL + "' with value '" + leaderUrl + "'", e);
+          } else {
+            throw new SolrException(SolrException.ErrorCode.FORBIDDEN,
+                "The '" + LEADER_URL + "' parameter value '" + leaderUrl
+                    + "' is not in the '" + HttpShardHandlerFactory.INIT_SHARDS_WHITELIST + "'."
+                    + HttpShardHandlerFactory.SET_SOLR_DISABLE_SHARDS_WHITELIST_CLUE);
+          }
+        }
+      }
+    }
+    this.leaderUrl = leaderUrl;
+  }
+
   @SuppressWarnings({"unchecked"})
   protected <T> T getParameter(@SuppressWarnings({"rawtypes"})NamedList initArgs, String configKey, T defaultValue, StringBuilder sb) {
     T toReturn = defaultValue;
@@ -405,7 +435,7 @@ IndexFetchResult fetchLatestIndex(boolean forceReplication, boolean forceCoreRel
           return IndexFetchResult.LEADER_IS_NOT_ACTIVE;
         }
         if (!replica.getCoreUrl().equals(leaderUrl)) {
-          leaderUrl = replica.getCoreUrl();
+          setLeaderUrl(replica.getCoreUrl());
           log.info("Updated leaderUrl to {}", leaderUrl);
           // TODO: Do we need to set forceReplication = true?
         } else {

diff --git a/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java b/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
@@ -140,9 +140,9 @@ public class HttpShardHandlerFactory extends ShardHandlerFactory implements org.
 
   public static final String INIT_SHARDS_WHITELIST = "shardsWhitelist";
 
-  static final String INIT_SOLR_DISABLE_SHARDS_WHITELIST = "solr.disable." + INIT_SHARDS_WHITELIST;
+  public static final String INIT_SOLR_DISABLE_SHARDS_WHITELIST = "solr.disable." + INIT_SHARDS_WHITELIST;
 
-  static final String SET_SOLR_DISABLE_SHARDS_WHITELIST_CLUE = " set -D"+INIT_SOLR_DISABLE_SHARDS_WHITELIST+"=true to disable shards whitelist checks";
+  public static final String SET_SOLR_DISABLE_SHARDS_WHITELIST_CLUE = " set -D"+INIT_SOLR_DISABLE_SHARDS_WHITELIST+"=true to disable shards whitelist checks";
 
   /**
    * Get {@link ShardHandler} that uses the default http client.
@@ -510,7 +510,7 @@ protected void checkWhitelist(String shardsParamValue, List<String> shardUrls) {
      * @param shardsParamValue The original shards parameter
      * @param shardUrls The list of cores generated from the shards parameter.
      */
-    protected void checkWhitelist(ClusterState clusterState, String shardsParamValue, List<String> shardUrls) {
+    public void checkWhitelist(ClusterState clusterState, String shardsParamValue, List<String> shardUrls) {
       if (!whitelistHostCheckingEnabled) {
         return;
       }
@@ -540,8 +540,6 @@ protected void checkWhitelist(ClusterState clusterState, String shardsParamValue
           throw new SolrException(ErrorCode.BAD_REQUEST, "Invalid URL syntax in \"shards\" parameter: " + shardsParamValue);
         }
         if (!localWhitelistHosts.contains(url.getHost() + ":" + url.getPort())) {
-          log.warn("The '{}' parameter value '{}' contained value(s) not on the shards whitelist ({}), shardUrl: '{}'"
-              , ShardParams.SHARDS, shardsParamValue, localWhitelistHosts, shardUrl);
           throw new SolrException(ErrorCode.FORBIDDEN,
               "The '"+ShardParams.SHARDS+"' parameter value '"+shardsParamValue+"' contained value(s) not on the shards whitelist. shardUrl:" + shardUrl + "." +
                   HttpShardHandlerFactory.SET_SOLR_DISABLE_SHARDS_WHITELIST_CLUE);

diff --git a/solr/core/src/test/org/apache/solr/cloud/cdcr/CdcrBootstrapTest.java b/solr/core/src/test/org/apache/solr/cloud/cdcr/CdcrBootstrapTest.java
@@ -37,6 +37,8 @@
 import org.apache.solr.common.params.CommonParams;
 import org.apache.solr.common.params.ModifiableSolrParams;
 import org.apache.solr.handler.CdcrParams;
+import org.junit.After;
+import org.junit.Before;
 import org.junit.Test;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -45,6 +47,20 @@ public class CdcrBootstrapTest extends SolrTestCaseJ4 {
 
   private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
 
+  @Override
+  @Before
+  public void setUp() throws Exception {
+    super.setUp();
+    systemSetPropertySolrDisableShardsWhitelist("true");
+  }
+
+  @Override
+  @After
+  public void tearDown() throws Exception {
+    super.tearDown();
+    systemClearPropertySolrDisableShardsWhitelist();
+  }
+
   /**
    * Starts a source cluster with no CDCR configuration, indexes enough documents such that
    * the at least one old tlog is closed and thrown away so that the source cluster does not have

diff --git a/solr/core/src/test/org/apache/solr/handler/TestReplicationHandler.java b/solr/core/src/test/org/apache/solr/handler/TestReplicationHandler.java
@@ -79,6 +79,7 @@
 import org.apache.solr.core.SolrCore;
 import org.apache.solr.core.StandardDirectoryFactory;
 import org.apache.solr.core.snapshots.SolrSnapshotMetaDataManager;
+import org.apache.solr.handler.component.HttpShardHandlerFactory;
 import org.apache.solr.util.FileUtils;
 import org.apache.solr.util.TestInjection;
 import org.apache.solr.util.TimeOut;
@@ -131,6 +132,7 @@ public static void beforeClass() {
   @Before
   public void setUp() throws Exception {
     super.setUp();
+    systemSetPropertySolrDisableShardsWhitelist("true");
 //    System.setProperty("solr.directoryFactory", "solr.StandardDirectoryFactory");
     // For manual testing only
     // useFactory(null); // force an FS factory.
@@ -160,6 +162,7 @@ public void clearIndexWithReplication() throws Exception {
   @After
   public void tearDown() throws Exception {
     super.tearDown();
+    systemClearPropertySolrDisableShardsWhitelist();
     if (null != leaderJetty) {
       leaderJetty.stop();
       leaderJetty = null;
@@ -302,6 +305,25 @@ public void doTestHandlerPathUnchanged() throws Exception {
     assertEquals("/replication", ReplicationHandler.PATH);
   }
 
+  @Test
+  public void testShardsWhitelist() throws Exception {
+    // Run another test with shards whitelist enabled and whitelist is empty.
+    // Expect an exception because the leader URL is not allowed.
+    systemClearPropertySolrDisableShardsWhitelist();
+    SolrException e = expectThrows(SolrException.class, this::doTestDetails);
+    assertTrue(e.getMessage().contains("is not in the '" + HttpShardHandlerFactory.INIT_SHARDS_WHITELIST + "'"));
+
+    // Set the whitelist to allow the leader URL.
+    // Expect the same test to pass now.
+    String propertyName = "solr.tests." + HttpShardHandlerFactory.INIT_SHARDS_WHITELIST;
+    System.setProperty(propertyName, leaderJetty.getBaseUrl() + "," + followerJetty.getBaseUrl());
+    try {
+      doTestDetails();
+    } finally {
+      System.clearProperty(propertyName);
+    }
+  }
+
   @Test
   public void doTestDetails() throws Exception {
     followerJetty.stop();

diff --git a/solr/core/src/test/org/apache/solr/handler/TestReplicationHandlerDiskOverFlow.java b/solr/core/src/test/org/apache/solr/handler/TestReplicationHandlerDiskOverFlow.java
@@ -37,6 +37,7 @@
 import org.apache.solr.client.solrj.response.QueryResponse;
 import org.apache.solr.common.util.Utils;
 import org.apache.solr.common.SolrException;
+import org.apache.solr.handler.component.HttpShardHandlerFactory;
 import org.apache.solr.util.LogLevel;
 import org.junit.After;
 import org.junit.Before;
@@ -78,6 +79,7 @@ public void setUp() throws Exception {
     leader.setUp();
     leaderJetty = createAndStartJetty(leader);
     leaderClient = createNewSolrClient(leaderJetty.getLocalPort());
+    System.setProperty("solr.tests." + HttpShardHandlerFactory.INIT_SHARDS_WHITELIST, leaderJetty.getBaseUrl().toString());
 
     follower = new TestReplicationHandler.SolrInstance(createTempDir("solr-instance").toFile(), "follower", leaderJetty.getLocalPort());
     follower.setUp();
@@ -108,6 +110,7 @@ public void tearDown() throws Exception {
       followerClient.close();
       followerClient = null;
     }
+    System.clearProperty("solr.tests." + HttpShardHandlerFactory.INIT_SHARDS_WHITELIST);
     System.clearProperty("solr.indexfetcher.sotimeout");
 
     IndexFetcher.usableDiskSpaceProvider = originalDiskSpaceprovider;