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 09:37:16 UTC
[GitHub] [lucene-solr] bruno-roustant opened a new pull request #2472: SOLR-15217: Use shardsWhitelist in ReplicationHandler.
bruno-roustant opened a new pull request #2472:
URL: https://github.com/apache/lucene-solr/pull/2472
--
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
[GitHub] [lucene-solr] cholpas commented on a change in pull request #2472: SOLR-15217: Use shardsWhitelist in ReplicationHandler.
Posted by GitBox <gi...@apache.org>.
cholpas commented on a change in pull request #2472:
URL: https://github.com/apache/lucene-solr/pull/2472#discussion_r648227029
##########
File path: solr/core/src/java/org/apache/solr/handler/IndexFetcher.java
##########
@@ -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));
Review comment:
I'm not using distributed search, just old style master/slave or in new terms leader/follower replication. There is no configuration about shards, SolrCloud or whatsoever, but this whitelist check throws the following exception: `Invalid URL syntax in "shards" parameter: null`
I don't believe that this is the desired behaviour when configuring only a `solr.ReplicationHandler` in solrconfig.xml. In the documentation https://solr.apache.org/guide/8_8/index-replication.html there is no word mentioned about shards.
--
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
[GitHub] [lucene-solr] atris commented on a change in pull request #2472: SOLR-15217: Use shardsWhitelist in ReplicationHandler.
Posted by GitBox <gi...@apache.org>.
atris commented on a change in pull request #2472:
URL: https://github.com/apache/lucene-solr/pull/2472#discussion_r603153830
##########
File path: solr/core/src/java/org/apache/solr/handler/IndexFetcher.java
##########
@@ -271,7 +273,33 @@ 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 && solrCore.getCoreContainer().getShardHandlerFactory() instanceof HttpShardHandlerFactory) {
Review comment:
NIT: Lets get the ZKController once and reuse?
--
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
[GitHub] [lucene-solr] cholpas commented on a change in pull request #2472: SOLR-15217: Use shardsWhitelist in ReplicationHandler.
Posted by GitBox <gi...@apache.org>.
cholpas commented on a change in pull request #2472:
URL: https://github.com/apache/lucene-solr/pull/2472#discussion_r648376275
##########
File path: solr/core/src/java/org/apache/solr/handler/IndexFetcher.java
##########
@@ -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));
Review comment:
Hmm, yes... I tried now tried twice to create a jira ticket and spent quite some time formatting it nicely. Both times (with different IPs) when submitting the ticket I got `Your request has been blocked. If you feel this is in error, please let us know at: abuse@infra.apache.org`. Srsly, wtf... I already sent an email there, but didn't get a response.
--
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
[GitHub] [lucene-solr] bruno-roustant commented on a change in pull request #2472: SOLR-15217: Use shardsWhitelist in ReplicationHandler.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #2472:
URL: https://github.com/apache/lucene-solr/pull/2472#discussion_r604092130
##########
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:
Finally I could remove this particular use of the deprecated method in favour of settings the whitelist (which is not possible in TestReplicationHandler because it creates JettySolrRunner later so we don't know the ports ahead of time).
--
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
[GitHub] [lucene-solr] bruno-roustant commented on a change in pull request #2472: SOLR-15217: Use shardsWhitelist in ReplicationHandler.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #2472:
URL: https://github.com/apache/lucene-solr/pull/2472#discussion_r603916224
##########
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:
Good point. I'll replace by JettySolrRunner.getBaseUrl().
--
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
[GitHub] [lucene-solr] bruno-roustant merged pull request #2472: SOLR-15217: Use shardsWhitelist in ReplicationHandler.
Posted by GitBox <gi...@apache.org>.
bruno-roustant merged pull request #2472:
URL: https://github.com/apache/lucene-solr/pull/2472
--
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
[GitHub] [lucene-solr] bruno-roustant commented on a change in pull request #2472: SOLR-15217: Use shardsWhitelist in ReplicationHandler.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #2472:
URL: https://github.com/apache/lucene-solr/pull/2472#discussion_r603910635
##########
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:
For backward compatibility I have added systemSetPropertySolrDisableShardsWhitelist("true") in setUp() and systemClearPropertySolrDisableShardsWhitelist() in tearDown() as this is already done in other tests (otherwise most test methods fail).
So here the idea is to have one method that does not disable shard whitelisting. Another approach could be to move systemSetPropertySolrDisableShardsWhitelist() to each test method, but this doesn't improve readibility nor scalability if we increase the number of test cases. I would prefer to keep this call here as we are effectively in a test class that requires backqard compatibility about whitelisting.
--
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
[GitHub] [lucene-solr] cholpas commented on a change in pull request #2472: SOLR-15217: Use shardsWhitelist in ReplicationHandler.
Posted by GitBox <gi...@apache.org>.
cholpas commented on a change in pull request #2472:
URL: https://github.com/apache/lucene-solr/pull/2472#discussion_r648148773
##########
File path: solr/core/src/java/org/apache/solr/handler/IndexFetcher.java
##########
@@ -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));
Review comment:
When using Solr with only leader/follower replication and not with SolrCloud, this whitelist check is failing - unless `-Dsolr.disable.shardsWhitelist=true` is passed. In my point of view it does not make sense to have to pass this flag when SolrCloud is not being used. What do you think?
--
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
[GitHub] [lucene-solr] bruno-roustant commented on a change in pull request #2472: SOLR-15217: Use shardsWhitelist in ReplicationHandler.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #2472:
URL: https://github.com/apache/lucene-solr/pull/2472#discussion_r603919909
##########
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 copied this code from the existing HttpShardHandlerFactory.WhitelistHostChecker.checkWhiteLlist().
Yes that seems redundant. I'll remove in both places.
--
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
[GitHub] [lucene-solr] madrob commented on a change in pull request #2472: SOLR-15217: Use shardsWhitelist in ReplicationHandler.
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [lucene-solr] bruno-roustant commented on a change in pull request #2472: SOLR-15217: Use shardsWhitelist in ReplicationHandler.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #2472:
URL: https://github.com/apache/lucene-solr/pull/2472#discussion_r603926945
##########
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:
This is also for backward compatibility for an existing test. The same pattern is used for 10 other existing tests.
Maybe I can try to replace that by a dynamic solr config in the 9x patch, but should we do that here?
--
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
[GitHub] [lucene-solr] janhoy commented on a change in pull request #2472: SOLR-15217: Use shardsWhitelist in ReplicationHandler.
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #2472:
URL: https://github.com/apache/lucene-solr/pull/2472#discussion_r648234288
##########
File path: solr/core/src/java/org/apache/solr/handler/IndexFetcher.java
##########
@@ -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));
Review comment:
Ok, this is for validation of `leaderUrl` in replication handler. In SOLR-15340 the setting is renamed to `allowUrls` which better reflects the generic intent for it, such as validating any user/config provided URL in the system.
As this sounds like a bug, please file a NEW Jira issue with a clear description on how to reproduce the issue, including the Exception stacktrace you see, so we can get a bug fix in the next bugfix release.
--
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
[GitHub] [lucene-solr] bruno-roustant commented on a change in pull request #2472: SOLR-15217: Use shardsWhitelist in ReplicationHandler.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #2472:
URL: https://github.com/apache/lucene-solr/pull/2472#discussion_r603922188
##########
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:
This suggestion exists in 3 other places: HttpShardHandler, HttpShardHandlerFactory, TermsComponent.
I propose we keep it for this minimal patch for branch 8x and we can add a link to the doc in the change for main 9x?
--
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
[GitHub] [lucene-solr] janhoy commented on a change in pull request #2472: SOLR-15217: Use shardsWhitelist in ReplicationHandler.
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #2472:
URL: https://github.com/apache/lucene-solr/pull/2472#discussion_r648218229
##########
File path: solr/core/src/java/org/apache/solr/handler/IndexFetcher.java
##########
@@ -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));
Review comment:
This is by design and on purpose, please see documentation of the feature here https://solr.apache.org/guide/8_8/distributed-requests.html#configuring-the-shardhandlerfactory
In leader/follower installs, it is expected that you configure the `shardsWhitelist` setting in solr.xml with a list of all your shards, to avoid user queries with shards parameter being able to connect to any host on the network.
--
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