From fdb02f4f990053f1ba9e86a709580904c992632e Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Tue, 5 Dec 2017 11:31:48 +0100 Subject: [PATCH] Watcher: Fix pagerduty action to send context data (elastic/x-pack-elasticsearch#3185) The pagerduty action allows to send contexts, which contains an array of texts or images, each with a link. The field of this data was named 'context' instead of 'contexts' and thus those contects were never correctly parsed on the pagerduty side. Unfortunately pagerduty accepts any JSON, thus this was not caught so far. This commit allows parsing of the old field name to retain BWC, but when written out via toXContent, it will always use the 'contexts' field name. relates elastic/x-pack-elasticsearch#3184 Original commit: elastic/x-pack-elasticsearch@50f0b65d5684d35fad3e02fe08bd3d45d92a4237 --- docs/en/watcher/actions/pagerduty.asciidoc | 2 +- .../notification/pagerduty/IncidentEvent.java | 16 ++- .../pagerduty/PagerDutyActionTests.java | 9 +- .../pagerduty/PagerDutyAccountsTests.java | 127 ++++-------------- .../integration/PagerDutyServiceTests.java | 5 +- 5 files changed, 47 insertions(+), 112 deletions(-) diff --git a/docs/en/watcher/actions/pagerduty.asciidoc b/docs/en/watcher/actions/pagerduty.asciidoc index a7a3cfd8545..ce19020ce4a 100644 --- a/docs/en/watcher/actions/pagerduty.asciidoc +++ b/docs/en/watcher/actions/pagerduty.asciidoc @@ -45,7 +45,7 @@ payload as well as an array of contexts to the action. "attach_payload" : true, "client" : "/foo/bar/{{ctx.watch_id}}", "client_url" : "http://www.example.org/", - "context" : [ + "contexts" : [ { "type": "link", "href": "http://acme.pagerduty.com" diff --git a/plugin/src/main/java/org/elasticsearch/xpack/watcher/notification/pagerduty/IncidentEvent.java b/plugin/src/main/java/org/elasticsearch/xpack/watcher/notification/pagerduty/IncidentEvent.java index 28092ef460f..4353b099c75 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/watcher/notification/pagerduty/IncidentEvent.java +++ b/plugin/src/main/java/org/elasticsearch/xpack/watcher/notification/pagerduty/IncidentEvent.java @@ -121,7 +121,7 @@ public class IncidentEvent implements ToXContentObject { builder.endObject(); } if (contexts != null && contexts.length > 0) { - builder.startArray(Fields.CONTEXT.getPreferredName()); + builder.startArray(Fields.CONTEXTS.getPreferredName()); for (IncidentEventContext context : contexts) { context.toXContent(builder, params); } @@ -154,7 +154,7 @@ public class IncidentEvent implements ToXContentObject { } builder.field(Fields.ATTACH_PAYLOAD.getPreferredName(), attachPayload); if (contexts != null) { - builder.startArray(Fields.CONTEXT.getPreferredName()); + builder.startArray(Fields.CONTEXTS.getPreferredName()); for (IncidentEventContext context : contexts) { context.toXContent(builder, params); } @@ -265,7 +265,7 @@ public class IncidentEvent implements ToXContentObject { proxy.toXContent(builder, params); } if (contexts != null) { - builder.startArray(Fields.CONTEXT.getPreferredName()); + builder.startArray(Fields.CONTEXTS.getPreferredName()); for (IncidentEventContext.Template context : contexts) { context.toXContent(builder, params); } @@ -341,7 +341,7 @@ public class IncidentEvent implements ToXContentObject { throw new ElasticsearchParseException("could not parse pager duty event template. failed to parse field [{}], " + "expected a boolean value but found [{}] instead", Fields.ATTACH_PAYLOAD.getPreferredName(), token); } - } else if (Fields.CONTEXT.match(currentFieldName)) { + } else if (Fields.CONTEXTS.match(currentFieldName) || Fields.CONTEXT_DEPRECATED.match(currentFieldName)) { if (token == XContentParser.Token.START_ARRAY) { List list = new ArrayList<>(); while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { @@ -349,7 +349,7 @@ public class IncidentEvent implements ToXContentObject { list.add(IncidentEventContext.Template.parse(parser)); } catch (ElasticsearchParseException e) { throw new ElasticsearchParseException("could not parse pager duty event template. failed to parse field " + - "[{}]", Fields.CONTEXT.getPreferredName()); + "[{}]", parser.currentName()); } } contexts = list.toArray(new IncidentEventContext.Template[list.size()]); @@ -438,7 +438,11 @@ public class IncidentEvent implements ToXContentObject { ParseField CLIENT = new ParseField("client"); ParseField CLIENT_URL = new ParseField("client_url"); ParseField ATTACH_PAYLOAD = new ParseField("attach_payload"); - ParseField CONTEXT = new ParseField("context"); + ParseField CONTEXTS = new ParseField("contexts"); + // this field exists because in versions prior 6.0 we accidentally used context instead of contexts and thus the correct data + // was never picked up on the pagerduty side + // we need to keep this for BWC + ParseField CONTEXT_DEPRECATED = new ParseField("context"); ParseField SERVICE_KEY = new ParseField("service_key"); ParseField PAYLOAD = new ParseField("payload"); diff --git a/plugin/src/test/java/org/elasticsearch/xpack/watcher/actions/pagerduty/PagerDutyActionTests.java b/plugin/src/test/java/org/elasticsearch/xpack/watcher/actions/pagerduty/PagerDutyActionTests.java index 7cdd96cc457..6b199a50e44 100644 --- a/plugin/src/test/java/org/elasticsearch/xpack/watcher/actions/pagerduty/PagerDutyActionTests.java +++ b/plugin/src/test/java/org/elasticsearch/xpack/watcher/actions/pagerduty/PagerDutyActionTests.java @@ -14,20 +14,20 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.watcher.actions.Action; import org.elasticsearch.xpack.watcher.common.http.HttpProxy; import org.elasticsearch.xpack.watcher.common.http.HttpRequest; import org.elasticsearch.xpack.watcher.common.http.HttpResponse; import org.elasticsearch.xpack.watcher.common.text.TextTemplate; import org.elasticsearch.xpack.watcher.common.text.TextTemplateEngine; +import org.elasticsearch.xpack.watcher.execution.WatchExecutionContext; +import org.elasticsearch.xpack.watcher.execution.Wid; import org.elasticsearch.xpack.watcher.notification.pagerduty.IncidentEvent; import org.elasticsearch.xpack.watcher.notification.pagerduty.IncidentEventContext; import org.elasticsearch.xpack.watcher.notification.pagerduty.IncidentEventDefaults; import org.elasticsearch.xpack.watcher.notification.pagerduty.PagerDutyAccount; import org.elasticsearch.xpack.watcher.notification.pagerduty.PagerDutyService; import org.elasticsearch.xpack.watcher.notification.pagerduty.SentEvent; -import org.elasticsearch.xpack.watcher.actions.Action; -import org.elasticsearch.xpack.watcher.execution.WatchExecutionContext; -import org.elasticsearch.xpack.watcher.execution.Wid; import org.elasticsearch.xpack.watcher.watch.Payload; import org.joda.time.DateTime; import org.joda.time.DateTimeZone; @@ -176,7 +176,8 @@ public class PagerDutyActionTests extends ESTestCase { IncidentEventContext.Template.link(new TextTemplate("_href"), new TextTemplate("_text")), IncidentEventContext.Template.image(new TextTemplate("_src"), new TextTemplate("_href"), new TextTemplate("_alt")) }; - builder.array("context", (Object) contexts); + String fieldName = randomBoolean() ? "contexts" : "context"; + builder.array(fieldName, (Object) contexts); } builder.endObject(); diff --git a/plugin/src/test/java/org/elasticsearch/xpack/watcher/notification/pagerduty/PagerDutyAccountsTests.java b/plugin/src/test/java/org/elasticsearch/xpack/watcher/notification/pagerduty/PagerDutyAccountsTests.java index 88e5b90fa69..fd476629203 100644 --- a/plugin/src/test/java/org/elasticsearch/xpack/watcher/notification/pagerduty/PagerDutyAccountsTests.java +++ b/plugin/src/test/java/org/elasticsearch/xpack/watcher/notification/pagerduty/PagerDutyAccountsTests.java @@ -5,10 +5,12 @@ */ package org.elasticsearch.xpack.watcher.notification.pagerduty; +import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.settings.SettingsException; +import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.test.rest.yaml.ObjectPath; import org.elasticsearch.xpack.watcher.common.http.HttpClient; import org.elasticsearch.xpack.watcher.common.http.HttpProxy; import org.elasticsearch.xpack.watcher.common.http.HttpRequest; @@ -20,9 +22,7 @@ import org.mockito.ArgumentCaptor; import java.util.Collections; -import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; -import static org.hamcrest.Matchers.isOneOf; import static org.hamcrest.Matchers.notNullValue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -36,102 +36,6 @@ public class PagerDutyAccountsTests extends ESTestCase { httpClient = mock(HttpClient.class); } - public void testSingleAccount() throws Exception { - Settings.Builder builder = Settings.builder() - .put("xpack.notification.pagerduty.default_account", "account1"); - addAccountSettings("account1", builder); - PagerDutyService service = new PagerDutyService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, - Collections.singleton(PagerDutyService.PAGERDUTY_ACCOUNT_SETTING))); - PagerDutyAccount account = service.getAccount("account1"); - assertThat(account, notNullValue()); - assertThat(account.name, equalTo("account1")); - account = service.getAccount(null); // falling back on the default - assertThat(account, notNullValue()); - assertThat(account.name, equalTo("account1")); - } - - public void testSingleAccountNoExplicitDefault() throws Exception { - Settings.Builder builder = Settings.builder(); - addAccountSettings("account1", builder); - - PagerDutyService service = new PagerDutyService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, - Collections.singleton(PagerDutyService.PAGERDUTY_ACCOUNT_SETTING))); - PagerDutyAccount account = service.getAccount("account1"); - assertThat(account, notNullValue()); - assertThat(account.name, equalTo("account1")); - account = service.getAccount(null); // falling back on the default - assertThat(account, notNullValue()); - assertThat(account.name, equalTo("account1")); - } - - public void testMultipleAccounts() throws Exception { - Settings.Builder builder = Settings.builder() - .put("xpack.notification.pagerduty.default_account", "account1"); - addAccountSettings("account1", builder); - addAccountSettings("account2", builder); - - PagerDutyService service = new PagerDutyService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, - Collections.singleton(PagerDutyService.PAGERDUTY_ACCOUNT_SETTING))); - PagerDutyAccount account = service.getAccount("account1"); - assertThat(account, notNullValue()); - assertThat(account.name, equalTo("account1")); - account = service.getAccount("account2"); - assertThat(account, notNullValue()); - assertThat(account.name, equalTo("account2")); - account = service.getAccount(null); // falling back on the default - assertThat(account, notNullValue()); - assertThat(account.name, equalTo("account1")); - } - - public void testMultipleAccounts_NoExplicitDefault() throws Exception { - Settings.Builder builder = Settings.builder() - .put("xpack.notification.pagerduty.default_account", "account1"); - addAccountSettings("account1", builder); - addAccountSettings("account2", builder); - - PagerDutyService service = new PagerDutyService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, - Collections.singleton(PagerDutyService.PAGERDUTY_ACCOUNT_SETTING))); - PagerDutyAccount account = service.getAccount("account1"); - assertThat(account, notNullValue()); - assertThat(account.name, equalTo("account1")); - account = service.getAccount("account2"); - assertThat(account, notNullValue()); - assertThat(account.name, equalTo("account2")); - account = service.getAccount(null); - assertThat(account, notNullValue()); - assertThat(account.name, isOneOf("account1", "account2")); - } - - public void testMultipleAccounts_UnknownDefault() throws Exception { - expectThrows(SettingsException.class, () -> { - Settings.Builder builder = Settings.builder() - .put("xpack.notification.pagerduty.default_account", "unknown"); - addAccountSettings("account1", builder); - addAccountSettings("account2", builder); - new PagerDutyService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, - Collections.singleton(PagerDutyService.PAGERDUTY_ACCOUNT_SETTING))); - }); - } - - public void testNoAccount() throws Exception { - expectThrows(IllegalArgumentException.class, () -> { - Settings.Builder builder = Settings.builder(); - PagerDutyService service = new PagerDutyService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, - Collections.singleton(PagerDutyService.PAGERDUTY_ACCOUNT_SETTING))); - service.getAccount(null); - }); - } - - public void testNoAccount_WithDefaultAccount() throws Exception { - try { - Settings.Builder builder = Settings.builder() - .put("xpack.notification.pagerduty.default_account", "unknown"); - new PagerDutyService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, - Collections.singleton(PagerDutyService.PAGERDUTY_ACCOUNT_SETTING))); - fail("Expected a SettingsException to happen"); - } catch (SettingsException e) {} - } - public void testProxy() throws Exception { Settings.Builder builder = Settings.builder().put("xpack.notification.pagerduty.default_account", "account1"); addAccountSettings("account1", builder); @@ -150,6 +54,31 @@ public class PagerDutyAccountsTests extends ESTestCase { assertThat(request.proxy(), is(proxy)); } + // in earlier versions of the PD action the wrong JSON was sent, because the contexts field was named context + // the pagerduty API accepts any JSON, thus this was never caught + public void testContextIsSentCorrect() throws Exception { + Settings.Builder builder = Settings.builder().put("xpack.notification.pagerduty.default_account", "account1"); + addAccountSettings("account1", builder); + PagerDutyService service = new PagerDutyService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, + Collections.singleton(PagerDutyService.PAGERDUTY_ACCOUNT_SETTING))); + PagerDutyAccount account = service.getAccount("account1"); + + ArgumentCaptor argumentCaptor = ArgumentCaptor.forClass(HttpRequest.class); + when(httpClient.execute(argumentCaptor.capture())).thenReturn(new HttpResponse(200)); + + IncidentEventContext[] contexts = { + IncidentEventContext.link("https://www.elastic.co/products/x-pack/alerting", "Go to the Elastic.co Alerting website"), + IncidentEventContext.image("https://www.elastic.co/assets/blte5d899fd0b0e6808/icon-alerting-bb.svg", + "https://www.elastic.co/products/x-pack/alerting", "X-Pack-Alerting website link with log") + }; + IncidentEvent event = new IncidentEvent("foo", null, null, null, null, account.getName(), true, contexts, HttpProxy.NO_PROXY); + account.send(event, Payload.EMPTY); + + HttpRequest request = argumentCaptor.getValue(); + ObjectPath source = ObjectPath.createFromXContent(JsonXContent.jsonXContent, new BytesArray(request.body())); + assertThat(source.evaluate("contexts"), notNullValue()); + } + private void addAccountSettings(String name, Settings.Builder builder) { builder.put("xpack.notification.pagerduty.account." + name + ".service_api_key", randomAlphaOfLength(50)); Settings defaults = SlackMessageDefaultsTests.randomSettings(); diff --git a/plugin/src/test/java/org/elasticsearch/xpack/watcher/test/integration/PagerDutyServiceTests.java b/plugin/src/test/java/org/elasticsearch/xpack/watcher/test/integration/PagerDutyServiceTests.java index 0998f578a4f..d0275a94c79 100644 --- a/plugin/src/test/java/org/elasticsearch/xpack/watcher/test/integration/PagerDutyServiceTests.java +++ b/plugin/src/test/java/org/elasticsearch/xpack/watcher/test/integration/PagerDutyServiceTests.java @@ -48,8 +48,9 @@ public class PagerDutyServiceTests extends AbstractWatcherIntegrationTestCase { IncidentEvent event = new IncidentEvent("#testIncidentEvent()", null, null, "PagerDutyServiceTests", "_client_url", "_account", true, new IncidentEventContext[] { - IncidentEventContext.link("_href", "_text"), - IncidentEventContext.image("_src", "_href", "_alt") + IncidentEventContext.link("https://www.elastic.co/products/x-pack/alerting", "Go to the Elastic.co Alerting website"), + IncidentEventContext.image("https://www.elastic.co/assets/blte5d899fd0b0e6808/icon-alerting-bb.svg", + "https://www.elastic.co/products/x-pack/alerting", "X-Pack-Alerting website link with log") }, null); Payload payload = new Payload.Simple("_key", "_val");