solr
solr copied to clipboard
apache
SOLR-15955 Update Jetty dependency to 10
My local .git folder got mangled somehow so I'm transferring changes to a fresh checkout - still a few things that are off and I'm working on syncing up. Have a couple nits left to work out as well.
Thanks for taking a look. I have a n update I'll push I'll push tomorrow.
A couple of those are extra lines that came in when I merged to a clean repo (test policy and s3 build.gradle I believe).
I'll get the follow up pushed tomorrow - I rebased yesterday and after resolving new conflicts, tests were flakier (a handful failed first run) and one test failed consistently (managed storage test I believe). So I held back to look into that a bit, but in the in end, it seems I see the same behavior on a clean checkout, so should not be related this work.
That was a bit more effort than expected, but updated jetty from 10.0.7 to 10.0.8. That removes the need for the security policy read permission for "/".
Rebased to latest main, let's see if these build checks pass, did a couple test runs without issue.
I don't think we need this - tried running tests locally without it and using S3_MOCK_RULE to build the client directly and things seemed fine.
I actually took things out one by one from this class on the last update, and I left this because the tests passed as I removed up to this point and then failed when I removed this. But I tried again, and it seems to have passed, so perhaps some stale result issue I saw. Looks like it can come out if we go by your result and my latest.
Okay, I had missed in the patch you passed me how you proposed dealing with the toolchain dep issue - just using that instead of servlet-api - that seems fine to me.
I spent a bit of time on the 0 byte inputstream issue, but in the end, I think your approach is likely fine to start with - given that it works in the case that we currently know causes an issue, and if it stopped working for that case, tests would fail, I've come to think this solution is fine until we find it not to be.
This cleanup push that just went up includes a couple required changes to jetty config that I had forgotten are required and introduce the biggest upgrade issues a user would face - the existing config will not work. There is a fix required in the main jetty xml config where the rewrite handler is referenced by id instead of refid - that fails on jetty 10. And there is gzip config that references methods that are now removed and for the most part without replacement. They have to be changed.
Is there more to do here? I know @anshumg asked that Jetty 9.4.x upgrade be superseded by this change. There have been a few more Jetty 9.4.x and 10.x releases.
I can put up a more recent rebase.
The main reason this has sat around is a small but prominent back compat issue.
We ship existing jetty.xml config with the use of a id tag that has always been incorrect and should be refid. In Jetty 10 that will hard fail. We also use methods on the gzip handler that were previously deprecated and then removed in 10. That will hard fail.
As a result, with a default, clean Solr 9 release, without some potential solution, if you updated to Jetty 10 on a 9.x point release, Solr would fail to start on an existing install without a couple small, simple tweaks to the jetty xml config. That kind of break would be very abnormal for a point release.
The only potential workaround I’ve thought up so far is a kind of ugly, have the start script always check for this and and attempt to manually correct it, maybe only if the user hasn’t modified the files or something. But that’s far from a great solution.
We've only shipped Solr 9.0... Which I suspect means that very few people have actually upgraded from the 8.x line... This back compat issue will become more of a problem from 9.1 and later when more people adopt Solr 9.
Is modifying Jetty.xml a common thing to do? I suspect it's a "poweruser" type of change, so then maybe the release notes will warn them? Be a shame to wait another 18 months till Solr 10 comes out just to finally get this done....
I don’t know that modifying jetty.xml is that common. If the only case you were affected is if you had, I would just say release note it.
The problem is, you don’t need to have modified it. The version we shipped with won’t work, so you would have to overwrite that file on upgrade to avoid the issue.
If that is the standard upgrade process being used, then perhaps this is less of an issue.
What I imagine is a typical install layout that includes your Solr core directories and indexes and a point release upgrade would be commonly done by leaving your config and directories in place and just replacing the binary jars.
I don’t point release upgrade though, so maybe it’s normal to do a process that blows the jetty config files away?
Of course you could release note that you need blow these files away. My concern is that to my knowledge we have not had a release note that if not followed means a standard upgrade process would result in Jetty failing to start on a point release upgrade.
Yeah, I think it depends on how people do upgrades. I never do an inplace upgrade without having tested it in Dev first, and I don't do inplace if I can help it!
Is there anyway around this upgrade issue? If not, is this worth a dev mailing list post to get some consensus on?
I'd say jetty.xml is an implementation detail and officially we don't need to give back-compat guarantees for it, and we should be allowed to assume that users do not hand-edit those files. In upgrade notes we can highlight that upgrading to 9.1 will replace jetty.xml, and users who rely on customizations to it will need to re-apply customizations after upgrade.
+1 to get Jetty 10 out in 9.x.
I don’t point release upgrade though, so maybe it’s normal to do a process that blows the jetty config files away?
Docker users will always have a complete new image of course. Our official upgrade guide recommends taking down each node and doing a fresh install of the new version (https://solr.apache.org/guide/solr/latest/deployment-guide/upgrading-a-solr-cluster.html#upgrade-process).
The Linux install script unpacks each version separately and uses a symlink to /opt/solr to isolate (https://solr.apache.org/guide/solr/latest/deployment-guide/taking-solr-to-production.html#service-installation-script).
So imo the project recommends a "fresh" install as best practice. Only in exceptional cases such as patching log4j jars do we recommend people to do in-place updates of individual jar files in an install. We can stress this further in the upgrade notes for 9.1.
@markrmiller how can I help to get this ready to merge? Or am I jumping the gun based on @janhoy +1 comment ?
If you don't want to write the release notes, I could take a stab at it.
I have done some manual edits on occasion to jetty.xml ... either to increase the header size limit so a larger query string can be handled with GET, or to enable request logging. I expect that there will be a new jetty.xml when I upgrade -- I use the service installer, which means that the upgrade will create an entirely new program directory and update the symlink. If instead of upgrading, I reinstall the same version with the -f flag, I think that would probably overwrite jetty.xml ... but I have never tried installing the same version.
I do not know what typically happens when upgrading a docker setup, as I have never installed Solr that way. Does it create a new container that utilizes the same solr home that the old container was connected to? I suspect that jetty.xml edits are never propagated to the new version when doing an upgrade in a docker setup, the same as upgrading a non-docker setup with the service installer script.
What would it realistically take to upgrade to Jetty 11 instead of 10? Is that even possible without an extensive overhaul? I see on the jetty website that 11 uses the new jakartaee.servlet.* packages and servlet API 5, while Jetty 10 uses the same javax.servlet.* packages as 9, and targets servlet API 4. I would hope that version 11 has shim classes that allow webapps made for 9 to run.
We might want to provide mechanisms to make the most common jetty.xml edits configurable, which mostly eliminates that problem. It is my opinion that request logging should be turned on by default if there is a way to have them automatically rotate and prune, but I am not going to insist on that color for the bikeshed.
It is my opinion that request logging should be turned on by default
You should read up on the 9.0 release :) https://solr.apache.org/guide/solr/latest/deployment-guide/configuring-logging.html#request-logging ; https://issues.apache.org/jira/browse/SOLR-14142
Agreed on making some of the configs in Jetty more accessible without changing files ;-). Let's get to Jetty 10. I'd love to see a list of what you might want to make more configurable... CORS via Jetty filters is one that comes to mind for me...
I’m happy to treat jetty.xml as an implantation detail, really only concerned with the semi odd shock of not being able to start up with an existing, unmodified file. If people are okay with a release note on that, I have no problem with it though.
I’ll rebase the patch again.
A CVE, recorded as SOLR-16332, just showed up that would be fixed by this upgrade ;-).
Anything I can do to help @markrmiller ? I'm happy to take a stab at writing the Release Note for this...
I’m happy to treat jetty.xml as an implantation detail, really only concerned with the semi odd shock of not being able to start up with an existing, unmodified file. If people are okay with a release note on that, I have no problem with it though.
I'm of the opinion that jetty.xml is implementation detail and release note is good enough. I can help merge main into this again see where we are with tests.
Well this was unexpected first time after fixing the static build stuff:
./gradlew clean
...
./gradlew check -Pvalidation.errorprone=true
BUILD SUCCESSFUL in 20m 50s
586 actionable tasks: 198 executed, 388 up-to-date
After most recent changes :)
./gradlew check -Pvalidation.errorprone=true
BUILD SUCCESSFUL in 20m 58s
586 actionable tasks: 196 executed, 390 up-to-date
Shawn Heisey correctly pointed out to me that metrics-jetty9 was still being used. Upgraded to metrics-jetty10 in 3281463.
That made me go back and check other Jetty 9 references and found dtd should be updated. Fixed in d6a4c0d6f72ae7195b1233f6421279eb6a5c2baf
SLF4J: Class path contains SLF4J bindings targeting slf4j-api versions prior to 1.8.
Hmmm - thats concerning..... we have slf4j 1.7.x in versions.lock and no other slf4j according to versions.lock. So somehow slf4j > 1.7 is getting included.
I'll take a closer look and see what is going on.
I found that solr/packaging/build/solr-10.0.0-SNAPSHOT/server/lib/slf4j-api-2.0.0.jar and solr/packaging/build/solr-10.0.0-SNAPSHOT/server/solr-webapp/webapp/WEB-INF/lib/slf4j-api-2.0.0.jar exists - which is super weird. going to keep digging
Ok so slf4j-api is used by jetty classes now. serverLib and jettyClientImplementation configurations were being used to determine which dependencies end up in which directories. Since Jetty was pulling in slf4j-api 2.0 it was getting put in a few places it shouldn't.
I fixed this by:
- Upgrading slf4j-api to 2.0 in Solr so there aren't multiple different versions floating around
- excluding slf4j-api from jettyClientImplementation dependencies since then slfj4j-api doesn't end up in WEB-INF
This ensures a single slf4j-api version and logging works now :)
Need to address this:
2022-10-24 02:17:24.106 WARN (main) [] o.e.j.x.XmlConfiguration Deprecated method public void org.eclipse.jetty.server.handler.gzip.GzipHandler.setDeflaterPoolCapacity(int) in file:///Users/risdenk/repos/apache/solr/solr/packaging/build/solr-10.0.0-SNAPSHOT/server/etc/jetty-gzip.xml
setDeflaterPoolCapacity addressed in 22cfe1da7d3c9b88c5210447fb664957e29d3ee1