You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "VishnuPriyaChandraSekar (via GitHub)" <gi...@apache.org> on 2026/04/09 01:08:16 UTC
[PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
VishnuPriyaChandraSekar opened a new pull request, #4272:
URL: https://github.com/apache/solr/pull/4272
https://issues.apache.org/jira/browse/SOLR-18055
<!--
_(If you are a project committer then you may remove some/all of the following template.)_
Before creating a pull request, please file an issue in the ASF Jira system for Solr:
* https://issues.apache.org/jira/projects/SOLR
For something minor (i.e. that wouldn't be worth putting in release notes), you can skip JIRA.
To create a Jira issue, you will need to create an account there first.
The title of the PR should reference the Jira issue number in the form:
* SOLR-####: <short description of problem or changes>
SOLR must be fully capitalized. A short description helps people scanning pull requests for items they can work on.
Properly referencing the issue in the title ensures that Jira is correctly updated with code review comments and commits. -->
# 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
* Currently, HttpShardHandlerFactory detects the url scheme from the configuration file (`<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()` and `getBaseUrlV2ForNodeName()` 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 property
* Additionally, I deprecated the usage of Cluster property in ZkController, ReplicaMutator, SliceMutator and SystemInfoProvider
# Tests
* Added new test cases in ZKStateReader to verify the correctness of getUrlScheme
* Replaced "urlScheme" system property by "solr.ssl.enabled" in the following test files
- SolrTestCaseJ4.java
- TestMiniSolrCloudClusterSSL.java
* Removed the usage of urlScheme in the solr configuration files
* Modified the existing tests in solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java
# Checklist
Please review the following and check all that apply:
- [X] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability.
- [X] I have created a Jira issue and added the issue ID to my pull request title.
- [X] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
- [X] I have developed this patch against the `main` branch.
- [X] I have run `./gradlew check`.
- [X] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
- [ ] I have added a [changelog entry](https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc) for my change (Not required)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
Posted by "VishnuPriyaChandraSekar (via GitHub)" <gi...@apache.org>.
VishnuPriyaChandraSekar commented on code in PR #4272:
URL: https://github.com/apache/solr/pull/4272#discussion_r3055147596
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -365,13 +365,7 @@ public ZkController(
});
zkStateReader.createClusterStateWatchersAndUpdate(); // and reads cluster properties
-
- // note: Can't read cluster properties until createClusterState ^ is called
Review Comment:
Ok, I put it back in the next revision
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
Posted by "VishnuPriyaChandraSekar (via GitHub)" <gi...@apache.org>.
VishnuPriyaChandraSekar commented on code in PR #4272:
URL: https://github.com/apache/solr/pull/4272#discussion_r3126190037
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java:
##########
@@ -472,29 +472,41 @@ public void testZkClusterStateProviderUrlScheme() throws Exception {
}
private void testUrlSchemeDefault(SolrZkClient client) throws Exception {
+ if (isSSLMode()) {
Review Comment:
TIL :) Thanks for the suggestion
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4272:
URL: https://github.com/apache/solr/pull/4272#discussion_r3097244058
##########
changelog/unreleased/SOLR-18056-urlScheme-csp.yml:
##########
@@ -1,8 +1,15 @@
# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
-title: Improved CloudSolrClient's urlScheme detection by using the scheme of provided Solr URLs, or looking at "solr.ssl.enabled".
+title: Improved url scheme detection in client API and core components
type: changed
authors:
- name: Vishnu Priya Chandra Sekar
links:
- name: SOLR-18056
url: https://issues.apache.org/jira/browse/SOLR-18056
+ - name: SOLR-18055
+ url: https://issues.apache.org/jira/browse/SOLR-18055
+important_notes:
Review Comment:
multiple sentences if fine: https://github.com/apache/solr/blob/a1df8522723b6e4d5687cf44e0b28643b7d2d144/gradle/changelog.gradle#L66
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
Posted by "VishnuPriyaChandraSekar (via GitHub)" <gi...@apache.org>.
VishnuPriyaChandraSekar commented on PR #4272:
URL: https://github.com/apache/solr/pull/4272#issuecomment-4356278565
Looking into failed workflow. Requesting reviewers to pause until I fix the failed workflows
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
Posted by "VishnuPriyaChandraSekar (via GitHub)" <gi...@apache.org>.
VishnuPriyaChandraSekar commented on code in PR #4272:
URL: https://github.com/apache/solr/pull/4272#discussion_r3095480912
##########
changelog/unreleased/SOLR-18056-urlScheme-csp.yml:
##########
@@ -1,8 +1,15 @@
# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
-title: Improved CloudSolrClient's urlScheme detection by using the scheme of provided Solr URLs, or looking at "solr.ssl.enabled".
+title: Improved url scheme detection in client API and core components
Review Comment:
How about this "Standardize urlScheme detection with precedence for "solr.ssl.enabled" ?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
Posted by "VishnuPriyaChandraSekar (via GitHub)" <gi...@apache.org>.
VishnuPriyaChandraSekar commented on code in PR #4272:
URL: https://github.com/apache/solr/pull/4272#discussion_r3060194382
##########
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java:
##########
@@ -104,8 +105,8 @@ public class HttpShardHandlerFactory extends ShardHandlerFactory
private RequestReplicaListTransformerGenerator requestReplicaListTransformerGenerator =
new RequestReplicaListTransformerGenerator();
- // URL scheme to be used in distributed search.
- static final String INIT_URL_SCHEME = "urlScheme";
Review 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
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
Posted by "VishnuPriyaChandraSekar (via GitHub)" <gi...@apache.org>.
VishnuPriyaChandraSekar commented on code in PR #4272:
URL: https://github.com/apache/solr/pull/4272#discussion_r3055142857
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java:
##########
@@ -472,29 +472,41 @@ public void testZkClusterStateProviderUrlScheme() throws Exception {
}
private void testUrlSchemeDefault(SolrZkClient client) throws Exception {
+ if (isSSLMode()) {
Review Comment:
Looks like the baseClass (SolrTestCaseJ4) randomly runs the server is SSL. 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)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4272:
URL: https://github.com/apache/solr/pull/4272#discussion_r3055261987
##########
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java:
##########
@@ -433,6 +429,16 @@ private String buildUrl(String url) {
return url;
}
+ /**
+ * Get url scheme of a standalone server based on system property "solr.ssl.enabled".
Review Comment:
it's mode-neutral.
##########
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java:
##########
@@ -433,6 +429,16 @@ private String buildUrl(String url) {
return url;
}
+ /**
+ * Get url scheme of a standalone server based on system property "solr.ssl.enabled".
+ *
+ * @return http or https
+ */
+ private String getUrlScheme() {
Review 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:
##########
@@ -104,8 +105,8 @@ public class HttpShardHandlerFactory extends ShardHandlerFactory
private RequestReplicaListTransformerGenerator requestReplicaListTransformerGenerator =
new RequestReplicaListTransformerGenerator();
- // URL scheme to be used in distributed search.
- static final String INIT_URL_SCHEME = "urlScheme";
Review 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.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
Posted by "VishnuPriyaChandraSekar (via GitHub)" <gi...@apache.org>.
VishnuPriyaChandraSekar commented on PR #4272:
URL: https://github.com/apache/solr/pull/4272#issuecomment-4337625374
Thanks for updating the doc :) Should I resolve the conflict and publish a new revision ?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
Posted by "VishnuPriyaChandraSekar (via GitHub)" <gi...@apache.org>.
VishnuPriyaChandraSekar commented on code in PR #4272:
URL: https://github.com/apache/solr/pull/4272#discussion_r3095440475
##########
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java:
##########
@@ -433,6 +432,31 @@ private String buildUrl(String url) {
return url;
}
+ /**
+ * Get url scheme of host
+ *
+ * <p>Examples:
+ *
+ * <ul>
+ * <li>Returns {@code "https"} when SSL is enabled via configuration.
+ * <li>Returns {@code null} when ssl is disabled or no explicit configuration is provided in
+ * solr.xml.
+ * </ul>
+ *
+ * @return http or https or null
+ */
+ private String getUrlScheme(NamedList<?> args, StringBuilder sb) {
Review Comment:
That makes sense :)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4272:
URL: https://github.com/apache/solr/pull/4272#discussion_r3055049192
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java:
##########
@@ -472,29 +472,41 @@ public void testZkClusterStateProviderUrlScheme() throws Exception {
}
private void testUrlSchemeDefault(SolrZkClient client) throws Exception {
+ if (isSSLMode()) {
Review Comment:
why are you this back & forth? in some methods?
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -365,13 +365,7 @@ public ZkController(
});
zkStateReader.createClusterStateWatchersAndUpdate(); // and reads cluster properties
-
- // note: Can't read cluster properties until createClusterState ^ is called
Review Comment:
this comment is important to keep
##########
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java:
##########
@@ -433,6 +429,16 @@ private String buildUrl(String url) {
return url;
}
+ /**
+ * Get url scheme of a standalone server based on system property "solr.ssl.enabled".
Review Comment:
why speak of "standalone"?
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -1240,8 +1241,20 @@ public String getBaseUrlForNodeName(final String nodeName) {
* @return url that looks like {@code https://localhost:8983/api}
*/
public String getBaseUrlV2ForNodeName(final String nodeName) {
- String urlScheme = getClusterProperty(URL_SCHEME, "http");
- return Utils.getBaseUrlForNodeName(nodeName, urlScheme, true);
+ return URLUtil.getBaseUrlForNodeName(nodeName, getUrlScheme(), true);
+ }
+
+ /**
+ * Deduce url scheme using environment variable if available otherwise from cluster property
+ *
+ * @return url scheme of host
Review Comment:
show the 2 examples please
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -1240,8 +1241,20 @@ public String getBaseUrlForNodeName(final String nodeName) {
* @return url that looks like {@code https://localhost:8983/api}
*/
public String getBaseUrlV2ForNodeName(final String nodeName) {
- String urlScheme = getClusterProperty(URL_SCHEME, "http");
- return Utils.getBaseUrlForNodeName(nodeName, urlScheme, true);
+ return URLUtil.getBaseUrlForNodeName(nodeName, getUrlScheme(), true);
+ }
+
+ /**
+ * Deduce url scheme using environment variable if available otherwise from cluster property
Review Comment:
IMO _in general_, how a method does its job is not the business of the caller to know. Less to document, less to potentially get out of sync.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4272:
URL: https://github.com/apache/solr/pull/4272#discussion_r3061585194
##########
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java:
##########
@@ -104,8 +105,8 @@ public class HttpShardHandlerFactory extends ShardHandlerFactory
private RequestReplicaListTransformerGenerator requestReplicaListTransformerGenerator =
new RequestReplicaListTransformerGenerator();
- // URL scheme to be used in distributed search.
- static final String INIT_URL_SCHEME = "urlScheme";
Review Comment:
I agree. Thanks for looking into it.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #4272:
URL: https://github.com/apache/solr/pull/4272#issuecomment-4337679574
Thanks; that'd be helpful. Then I'll merge.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
Posted by "VishnuPriyaChandraSekar (via GitHub)" <gi...@apache.org>.
VishnuPriyaChandraSekar commented on code in PR #4272:
URL: https://github.com/apache/solr/pull/4272#discussion_r3095551966
##########
changelog/unreleased/SOLR-18056-urlScheme-csp.yml:
##########
@@ -1,8 +1,15 @@
# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
-title: Improved CloudSolrClient's urlScheme detection by using the scheme of provided Solr URLs, or looking at "solr.ssl.enabled".
+title: Improved url scheme detection in client API and core components
type: changed
authors:
- name: Vishnu Priya Chandra Sekar
links:
- name: SOLR-18056
url: https://issues.apache.org/jira/browse/SOLR-18056
+ - name: SOLR-18055
+ url: https://issues.apache.org/jira/browse/SOLR-18055
+important_notes:
Review Comment:
Well, there is no other field I could use for.
Based on this, I will just have a self descriptive title (IMO this is the hardest part) like "Standardize urlScheme detection by prioritizing "solr.ssl.enabled" over solr.xml and cluster property"
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4272:
URL: https://github.com/apache/solr/pull/4272#discussion_r3141114446
##########
changelog/unreleased/SOLR-18056-urlScheme-csp.yml:
##########
@@ -1,8 +1,15 @@
# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
-title: Improved CloudSolrClient's urlScheme detection by using the scheme of provided Solr URLs, or looking at "solr.ssl.enabled".
+title: Improved url scheme detection in client API and core components
Review Comment:
I don't want to lose the original title's information concerning CloudSolrClient. Suggested total title:
> Improved CloudSolrClient's http/https (SSL) detection by using the scheme of provided Solr URLs, or looking at the "solr.ssl.enabled" system property. Likewise improved Solr itself to consider this property as an alternative to the urlScheme cluster property.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
Posted by "VishnuPriyaChandraSekar (via GitHub)" <gi...@apache.org>.
VishnuPriyaChandraSekar commented on code in PR #4272:
URL: https://github.com/apache/solr/pull/4272#discussion_r3126135855
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java:
##########
@@ -472,29 +472,41 @@ public void testZkClusterStateProviderUrlScheme() throws Exception {
}
private void testUrlSchemeDefault(SolrZkClient client) throws Exception {
+ if (isSSLMode()) {
+ System.clearProperty(SOLR_SSL_ENABLED);
+ }
try (var zkStateReader = new ZkStateReader(client);
var clusterStateProvider = new ZkClientClusterStateProvider(zkStateReader)) {
assertEquals("http", clusterStateProvider.getUrlScheme());
+ } finally {
+ if (isSSLMode()) {
+ System.setProperty(SOLR_SSL_ENABLED, "true");
+ }
}
}
private void testUrlSchemeWithSystemProperties(SolrZkClient client) throws Exception {
- System.setProperty(SOLR_SSL_ENABLED, "true");
Review Comment:
TIL :)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
Posted by "VishnuPriyaChandraSekar (via GitHub)" <gi...@apache.org>.
VishnuPriyaChandraSekar commented on code in PR #4272:
URL: https://github.com/apache/solr/pull/4272#discussion_r3055148811
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -1240,8 +1241,20 @@ public String getBaseUrlForNodeName(final String nodeName) {
* @return url that looks like {@code https://localhost:8983/api}
*/
public String getBaseUrlV2ForNodeName(final String nodeName) {
- String urlScheme = getClusterProperty(URL_SCHEME, "http");
- return Utils.getBaseUrlForNodeName(nodeName, urlScheme, true);
+ return URLUtil.getBaseUrlForNodeName(nodeName, getUrlScheme(), true);
+ }
+
+ /**
+ * Deduce url scheme using environment variable if available otherwise from cluster property
+ *
+ * @return url scheme of host
Review Comment:
Will add some in the next revision
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
Posted by "VishnuPriyaChandraSekar (via GitHub)" <gi...@apache.org>.
VishnuPriyaChandraSekar commented on code in PR #4272:
URL: https://github.com/apache/solr/pull/4272#discussion_r3055142857
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java:
##########
@@ -472,29 +472,41 @@ public void testZkClusterStateProviderUrlScheme() throws Exception {
}
private void testUrlSchemeDefault(SolrZkClient client) throws Exception {
+ if (isSSLMode()) {
Review 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)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #4272:
URL: https://github.com/apache/solr/pull/4272#issuecomment-4356292078
The precommit workflow error is not caused by this PR as I see a recent Jenkins job reporting the same issue relating to an errorprone finding.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley merged PR #4272:
URL: https://github.com/apache/solr/pull/4272
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
Posted by "VishnuPriyaChandraSekar (via GitHub)" <gi...@apache.org>.
VishnuPriyaChandraSekar commented on code in PR #4272:
URL: https://github.com/apache/solr/pull/4272#discussion_r3055142857
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java:
##########
@@ -472,29 +472,41 @@ public void testZkClusterStateProviderUrlScheme() throws Exception {
}
private void testUrlSchemeDefault(SolrZkClient client) throws Exception {
+ if (isSSLMode()) {
Review Comment:
Looks like the baseClass (SolrTestCaseJ4) randomly runs the server is SSL. Thus, I had to put back the system property if the server runs in https.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
Posted by "VishnuPriyaChandraSekar (via GitHub)" <gi...@apache.org>.
VishnuPriyaChandraSekar commented on code in PR #4272:
URL: https://github.com/apache/solr/pull/4272#discussion_r3055152343
##########
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java:
##########
@@ -433,6 +429,16 @@ private String buildUrl(String url) {
return url;
}
+ /**
+ * Get url scheme of a standalone server based on system property "solr.ssl.enabled".
Review Comment:
Oh, I thought this class is called only for standalone server. Is my understanding incorrect ?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4272:
URL: https://github.com/apache/solr/pull/4272#discussion_r3097319973
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java:
##########
@@ -472,29 +472,41 @@ public void testZkClusterStateProviderUrlScheme() throws Exception {
}
private void testUrlSchemeDefault(SolrZkClient client) throws Exception {
+ if (isSSLMode()) {
+ System.clearProperty(SOLR_SSL_ENABLED);
+ }
try (var zkStateReader = new ZkStateReader(client);
var clusterStateProvider = new ZkClientClusterStateProvider(zkStateReader)) {
assertEquals("http", clusterStateProvider.getUrlScheme());
+ } finally {
+ if (isSSLMode()) {
+ System.setProperty(SOLR_SSL_ENABLED, "true");
+ }
}
}
private void testUrlSchemeWithSystemProperties(SolrZkClient client) throws Exception {
- System.setProperty(SOLR_SSL_ENABLED, "true");
Review Comment:
`assumeTrue(isSSLMode())``
##########
solr/core/src/test/org/apache/solr/cloud/overseer/ZkStateReaderTest.java:
##########
@@ -995,4 +995,23 @@ public void testFetchLowestSolrVersion_noLiveNodes() throws Exception {
var lowestVersion = reader.fetchLowestSolrVersion();
assertFalse("Expected no lowest version when no live nodes exist", lowestVersion.isPresent());
}
+
+ public void testGetUrlScheme_validSystemProperty() {
+ String expectedUrlScheme = isSSLMode() ? "https" : "http";
+ assertEquals(expectedUrlScheme, fixture.reader.getUrlScheme());
+ }
+
+ public void testGetUrlScheme_noClusterAndSystemProperty() {
+ // By default, the base class sets the system property for ssl
Review Comment:
rather than the flip flop... you could just add an `assumeFalse(isSSLMode())` -- thus effectively disable the test sometimes
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java:
##########
@@ -472,29 +472,41 @@ public void testZkClusterStateProviderUrlScheme() throws Exception {
}
private void testUrlSchemeDefault(SolrZkClient client) throws Exception {
+ if (isSSLMode()) {
Review Comment:
ah, right, thanks. Let's make this simple: `assumeFalse(isSSLMode())`
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4272:
URL: https://github.com/apache/solr/pull/4272#discussion_r3090847555
##########
changelog/unreleased/SOLR-18056-urlScheme-csp.yml:
##########
@@ -1,8 +1,15 @@
# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
-title: Improved CloudSolrClient's urlScheme detection by using the scheme of provided Solr URLs, or looking at "solr.ssl.enabled".
+title: Improved url scheme detection in client API and core components
Review Comment:
IMO this has become much more vague to a reader of the changelog who is anyone other than you and me. The "title" label is misleading -- add more sentences.
##########
changelog/unreleased/SOLR-18056-urlScheme-csp.yml:
##########
@@ -1,8 +1,15 @@
# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
-title: Improved CloudSolrClient's urlScheme detection by using the scheme of provided Solr URLs, or looking at "solr.ssl.enabled".
+title: Improved url scheme detection in client API and core components
type: changed
authors:
- name: Vishnu Priya Chandra Sekar
links:
- name: SOLR-18056
url: https://issues.apache.org/jira/browse/SOLR-18056
+ - name: SOLR-18055
+ url: https://issues.apache.org/jira/browse/SOLR-18055
+important_notes:
Review Comment:
This is the first I've seen of this... and you are the first to attempt to use it :-) Our only references to it thus far don't even truly say what it's for.
[Documentation.](https://logchange.dev/tools/logchange/faq/?h=important_notes#q-how-do-i-handle-breaking-changes) Seems for "breaking changes" or presumably anything befitting of that label. What you added doesn't qualify IMO.
Well the "deprecated" part does... albeit I think I said it's premature to deprecate that.
##########
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java:
##########
@@ -433,6 +432,31 @@ private String buildUrl(String url) {
return url;
}
+ /**
+ * Get url scheme of host
+ *
+ * <p>Examples:
+ *
+ * <ul>
+ * <li>Returns {@code "https"} when SSL is enabled via configuration.
+ * <li>Returns {@code null} when ssl is disabled or no explicit configuration is provided in
+ * solr.xml.
+ * </ul>
+ *
+ * @return http or https or null
+ */
+ private String getUrlScheme(NamedList<?> args, StringBuilder sb) {
Review Comment:
maybe prefix with 'init' not 'get' to emphasize this is for initialization and not any method in this class that might want it. On further reflection, there's `getParameter` called by init() a bunch of times. This method here was merely extracted from init(). You could put it back. Or call it `getParameterUrlScheme`
##########
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java:
##########
@@ -433,6 +432,31 @@ private String buildUrl(String url) {
return url;
}
+ /**
+ * Get url scheme of host
+ *
+ * <p>Examples:
+ *
+ * <ul>
+ * <li>Returns {@code "https"} when SSL is enabled via configuration.
+ * <li>Returns {@code null} when ssl is disabled or no explicit configuration is provided in
+ * solr.xml.
+ * </ul>
Review Comment:
sorry, this is *way* too much documentation for a private method
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18055 Use solr.ssl.enabled property for url scheme detection [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #4272:
URL: https://github.com/apache/solr/pull/4272#issuecomment-4317761024
Please don't force-push as it resets the PR review state.
I updated the ref guide in several areas. I also saw that SOLR_SSL_ENABLED wouldn't be converted to a corresponding sys-prop -- so I removed the line that was blockingn that.
I think this is ready to merge.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org