Unwrap DecoderExceptions on connections (OriginConnectExceptions) int…

…o RequestAttempts (#1727)

zuul-core/src/main/java/com/netflix/zuul/netty/connectionpool/PerServerConnectionPool.java

@@ -25,6 +25,7 @@
 import com.netflix.zuul.passport.PassportState;
 import io.netty.channel.ChannelFuture;
 import io.netty.channel.EventLoop;
+import io.netty.handler.codec.DecoderException;
 import io.netty.util.AttributeKey;
 import io.netty.util.concurrent.Promise;
 import org.slf4j.Logger;
@@ -296,8 +297,16 @@ protected void handleConnectCompletion(
             createConnection(cf, callerPromise, passport);
         } else {
             createConnFailedCounter.increment();
-            callerPromise.setFailure(
-                    new OriginConnectException(cf.cause().getMessage(), cf.cause(), OutboundErrorType.CONNECT_ERROR));
+
+            // unwrap DecoderExceptions to get a better indication of why decoding failed
+            // as decoding failures are not indicative of actual connection causes
+            if (cf.cause() instanceof DecoderException de && de.getCause() != null) {
+                callerPromise.setFailure(new OriginConnectException(
+                        de.getCause().getMessage(), de.getCause(), OutboundErrorType.CONNECT_ERROR));
+            } else {
+                callerPromise.setFailure(new OriginConnectException(
+                        cf.cause().getMessage(), cf.cause(), OutboundErrorType.CONNECT_ERROR));
+            }
         }
     }
 

zuul-core/src/main/java/com/netflix/zuul/niws/RequestAttempt.java

@@ -191,6 +191,10 @@ public long getDuration() {
         return this.duration;
     }
 
+    public String getCause() {
+        return cause;
+    }
+
     public String getError() {
         return error;
     }
@@ -304,22 +308,29 @@ public void setException(Throwable t) {
             if (t instanceof ReadTimeoutException) {
                 error = "READ_TIMEOUT";
                 exceptionType = t.getClass().getSimpleName();
-            } else if (t instanceof OriginConnectException) {
-                OriginConnectException oce = (OriginConnectException) t;
+            } else if (t instanceof OriginConnectException oce) {
                 if (oce.getErrorType() != null) {
                     error = oce.getErrorType().toString();
                 } else {
                     error = "ORIGIN_CONNECT_ERROR";
                 }
 
-                final Throwable cause = t.getCause();
-                if (cause != null) {
-                    exceptionType = t.getCause().getClass().getSimpleName();
+                Throwable oceCause = oce.getCause();
+
+                // unwrap ssl handshake exceptions to emit the underlying handshake failure causes
+                if (oceCause instanceof SSLHandshakeException sslHandshakeException
+                        && sslHandshakeException.getCause() != null) {
+                    oceCause = sslHandshakeException.getCause();
+                }
+
+                if (oceCause != null) {
+                    exceptionType = oce.getCause().getClass().getSimpleName();
+                    cause = oceCause.getMessage();
                 } else {
-                    exceptionType = t.getClass().getSimpleName();
+                    exceptionType = oce.getClass().getSimpleName();
                 }
-            } else if (t instanceof OutboundException) {
-                OutboundException obe = (OutboundException) t;
+
+            } else if (t instanceof OutboundException obe) {
                 error = obe.getOutboundErrorType().toString();
                 exceptionType = OutboundException.class.getSimpleName();
             } else if (t instanceof SSLHandshakeException) {

zuul-core/src/test/java/com/netflix/zuul/netty/connectionpool/PerServerConnectionPoolTest.java

@@ -40,6 +40,7 @@
 import io.netty.channel.local.LocalAddress;
 import io.netty.channel.local.LocalChannel;
 import io.netty.channel.local.LocalServerChannel;
+import io.netty.handler.codec.DecoderException;
 import io.netty.util.concurrent.Promise;
 import org.apache.commons.configuration.AbstractConfiguration;
 import org.junit.jupiter.api.AfterAll;
@@ -49,6 +50,7 @@
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
 
+import javax.net.ssl.SSLHandshakeException;
 import java.util.Deque;
 import java.util.UUID;
 import java.util.concurrent.ExecutionException;
@@ -58,6 +60,7 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
 import static org.junit.jupiter.api.Assertions.assertNotEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
@@ -363,6 +366,37 @@ void gracefulDrain() {
         assertTrue(connection2.getChannel().closeFuture().isSuccess());
     }
 
+    @Test
+    void handleConnectCompletionWithException() {
+
+        EmbeddedChannel channel = new EmbeddedChannel();
+        Promise<PooledConnection> promise = CLIENT_EVENT_LOOP.newPromise();
+        pool.handleConnectCompletion(
+                channel.newFailedFuture(new RuntimeException("runtime failure")), promise, CurrentPassport.create());
+
+        assertFalse(promise.isSuccess());
+        assertNotNull(promise.cause());
+        assertInstanceOf(OriginConnectException.class, promise.cause());
+        assertInstanceOf(RuntimeException.class, promise.cause().getCause(), "expect cause remains");
+    }
+
+    @Test
+    void handleConnectCompletionWithDecoderExceptionIsUnwrapped() {
+
+        EmbeddedChannel channel = new EmbeddedChannel();
+        Promise<PooledConnection> promise = CLIENT_EVENT_LOOP.newPromise();
+        pool.handleConnectCompletion(
+                channel.newFailedFuture(new DecoderException(new SSLHandshakeException("Invalid tls cert"))),
+                promise,
+                CurrentPassport.create());
+
+        assertFalse(promise.isSuccess());
+        assertNotNull(promise.cause());
+        assertInstanceOf(OriginConnectException.class, promise.cause());
+        assertInstanceOf(
+                SSLHandshakeException.class, promise.cause().getCause(), "expect decoder exception is unwrapped");
+    }
+
     private void checkChannelState(PooledConnection connection, CurrentPassport passport, int expectedUsage) {
         Channel channel = connection.getChannel();
         assertEquals(expectedUsage, connection.getUsageCount());

zuul-core/src/test/java/com/netflix/zuul/niws/RequestAttemptTest.java

@@ -0,0 +1,80 @@
+/*
+ * Copyright 2024 Netflix, Inc.
+ *
+ *      Licensed 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 com.netflix.zuul.niws;
+
+import com.netflix.zuul.exception.OutboundErrorType;
+import com.netflix.zuul.netty.connectionpool.OriginConnectException;
+import org.junit.jupiter.api.Test;
+
+import javax.net.ssl.SSLHandshakeException;
+import java.io.IOException;
+import java.security.cert.CertificateException;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class RequestAttemptTest {
+
+    @Test
+    void exceptionHandled() {
+
+        RequestAttempt attempt = new RequestAttempt(1, null, null, "target", "chosen", 200, null, null, 0, 0, 0);
+        attempt.setException(new RuntimeException("runtime failure"));
+
+        assertEquals("runtime failure", attempt.getError());
+    }
+
+    @Test
+    void originConnectExceptionUnwrapped() {
+
+        RequestAttempt attempt = new RequestAttempt(1, null, null, "target", "chosen", 200, null, null, 0, 0, 0);
+        attempt.setException(new OriginConnectException(
+                "origin connect failure",
+                new SSLHandshakeException("Invalid tls cert"),
+                OutboundErrorType.CONNECT_ERROR));
+
+        assertEquals("ORIGIN_CONNECT_ERROR", attempt.getError());
+        assertEquals("Invalid tls cert", attempt.getCause());
+    }
+
+    @Test
+    void originConnectExceptionWithSSLHandshakeCauseUnwrapped() {
+
+        SSLHandshakeException handshakeException = mock(SSLHandshakeException.class);
+        when(handshakeException.getCause()).thenReturn(new CertificateException("Cert doesn't match expected"));
+
+        RequestAttempt attempt = new RequestAttempt(1, null, null, "target", "chosen", 200, null, null, 0, 0, 0);
+        attempt.setException(new OriginConnectException(
+                "origin connect failure", handshakeException, OutboundErrorType.CONNECT_ERROR));
+
+        assertEquals("ORIGIN_CONNECT_ERROR", attempt.getError());
+        assertEquals("Cert doesn't match expected", attempt.getCause());
+    }
+
+    @Test
+    void originConnectExceptionWithCauseNotUnwrapped() {
+        RequestAttempt attempt = new RequestAttempt(1, null, null, "target", "chosen", 200, null, null, 0, 0, 0);
+        attempt.setException(new OriginConnectException(
+                "origin connect failure",
+                new IOException(new RuntimeException("socket failure")),
+                OutboundErrorType.CONNECT_ERROR));
+
+        assertEquals("ORIGIN_CONNECT_ERROR", attempt.getError());
+        assertEquals("java.lang.RuntimeException: socket failure", attempt.getCause());
+    }
+}