nick-boss-tech commented on PR #5112: URL: https://github.com/apache/solr/pull/5112#issuecomment-6450355228
🤖 *AI text below* 🤖 *(reviewed by Nick Shanin)* @dsmiley, the example has **no targeting query parameter**. Suppose this request reaches node A, with the header naming live node B: ```http GET /solr/admin/info/system?foo=X-Solr-targetNode HTTP/1.1 X-Solr-targetNode: node-B.example.com:8983_solr ``` The [substring check](https://github.com/apache/solr/blob/b4fa46f1ecb11a60cc2cfd4783492639bb4e3a85/solr/core/src/java/org/apache/solr/servlet/NodeTargetProxyFilter.java#L79-L87) sees `X-Solr-targetNode` inside `foo`'s value, so it parses the query. But `params.get(TARGET_NODE_HEADER)` returns null: the only parameter is `foo`. Assigning that null to `targetNode` discards the header, and `/admin/info/system` runs on A instead of B. I prepared a [fix and regression tests on top of your current head](https://github.com/nick-boss-tech/solr/compare/b4fa46f1ecb11a60cc2cfd4783492639bb4e3a85...46bc7e428b5be519a497ecf65dcd0256b65fa84c). The fix only replaces the header target when the actual targeting parameter exists. Supplying both still returns 400. The diff also corrects the guide's 404 to the agreed 400 and clarifies the hostname shorthand rule. <details> <summary>Linux verification: 29 tests passed; both routing regressions fail before the fix</summary> Tested at `46bc7e428b5b`, seed `5112C0DE`: - `NodeTargetProxyFilterTest` and `NodeTargetProxyFilterAuthTest`: 18 passed, no failures, errors or skips. - `HttpSolrProxyTest` and `SolrRequestParserTest`: 11 passed, no failures, errors or skips. - Both new header-loss regressions fail on your original production code with the new test file: they expect the selected node but receive the original node. These cover the header name appearing in an unrelated query value and in a longer parameter name. The other two new tests, for no-header handling and header/query conflict, passed with the fix; they were not run against the original code. - `tidy` leaves a clean tree. `check -x test` passed with `NODE_ENV` unset after reinstalling the Antora tabs dependency. Two earlier attempts failed because `NODE_ENV=production` caused npm to omit that devDependency. [Verification details](https://github.com/nick-boss-tech/solr/blob/c7c5777b87fed3f1ed8578da8090ebb643ab4509/receipts/PR-5112-target-header-followup.md). </details> To take the fix and its Javadoc formatting follow-up: ```sh git switch SOLR-18537-proxyRequestsByHeader git fetch https://github.com/nick-boss-tech/solr.git solr-18537-submit git merge --ff-only 46bc7e428b5be519a497ecf65dcd0256b65fa84c ``` You're welcome to take the commits or adapt the changes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
