Skip to content

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

Merged
dsmiley merged 7 commits into
apache:mainfrom
VishnuPriyaChandraSekar:SOLR-18055-zkstateReader-urlScheme
May 9, 2026
Merged

SOLR-18055 Use solr.ssl.enabled property for url scheme detection#4272
dsmiley merged 7 commits into
apache:mainfrom
VishnuPriyaChandraSekar:SOLR-18055-zkstateReader-urlScheme

Conversation

@VishnuPriyaChandraSekar

@VishnuPriyaChandraSekar VishnuPriyaChandraSekar commented Apr 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@dsmiley dsmiley left a comment

Copy link
Copy Markdown
Contributor

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";

@dsmiley dsmiley Apr 9, 2026

Copy link
Copy Markdown
Contributor

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.

@VishnuPriyaChandraSekar VishnuPriyaChandraSekar Apr 9, 2026

Copy link
Copy Markdown
Contributor Author

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

@dsmiley dsmiley Apr 10, 2026

Copy link
Copy Markdown
Contributor

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

@dsmiley dsmiley Apr 16, 2026

Copy link
Copy Markdown
Contributor

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.

@VishnuPriyaChandraSekar VishnuPriyaChandraSekar Apr 16, 2026

Copy link
Copy Markdown
Contributor Author

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" ?

@dsmiley dsmiley Apr 25, 2026

Copy link
Copy Markdown
Contributor

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

dsmiley commented Apr 25, 2026

Copy link
Copy Markdown
Contributor

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

VishnuPriyaChandraSekar commented Apr 28, 2026

Copy link
Copy Markdown
Contributor Author

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

@dsmiley

dsmiley commented Apr 28, 2026

Copy link
Copy Markdown
Contributor

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

@VishnuPriyaChandraSekar

VishnuPriyaChandraSekar commented Apr 30, 2026

Copy link
Copy Markdown
Contributor Author

Looking into failed workflow. Requesting reviewers to pause until I fix the failed workflows

@dsmiley

dsmiley commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

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.

@dsmiley dsmiley merged commit bdc4e27 into apache:main May 9, 2026
6 of 7 checks passed
dsmiley added a commit that referenced this pull request May 12, 2026
@VishnuPriyaChandraSekar @dsmiley
SOLR-18055: Improve SSL/HTTPS URL scheme detection (#4272)
6434c2b 
Prioritize the solr.ssl.enabled system property for determining the URL scheme used in inter-node communication.  CloudSolrClient was recently also improved similarly.
This makes SSL configuration more straightforward for users, although the urlScheme cluster property is still supported and so is the urlScheme config field of HttpShardHandlerFactory in solr.xml.  Those might become deprecated if they don't have a purpose.

Co-authored-by: David Smiley <dsmiley@apache.org>
@VishnuPriyaChandraSekar VishnuPriyaChandraSekar deleted the SOLR-18055-zkstateReader-urlScheme branch June 5, 2026 04:41
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