apache / solr-operator

Official Kubernetes operator for Apache Solr
https://solr.apache.org/operator
Apache License 2.0
243 stars 112 forks source link

Add sysprop driven XML tags to default solr.xml #636

Closed gerlowskija closed 9 months ago

gerlowskija commented 9 months ago

The default solr.xml now has the necessary plumbing to obey the commonly used solr.sharedLib, solr.allowPaths, and enableMetrics system properties.

Resolves #635

HoustonPutman commented 9 months ago

Oh, might as well include the solr.port.advertise here as well, unless you wanted to do that in a separate PR. No worries either way.

gerlowskija commented 9 months ago

If the user doesn't provide solr.sharedLib, the xml section will start with ,.

Hmm, not quite sure what you mean. Are you talking about the GenerateAdditionalLibXMLPart change here?

If so, AFAICT, 'libList' will never be empty because it at least always has ${solr.sharedLib:}. So if the user doesn't use any backups or modules, the produced XML should look like:

<str name="sharedLib">${solr.sharedLib:}</str>

But, maybe I'm missing something @HoustonPutman ?

might as well include the solr.port.advertise here as well

Makes sense to add that in while we're here, sure. Will add shortly

HoustonPutman commented 9 months ago

If the user doesn't provide solr.sharedLib, the xml section will start with ,.

Hmm, not quite sure what you mean. Are you talking about the GenerateAdditionalLibXMLPart change here?

If so, AFAICT, 'libList' will never be empty because it at least always has ${solr.sharedLib:}. So if the user doesn't use any backups or modules, the produced XML should look like:

<str name="sharedLib">${solr.sharedLib:}</str>

But, maybe I'm missing something @HoustonPutman ?

So my thought was if ${solr.sharedLib:} equates to an empty string, which it will by default, that Solr wouldn't handle the , in the beginning very well. But I just tested it out, and it works fine! So no issue there 🙂

gerlowskija commented 9 months ago

Ah, yes - sorry, I was being dense. Had an epiphany about what you meant while I was afk getting dinner. I tested it as well - looks good on 8.11 and 9.3 both. So I think we're safe 😅

gerlowskija commented 9 months ago

might as well include the solr.port.advertise here as well

I've added the necessary changes to work solr.port.advertise into generated solr.xml files, though I think it'll need tweaked a bit.

The generated solr.xml was already set up to reference a different system property called hostPort. It looks like we made pretty regular use of that sysprop by default: even going so far as to error out of reconciliation whenever a custom solr.xml didn't include <int name="hostPort">${hostPort:80}</int>. This PR (right now) updates this all to use solr.port.advertise instead.

Still doing a bit of testing on this and thinking through the upgrade ramifications (little worried about what this change means for administrators with custom solr.xml files that want to upgrade their operator), but this should all be ready for review.

gerlowskija commented 9 months ago

Alright, should be ready for review again. We're now setting the SOLR_PORT_ADVERTISE env var instead of adding -Dsolr.port.advertise to solrOpts. The custom solr.xml validation now accepts both hostPort and solr.port.advertise as placeholders to give users a release or two to upgrade any custom solr.xml files.