From 715ee4e467a5f41130510c407c0aa318e5ec5eaf Mon Sep 17 00:00:00 2001 From: Gavin Bunney <409207+gavinbunney@users.noreply.github.com> Date: Fri, 19 Jan 2024 13:35:21 -0800 Subject: [PATCH] Unwrap DecoderExceptions on connections (OriginConnectExceptions) into RequestAttempts (#1727) --- .../PerServerConnectionPool.java | 13 ++- .../com/netflix/zuul/niws/RequestAttempt.java | 27 +++++-- .../PerServerConnectionPoolTest.java | 34 ++++++++ .../netflix/zuul/niws/RequestAttemptTest.java | 80 +++++++++++++++++++ 4 files changed, 144 insertions(+), 10 deletions(-) create mode 100644 zuul-core/src/test/java/com/netflix/zuul/niws/RequestAttemptTest.java diff --git a/zuul-core/src/main/java/com/netflix/zuul/netty/connectionpool/PerServerConnectionPool.java b/zuul-core/src/main/java/com/netflix/zuul/netty/connectionpool/PerServerConnectionPool.java index 3c21447bfd..5820e6414e 100644 --- a/zuul-core/src/main/java/com/netflix/zuul/netty/connectionpool/PerServerConnectionPool.java +++ b/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)); + } } } diff --git a/zuul-core/src/main/java/com/netflix/zuul/niws/RequestAttempt.java b/zuul-core/src/main/java/com/netflix/zuul/niws/RequestAttempt.java index b210744895..6b1b2e9850 100644 --- a/zuul-core/src/main/java/com/netflix/zuul/niws/RequestAttempt.java +++ b/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) { diff --git a/zuul-core/src/test/java/com/netflix/zuul/netty/connectionpool/PerServerConnectionPoolTest.java b/zuul-core/src/test/java/com/netflix/zuul/netty/connectionpool/PerServerConnectionPoolTest.java index 089e98c461..d719f9230a 100644 --- a/zuul-core/src/test/java/com/netflix/zuul/netty/connectionpool/PerServerConnectionPoolTest.java +++ b/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 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 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()); diff --git a/zuul-core/src/test/java/com/netflix/zuul/niws/RequestAttemptTest.java b/zuul-core/src/test/java/com/netflix/zuul/niws/RequestAttemptTest.java new file mode 100644 index 0000000000..df9e305f04 --- /dev/null +++ b/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()); + } +} \ No newline at end of file