dsmiley commented on code in PR #4162: URL: https://github.com/apache/solr/pull/4162#discussion_r2849828800
########## changelog/unreleased/SOLR-18056-urlScheme-csp.yml: ########## @@ -0,0 +1,8 @@ +# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc +title: Added method to get url scheme based on cluster type Review Comment: ```suggestion title: Improved CloudSolrClient's urlScheme detection by using the scheme of provided Solr URLs, or looking at "solr.ssl.enabled". ``` ########## solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java: ########## @@ -339,4 +339,16 @@ public int getZkClientTimeout() { public void setZkClientTimeout(int zkClientTimeout) { this.zkClientTimeout = zkClientTimeout; } + + /** + * @return url scheme with the help of cluster property or environment variable. + */ + @Override + public String getUrlScheme() { + Object urlSchemeClusterProperty = getClusterProperty(ClusterState.URL_SCHEME); + if (urlSchemeClusterProperty != null) { + return urlSchemeClusterProperty.toString(); + } + return Boolean.parseBoolean(System.getProperty(SOLR_SSL_ENABLED, "false")) ? "https" : "http"; Review Comment: This should come first -- thus if set, we ignore the cluster properties ########## solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderUrlSchemeTest.java: ########## @@ -0,0 +1,98 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.client.solrj.impl; + +import java.nio.file.Path; +import java.util.List; +import org.apache.solr.client.solrj.jetty.HttpJettySolrClient; +import org.apache.solr.cloud.SolrCloudTestCase; +import org.apache.solr.cloud.ZkController; +import org.apache.solr.cloud.ZkTestServer; +import org.apache.solr.common.cloud.ClusterProperties; +import org.apache.solr.common.cloud.SolrZkClient; +import org.apache.solr.common.cloud.ZkStateReader; +import org.junit.Test; + +public class ClusterStateProviderUrlSchemeTest extends SolrCloudTestCase { + private static final String SOLR_SSL_ENABLED = "solr.ssl.enabled"; + + @Test + public void testHttpClusterStateProviderUrlScheme() throws Exception { + List<String> solrUrls = List.of("http://localhost:8983/solr";); + try (var httpClient = new HttpJettySolrClient.Builder().build(); + ClusterStateProvider clusterStateProvider = + new HttpClusterStateProvider<>(solrUrls, httpClient)) { + assertEquals("http", clusterStateProvider.getUrlScheme()); + } + } + + @Test + public void testHttpClusterStateProviderUrlSchemeWithMixedProtocol() { + List<String> solrUrls = List.of("http://localhost:8983/solr";, "https://localhost:8984/solr";); + try (var httpClient = new HttpJettySolrClient.Builder().build()) { + expectThrows( + IllegalArgumentException.class, + () -> new HttpClusterStateProvider<>(solrUrls, httpClient)); + } + } + + @Test + public void testZkClusterStateProviderUrlScheme() throws Exception { + Path zkDir = createTempDir("zkData"); + + ZkTestServer server = new ZkTestServer(zkDir); + try { + server.run(); + SolrZkClient client = new SolrZkClient.Builder().withUrl(server.getZkAddress()).build(); + ZkController.createClusterZkNodes(client); + testUrlSchemeWithClusterProperties(client); + testUrlSchemeDefault(client); + testUrlSchemeWithSystemProperties(client); + client.close(); + } finally { + server.shutdown(); + } + } + + private void testUrlSchemeDefault(SolrZkClient client) throws Exception { + try (var zkStateReader = new ZkStateReader(client); + var clusterStateProvider = new ZkClientClusterStateProvider(zkStateReader)) { + assertEquals("http", clusterStateProvider.getUrlScheme()); + } + } + + private void testUrlSchemeWithSystemProperties(SolrZkClient client) throws Exception { + System.setProperty(SOLR_SSL_ENABLED, "true"); + try (var zkStateReader = new ZkStateReader(client); + var clusterStateProvider = new ZkClientClusterStateProvider(zkStateReader)) { + assertEquals("https", clusterStateProvider.getUrlScheme()); + } finally { + System.clearProperty(SOLR_SSL_ENABLED); + assertNull(System.getProperty(SOLR_SSL_ENABLED)); Review Comment: jeesh; do we not trust Java? ########## solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderUrlSchemeTest.java: ########## Review Comment: Did you not want to put these tests into ClusterStateProviderTest ? ########## solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java: ########## @@ -73,11 +73,16 @@ public abstract class BaseHttpClusterStateProvider implements ClusterStateProvid Executors.newSingleThreadScheduledExecutor( new SolrNamedThreadFactory("liveNodeReloadingExecutor")); - protected void initConfiguredNodes(List<String> solrUrls) throws Exception { + protected void initConfiguredNodes(List<String> solrUrls) { this.configuredNodes = solrUrls.stream() .map(BaseHttpClusterStateProvider::stringToUrl) .collect(Collectors.toList()); + if (this.configuredNodes.stream().map(URL::getProtocol).collect(Collectors.toSet()).size() Review Comment: You could also consider simply not checking/enforcing this, and not even storing a field. Grabbing the first in `getScheme` is fine. ########## solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java: ########## @@ -73,11 +73,16 @@ public abstract class BaseHttpClusterStateProvider implements ClusterStateProvid Executors.newSingleThreadScheduledExecutor( new SolrNamedThreadFactory("liveNodeReloadingExecutor")); - protected void initConfiguredNodes(List<String> solrUrls) throws Exception { + protected void initConfiguredNodes(List<String> solrUrls) { this.configuredNodes = solrUrls.stream() .map(BaseHttpClusterStateProvider::stringToUrl) .collect(Collectors.toList()); + if (this.configuredNodes.stream().map(URL::getProtocol).collect(Collectors.toSet()).size() Review Comment: ```suggestion if (this.configuredNodes.stream().map(URL::getProtocol).distinct().count() ``` ########## solr/solrj/src/java/org/apache/solr/client/solrj/impl/ClusterStateProvider.java: ########## @@ -127,4 +127,7 @@ default <T> T getClusterProperty(String key, T defaultValue) { void connect(); String getQuorumHosts(); + + /** Get url scheme. */ Review Comment: ```suggestion /** Get URL scheme -- http or https. Never null. */ ``` -- 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]
