FrankChen021 commented on code in PR #20151:
URL: https://github.com/apache/druid/pull/20151#discussion_r3968739411
##########
server/src/main/java/org/apache/druid/client/DirectDruidClient.java:
##########
@@ -238,11 +249,139 @@ private InputStream dequeue() throws InterruptedException
return holder.getStream();
}
+ /**
+ * Scans past leading whitespace in {@code buffer} looking for the
first content byte, without consuming
+ * (advancing the reader index of) the buffer, and returns it. Once a
non-whitespace byte is found, the prefix
+ * is considered resolved (see {@link #bodyPrefixResolved}) and later
calls return null. If {@code buffer} is
+ * empty or entirely whitespace, the prefix remains unresolved (and
null is returned) so a later call, from a
+ * subsequent chunk, can retry the check; this matters for chunked
responses, where the initial
+ * {@link HttpResponse} can carry an empty body and the real content,
HTML or otherwise, only arrives via
+ * {@link #handleChunk}.
+ */
+ @Nullable
+ private Byte bodyPrefixByte(ChannelBuffer buffer)
+ {
+ if (bodyPrefixResolved.get()) {
+ return null;
+ }
+ final int readerIndex = buffer.readerIndex();
+ final int readable = buffer.readableBytes();
+ for (int i = 0; i < readable; i++) {
+ byte b = buffer.getByte(readerIndex + i);
+ if (b == ' ' || b == '\n' || b == '\r' || b == '\t') {
+ continue;
+ }
+ bodyPrefixResolved.set(true);
+ return b;
+ }
+ return null;
+ }
+
+ /**
+ * Classifies the body prefix in {@code buffer} (see {@link
#bodyPrefixByte}) and fails the query if it is not
+ * JSON. HTML always fails; any other non-JSON body fails only when
the status is 429/503, since Druid itself
+ * never sends a non-JSON body with those statuses but proxies
routinely do (an HTML error page from nginx or
+ * a load balancer, a plain-text "upstream connect error" from Envoy).
A JSON body, whatever the status, is
+ * left alone so that the normal parse path can surface the server's
own structured error.
+ *
+ * @param contentType Content-Type header of the initial response,
possibly null; a text/html value fails the
+ * query regardless of the body prefix
+ * @param chunkNum 0 for the initial response body, else the chunk
number
+ */
+ private void failIfNonJsonBody(String contentType, ChannelBuffer
buffer, long chunkNum)
+ {
+ final boolean isHtmlContentType =
+ contentType != null &&
StringUtils.toLowerCase(contentType).contains("text/html");
+ final Byte prefix = bodyPrefixByte(buffer);
+ final boolean isHtml = isHtmlContentType || (prefix != null &&
prefix == '<');
+ final boolean isNonJson = prefix != null && prefix != '{' && prefix
!= '[';
+ final int statusCode = responseStatusCode;
+ if (isHtml || (isNonJson && (statusCode == 429 || statusCode ==
503))) {
+ throwForNonJsonBody(statusCode, contentType, buffer, chunkNum,
isHtml);
+ }
+ }
+
+ /**
+ * Returns up to 256 characters of {@code buffer} (read from at most
its first 512 bytes) for inclusion in an
+ * error message, without consuming the buffer.
+ */
+ private String bodyPreview(ChannelBuffer buffer)
+ {
+ final int len = Math.min(buffer.readableBytes(), 512);
+ if (len == 0) {
+ return "";
+ }
+ final byte[] previewBytes = new byte[len];
+ buffer.getBytes(buffer.readerIndex(), previewBytes);
+ final String preview = StringUtils.fromUtf8(previewBytes);
+ return preview.substring(0, Math.min(preview.length(), 256));
+ }
+
+ /**
+ * Fails the query because the response body is not JSON, typically an
error page produced by a load balancer
+ * or reverse proxy sitting in front of the data server. A 429/503
status is reported as
+ * {@link QueryCapacityExceededException} since that is what such
intermediaries return when the server is
+ * over capacity; any other status is reported as a {@link
QueryInterruptedException}. Either way, the caller
+ * gets a message that says what actually came back instead of a
{@code JsonParseException} on {@code '<'}.
+ *
+ * @param statusCode HTTP status of the initial response
+ * @param contentType Content-Type header of the initial response,
possibly null
+ * @param buffer the buffer in which the non-JSON body was
detected (the initial response body or a chunk)
+ * @param chunkNum 0 if detected in the initial response body, else
the chunk number
+ * @param isHtml whether the body was identified as HTML
specifically (vs. some other non-JSON content)
+ */
+ private void throwForNonJsonBody(
+ int statusCode,
+ String contentType,
+ ChannelBuffer buffer,
+ long chunkNum,
+ boolean isHtml
+ )
+ {
+ final String preview = bodyPreview(buffer);
Review Comment:
[P2] Do not echo arbitrary response bytes
bodyPreview copies up to 256 characters of arbitrary upstream response data
directly into a QueryException message. That message is logged by the client
and serialized through the broker to the query caller; a proxy or server can
include sensitive diagnostics or CR/LF, causing data disclosure and
log-forging. Keep the diagnostic to sanitized metadata (status/content type) or
escape/redact the body before placing it in the exception.
##########
server/src/main/java/org/apache/druid/client/DirectDruidClient.java:
##########
@@ -370,6 +509,13 @@ public ClientResponse<InputStream> handleChunk(
checkTotalBytesLimit(bytes);
+ // The initial HttpResponse for a chunked reply can have an empty
body, so the JSON-vs-not prefix check done
+ // in handleResponse may not have resolved yet. Retry it here,
against this chunk, before the bytes are
+ // enqueued for JSON parsing: otherwise an HTML error page (for
example from a load balancer or reverse
+ // proxy) delivered as chunked content is enqueued blind and only
surfaces later as a confusing
+ // JsonParseException. This is a no-op once the prefix has been
resolved.
+ failIfNonJsonBody(null, channelBuffer, chunkNum);
Review Comment:
[P1] Preserve the later-chunk exception type
For a real chunked Netty response, handleResponse returns a finished
SequenceInputStream, so NettyHttpClient completes retVal before this method
sees later chunks. When this check throws, the catch path can only call
exceptionCaught; DirectDruidClient.exceptionCaught stores a string and an
IOException in the stream, while JsonParserIterator can surface that as RE or a
generic QueryInterruptedException rather than the
QueryCapacityExceededException created here. The real chunked 503 path
therefore does not reach callers/retry logic as capacity-exceeded; the scripted
test invokes the handler synchronously and misses this lifecycle. Preserve the
original exception through the stream/future or add an end-to-end regression.
##########
server/src/main/java/org/apache/druid/client/DirectDruidClient.java:
##########
@@ -238,11 +249,139 @@ private InputStream dequeue() throws InterruptedException
return holder.getStream();
}
+ /**
+ * Scans past leading whitespace in {@code buffer} looking for the
first content byte, without consuming
+ * (advancing the reader index of) the buffer, and returns it. Once a
non-whitespace byte is found, the prefix
+ * is considered resolved (see {@link #bodyPrefixResolved}) and later
calls return null. If {@code buffer} is
+ * empty or entirely whitespace, the prefix remains unresolved (and
null is returned) so a later call, from a
+ * subsequent chunk, can retry the check; this matters for chunked
responses, where the initial
+ * {@link HttpResponse} can carry an empty body and the real content,
HTML or otherwise, only arrives via
+ * {@link #handleChunk}.
+ */
+ @Nullable
+ private Byte bodyPrefixByte(ChannelBuffer buffer)
+ {
+ if (bodyPrefixResolved.get()) {
+ return null;
+ }
+ final int readerIndex = buffer.readerIndex();
+ final int readable = buffer.readableBytes();
+ for (int i = 0; i < readable; i++) {
+ byte b = buffer.getByte(readerIndex + i);
+ if (b == ' ' || b == '\n' || b == '\r' || b == '\t') {
+ continue;
+ }
+ bodyPrefixResolved.set(true);
+ return b;
+ }
+ return null;
+ }
+
+ /**
+ * Classifies the body prefix in {@code buffer} (see {@link
#bodyPrefixByte}) and fails the query if it is not
+ * JSON. HTML always fails; any other non-JSON body fails only when
the status is 429/503, since Druid itself
+ * never sends a non-JSON body with those statuses but proxies
routinely do (an HTML error page from nginx or
+ * a load balancer, a plain-text "upstream connect error" from Envoy).
A JSON body, whatever the status, is
+ * left alone so that the normal parse path can surface the server's
own structured error.
+ *
+ * @param contentType Content-Type header of the initial response,
possibly null; a text/html value fails the
+ * query regardless of the body prefix
+ * @param chunkNum 0 for the initial response body, else the chunk
number
+ */
+ private void failIfNonJsonBody(String contentType, ChannelBuffer
buffer, long chunkNum)
+ {
+ final boolean isHtmlContentType =
+ contentType != null &&
StringUtils.toLowerCase(contentType).contains("text/html");
+ final Byte prefix = bodyPrefixByte(buffer);
+ final boolean isHtml = isHtmlContentType || (prefix != null &&
prefix == '<');
+ final boolean isNonJson = prefix != null && prefix != '{' && prefix
!= '[';
Review Comment:
[P1] Smile errors are classified as non-JSON
Production DirectDruidClient is built with the @Smile ObjectMapper and sends
Smile requests, so a Druid error response for a 429/503 is Smile-encoded and
starts with binary Smile bytes rather than `{` or `[`. `isNonJson` therefore
becomes true for every structured Smile 429/503, and the next branch replaces
the server's structured error (including a 503 SERVICE_UNAVAILABLE) with a
synthesized QueryCapacityExceededException. Detect the wire format (or use the
response content type/Smile header) before applying this prefix shortcut, and
add a Smile-mode regression.
--
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]
