dsmiley commented on code in PR #2892:
URL: https://github.com/apache/solr/pull/2892#discussion_r1865912289
##########
solr/core/src/test/org/apache/solr/security/AllowListUrlCheckerTest.java:
##########
@@ -196,6 +200,46 @@ public void testHostParsingNoProtocol() throws Exception {
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 =
+ new HashSet<>(Arrays.asList("1.2.3.4:8983_solr", "1.2.3.4:9000_",
"1.2.3.4:9001_solr-2"));
Review Comment:
`Set.of` instead?
##########
solr/core/src/java/org/apache/solr/security/AllowListUrlChecker.java:
##########
@@ -154,6 +157,29 @@ public void checkAllowList(List<String> urls, ClusterState
clusterState)
}
}
+ /**
+ * 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();
+ }
+ Set<String> liveNodes = clusterState.getLiveNodes();
+ if (liveHostUrlsCache == null || liveNodes != liveNodesCache) {
+ liveHostUrlsCache = buildLiveHostUrls(liveNodes);
+ liveNodesCache = liveNodes;
+ }
Review Comment:
I'm a bit nervous of some race... maybe a synchronized is simple and in
practice shouldn't bottleneck as live nodes doesn't change?
##########
solr/core/src/test/org/apache/solr/security/AllowListUrlCheckerTest.java:
##########
@@ -196,6 +200,46 @@ public void testHostParsingNoProtocol() throws Exception {
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 =
+ new HashSet<>(Arrays.asList("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 = new HashSet<>(Arrays.asList("2.3.4.5:8983_solr",
"2.3.4.5:9000_", "2.3.4.5:9001_solr-2"));
Review Comment:
Set.of?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
