SOLR-18055 Use solr.ssl.enabled property for url scheme detection#4272
Conversation
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
Thanks.
Remember to ./gradle writeChangelog too
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
Show resolved
Hide resolved
| } | ||
|
|
||
| private void testUrlSchemeDefault(SolrZkClient client) throws Exception { | ||
| if (isSSLMode()) { |
There was a problem hiding this comment.
why are you this back & forth? in some methods?
There was a problem hiding this comment.
Looks like the baseClass (SolrTestCaseJ4) randomly runs the server in SSL mode. Thus, I had to put back the system property if the server runs in https (If I don't do this, the next consecutive tests becomes flaky)
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java
Show resolved
Hide resolved
| /** | ||
| * Deduce url scheme using environment variable if available otherwise from cluster property | ||
| * | ||
| * @return url scheme of host |
There was a problem hiding this comment.
Will add some in the next revision
| * | ||
| * @return http or https | ||
| */ | ||
| private String getUrlScheme() { |
There was a problem hiding this comment.
As a method it's suggestive for anyone to potentially call it beyond the only caller -- init(). Notice that init() is only trying to initialize this.scheme and that null is very valid; see buildUrl(). So lets just do that without this method here, leaving it as null if the sys prop isn't set.
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
Show resolved
Hide resolved
| new RequestReplicaListTransformerGenerator(); | ||
|
|
||
| // URL scheme to be used in distributed search. | ||
| static final String INIT_URL_SCHEME = "urlScheme"; |
There was a problem hiding this comment.
Curious; was urlScheme arg documented in the ref guide or present in a published solr.xml that people use? If so we'll want to keep it.
There was a problem hiding this comment.
Looks like urlScheme is an optional field in solr.xml [1]. In order to provide backward compatibility, I think the precedence to lookup the urlScheme should be 1) solr.ssl.enabled 2) urlScheme from solr.xml 3) default (null).
[1] https://solr.apache.org/guide/solr/latest/configuration-guide/configuring-solr-xml.html
There was a problem hiding this comment.
I agree. Thanks for looking into it.
2 participants
https://issues.apache.org/jira/browse/SOLR-18055
Description
Currently, the client is compelled to provide cluster property in order to indicate the url scheme. This is annoying. In this PR, the url scheme is detected using the "solr.ssl.enabled" system property instead of cluster property.
Solution
<str name="urlScheme">${urlScheme:}</str>in solr.xml). In this PR, it detects the url scheme based on the system property ("solr.ssl.enabled").ZKStateReader.getBaseUrlForNodeName()andgetBaseUrlV2ForNodeName()uses cluster property to detect the url scheme for the nodes. In this PR, it detects the url scheme either from system property ("solr.ssl.enabled") or cluster propertyTests
- SolrTestCaseJ4.java
- TestMiniSolrCloudClusterSSL.java
Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.