From 8651c0ad9fa1a6c7e50e0788698d841821d73f13 Mon Sep 17 00:00:00 2001
From: Simon Willnauer <simonw@apache.org>
Date: Mon, 23 Jan 2017 15:08:48 +0100
Subject: [PATCH] Log exceptions in onFailure instead of bubbling up
 (elastic/elasticsearch#4709)

Today we wrap the checked IOException in an unchecked exception when
sending back a failure the security transport interceptor. Yet, if that failure handling
in-turn fails due to a broken response channel we should rather log the exception instead
of bubbling it up since it can have unforeseeable side-effects.

Relates to elastic/elasticsearch#4706

* fix line len

Original commit: elastic/x-pack-elasticsearch@03c3826112b586d6c26e54debec69b34b9c66f4d
---
 .../SecurityServerTransportInterceptor.java   | 26 +++++++++++--------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/security/transport/SecurityServerTransportInterceptor.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/security/transport/SecurityServerTransportInterceptor.java
index ddbc803aef5..c4e2e2fe99f 100644
--- a/elasticsearch/src/main/java/org/elasticsearch/xpack/security/transport/SecurityServerTransportInterceptor.java
+++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/security/transport/SecurityServerTransportInterceptor.java
@@ -5,10 +5,12 @@
  */
 package org.elasticsearch.xpack.security.transport;
 
+import org.apache.logging.log4j.Logger;
 import org.elasticsearch.Version;
 import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.action.support.DestructiveOperations;
 import org.elasticsearch.common.CheckedConsumer;
+import org.elasticsearch.common.component.AbstractComponent;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.util.concurrent.AbstractRunnable;
 import org.elasticsearch.common.util.concurrent.ThreadContext;
@@ -38,8 +40,6 @@ import org.elasticsearch.xpack.security.user.SystemUser;
 import org.elasticsearch.xpack.security.user.User;
 import org.elasticsearch.xpack.ssl.SSLService;
 
-import java.io.IOException;
-import java.io.UncheckedIOException;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
@@ -48,7 +48,7 @@ import java.util.concurrent.Executor;
 import static org.elasticsearch.xpack.XPackSettings.TRANSPORT_SSL_ENABLED;
 import static org.elasticsearch.xpack.security.Security.setting;
 
-public class SecurityServerTransportInterceptor implements TransportInterceptor {
+public class SecurityServerTransportInterceptor extends AbstractComponent implements TransportInterceptor {
 
     private static final String SETTING_NAME = "xpack.security.type";
 
@@ -70,6 +70,7 @@ public class SecurityServerTransportInterceptor implements TransportInterceptor
                                               SSLService sslService,
                                               SecurityContext securityContext,
                                               DestructiveOperations destructiveOperations) {
+        super(settings);
         this.settings = settings;
         this.threadPool = threadPool;
         this.authcService = authcService;
@@ -131,7 +132,7 @@ public class SecurityServerTransportInterceptor implements TransportInterceptor
     public <T extends TransportRequest> TransportRequestHandler<T> interceptHandler(String action, String executor,
                                                                                     boolean forceExecution,
                                                                                     TransportRequestHandler<T> actualHandler) {
-        return new ProfileSecuredRequestHandler<>(action, forceExecution, executor, actualHandler, profileFilters,
+        return new ProfileSecuredRequestHandler<>(logger, action, forceExecution, executor, actualHandler, profileFilters,
                 licenseState, threadPool);
     }
 
@@ -173,18 +174,20 @@ public class SecurityServerTransportInterceptor implements TransportInterceptor
 
     public static class ProfileSecuredRequestHandler<T extends TransportRequest> implements TransportRequestHandler<T> {
 
-        protected final String action;
-        protected final TransportRequestHandler<T> handler;
+        private final String action;
+        private final TransportRequestHandler<T> handler;
         private final Map<String, ServerTransportFilter> profileFilters;
         private final XPackLicenseState licenseState;
         private final ThreadContext threadContext;
         private final String executorName;
         private final ThreadPool threadPool;
         private final boolean forceExecution;
+        private final Logger logger;
 
-        ProfileSecuredRequestHandler(String action, boolean forceExecution, String executorName, TransportRequestHandler<T> handler,
-                                     Map<String, ServerTransportFilter> profileFilters, XPackLicenseState licenseState,
-                                     ThreadPool threadPool) {
+        ProfileSecuredRequestHandler(Logger logger, String action, boolean forceExecution, String executorName,
+                                     TransportRequestHandler<T> handler, Map<String, ServerTransportFilter> profileFilters,
+                                     XPackLicenseState licenseState, ThreadPool threadPool) {
+            this.logger = logger;
             this.action = action;
             this.executorName = executorName;
             this.handler = handler;
@@ -206,8 +209,9 @@ public class SecurityServerTransportInterceptor implements TransportInterceptor
                 public void onFailure(Exception e) {
                     try {
                         channel.sendResponse(e);
-                    } catch (IOException e1) {
-                        throw new UncheckedIOException(e1);
+                    } catch (Exception e1) {
+                        e1.addSuppressed(e);
+                        logger.warn("failed to send exception response for action [" + action + "]", e1);
                     }
                 }