Skip to content

SOLR-18055 Use solr.ssl.enabled property for url scheme detection#4272

Open
VishnuPriyaChandraSekar wants to merge 4 commits intoapache:mainfrom
VishnuPriyaChandraSekar:SOLR-18055-zkstateReader-urlScheme
Open

SOLR-18055 Use solr.ssl.enabled property for url scheme detection#4272
VishnuPriyaChandraSekar wants to merge 4 commits intoapache:mainfrom
VishnuPriyaChandraSekar:SOLR-18055-zkstateReader-urlScheme

Conversation

@VishnuPriyaChandraSekar
Copy link
Copy Markdown
Contributor

@VishnuPriyaChandraSekar VishnuPriyaChandraSekar commented Apr 9, 2026 •
edited
Loading

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

  • Improved HttpShardHandlerFactory's url scheme detection by giving higher precedence for "solr.ssl.enabled" over explicit configuration from solr.xml
  • Improved ZkStateReader's url scheme detection by giving higher precedence for "solr.ssl.enabled" over cluster property
  • Deprecated the usage of Cluster property in ZkController, ReplicaMutator, SliceMutator and SystemInfoProvider. These classes will depend on ClusterStateProvider to know the url scheme.

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:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide
  • I have added a changelog entry for my change

dsmiley
dsmiley reviewed Apr 9, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@dsmiley dsmiley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.
Remember to ./gradle writeChangelog too

Comment thread solr/core/src/java/org/apache/solr/cloud/ZkController.java
Comment thread solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java Outdated
Comment thread solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java Outdated
Comment thread solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java Outdated
Comment thread solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java Outdated
dsmiley
dsmiley reviewed Apr 9, 2026
View reviewed changes
Comment thread solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java Outdated
Comment thread solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java Outdated
Comment thread solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
new RequestReplicaListTransformerGenerator();

// URL scheme to be used in distributed search.
static final String INIT_URL_SCHEME = "urlScheme";
Copy link
Copy Markdown
Contributor

@dsmiley dsmiley Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@VishnuPriyaChandraSekar VishnuPriyaChandraSekar Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@dsmiley dsmiley Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Thanks for looking into it.

@VishnuPriyaChandraSekar VishnuPriyaChandraSekar marked this pull request as draft April 15, 2026 22:18
dsmiley
dsmiley reviewed Apr 16, 2026
View reviewed changes
Comment thread changelog/unreleased/SOLR-18056-urlScheme-csp.yml Outdated
@@ -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
Copy link
Copy Markdown
Contributor

@dsmiley dsmiley Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@VishnuPriyaChandraSekar VishnuPriyaChandraSekar Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this "Standardize urlScheme detection with precedence for "solr.ssl.enabled" ?

Copy link
Copy Markdown
Contributor

@dsmiley dsmiley Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread changelog/unreleased/SOLR-18056-urlScheme-csp.yml Outdated
Comment thread solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java Outdated
Comment thread solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java Outdated
dsmiley
dsmiley reviewed Apr 17, 2026
View reviewed changes
Comment thread solr/core/src/test/org/apache/solr/cloud/overseer/ZkStateReaderTest.java Outdated
Comment thread solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java
Comment thread solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java Outdated
@dsmiley dsmiley marked this pull request as ready for review April 17, 2026 01:07
@VishnuPriyaChandraSekar
SOLR-18055 Improved url scheme detection by prioritizing solr.ssl.ena…
507ee79 
…bled over solr.xml and cluster property

- Improved ZkStateReader's urlScheme detection by giving higher precedence to "solr.ssl.enabled" over cluster property
- Improved HttpShardHandlerFactory's urlScheme detection by giving higher precedence to "solr.ssl.enabled" over explicit configuration (ie., solr.xml)
- Deprecated usage of the cluster property for detecting urlScheme in ZkController, ReplicaMutator, SliceMutator, and SystemInfoProvider; these now rely on ClusterStateProvider#getUrlScheme
@VishnuPriyaChandraSekar VishnuPriyaChandraSekar force-pushed the SOLR-18055-zkstateReader-urlScheme branch from a5ec761 to 507ee79 Compare April 22, 2026 21:02
@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Apr 25, 2026
@dsmiley
Copy link
Copy Markdown
Contributor

dsmiley commented Apr 25, 2026

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.

@VishnuPriyaChandraSekar
Copy link
Copy Markdown
Contributor Author

VishnuPriyaChandraSekar commented Apr 28, 2026

Thanks for updating the doc :) Should I resolve the conflict and publish a new revision ?

@dsmiley
Copy link
Copy Markdown
Contributor

dsmiley commented Apr 28, 2026

Thanks; that'd be helpful. Then I'll merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dsmiley dsmiley dsmiley left review comments

Assignees

No one assigned

Labels

cat:cloud cat:search client:solrj documentation Improvements or additions to documentation module:clustering module:ltr module:opentelemetry test-framework tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@VishnuPriyaChandraSekar @dsmiley