SOLR-18270 CertAuthPlugin : update the lookup attribute (without parametrization)#4474
SOLR-18270 CertAuthPlugin : update the lookup attribute (without parametrization)#4474heitzjm wants to merge 7 commits into
Conversation
|
I'd rather have jakarta as default and javax as fallback for code back-portability and compatibility. |
|
As asked, I've just updated this PR with the fallback. I ran the failed test again, and it was successful. |
| (X509Certificate[]) request.getAttribute("javax.servlet.request.X509Certificate"); | ||
| (X509Certificate[]) request.getAttribute(CURRENT_REQUEST_ATTRIBUTE_NAME); | ||
| if (certs == null || certs.length == 0) { | ||
| certs=(X509Certificate[]) request.getAttribute(OLD_REQUEST_ATTRIBUTE_NAME); |
There was a problem hiding this comment.
Do we still need to check the old name?
My understanding is this name changed with new Jetty. I don't think we will ever go back to the other name. We are good with checking only the new name in 10+.
There was a problem hiding this comment.
I think both should stay - in my understanding, SOLR, at its basis, is a web application that could run in other java web application servers than the Jetty provided in the Docker image - so I think it would be better to keep both names to ease integration. This way, this PR might even be merged to SOLR 9 as well.
There was a problem hiding this comment.
Makes sense. Lets keep it this way.
There was a problem hiding this comment.
You may want to rename the variables to something meaningful, how about:
JAVAX_REQUEST_ATTRIBUTE_NAME
JAKARTA_REQUEST_ATTRIBUTE_NAME ?
Also, you will need to run spotless check, you're missing some white spaces
There was a problem hiding this comment.
@heitzjm these days, Solr only runs in Jetty, and we've been getting more specific to just that. Notice we no longer produce a .war file. I think the only place where you might have solr run in another container is via the EmbeddedSolr approach.. Having said that, I don't think we have made a "This is how it is" explicit decision, and I'm fine with the backcompat for now.
|
If nothing else comes to the PR, I'll merge it early next week. |
| (X509Certificate[]) request.getAttribute("javax.servlet.request.X509Certificate"); | ||
| (X509Certificate[]) request.getAttribute(CURRENT_REQUEST_ATTRIBUTE_NAME); | ||
| if (certs == null || certs.length == 0) { | ||
| certs=(X509Certificate[]) request.getAttribute(OLD_REQUEST_ATTRIBUTE_NAME); |
There was a problem hiding this comment.
You may want to rename the variables to something meaningful, how about:
JAVAX_REQUEST_ATTRIBUTE_NAME
JAKARTA_REQUEST_ATTRIBUTE_NAME ?
Also, you will need to run spotless check, you're missing some white spaces
…itespace formatting
|
Thanks @laminelam and @psalagnac . I have just updated the code. I hope it is fine now. |
|
@heitzjm Formatting is still broken. Please make sure to run |
|
Thanks @psalagnac for the gradlew commands, I have just run them. |
|
I just triggered the builds... |
https://issues.apache.org/jira/browse/SOLR-18270
Description
This PR only update the request attribute used by CertAuthPlugin to match the one filled by Jetty 12 - for SOLR 10. SOLR 9 works fine in the Official Docker Image, as it uses a lower major Jetty version.
Solution
This PR only updates the request attribute used by CertAuthPlugin to match the one filled by Jetty 12 - for SOLR 10, and provides, after revision, as advised by @laminelam, a fallback on the old attribute.
Tests
I checked locally by compiling SOLR from source, as well as building Docker Image from source and tested it in a K8S environment.
Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.5582 tests completed, 1 failed, 184 skipped
ERROR: The following test(s) have failed:
=> however, in my opinion, this test does not seem to be relevant for this PR
unreleased/SOLR-18270-easy-fix-without-parametrization.yml