This is an automated email from the ASF dual-hosted git repository.
broustant pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/solr.git
The following commit(s) were added to refs/heads/main by this push:
new 83391964df5 SOLR-17574: Move host allow list cache to
AllowListUrlChecker. (#2892)
83391964df5 is described below
commit 83391964df5b01839329be44fb31a030864770c1
Author: Bruno Roustant <33934988+bruno-roust...@users.noreply.github.com>
AuthorDate: Wed Dec 4 11:23:26 2024 +0100
SOLR-17574: Move host allow list cache to AllowListUrlChecker. (#2892)
---
solr/CHANGES.txt | 2 +
.../apache/solr/security/AllowListUrlChecker.java | 36 ++++++++++++++--
.../handler/component/TestShardHandlerFactory.java | 17 --------
.../solr/security/AllowListUrlCheckerTest.java | 48 ++++++++++++++++++++++
.../org/apache/solr/common/cloud/ClusterState.java | 27 +++---------
5 files changed, 89 insertions(+), 41 deletions(-)
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index 9a5f4aae64b..68b61331a79 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -234,6 +234,8 @@ Bug Fixes
* SOLR-17575: Fixed broken backwards compatibility with the legacy
"langid.whitelist" config in Solr Langid. (Jan Høydahl, Alexander Zagniotov)
+* SOLR-17574: Fix AllowListUrlChecker when liveNodes changes. Remove
ClusterState.getHostAllowList (Bruno Roustant, David Smiley)
+
Dependency Upgrades
---------------------
(No changes)
diff --git
a/solr/core/src/java/org/apache/solr/security/AllowListUrlChecker.java
b/solr/core/src/java/org/apache/solr/security/AllowListUrlChecker.java
index 9bcede9b060..9fbffc4cdfb 100644
--- a/solr/core/src/java/org/apache/solr/security/AllowListUrlChecker.java
+++ b/solr/core/src/java/org/apache/solr/security/AllowListUrlChecker.java
@@ -27,6 +27,7 @@ import java.util.List;
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
+import java.util.stream.Collectors;
import org.apache.solr.common.SolrException;
import org.apache.solr.common.cloud.ClusterState;
import org.apache.solr.core.NodeConfig;
@@ -85,6 +86,9 @@ public class AllowListUrlChecker {
/** Allow list of hosts. Elements in the list will be host:port (no protocol
or context). */
private final Set<String> hostAllowList;
+ private volatile Set<String> liveHostUrlsCache;
+ private volatile Set<String> liveNodesCache;
+
/**
* @param urlAllowList List of allowed URLs. URLs must be well-formed,
missing protocol is
* tolerated. An empty list means there is no explicit allow-list of
URLs, in this case no URL
@@ -136,11 +140,10 @@ public class AllowListUrlChecker {
*/
public void checkAllowList(List<String> urls, ClusterState clusterState)
throws MalformedURLException {
- Set<String> clusterHostAllowList =
- clusterState == null ? Collections.emptySet() :
clusterState.getHostAllowList();
+ Set<String> liveHostUrls = getLiveHostUrls(clusterState);
for (String url : urls) {
String hostPort = parseHostPort(url);
- if (clusterHostAllowList.stream().noneMatch(hostPort::equalsIgnoreCase)
+ if (liveHostUrls.stream().noneMatch(hostPort::equalsIgnoreCase)
&& hostAllowList.stream().noneMatch(hostPort::equalsIgnoreCase)) {
throw new SolrException(
SolrException.ErrorCode.FORBIDDEN,
@@ -154,6 +157,33 @@ public class AllowListUrlChecker {
}
}
+ /**
+ * Gets the set of live hosts urls (host:port) built from the set of live
nodes. The set is cached
+ * to be reused until the live nodes change.
+ */
+ private Set<String> getLiveHostUrls(ClusterState clusterState) {
+ if (clusterState == null) {
+ return Set.of();
+ }
+ if (liveHostUrlsCache == null || clusterState.getLiveNodes() !=
liveNodesCache) {
+ synchronized (this) {
+ Set<String> liveNodes = clusterState.getLiveNodes();
+ if (liveHostUrlsCache == null || liveNodes != liveNodesCache) {
+ liveHostUrlsCache = buildLiveHostUrls(liveNodes);
+ liveNodesCache = liveNodes;
+ }
+ }
+ }
+ return liveHostUrlsCache;
+ }
+
+ @VisibleForTesting
+ Set<String> buildLiveHostUrls(Set<String> liveNodes) {
+ return liveNodes.stream()
+ .map((liveNode) -> liveNode.substring(0, liveNode.indexOf('_')))
+ .collect(Collectors.toSet());
+ }
+
/** Whether this checker has been created with a non-empty allow-list of
URLs. */
public boolean hasExplicitAllowList() {
return !hostAllowList.isEmpty();
diff --git
a/solr/core/src/test/org/apache/solr/handler/component/TestShardHandlerFactory.java
b/solr/core/src/test/org/apache/solr/handler/component/TestShardHandlerFactory.java
index 541d2d845f3..970fa7bb48a 100644
---
a/solr/core/src/test/org/apache/solr/handler/component/TestShardHandlerFactory.java
+++
b/solr/core/src/test/org/apache/solr/handler/component/TestShardHandlerFactory.java
@@ -17,21 +17,16 @@
package org.apache.solr.handler.component;
import static org.hamcrest.CoreMatchers.equalTo;
-import static org.hamcrest.CoreMatchers.hasItem;
import static org.hamcrest.CoreMatchers.instanceOf;
-import static org.hamcrest.CoreMatchers.is;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.Arrays;
-import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
-import java.util.Set;
import org.apache.solr.SolrTestCaseJ4;
import org.apache.solr.client.solrj.impl.LBSolrClient;
import org.apache.solr.client.solrj.request.QueryRequest;
-import org.apache.solr.common.cloud.ClusterState;
import org.apache.solr.common.params.CommonParams;
import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.common.params.ShardParams;
@@ -155,18 +150,6 @@ public class TestShardHandlerFactory extends
SolrTestCaseJ4 {
}
}
- @Test
- public void testLiveNodesToHostUrl() {
- Set<String> liveNodes =
- new HashSet<>(Arrays.asList("1.2.3.4:8983_solr", "1.2.3.4:9000_",
"1.2.3.4:9001_solr-2"));
- ClusterState cs = new ClusterState(liveNodes, new HashMap<>());
- Set<String> hostSet = cs.getHostAllowList();
- assertThat(hostSet.size(), is(3));
- assertThat(hostSet, hasItem("1.2.3.4:8983"));
- assertThat(hostSet, hasItem("1.2.3.4:9000"));
- assertThat(hostSet, hasItem("1.2.3.4:9001"));
- }
-
@Test
public void testXML() {
Path home = TEST_PATH();
diff --git
a/solr/core/src/test/org/apache/solr/security/AllowListUrlCheckerTest.java
b/solr/core/src/test/org/apache/solr/security/AllowListUrlCheckerTest.java
index b32c2124c15..0a4f57ba5af 100644
--- a/solr/core/src/test/org/apache/solr/security/AllowListUrlCheckerTest.java
+++ b/solr/core/src/test/org/apache/solr/security/AllowListUrlCheckerTest.java
@@ -24,11 +24,14 @@ import static org.hamcrest.CoreMatchers.is;
import java.net.MalformedURLException;
import java.util.Arrays;
import java.util.Collections;
+import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
import org.apache.solr.SolrTestCaseJ4;
import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ClusterState;
import org.junit.Test;
/** Tests {@link AllowListUrlChecker}. */
@@ -196,6 +199,51 @@ public class AllowListUrlCheckerTest extends
SolrTestCaseJ4 {
equalTo(AllowListUrlChecker.parseHostPorts(urls("https://abc-1.com:8983/solr";))));
}
+ @Test
+ public void testLiveNodesToHostUrlCache() throws Exception {
+ // Given some live nodes defined in the cluster state.
+ Set<String> liveNodes = Set.of("1.2.3.4:8983_solr", "1.2.3.4:9000_",
"1.2.3.4:9001_solr-2");
+ ClusterState clusterState1 = new ClusterState(liveNodes, new HashMap<>());
+
+ // When we call the AllowListUrlChecker.checkAllowList method on both
valid and invalid urls.
+ AtomicInteger callCount = new AtomicInteger();
+ AllowListUrlChecker checker =
+ new AllowListUrlChecker(List.of()) {
+ @Override
+ Set<String> buildLiveHostUrls(Set<String> liveNodes) {
+ callCount.incrementAndGet();
+ return super.buildLiveHostUrls(liveNodes);
+ }
+ };
+ for (int i = 0; i < 3; i++) {
+ checker.checkAllowList(
+ List.of("1.2.3.4:8983", "1.2.3.4:9000", "1.2.3.4:9001"),
clusterState1);
+ SolrException exception =
+ expectThrows(
+ SolrException.class,
+ () -> checker.checkAllowList(List.of("1.1.3.4:8983"),
clusterState1));
+ assertThat(exception.code(),
equalTo(SolrException.ErrorCode.FORBIDDEN.code));
+ }
+ // Then we verify that the AllowListUrlChecker caches the live host urls
and only builds them
+ // once.
+ assertThat(callCount.get(), equalTo(1));
+
+ // And when the ClusterState live nodes change.
+ liveNodes = Set.of("2.3.4.5:8983_solr", "2.3.4.5:9000_",
"2.3.4.5:9001_solr-2");
+ ClusterState clusterState2 = new ClusterState(liveNodes, new HashMap<>());
+ for (int i = 0; i < 3; i++) {
+ checker.checkAllowList(
+ List.of("2.3.4.5:8983", "2.3.4.5:9000", "2.3.4.5:9001"),
clusterState2);
+ SolrException exception =
+ expectThrows(
+ SolrException.class,
+ () -> checker.checkAllowList(List.of("1.1.3.4:8983"),
clusterState2));
+ assertThat(exception.code(),
equalTo(SolrException.ErrorCode.FORBIDDEN.code));
+ }
+ // Then the AllowListUrlChecker rebuilds the cache of live host urls.
+ assertThat(callCount.get(), equalTo(2));
+ }
+
private static List<String> urls(String... urls) {
return Arrays.asList(urls);
}
diff --git a/solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java
b/solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java
index 19bfc2565d6..22e1005ed8b 100644
--- a/solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java
+++ b/solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java
@@ -34,7 +34,6 @@ import java.util.Set;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.Consumer;
import java.util.function.Function;
-import java.util.stream.Collectors;
import java.util.stream.Stream;
import org.apache.solr.common.MapWriter;
import org.apache.solr.common.SolrException;
@@ -53,6 +52,8 @@ import org.slf4j.LoggerFactory;
* Immutable state of the cloud. Normally you can get the state by using {@code
* ZkStateReader#getClusterState()}.
*
+ * <p>However, the {@link #setLiveNodes list of live nodes} is updated when
nodes go up and down.
+ *
* @lucene.experimental
*/
public class ClusterState implements MapWriter {
@@ -63,8 +64,7 @@ public class ClusterState implements MapWriter {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
private final Map<String, CollectionRef> collectionStates,
immutableCollectionStates;
- private Set<String> liveNodes;
- private Set<String> hostAllowList;
+ private volatile Set<String> liveNodes;
/** Use this constr when ClusterState is meant for consumption. */
public ClusterState(Set<String> liveNodes, Map<String, DocCollection>
collectionStates) {
@@ -85,8 +85,7 @@ public class ClusterState implements MapWriter {
* loaded (parameter order different from constructor above to have
different erasures)
*/
public ClusterState(Map<String, CollectionRef> collectionStates, Set<String>
liveNodes) {
- this.liveNodes = CollectionUtil.newHashSet(liveNodes.size());
- this.liveNodes.addAll(liveNodes);
+ setLiveNodes(liveNodes);
this.collectionStates = new LinkedHashMap<>(collectionStates);
this.immutableCollectionStates =
Collections.unmodifiableMap(this.collectionStates);
}
@@ -189,7 +188,7 @@ public class ClusterState implements MapWriter {
/** Get names of the currently live nodes. */
public Set<String> getLiveNodes() {
- return Collections.unmodifiableSet(liveNodes);
+ return liveNodes;
}
@Deprecated
@@ -387,7 +386,7 @@ public class ClusterState implements MapWriter {
/** Internal API used only by ZkStateReader */
void setLiveNodes(Set<String> liveNodes) {
- this.liveNodes = liveNodes;
+ this.liveNodes = Set.copyOf(liveNodes);
}
/**
@@ -401,20 +400,6 @@ public class ClusterState implements MapWriter {
return immutableCollectionStates;
}
- /**
- * Gets the set of allowed hosts (host:port) built from the set of live
nodes. The set is cached
- * to be reused.
- */
- public Set<String> getHostAllowList() {
- if (hostAllowList == null) {
- hostAllowList =
- getLiveNodes().stream()
- .map((liveNode) -> liveNode.substring(0, liveNode.indexOf('_')))
- .collect(Collectors.toSet());
- }
- return hostAllowList;
- }
-
/**
* Streams the resolved {@link DocCollection}s, which will often fetch from
ZooKeeper for each one
* for a many-collection scenario. Use this sparingly; some users have
thousands of collections!
