From 5e2bdf197db7ac54e5e0621433c83fa25c8d777f Mon Sep 17 00:00:00 2001 From: Justin Guerra Date: Wed, 30 Oct 2024 13:37:41 -0600 Subject: [PATCH] Handle close_notify in ConnectionPoolHandler (#1841) * Close on completion close event * Add handling to OriginResponseReceiver * Use a new status category * Fix index * Add unit test for ids being unique * 504 is probably better than 500 * Switch to debug logging * Use 502 for close_notify --- .../zuul/exception/OutboundErrorType.java | 4 ++ .../connectionpool/ConnectionPoolHandler.java | 11 +++++- .../netty/server/OriginResponseReceiver.java | 28 ++++++++++---- .../zuul/stats/status/ZuulStatusCategory.java | 3 +- .../stats/status/ZuulStatusCategoryTest.java | 38 +++++++++++++++++++ 5 files changed, 73 insertions(+), 11 deletions(-) create mode 100644 zuul-core/src/test/java/com/netflix/zuul/stats/status/ZuulStatusCategoryTest.java diff --git a/zuul-core/src/main/java/com/netflix/zuul/exception/OutboundErrorType.java b/zuul-core/src/main/java/com/netflix/zuul/exception/OutboundErrorType.java index b39fe87682..451a79a768 100644 --- a/zuul-core/src/main/java/com/netflix/zuul/exception/OutboundErrorType.java +++ b/zuul-core/src/main/java/com/netflix/zuul/exception/OutboundErrorType.java @@ -55,6 +55,10 @@ public enum OutboundErrorType implements ErrorType { ERROR_TYPE_ORIGIN_RESET_CONN_STATUS.get(), ZuulStatusCategory.FAILURE_ORIGIN_RESET_CONNECTION, ClientException.ErrorType.CONNECT_EXCEPTION), + CLOSE_NOTIFY_CONNECTION( + 502, + ZuulStatusCategory.FAILURE_ORIGIN_CLOSE_NOTIFY_CONNECTION, + ClientException.ErrorType.CONNECT_EXCEPTION), CANCELLED(400, ZuulStatusCategory.FAILURE_CLIENT_CANCELLED, ClientException.ErrorType.SOCKET_TIMEOUT_EXCEPTION), ORIGIN_CONCURRENCY_EXCEEDED( ERROR_TYPE_ORIGIN_CONCURRENCY_EXCEEDED_STATUS.get(), diff --git a/zuul-core/src/main/java/com/netflix/zuul/netty/connectionpool/ConnectionPoolHandler.java b/zuul-core/src/main/java/com/netflix/zuul/netty/connectionpool/ConnectionPoolHandler.java index f344f8cb13..6c97a59430 100644 --- a/zuul-core/src/main/java/com/netflix/zuul/netty/connectionpool/ConnectionPoolHandler.java +++ b/zuul-core/src/main/java/com/netflix/zuul/netty/connectionpool/ConnectionPoolHandler.java @@ -28,6 +28,7 @@ import io.netty.channel.ChannelHandlerContext; import io.netty.handler.codec.http.HttpHeaderNames; import io.netty.handler.codec.http.HttpResponse; +import io.netty.handler.ssl.SslCloseCompletionEvent; import io.netty.handler.timeout.IdleStateEvent; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -48,6 +49,7 @@ public class ConnectionPoolHandler extends ChannelDuplexHandler { private final Counter inactiveCounter; private final Counter errorCounter; private final Counter headerCloseCounter; + private final Counter sslCloseCompletionCounter; public ConnectionPoolHandler(OriginName originName) { if (originName == null) { @@ -58,6 +60,7 @@ public ConnectionPoolHandler(OriginName originName) { this.inactiveCounter = SpectatorUtils.newCounter(METRIC_PREFIX + "_inactive", originName.getMetricId()); this.errorCounter = SpectatorUtils.newCounter(METRIC_PREFIX + "_error", originName.getMetricId()); this.headerCloseCounter = SpectatorUtils.newCounter(METRIC_PREFIX + "_headerClose", originName.getMetricId()); + this.sslCloseCompletionCounter = SpectatorUtils.newCounter(METRIC_PREFIX + "_sslClose", originName.getMetricId()); } @Override @@ -71,12 +74,11 @@ public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exc final String msg = "Origin channel for origin - " + originName + " - idle timeout has fired. " + ChannelUtils.channelInfoForLogging(ctx.channel()); closeConnection(ctx, msg); - } else if (evt instanceof CompleteEvent) { + } else if (evt instanceof CompleteEvent completeEvt) { // The HttpLifecycleChannelHandler instance will fire this event when either a response has finished being // written, or // the channel is no longer active or disconnected. // Return the connection to pool. - CompleteEvent completeEvt = (CompleteEvent) evt; final CompleteReason reason = completeEvt.getReason(); if (reason == CompleteReason.SESSION_COMPLETE) { final PooledConnection conn = PooledConnection.getFromChannel(ctx.channel()); @@ -97,6 +99,11 @@ public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exc + reason.name() + ", " + ChannelUtils.channelInfoForLogging(ctx.channel()); closeConnection(ctx, msg); } + } else if(evt instanceof SslCloseCompletionEvent event) { + sslCloseCompletionCounter.increment(); + String msg = "Origin channel for origin - " + originName + " - received SslCloseCompletionEvent " + event + ". " + + ChannelUtils.channelInfoForLogging(ctx.channel()); + closeConnection(ctx, msg); } } diff --git a/zuul-core/src/main/java/com/netflix/zuul/netty/server/OriginResponseReceiver.java b/zuul-core/src/main/java/com/netflix/zuul/netty/server/OriginResponseReceiver.java index ef4e4e9df0..a60ebdcc07 100644 --- a/zuul-core/src/main/java/com/netflix/zuul/netty/server/OriginResponseReceiver.java +++ b/zuul-core/src/main/java/com/netflix/zuul/netty/server/OriginResponseReceiver.java @@ -38,6 +38,7 @@ import io.netty.handler.codec.http.HttpRequest; import io.netty.handler.codec.http.HttpResponse; import io.netty.handler.codec.http.HttpVersion; +import io.netty.handler.ssl.SslCloseCompletionEvent; import io.netty.handler.ssl.SslHandshakeCompletionEvent; import io.netty.handler.timeout.IdleStateEvent; import io.netty.handler.timeout.ReadTimeoutException; @@ -59,6 +60,8 @@ public class OriginResponseReceiver extends ChannelDuplexHandler { private static final Logger logger = LoggerFactory.getLogger(OriginResponseReceiver.class); private static final AttributeKey SSL_HANDSHAKE_UNSUCCESS_FROM_ORIGIN_THROWABLE = AttributeKey.newInstance("_ssl_handshake_from_origin_throwable"); + private static final AttributeKey SSL_CLOSE_NOTIFY_SEEN = + AttributeKey.newInstance("_ssl_close_notify_seen"); public static final String CHANNEL_HANDLER_NAME = "_origin_response_receiver"; public OriginResponseReceiver(final ProxyEndpoint edgeProxy) { @@ -106,15 +109,20 @@ protected void channelReadInternal(final ChannelHandlerContext ctx, Object msg) @Override public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exception { - if (evt instanceof CompleteEvent) { - final CompleteReason reason = ((CompleteEvent) evt).getReason(); + if (evt instanceof CompleteEvent completeEvent) { + final CompleteReason reason = completeEvent.getReason(); if ((reason != CompleteReason.SESSION_COMPLETE) && (edgeProxy != null)) { - logger.error( - "Origin request completed with reason other than COMPLETE: {}, {}", - reason.name(), - ChannelUtils.channelInfoForLogging(ctx.channel())); - final ZuulException ze = new ZuulException("CompleteEvent", reason.name(), true); - edgeProxy.errorFromOrigin(ze); + if(reason == CompleteReason.CLOSE && Boolean.TRUE.equals(ctx.channel().attr(SSL_CLOSE_NOTIFY_SEEN).get())) { + logger.warn("Origin request completed with close, after getting a SslCloseCompletionEvent event: {}", ChannelUtils.channelInfoForLogging(ctx.channel())); + edgeProxy.errorFromOrigin(new OriginConnectException("Origin connection close_notify", OutboundErrorType.CLOSE_NOTIFY_CONNECTION)); + } else { + logger.error( + "Origin request completed with reason other than COMPLETE: {}, {}", + reason.name(), + ChannelUtils.channelInfoForLogging(ctx.channel())); + final ZuulException ze = new ZuulException("CompleteEvent", reason.name(), true); + edgeProxy.errorFromOrigin(ze); + } } // First let this event propagate along the pipeline, before cleaning vars from the channel. @@ -135,6 +143,10 @@ public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exc new OutboundException(OutboundErrorType.READ_TIMEOUT, edgeProxy.getRequestAttempts())); } super.userEventTriggered(ctx, evt); + } else if(evt instanceof SslCloseCompletionEvent) { + logger.debug("Received SslCloseCompletionEvent on {}", ChannelUtils.channelInfoForLogging(ctx.channel())); + ctx.channel().attr(SSL_CLOSE_NOTIFY_SEEN).set(true); + super.userEventTriggered(ctx, evt); } else { super.userEventTriggered(ctx, evt); } diff --git a/zuul-core/src/main/java/com/netflix/zuul/stats/status/ZuulStatusCategory.java b/zuul-core/src/main/java/com/netflix/zuul/stats/status/ZuulStatusCategory.java index 48f1864fd0..c44e39e0b2 100644 --- a/zuul-core/src/main/java/com/netflix/zuul/stats/status/ZuulStatusCategory.java +++ b/zuul-core/src/main/java/com/netflix/zuul/stats/status/ZuulStatusCategory.java @@ -69,7 +69,8 @@ public enum ZuulStatusCategory implements StatusCategory { FAILURE_ORIGIN_THROTTLED(ZuulStatusCategoryGroup.FAILURE, 6, "Throttled by origin returning 503 status"), FAILURE_ORIGIN_NO_SERVERS(ZuulStatusCategoryGroup.FAILURE, 14, "No UP origin servers available in Discovery"), FAILURE_ORIGIN_RESET_CONNECTION( - ZuulStatusCategoryGroup.FAILURE, 15, "Connection reset on an established origin connection"); + ZuulStatusCategoryGroup.FAILURE, 15, "Connection reset on an established origin connection"), + FAILURE_ORIGIN_CLOSE_NOTIFY_CONNECTION(ZuulStatusCategoryGroup.FAILURE, 16, "Connection TLS session shutdown"); private final StatusCategoryGroup group; private final String id; diff --git a/zuul-core/src/test/java/com/netflix/zuul/stats/status/ZuulStatusCategoryTest.java b/zuul-core/src/test/java/com/netflix/zuul/stats/status/ZuulStatusCategoryTest.java new file mode 100644 index 0000000000..4af2a07935 --- /dev/null +++ b/zuul-core/src/test/java/com/netflix/zuul/stats/status/ZuulStatusCategoryTest.java @@ -0,0 +1,38 @@ +/* + * 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.stats.status; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import java.util.Arrays; +import java.util.Set; +import java.util.stream.Collectors; +import org.junit.jupiter.api.Test; + +/** + * @author Justin Guerra + * @since 10/29/24 + */ +public class ZuulStatusCategoryTest { + + @Test + public void categoriesUseUniqueIds() { + ZuulStatusCategory[] values = ZuulStatusCategory.values(); + Set ids = Arrays.stream(values).map(ZuulStatusCategory::getId).collect(Collectors.toSet()); + assertEquals(values.length, ids.size()); + } +}