From 273a9fb46fee56a44c264cb6caec5f9ff589bc7a Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Mon, 19 Sep 2016 10:06:02 +0200 Subject: [PATCH] Watcher: Fix possible chained input NPE for execution failures (elastic/elasticsearch#3490) Due to untested code there was an NPE happening in production, when a chained input execution failed, but the chained input tried to access the resulting payload (which is never set on failures). This payload now defaults to being empty. This commit also drive-by fixes a broken logging statement, that on the one side returned not the watch id, but a useless watch toString() representation, and on the other hand only logs an error message, but not a stack trace into the log, as this is what the history is for. Original commit: elastic/x-pack-elasticsearch@7dbe1afd90ad3ae703c121f19a364170536ef978 --- .../xpack/watcher/input/Input.java | 3 +- .../input/chain/ExecutableChainInput.java | 4 +- .../input/http/ExecutableHttpInput.java | 4 +- .../input/search/ExecutableSearchInput.java | 4 +- .../input/chain/ChainIntegrationTests.java | 9 +-- .../chain/ExecutableChainInputTests.java | 74 +++++++++++++++++++ 6 files changed, 80 insertions(+), 18 deletions(-) create mode 100644 elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/chain/ExecutableChainInputTests.java diff --git a/elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/xpack/watcher/input/Input.java b/elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/xpack/watcher/input/Input.java index 8f56c915669..c9bca989d49 100644 --- a/elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/xpack/watcher/input/Input.java +++ b/elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/xpack/watcher/input/Input.java @@ -43,7 +43,7 @@ public interface Input extends ToXContent { this.status = Status.FAILURE; this.type = type; this.reason = ExceptionsHelper.detailedMessage(e); - this.payload = null; + this.payload = Payload.EMPTY; } public String type() { @@ -55,7 +55,6 @@ public interface Input extends ToXContent { } public Payload payload() { - assert status == Status.SUCCESS; return payload; } diff --git a/elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/xpack/watcher/input/chain/ExecutableChainInput.java b/elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/xpack/watcher/input/chain/ExecutableChainInput.java index 5493f1a5e2e..c2f9372ba2f 100644 --- a/elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/xpack/watcher/input/chain/ExecutableChainInput.java +++ b/elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/xpack/watcher/input/chain/ExecutableChainInput.java @@ -6,8 +6,6 @@ package org.elasticsearch.xpack.watcher.input.chain; import org.apache.logging.log4j.Logger; -import org.apache.logging.log4j.message.ParameterizedMessage; -import org.apache.logging.log4j.util.Supplier; import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.xpack.watcher.execution.WatchExecutionContext; import org.elasticsearch.xpack.watcher.input.ExecutableInput; @@ -44,7 +42,7 @@ public class ExecutableChainInput extends ExecutableInput) () -> new ParameterizedMessage("failed to execute [{}] input for [{}]", TYPE, ctx.watch().id()), e); + logger.error("failed to execute [{}] input for watch [{}], reason [{}]", TYPE, ctx.watch().id(), e.getMessage()); return new ChainInput.Result(e); } } diff --git a/elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/xpack/watcher/input/http/ExecutableHttpInput.java b/elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/xpack/watcher/input/http/ExecutableHttpInput.java index 34eea823f99..02bbd623b9e 100644 --- a/elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/xpack/watcher/input/http/ExecutableHttpInput.java +++ b/elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/xpack/watcher/input/http/ExecutableHttpInput.java @@ -7,8 +7,6 @@ package org.elasticsearch.xpack.watcher.input.http; import org.apache.logging.log4j.Logger; -import org.apache.logging.log4j.message.ParameterizedMessage; -import org.apache.logging.log4j.util.Supplier; import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentParser; @@ -49,7 +47,7 @@ public class ExecutableHttpInput extends ExecutableInput) () -> new ParameterizedMessage("failed to execute [{}] input for [{}]", TYPE, ctx.watch()), e); + logger.error("failed to execute [{}] input for watch [{}], reason [{}]", TYPE, ctx.watch().id(), e.getMessage()); return new HttpInput.Result(request, e); } } diff --git a/elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/xpack/watcher/input/search/ExecutableSearchInput.java b/elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/xpack/watcher/input/search/ExecutableSearchInput.java index b5bf6708117..501d0581f18 100644 --- a/elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/xpack/watcher/input/search/ExecutableSearchInput.java +++ b/elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/xpack/watcher/input/search/ExecutableSearchInput.java @@ -6,8 +6,6 @@ package org.elasticsearch.xpack.watcher.input.search; import org.apache.logging.log4j.Logger; -import org.apache.logging.log4j.message.ParameterizedMessage; -import org.apache.logging.log4j.util.Supplier; import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.action.search.SearchType; import org.elasticsearch.common.Nullable; @@ -60,7 +58,7 @@ public class ExecutableSearchInput extends ExecutableInput) () -> new ParameterizedMessage("failed to execute [{}] input for [{}]", TYPE, ctx.watch()), e); + logger.error("failed to execute [{}] input for watch [{}], reason [{}]", TYPE, ctx.watch().id(), e.getMessage()); return new SearchInput.Result(request, e); } } diff --git a/elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/chain/ChainIntegrationTests.java b/elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/chain/ChainIntegrationTests.java index e2d249a69e5..b2e22c2ccdd 100644 --- a/elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/chain/ChainIntegrationTests.java +++ b/elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/chain/ChainIntegrationTests.java @@ -12,9 +12,9 @@ import org.elasticsearch.index.IndexNotFoundException; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.transport.Netty3Plugin; import org.elasticsearch.transport.Netty4Plugin; -import org.elasticsearch.xpack.watcher.input.http.HttpInput; import org.elasticsearch.xpack.common.http.HttpRequestTemplate; import org.elasticsearch.xpack.common.http.auth.basic.BasicAuth; +import org.elasticsearch.xpack.watcher.input.http.HttpInput; import org.elasticsearch.xpack.watcher.test.AbstractWatcherIntegrationTestCase; import java.net.InetSocketAddress; @@ -79,12 +79,7 @@ public class ChainIntegrationTests extends AbstractWatcherIntegrationTestCase { timeWarp().scheduler().trigger("_name"); refresh(); } else { - assertBusy(new Runnable() { - @Override - public void run() { - assertWatchExecuted(); - } - }, 9, TimeUnit.SECONDS); + assertBusy(() -> assertWatchExecuted(), 9, TimeUnit.SECONDS); } assertWatchWithMinimumPerformedActionsCount("_name", 1, false); diff --git a/elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/chain/ExecutableChainInputTests.java b/elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/chain/ExecutableChainInputTests.java new file mode 100644 index 00000000000..09318c085ed --- /dev/null +++ b/elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/xpack/watcher/input/chain/ExecutableChainInputTests.java @@ -0,0 +1,74 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.xpack.watcher.input.chain; + +import org.elasticsearch.common.Nullable; +import org.elasticsearch.common.collect.Tuple; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.watcher.execution.WatchExecutionContext; +import org.elasticsearch.xpack.watcher.execution.Wid; +import org.elasticsearch.xpack.watcher.input.ExecutableInput; +import org.elasticsearch.xpack.watcher.input.Input; +import org.elasticsearch.xpack.watcher.input.simple.SimpleInput; +import org.elasticsearch.xpack.watcher.watch.Payload; +import org.joda.time.DateTime; +import org.joda.time.DateTimeZone; + +import java.io.IOException; +import java.util.Arrays; + +import static org.elasticsearch.xpack.watcher.input.Input.Result.Status; +import static org.elasticsearch.xpack.watcher.test.WatcherTestUtils.mockExecutionContextBuilder; +import static org.hamcrest.Matchers.is; + +public class ExecutableChainInputTests extends ESTestCase { + + public void testFailedResultHandling() throws Exception { + WatchExecutionContext ctx = createWatchExecutionContext(); + ChainInput chainInput = new ChainInput(Arrays.asList(new Tuple<>("whatever", new SimpleInput(Payload.EMPTY)))); + + Tuple tuple = new Tuple<>("whatever", new FailingExecutableInput()); + ExecutableChainInput executableChainInput = new ExecutableChainInput(chainInput, Arrays.asList(tuple), logger); + ChainInput.Result result = executableChainInput.execute(ctx, Payload.EMPTY); + assertThat(result.status(), is(Status.SUCCESS)); + } + + private class FailingExecutableInput extends ExecutableInput { + + protected FailingExecutableInput() { + super(new SimpleInput(Payload.EMPTY), ExecutableChainInputTests.this.logger); + } + + @Override + public Input.Result execute(WatchExecutionContext ctx, @Nullable Payload payload) { + return new FailingExecutableInputResult(new RuntimeException("foo")); + } + } + + private static class FailingExecutableInputResult extends Input.Result { + + protected FailingExecutableInputResult(Exception e) { + super("failing", e); + } + + @Override + protected XContentBuilder typeXContent(XContentBuilder builder, Params params) throws IOException { + return builder; + } + } + + private WatchExecutionContext createWatchExecutionContext() { + DateTime now = DateTime.now(DateTimeZone.UTC); + Wid wid = new Wid(randomAsciiOfLength(5), randomLong(), now); + return mockExecutionContextBuilder(wid.watchId()) + .wid(wid) + .payload(new Payload.Simple()) + .time(wid.watchId(), now) + .buildMock(); + } + +}