feat: HttpLoggingInterceptor logs only plain text response bodies

Signed-off-by: Marc Nuri <marc@marcnuri.com>

kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/BufferUtil.java

@@ -16,6 +16,9 @@
 package io.fabric8.kubernetes.client.http;
 
 import java.nio.ByteBuffer;
+import java.nio.charset.CharacterCodingException;
+import java.nio.charset.CharsetDecoder;
+import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
 import java.util.Collection;
 
@@ -70,4 +73,24 @@ public static ByteBuffer copy(ByteBuffer buffer) {
     buffer.position(position);
     return clone;
   }
+
+  /**
+   * Very rudimentary method to check if the provided ByteBuffer contains text.
+   * 
+   * @return true if the buffer contains text, false otherwise.
+   */
+  public static boolean isPlainText(ByteBuffer originalBuffer) {
+    if (originalBuffer == null) {
+      return false;
+    }
+    final ByteBuffer buffer = copy(originalBuffer);
+    final CharsetDecoder decoder = StandardCharsets.UTF_8.newDecoder();
+    try {
+      decoder.decode(buffer);
+      return true;
+    } catch (CharacterCodingException ex) {
+      return false;
+    }
+  }
+
 }

kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/HttpLoggingInterceptor.java

@@ -113,8 +113,9 @@ public DeferredLoggingConsumer(HttpLogger httpLogger, HttpRequest originalReques
 
     @Override
     public void consume(List<ByteBuffer> value, AsyncBody asyncBody) throws Exception {
-      // TODO: Should try to detect if the body is text or not? (HttpLoggingInterceptor.isPlainText)
-      value.stream().map(BufferUtil::copy).forEach(responseBody::add); // Potential leak
+      if (!value.isEmpty() && BufferUtil.isPlainText(value.iterator().next())) {
+        value.stream().map(BufferUtil::copy).forEach(responseBody::add); // Potential leak
+      }
       originalConsumer.consume(value, asyncBody);
     }
 
@@ -133,7 +134,7 @@ public void accept(Void v, Throwable throwable) {
 
     /**
      * Registers the asyncBody.done() callback.
-     * 
+     *
      * @param asyncBody the AsyncBody instance to register the callback on the done() future.
      */
     private void processAsyncBody(AsyncBody asyncBody) {
@@ -168,7 +169,7 @@ void logResponse(HttpResponse<?> response) {
     }
 
     void logResponseBody(Queue<ByteBuffer> responseBody) {
-      if (logger.isTraceEnabled() && responseBody != null) {
+      if (logger.isTraceEnabled() && responseBody != null && !responseBody.isEmpty()) {
         final StringBuilder bodyString = new StringBuilder();
         while (!responseBody.isEmpty()) {
           bodyString.append(StandardCharsets.UTF_8.decode(responseBody.poll()));

kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractHttpLoggingInterceptorTest.java

@@ -15,8 +15,16 @@
  */
 package io.fabric8.kubernetes.client.http;
 
+import io.fabric8.mockwebserver.Context;
 import io.fabric8.mockwebserver.DefaultMockServer;
+import io.fabric8.mockwebserver.ServerRequest;
+import io.fabric8.mockwebserver.ServerResponse;
+import io.fabric8.mockwebserver.internal.SimpleRequest;
 import io.fabric8.mockwebserver.utils.ResponseProviders;
+import okhttp3.mockwebserver.MockResponse;
+import okhttp3.mockwebserver.MockWebServer;
+import okhttp3.mockwebserver.RecordedRequest;
+import okio.Buffer;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.BeforeEach;
@@ -27,7 +35,11 @@
 import org.slf4j.Logger;
 
 import java.net.URI;
+import java.util.ArrayDeque;
 import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Queue;
 import java.util.concurrent.TimeUnit;
 
 import static org.mockito.ArgumentMatchers.anyInt;
@@ -36,18 +48,21 @@
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.timeout;
+import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.when;
 
 public abstract class AbstractHttpLoggingInterceptorTest {
 
   private static DefaultMockServer server;
+  private static Map<ServerRequest, Queue<ServerResponse>> responses;
   private Logger logger;
   private InOrder inOrder;
   private HttpClient httpClient;
 
   @BeforeAll
   static void beforeAll() {
-    server = new DefaultMockServer(false);
+    responses = new HashMap<>();
+    server = new DefaultMockServer(new Context(), new MockWebServer(), responses, false);
     server.start();
   }
 
@@ -157,6 +172,33 @@ public void httpResponseBodyLogged() throws Exception {
     inOrder.verify(logger).trace("-HTTP END-");
   }
 
+  @Test
+  @DisplayName("HTTP binary response body is skipped")
+  public void httpResponseBodySkipped() throws Exception {
+    final MockResponse binaryResponse = new MockResponse()
+        .setResponseCode(200)
+        .setBody(new Buffer().write(new byte[] { (byte) 0xFF, (byte) 0xD8, (byte) 0x00, (byte) 0x12, (byte) 0x34 }));
+    responses.computeIfAbsent(new SimpleRequest("/binary-response-body"), k -> new ArrayDeque<>()).add(
+        new ServerResponse() {
+          @Override
+          public boolean isRepeatable() {
+            return true;
+          }
+
+          @Override
+          public MockResponse toMockResponse(RecordedRequest recordedRequest) {
+            return binaryResponse;
+          }
+        });
+    httpClient.sendAsync(httpClient.newHttpRequestBuilder()
+        .uri(server.url("/binary-response-body"))
+        .build(), String.class).get(10, TimeUnit.SECONDS);
+    inOrder.verify(logger, timeout(1000L)).trace("-HTTP START-");
+    inOrder.verify(logger).trace(eq("< {} {}"), anyInt(), anyString());
+    inOrder.verify(logger, times(1)).trace(anyString()); // only -HTTP END- was logged
+    inOrder.verifyNoMoreInteractions();
+  }
+
   @Test
   @DisplayName("WS request URI is logged")
   public void wsRequestUriLogged() throws Exception {