FrankChen021 commented on code in PR #20151:
URL: https://github.com/apache/druid/pull/20151#discussion_r3979208851
##########
server/src/main/java/org/apache/druid/client/DirectDruidClient.java:
##########
@@ -436,6 +600,7 @@ private void setupResponseReadFailure(String msg, Throwable
th)
{
emitNodeMetrics(System.nanoTime() - requestStartTimeNs);
fail.set(msg);
+ failCause.set(th);
Review Comment:
[P1] Publish the failure cause before the failure flag
`fail` is the publication flag checked by both `SequenceInputStream`
callbacks, but it is written before the separate `failCause` atomic. A
concurrent reader can therefore observe non-null `fail` and enter
`failureException()` before the later `failCause.set(th)` is visible; for a
later-chunk `QueryCapacityExceededException`, that path returns a generic `RE`
and loses the typed capacity error that this fix is intended to preserve.
Publish `failCause` before `fail`, or publish both through one atomic state
object, so observing the failure flag also guarantees the cause is visible.
##########
server/src/main/java/org/apache/druid/client/DirectDruidClient.java:
##########
@@ -238,11 +254,152 @@ 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 (or Smile, when the request was sent as Smile per {@link
#isSmile}). HTML always fails; any other
+ * non-JSON/non-Smile body fails only when the status is 429/503,
since Druid itself never sends such a 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 structured body
in the request's own format, 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 == '<');
+ // A data server negotiates its response format from the request
(ResourceIOReaderWriterFactory#factorize),
+ // so a Smile request gets a Smile response, error bodies included;
those begin with the Smile format
+ // header's 0x3a byte rather than JSON's '{'/'['. Checking only
'{'/'[' here would misclassify every
+ // structured Smile 429/503 body as non-JSON and discard the
server's real error.
+ final boolean isNonJson = isSmile
+ ? prefix != null && prefix !=
SmileConstants.HEADER_BYTE_1
+ : 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. Control characters,
including CR/LF, are stripped so the
+ * preview can't forge extra lines into a log record or the message
it's embedded in.
+ */
+ 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);
+ final StringBuilder sanitized = new
StringBuilder(Math.min(preview.length(), 256));
+ for (int i = 0; i < preview.length() && sanitized.length() < 256;
i++) {
+ final char c = preview.charAt(i);
+ sanitized.append(c >= 0x20 && c != 0x7f ? c : ' ');
Review Comment:
[P2] Do not echo arbitrary upstream body content
Filtering ASCII control characters removes the CR/LF injection case, but
this still copies arbitrary upstream bytes into a `QueryException` message.
That message is logged by `JsonParserIterator` and, for a late stream failure,
is also exposed through the query error/trailer. The broker-to-server response
is an untrusted boundary and Druid's logging guidance forbids data in exception
messages; a proxy error page can contain diagnostics or reflected sensitive
content. Keep the diagnostic to bounded sanitized metadata (status/content
type) or explicitly redact the body instead of returning it verbatim.
--
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]
