You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2021/03/29 17:09:27 UTC
[GitHub] [lucene-solr] madrob commented on a change in pull request #2472: SOLR-15217: Use shardsWhitelist in ReplicationHandler.
madrob commented on a change in pull request #2472:
URL: https://github.com/apache/lucene-solr/pull/2472#discussion_r603457994
##########
File path: solr/core/src/test/org/apache/solr/handler/TestReplicationHandler.java
##########
@@ -302,6 +305,26 @@ 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();
Review comment:
This is marked deprecated, is there a different way we should be enabling the checks?
##########
File path: solr/core/src/test/org/apache/solr/handler/TestReplicationHandler.java
##########
@@ -302,6 +305,26 @@ 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, "127.0.0.1:" + leaderJetty.getLocalPort()
Review comment:
Is this always going to be the case? I'm somewhat wary of Java's default hostname resolution strategies, having gotten unexpected bindings in the past.
##########
File path: solr/core/src/test/org/apache/solr/handler/TestReplicationHandlerDiskOverFlow.java
##########
@@ -67,6 +67,7 @@
@Before
public void setUp() throws Exception {
+ systemSetPropertySolrDisableShardsWhitelist("true");
Review comment:
same question here, re: deprecation
##########
File path: solr/core/src/java/org/apache/solr/handler/IndexFetcher.java
##########
@@ -271,7 +274,36 @@ 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 {
+ log.warn("The '{}' parameter value '{}' is not in the '{}'",
+ LEADER_URL, leaderUrl, HttpShardHandlerFactory.INIT_SHARDS_WHITELIST);
+ 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);
+ }
Review comment:
I'm not a fan of log-and-throw.
##########
File path: solr/core/src/java/org/apache/solr/handler/IndexFetcher.java
##########
@@ -271,7 +274,36 @@ 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 {
+ log.warn("The '{}' parameter value '{}' is not in the '{}'",
+ LEADER_URL, leaderUrl, HttpShardHandlerFactory.INIT_SHARDS_WHITELIST);
+ 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);
Review comment:
Is suggesting how to disable the shards whitelist the most prudent strategy here? Maybe a link to the docs is safer?
I feel like this is going to hurt an operator on a page somewhere.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org