dsmiley commented on code in PR #3447:
URL: https://github.com/apache/solr/pull/3447#discussion_r2246823679
##########
solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java:
##########
@@ -736,107 +709,21 @@ void destroy() {
}
}
- // TODO using Http2Client
- private void remoteQuery(String coreUrl, HttpServletResponse resp) throws
IOException {
- HttpRequestBase method;
- HttpEntity httpEntity = null;
+ protected void sendRemoteQuery() throws IOException {
Review Comment:
TODO I want to rename this to "doRemoteProxy"
##########
solr/core/src/java/org/apache/solr/servlet/HttpSolrProxy.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.servlet;
+
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+import java.io.IOException;
+import java.io.OutputStream;
+import java.nio.ByteBuffer;
+import java.util.EnumSet;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import org.eclipse.jetty.client.HttpClient;
+import org.eclipse.jetty.client.InputStreamRequestContent;
+import org.eclipse.jetty.client.Request;
+import org.eclipse.jetty.client.Response;
+import org.eclipse.jetty.client.Result;
+import org.eclipse.jetty.http.HttpField;
+import org.eclipse.jetty.http.HttpFields;
+import org.eclipse.jetty.http.HttpHeader;
+
+/** Helper class for proxying the request to another Solr node. */
+class HttpSolrProxy {
+ // TODO add X-Forwarded-For and with comma delimited
+
+ private static final Set<HttpHeader> HOP_BY_HOP_HEADERS =
+ EnumSet.of(
+ HttpHeader.CONNECTION,
+ HttpHeader.KEEP_ALIVE,
+ HttpHeader.PROXY_AUTHENTICATE,
+ HttpHeader.PROXY_AUTHORIZATION,
+ HttpHeader.TE,
+ HttpHeader.TRANSFER_ENCODING,
+ HttpHeader.UPGRADE);
+
+ // Methods that shouldn't have a body according to HTTP spec
+ private static final Set<String> NO_BODY_METHODS = Set.of("GET", "HEAD",
"DELETE");
+
+ static void doHttpProxy(
+ HttpClient httpClient,
+ HttpServletRequest servletReq,
+ HttpServletResponse servletRsp,
+ String url)
+ throws Throwable {
+ Request proxyReq =
httpClient.newRequest(url).method(servletReq.getMethod());
+
+ // clearing them first to ensure there's no stock entries (e.g. user-agent)
+ proxyReq.headers(proxyFields -> copyRequestHeaders(servletReq,
proxyFields.clear()));
+
+ if (!NO_BODY_METHODS.contains(servletReq.getMethod())) {
+ proxyReq.body(
+ new InputStreamRequestContent(servletReq.getContentType(),
servletReq.getInputStream()));
+ }
+
+ CompletableFuture<Result> resultFuture = new CompletableFuture<>();
+
+ proxyReq.send(
+ new Response.Listener() {
+ private final byte[] buffer = new byte[8192];
+
+ @Override
+ public void onBegin(Response response) {
+ servletRsp.setStatus(response.getStatus());
+ }
+
+ @Override
+ public void onHeaders(Response response) {
+ copyResponseHeaders(response, servletRsp);
+ }
+
+ @Override
+ public void onContent(Response response, ByteBuffer content) {
+ try {
+ final OutputStream clientOutputStream =
servletRsp.getOutputStream();
+
+ // Copy content to the client's output stream in chunks using
the existing buffer
+ int remaining = content.remaining();
+ while (remaining > 0) {
+ int chunkSize = Math.min(remaining, buffer.length);
+ content.get(buffer, 0, chunkSize);
+ clientOutputStream.write(buffer, 0, chunkSize);
+ remaining -= chunkSize;
+ }
+ clientOutputStream.flush();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ @Override
+ public void onComplete(Result result) {
+ resultFuture.complete(result);
+ }
+ });
+
+ Result result = resultFuture.get(); // waits
+ var failure = result.getFailure();
+ if (failure != null) {
+ throw failure;
+ }
+ }
+
+ private static void copyRequestHeaders(
+ HttpServletRequest servletReq, HttpFields.Mutable proxyFields) {
+ servletReq
+ .getHeaderNames()
+ .asIterator()
+ .forEachRemaining(
+ headerName -> {
+ HttpHeader knownHeader = HttpHeader.CACHE.get(headerName); //
maybe null
+ if (!HOP_BY_HOP_HEADERS.contains(knownHeader)) {
+ // NOCOMMIT see SOLR-7274 SPNEGO support:
+ // && HttpHeader.HOST != knownHeader
+ // && HttpHeader.AUTHORIZATION != knownHeader
+ // && HttpHeader.ACCEPT != knownHeader) {
Review Comment:
@anshumg, 10 years ago in
[SOLR-17286](https://issues.apache.org/jira/browse/SOLR-17286), you committed
this particular exception list of 3 headers to not copy on the grounds of
SPNEGO. In the comments, it was actually @chatman who pointed this out. I
don't understand it; my thinking is that a proxy should be a proxy and pass
everything through except for the hop-by-hop header list according to RFC. If
I restore these exceptions, then the wonderful test recently added will fail:
https://github.com/apache/solr/blob/eaceca8911ef06c89a293f2437a6a560f34b1a43/solr/modules/jwt-auth/src/test/org/apache/solr/security/jwt/JWTAuthPluginIntegrationTest.java#L301
(see PR #3397 that added it)
##########
solr/core/src/java/org/apache/solr/servlet/HttpSolrProxy.java:
##########
Review Comment:
I refactored basically all of the remote proxy logic out of HttpSolrCall
into this new class.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
Review Comment:
Exposing the underlying HttpClient. This means it was pointless to have the
getter to getProtocolHandlers specifically.
##########
solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java:
##########
@@ -736,107 +709,21 @@ void destroy() {
}
}
- // TODO using Http2Client
- private void remoteQuery(String coreUrl, HttpServletResponse resp) throws
IOException {
- HttpRequestBase method;
- HttpEntity httpEntity = null;
+ protected void sendRemoteQuery() throws IOException {
+ SolrRequestInfo.setRequestInfo(new SolrRequestInfo(req, new
SolrQueryResponse(), action));
+ mustClearSolrRequestInfo = true;
+
+ HttpClient httpClient = cores.getDefaultHttpSolrClient().getHttpClient();
Review Comment:
FYI using the underlying Jetty `HttpClient`, one layer deeper than the
`Http2SolrClient`, thus skips over the authentication plugin interception stuff
since it's not registered at quite that depth. I abandoned a previous PR that
attempted to lower that logic down here.
##########
solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java:
##########
@@ -736,107 +709,21 @@ void destroy() {
}
}
- // TODO using Http2Client
- private void remoteQuery(String coreUrl, HttpServletResponse resp) throws
IOException {
- HttpRequestBase method;
- HttpEntity httpEntity = null;
+ protected void sendRemoteQuery() throws IOException {
+ SolrRequestInfo.setRequestInfo(new SolrRequestInfo(req, new
SolrQueryResponse(), action));
+ mustClearSolrRequestInfo = true;
+
+ HttpClient httpClient = cores.getDefaultHttpSolrClient().getHttpClient();
ModifiableSolrParams updatedQueryParams = new
ModifiableSolrParams(queryParams);
int forwardCount = queryParams.getInt(INTERNAL_REQUEST_COUNT, 0) + 1;
updatedQueryParams.set(INTERNAL_REQUEST_COUNT, forwardCount);
String queryStr = updatedQueryParams.toQueryString();
try {
- String urlstr = coreUrl + queryStr;
-
- boolean isPostOrPutRequest = "POST".equals(req.getMethod()) ||
"PUT".equals(req.getMethod());
- if ("GET".equals(req.getMethod())) {
- method = new HttpGet(urlstr);
- } else if ("HEAD".equals(req.getMethod())) {
- method = new HttpHead(urlstr);
- } else if (isPostOrPutRequest) {
- HttpEntityEnclosingRequestBase entityRequest =
- "POST".equals(req.getMethod()) ? new HttpPost(urlstr) : new
HttpPut(urlstr);
- InputStream in = req.getInputStream();
- HttpEntity entity = new InputStreamEntity(in, req.getContentLength());
- entityRequest.setEntity(entity);
- method = entityRequest;
- } else if ("DELETE".equals(req.getMethod())) {
- method = new HttpDelete(urlstr);
- } else if ("OPTIONS".equals(req.getMethod())) {
- method = new HttpOptions(urlstr);
- } else {
- throw new SolrException(
- SolrException.ErrorCode.SERVER_ERROR, "Unexpected method type: " +
req.getMethod());
- }
-
- for (Enumeration<String> e = req.getHeaderNames(); e.hasMoreElements();
) {
- String headerName = e.nextElement();
- if (!"host".equalsIgnoreCase(headerName)
- && !"authorization".equalsIgnoreCase(headerName)
- && !"accept".equalsIgnoreCase(headerName)) {
- method.addHeader(headerName, req.getHeader(headerName));
- }
- }
- // These headers not supported for HttpEntityEnclosingRequests
- if (method instanceof HttpEntityEnclosingRequest) {
- method.removeHeaders(TRANSFER_ENCODING_HEADER);
- method.removeHeaders(CONTENT_LENGTH_HEADER);
- }
-
- // Make sure the user principal is forwarded when its exist
- HttpClientContext httpClientRequestContext =
- HttpClientUtil.createNewHttpClientRequestContext();
- Principal userPrincipal = req.getUserPrincipal();
- if (userPrincipal != null) {
- // Normally the context contains a static userToken to enable reuse
resources. However, if a
- // personal Principal object exists, we use that instead, also as a
means to transfer
- // authentication information to Auth plugins that wish to intercept
the request later
- if (log.isDebugEnabled()) {
- log.debug("Forwarding principal {}", userPrincipal);
- }
- httpClientRequestContext.setUserToken(userPrincipal);
- }
Review Comment:
FYI @bct-timo-crabbe the logic you just added didn't have a role to play
when I switched over to Jetty HttpClient, as best as I could tell. I found
that letting the "Authorization" header through enabled your wonderful test to
pass.
--
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]
