Skip to content

Conversation

@epugh
Copy link
Contributor

@epugh epugh commented Nov 6, 2025

https://issues.apache.org/jira/browse/SOLR-17864

solr.keyStoreReload.enabled -> solr.keystore.reload.enabled
solr.jetty.sslContext.reload.scanInterval --> solr.jetty.ssl.context.reload.scan.interval.secs

A question... We already have a environment variable SOLR_SSL_RELOAD_ENABLED that controls the setting of solr.keystore.reload.enabled. Should we just call it solr.ssl.reload.enabled? Or maybe chnage the environment variable to SOLR_KEYSTORE_RELOAD_ENABLED?

solr.keyStoreReload.enabled -> solr.keystore.reload.enabled
solr.jetty.sslContext.reload.scanInterval --> solr.jetty.ssl.context.reload.scan.interval.secs
@epugh epugh requested a review from dsmiley November 6, 2025 11:51
@github-actions github-actions bot added documentation Improvements or additions to documentation jetty-server client:solrj start-scripts labels Nov 6, 2025
@epugh
Copy link
Contributor Author

epugh commented Nov 6, 2025

Hey @tflobbe in the docs for security it says the reload of keys is on by default, but I don't see that. In Http2SolrClient I do see this logic:

Long keyStoreReloadIntervalSecs = builder.keyStoreReloadIntervalSecs;
    if (keyStoreReloadIntervalSecs == null
        && EnvUtils.getPropertyAsBool("solr.keystore.reload.enabled", false)) {
      keyStoreReloadIntervalSecs =
          EnvUtils.getPropertyAsLong("solr.jetty.ssl.context.reload.scan.interval.secs", 30l);
    }

@epugh epugh requested a review from tflobbe November 6, 2025 11:53
epugh added 2 commits November 6, 2025 07:17
Dsolr.jetty.ssl.verify.client.hostname=HTTPS seems werid...   is it hostname?  in jetty it's the EndpointIdentificationAlgorithm variable
@github-actions github-actions bot added the tests label Nov 6, 2025
@epugh
Copy link
Contributor Author

epugh commented Nov 6, 2025

I don't like that solr.jetty.ssl.verify.client.hostname=HTTPS is what the start scripts do... When in jetty-ssl.xml we are populating EndpointIdentificationAlgorithm. What about solr.jetty.ssl.verify.client.algorithm=HTTPS or solr.jetty.ssl.verification.algorithm=HTTPS?

Copy link
Member

@tflobbe tflobbe left a comment

Choose a reason for hiding this comment

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

Hey @tflobbe in the docs for security it says the reload of keys is on by default, but I don't see that. In Http2SolrClient I do see this logic:

It looks like the the solr start script will set the default to true here

I understand and agree with the goals of this Jira, but I do want to point out that we are losing some information by doing some of these changes. Today, the solr.jetty... are system properties that are taken verbatim from Jetty's config and docs, by changing some of them they aren't "mappable" to Jetty configs anymore.
Maybe the answer is "we don't care, we don't want Solr to be mapped directly to Jetty since it's a standalone app and Jetty is an implementation detail", which has been largely discussed. In such case, I wonder why keeping the word "jetty" in there.

<Set name="idleTimeout"><Property name="solr.jetty.http.idleTimeout" default="120000"/></Set>
<Set name="acceptorPriorityDelta"><Property name="solr.jetty.http.acceptorPriorityDelta" default="0"/></Set>
<Set name="acceptQueueSize"><Property name="solr.jetty.http.acceptQueueSize" default="0"/></Set>
<Set name="idleTimeout"><Property name="solr.jetty.http.timeout.ms" default="120000"/></Set>
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I like removing the "idle", it makes the parameter confusing, as the user wouldn't know what kind of timeout this is (my first thought would be this is a request timeout)

</Arg>
<Set name="host"><Property name="solr.host.bind" default="127.0.0.1"/></Set>
<Set name="port"><Property name="solr.jetty.https.port" default="8983" /></Set>
<Set name="idleTimeout"><Property name="solr.jetty.https.timeout" default="120000"/></Set>
Copy link
Member

Choose a reason for hiding this comment

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

Note that this timeout property is different than the new one you are adding, for the same thing

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants