From 18b602280c0e865b06b428c481e3955b5f00e4c3 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Thu, 2 Apr 2020 08:55:56 -0400 Subject: [PATCH] Add validation to the usage service (#54617) Today the usage service can let in some issues, such as handlers that do not have a name, where the errors do not manifest until later (calling the usage API), or conflicting handlers with the same name. This commit addresses this by adding some validation to the usage service. --- .../rest/action/document/RestIndexAction.java | 2 +- .../org/elasticsearch/usage/UsageService.java | 31 ++++++++--- .../action/ActionModuleTests.java | 9 +++- .../usage/UsageServiceTests.java | 53 +++++++++++++++++++ 4 files changed, 87 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/rest/action/document/RestIndexAction.java b/server/src/main/java/org/elasticsearch/rest/action/document/RestIndexAction.java index bfd9e1f2317..3bc9054c11d 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/document/RestIndexAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/document/RestIndexAction.java @@ -103,7 +103,7 @@ public class RestIndexAction extends BaseRestHandler { @Override public String getName() { - return "document_create_action"; + return "document_create_action_auto_id"; } @Override diff --git a/server/src/main/java/org/elasticsearch/usage/UsageService.java b/server/src/main/java/org/elasticsearch/usage/UsageService.java index 9e1c2e03734..df1f53bcbb7 100644 --- a/server/src/main/java/org/elasticsearch/usage/UsageService.java +++ b/server/src/main/java/org/elasticsearch/usage/UsageService.java @@ -42,21 +42,21 @@ import org.elasticsearch.action.admin.cluster.node.usage.NodeUsage; import org.elasticsearch.cluster.node.DiscoveryNode; import org.elasticsearch.rest.BaseRestHandler; -import java.util.ArrayList; import java.util.HashMap; -import java.util.List; +import java.util.Locale; import java.util.Map; +import java.util.Objects; /** * A service to monitor usage of Elasticsearch features. */ public class UsageService { - private final List handlers; + private final Map handlers; private final long sinceTime; public UsageService() { - this.handlers = new ArrayList<>(); + this.handlers = new HashMap<>(); this.sinceTime = System.currentTimeMillis(); } @@ -67,7 +67,26 @@ public class UsageService { * the {@link BaseRestHandler} to add to the usage service. */ public void addRestHandler(BaseRestHandler handler) { - handlers.add(handler); + Objects.requireNonNull(handler); + if (handler.getName() == null) { + throw new IllegalArgumentException("handler of type [" + handler.getClass().getName() + "] does not have a name"); + } + final BaseRestHandler maybeHandler = handlers.put(handler.getName(), handler); + /* + * Handlers will be registered multiple times, once for each route that the handler handles. This means that we will see handlers + * multiple times, so we do not have a conflict if we are seeing the same instance multiple times. So, we only reject if a handler + * with the same name was registered before, and it is not the same instance as before. + */ + if (maybeHandler != null && maybeHandler != handler) { + final String message = String.format( + Locale.ROOT, + "handler of type [%s] conflicts with handler of type [%s] as they both have the same name [%s]", + handler.getClass().getName(), + maybeHandler.getClass().getName(), + handler.getName() + ); + throw new IllegalArgumentException(message); + } } /** @@ -85,7 +104,7 @@ public class UsageService { Map restUsageMap; if (restActions) { restUsageMap = new HashMap<>(); - handlers.forEach(handler -> { + handlers.values().forEach(handler -> { long usageCount = handler.getUsageCount(); if (usageCount > 0) { restUsageMap.put(handler.getName(), usageCount); diff --git a/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java b/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java index d5299410e83..3ec2c0b275c 100644 --- a/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java +++ b/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java @@ -132,7 +132,14 @@ public class ActionModuleTests extends ESTestCase { public List getRestHandlers(Settings settings, RestController restController, ClusterSettings clusterSettings, IndexScopedSettings indexScopedSettings, SettingsFilter settingsFilter, IndexNameExpressionResolver indexNameExpressionResolver, Supplier nodesInCluster) { - return singletonList(new RestMainAction()); + return singletonList(new RestMainAction() { + + @Override + public String getName() { + return "duplicated_" + super.getName(); + } + + }); } }; SettingsModule settings = new SettingsModule(Settings.EMPTY); diff --git a/server/src/test/java/org/elasticsearch/usage/UsageServiceTests.java b/server/src/test/java/org/elasticsearch/usage/UsageServiceTests.java index cfba6390bfb..7c9ac5568aa 100644 --- a/server/src/test/java/org/elasticsearch/usage/UsageServiceTests.java +++ b/server/src/test/java/org/elasticsearch/usage/UsageServiceTests.java @@ -32,6 +32,7 @@ import org.elasticsearch.test.rest.FakeRestRequest; import java.net.InetAddress; import java.util.Collections; import java.util.List; +import java.util.Locale; import java.util.Map; import static org.hamcrest.Matchers.equalTo; @@ -41,6 +42,58 @@ import static org.hamcrest.Matchers.sameInstance; public class UsageServiceTests extends ESTestCase { + /** + * Test that we can not add a null reference to a {@link org.elasticsearch.rest.RestHandler} to the {@link UsageService}. + */ + public void testHandlerCanNotBeNull() { + final UsageService service = new UsageService(); + expectThrows(NullPointerException.class, () -> service.addRestHandler(null)); + } + + /** + * Test that we can not add an instance of a {@link org.elasticsearch.rest.RestHandler} with no name to the {@link UsageService}. + */ + public void testAHandlerWithNoName() { + final UsageService service = new UsageService(); + final BaseRestHandler horse = new MockRestHandler(null); + final IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> service.addRestHandler(horse)); + assertThat( + e.getMessage(), + equalTo("handler of type [org.elasticsearch.usage.UsageServiceTests$MockRestHandler] does not have a name")); + } + + /** + * Test that we can add the same instance of a {@link org.elasticsearch.rest.RestHandler} to the {@link UsageService} multiple times. + */ + public void testHandlerWithConflictingNamesButSameInstance() { + final UsageService service = new UsageService(); + final String name = randomAlphaOfLength(8); + final BaseRestHandler first = new MockRestHandler(name); + service.addRestHandler(first); + // nothing bad ever happens to me + service.addRestHandler(first); + } + + /** + * Test that we can not add different instances of {@link org.elasticsearch.rest.RestHandler} with the same name to the + * {@link UsageService}. + */ + public void testHandlersWithConflictingNamesButDifferentInstances() { + final UsageService service = new UsageService(); + final String name = randomAlphaOfLength(8); + final BaseRestHandler first = new MockRestHandler(name); + final BaseRestHandler second = new MockRestHandler(name); + service.addRestHandler(first); + final IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> service.addRestHandler(second)); + final String expected = String.format( + Locale.ROOT, + "handler of type [%s] conflicts with handler of type [%1$s] as they both have the same name [%s]", + "org.elasticsearch.usage.UsageServiceTests$MockRestHandler", + name + ); + assertThat(e.getMessage(), equalTo(expected)); + } + public void testRestUsage() throws Exception { DiscoveryNode discoveryNode = new DiscoveryNode("foo", new TransportAddress(InetAddress.getByName("localhost"), 12345), Version.CURRENT);