From f11b4cdf6340617a2375ee67c807330a1410809c Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Fri, 30 Nov 2018 00:36:55 -0800 Subject: [PATCH 1/2] remove AbstractResourceFilter.isApplicable because it is not, add tests for OverlordResource.doShutdown and OverlordResource.shutdownTasksForDatasource --- .../basic/BasicSecurityResourceFilter.java | 18 ----- .../security/SupervisorResourceFilter.java | 15 ---- .../http/security/TaskResourceFilter.java | 14 ---- .../overlord/http/OverlordResourceTest.java | 75 ++++++++++++++++++- .../OverlordSecurityResourceFilterTest.java | 5 -- .../http/security/AbstractResourceFilter.java | 2 - .../http/security/ConfigResourceFilter.java | 9 --- .../security/DatasourceResourceFilter.java | 19 ----- .../http/security/RulesResourceFilter.java | 14 ---- .../http/security/StateResourceFilter.java | 15 ---- .../security/SecurityResourceFilterTest.java | 11 --- 11 files changed, 74 insertions(+), 123 deletions(-) diff --git a/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/BasicSecurityResourceFilter.java b/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/BasicSecurityResourceFilter.java index abb904c25647..8294ec1c6bd7 100644 --- a/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/BasicSecurityResourceFilter.java +++ b/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/BasicSecurityResourceFilter.java @@ -19,7 +19,6 @@ package org.apache.druid.security.basic; -import com.google.common.collect.ImmutableList; import com.google.inject.Inject; import com.sun.jersey.spi.container.ContainerRequest; import org.apache.druid.java.util.common.StringUtils; @@ -33,15 +32,9 @@ import javax.ws.rs.WebApplicationException; import javax.ws.rs.core.Response; -import java.util.List; public class BasicSecurityResourceFilter extends AbstractResourceFilter { - private static final List APPLICABLE_PATHS = ImmutableList.of( - "/druid-ext/basic-security/authentication", - "/druid-ext/basic-security/authorization" - ); - private static final String SECURITY_RESOURCE_NAME = "security"; @Inject @@ -76,15 +69,4 @@ public ContainerRequest filter(ContainerRequest request) return request; } - - @Override - public boolean isApplicable(String requestPath) - { - for (String path : APPLICABLE_PATHS) { - if (requestPath.startsWith(path) && !requestPath.equals(path)) { - return true; - } - } - return false; - } } diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/security/SupervisorResourceFilter.java b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/security/SupervisorResourceFilter.java index 5179a89cd1c1..e834c2e875e5 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/security/SupervisorResourceFilter.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/security/SupervisorResourceFilter.java @@ -23,7 +23,6 @@ import com.google.common.base.Optional; import com.google.common.base.Preconditions; import com.google.common.base.Predicate; -import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.inject.Inject; import com.sun.jersey.spi.container.ContainerRequest; @@ -41,7 +40,6 @@ import javax.ws.rs.WebApplicationException; import javax.ws.rs.core.PathSegment; import javax.ws.rs.core.Response; -import java.util.List; public class SupervisorResourceFilter extends AbstractResourceFilter { @@ -109,17 +107,4 @@ public boolean apply(PathSegment input) return request; } - - @Override - public boolean isApplicable(String requestPath) - { - List applicablePaths = ImmutableList.of("druid/indexer/v1/supervisor/"); - for (String path : applicablePaths) { - if (requestPath.startsWith(path) && !requestPath.equals(path)) { - return true; - } - } - return false; - } - } diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/security/TaskResourceFilter.java b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/security/TaskResourceFilter.java index 26c927f02fc0..8109961558c3 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/security/TaskResourceFilter.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/security/TaskResourceFilter.java @@ -22,7 +22,6 @@ import com.google.common.base.Optional; import com.google.common.base.Preconditions; import com.google.common.base.Predicate; -import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.inject.Inject; import com.sun.jersey.spi.container.ContainerRequest; @@ -41,7 +40,6 @@ import javax.ws.rs.WebApplicationException; import javax.ws.rs.core.PathSegment; import javax.ws.rs.core.Response; -import java.util.List; /** * Use this ResourceFilter when the datasource information is present after "task" segment in the request Path @@ -110,16 +108,4 @@ public boolean apply(PathSegment input) return request; } - - @Override - public boolean isApplicable(String requestPath) - { - List applicablePaths = ImmutableList.of("druid/indexer/v1/task/"); - for (String path : applicablePaths) { - if (requestPath.startsWith(path) && !requestPath.equals(path)) { - return true; - } - } - return false; - } } diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordResourceTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordResourceTest.java index 52955f09c259..0dd9c2348d42 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordResourceTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordResourceTest.java @@ -36,6 +36,7 @@ import org.apache.druid.indexing.common.task.Task; import org.apache.druid.indexing.overlord.IndexerMetadataStorageAdapter; import org.apache.druid.indexing.overlord.TaskMaster; +import org.apache.druid.indexing.overlord.TaskQueue; import org.apache.druid.indexing.overlord.TaskRunner; import org.apache.druid.indexing.overlord.TaskRunnerWorkItem; import org.apache.druid.indexing.overlord.TaskStorageQueryAdapter; @@ -132,7 +133,7 @@ private void expectAuthorizationTokenCheck() EasyMock.expect(req.getAttribute(AuthConfig.DRUID_AUTHORIZATION_CHECKED)).andReturn(null).atLeastOnce(); EasyMock.expect(req.getAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT)) .andReturn(authenticationResult) - .anyTimes(); + .atLeastOnce(); req.setAttribute(AuthConfig.DRUID_AUTHORIZATION_CHECKED, false); EasyMock.expectLastCall().anyTimes(); @@ -916,6 +917,78 @@ public void testGetTaskStatus() throws Exception Assert.assertEquals(new TaskStatusResponse("othertask", null), taskStatusResponse2); } + + @Test + public void testShutdownTask() + { + // This is disabled since OverlordResource.doShutdown is annotated with TaskResourceFilter + // This should be fixed in https://github.com/apache/incubator-druid/issues/6685. + // expectAuthorizationTokenCheck(); + TaskQueue mockQueue = EasyMock.createMock(TaskQueue.class); + EasyMock.expect(taskMaster.isLeader()).andReturn(true).anyTimes(); + EasyMock.expect(taskMaster.getTaskRunner()).andReturn( + Optional.of(taskRunner) + ).anyTimes(); + EasyMock.expect(taskMaster.getTaskQueue()).andReturn( + Optional.of(mockQueue) + ).anyTimes(); + mockQueue.shutdown("id_1", "Shutdown request from user"); + EasyMock.expectLastCall(); + + EasyMock.replay(taskRunner, taskMaster, taskStorageQueryAdapter, indexerMetadataStorageAdapter, req, mockQueue); + + final Map response = (Map) overlordResource + .doShutdown("id_1") + .getEntity(); + Assert.assertEquals("id_1", response.get("task")); + } + + + @Test + public void testShutdownAllTasks() + { + // This is disabled since OverlordResource.shutdownTasksForDataSource is annotated with DatasourceResourceFilter + // This should be fixed in https://github.com/apache/incubator-druid/issues/6685. + // expectAuthorizationTokenCheck(); + TaskQueue mockQueue = EasyMock.createMock(TaskQueue.class); + EasyMock.expect(taskMaster.isLeader()).andReturn(true).anyTimes(); + EasyMock.expect(taskMaster.getTaskRunner()).andReturn( + Optional.of(taskRunner) + ).anyTimes(); + EasyMock.expect(taskMaster.getTaskQueue()).andReturn( + Optional.of(mockQueue) + ).anyTimes(); + EasyMock.expect(taskStorageQueryAdapter.getActiveTaskInfo("datasource")).andStubReturn(ImmutableList.of( + new TaskInfo( + "id_1", + DateTime.now(ISOChronology.getInstanceUTC()), + TaskStatus.success("id_1"), + "datasource", + getTaskWithIdAndDatasource("id_1", "datasource") + ), + new TaskInfo( + "id_2", + DateTime.now(ISOChronology.getInstanceUTC()), + TaskStatus.success("id_2"), + "datasource", + getTaskWithIdAndDatasource("id_2", "datasource") + ) + )); + mockQueue.shutdown("id_1", "Shutdown request from user"); + EasyMock.expectLastCall(); + mockQueue.shutdown("id_2", "Shutdown request from user"); + EasyMock.expectLastCall(); + + EasyMock.replay(taskRunner, taskMaster, taskStorageQueryAdapter, indexerMetadataStorageAdapter, req, mockQueue); + + final Map response = (Map) overlordResource + .shutdownTasksForDataSource("datasource") + .getEntity(); + Assert.assertEquals("datasource", response.get("dataSource")); + } + + + @After public void tearDown() { diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/security/OverlordSecurityResourceFilterTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/security/OverlordSecurityResourceFilterTest.java index 27dd6a38fe7a..16c8afea1851 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/security/OverlordSecurityResourceFilterTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/security/OverlordSecurityResourceFilterTest.java @@ -33,13 +33,11 @@ import org.apache.druid.indexing.overlord.supervisor.SupervisorResource; import org.apache.druid.indexing.overlord.supervisor.SupervisorSpec; import org.apache.druid.indexing.worker.http.WorkerResource; -import org.apache.druid.server.http.security.AbstractResourceFilter; import org.apache.druid.server.http.security.ResourceFilterTestHelper; import org.apache.druid.server.security.AuthorizerMapper; import org.apache.druid.server.security.ForbiddenException; import org.easymock.EasyMock; import org.junit.After; -import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -171,7 +169,6 @@ public void testResourcesFilteringAccess() EasyMock.expect(request.getMethod()).andReturn(requestMethod).anyTimes(); EasyMock.replay(req, request, authorizerMapper); resourceFilter.getRequestFilter().filter(request); - Assert.assertTrue(((AbstractResourceFilter) resourceFilter.getRequestFilter()).isApplicable(requestPath)); } @Test(expected = ForbiddenException.class) @@ -180,7 +177,6 @@ public void testDatasourcesResourcesFilteringNoAccess() setUpMockExpectations(requestPath, false, requestMethod); EasyMock.expect(request.getEntity(Task.class)).andReturn(noopTask).anyTimes(); EasyMock.replay(req, request, authorizerMapper); - Assert.assertTrue(((AbstractResourceFilter) resourceFilter.getRequestFilter()).isApplicable(requestPath)); try { resourceFilter.getRequestFilter().filter(request); } @@ -195,7 +191,6 @@ public void testDatasourcesResourcesFilteringBadPath() final String badRequestPath = WORD.matcher(requestPath).replaceAll("droid"); EasyMock.expect(request.getPath()).andReturn(badRequestPath).anyTimes(); EasyMock.replay(req, request, authorizerMapper); - Assert.assertFalse(((AbstractResourceFilter) resourceFilter.getRequestFilter()).isApplicable(badRequestPath)); } @After diff --git a/server/src/main/java/org/apache/druid/server/http/security/AbstractResourceFilter.java b/server/src/main/java/org/apache/druid/server/http/security/AbstractResourceFilter.java index 1f2a245dd196..4119e0b9b747 100644 --- a/server/src/main/java/org/apache/druid/server/http/security/AbstractResourceFilter.java +++ b/server/src/main/java/org/apache/druid/server/http/security/AbstractResourceFilter.java @@ -92,6 +92,4 @@ protected Action getAction(ContainerRequest request) } return action; } - - public abstract boolean isApplicable(String requestPath); } diff --git a/server/src/main/java/org/apache/druid/server/http/security/ConfigResourceFilter.java b/server/src/main/java/org/apache/druid/server/http/security/ConfigResourceFilter.java index 9ebe98d4c7a1..7a45ca1d5bbb 100644 --- a/server/src/main/java/org/apache/druid/server/http/security/ConfigResourceFilter.java +++ b/server/src/main/java/org/apache/druid/server/http/security/ConfigResourceFilter.java @@ -68,13 +68,4 @@ public ContainerRequest filter(ContainerRequest request) return request; } - - @Override - public boolean isApplicable(String requestPath) - { - return requestPath.startsWith("druid/worker/v1") || - requestPath.startsWith("druid/indexer/v1") || - requestPath.startsWith("status/properties") || - requestPath.startsWith("druid/coordinator/v1/config"); - } } diff --git a/server/src/main/java/org/apache/druid/server/http/security/DatasourceResourceFilter.java b/server/src/main/java/org/apache/druid/server/http/security/DatasourceResourceFilter.java index c46277b18f79..e32446cfe5cb 100644 --- a/server/src/main/java/org/apache/druid/server/http/security/DatasourceResourceFilter.java +++ b/server/src/main/java/org/apache/druid/server/http/security/DatasourceResourceFilter.java @@ -21,7 +21,6 @@ import com.google.common.base.Preconditions; import com.google.common.base.Predicate; -import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.inject.Inject; import com.sun.jersey.spi.container.ContainerRequest; @@ -34,7 +33,6 @@ import org.apache.druid.server.security.ResourceType; import javax.ws.rs.core.PathSegment; -import java.util.List; /** * Use this ResourceFilter when the datasource information is present after "datasources" segment in the request Path @@ -93,21 +91,4 @@ public boolean apply(PathSegment input) Preconditions.checkNotNull(dataSourceName); return dataSourceName; } - - @Override - public boolean isApplicable(String requestPath) - { - List applicablePaths = ImmutableList.of( - "druid/coordinator/v1/datasources/", - "druid/coordinator/v1/metadata/datasources/", - "druid/v2/datasources/", - "druid/indexer/v1/datasources" - ); - for (String path : applicablePaths) { - if (requestPath.startsWith(path) && !requestPath.equals(path)) { - return true; - } - } - return false; - } } diff --git a/server/src/main/java/org/apache/druid/server/http/security/RulesResourceFilter.java b/server/src/main/java/org/apache/druid/server/http/security/RulesResourceFilter.java index d59aa20de11c..f314c77d7431 100644 --- a/server/src/main/java/org/apache/druid/server/http/security/RulesResourceFilter.java +++ b/server/src/main/java/org/apache/druid/server/http/security/RulesResourceFilter.java @@ -21,7 +21,6 @@ import com.google.common.base.Preconditions; import com.google.common.base.Predicate; -import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.inject.Inject; import com.sun.jersey.spi.container.ContainerRequest; @@ -34,7 +33,6 @@ import org.apache.druid.server.security.ResourceType; import javax.ws.rs.core.PathSegment; -import java.util.List; /** @@ -89,16 +87,4 @@ public boolean apply(PathSegment input) return request; } - - @Override - public boolean isApplicable(String requestPath) - { - List applicablePaths = ImmutableList.of("druid/coordinator/v1/rules/"); - for (String path : applicablePaths) { - if (requestPath.startsWith(path) && !requestPath.equals(path)) { - return true; - } - } - return false; - } } diff --git a/server/src/main/java/org/apache/druid/server/http/security/StateResourceFilter.java b/server/src/main/java/org/apache/druid/server/http/security/StateResourceFilter.java index cd97b5289ae3..b3231dc2b373 100644 --- a/server/src/main/java/org/apache/druid/server/http/security/StateResourceFilter.java +++ b/server/src/main/java/org/apache/druid/server/http/security/StateResourceFilter.java @@ -74,19 +74,4 @@ public ContainerRequest filter(ContainerRequest request) return request; } - - @Override - public boolean isApplicable(String requestPath) - { - return requestPath.startsWith("druid/broker/v1") || - requestPath.startsWith("druid/coordinator/v1") || - requestPath.startsWith("druid/historical/v1") || - requestPath.startsWith("druid/indexer/v1") || - requestPath.startsWith("druid/coordinator/v1/rules") || - requestPath.startsWith("druid/coordinator/v1/tiers") || - requestPath.startsWith("druid/worker/v1") || - requestPath.startsWith("druid/coordinator/v1/servers") || - requestPath.startsWith("druid/v2") || - requestPath.startsWith("status"); - } } diff --git a/server/src/test/java/org/apache/druid/server/http/security/SecurityResourceFilterTest.java b/server/src/test/java/org/apache/druid/server/http/security/SecurityResourceFilterTest.java index a4c4156560da..0d3be7fce0bd 100644 --- a/server/src/test/java/org/apache/druid/server/http/security/SecurityResourceFilterTest.java +++ b/server/src/test/java/org/apache/druid/server/http/security/SecurityResourceFilterTest.java @@ -105,7 +105,6 @@ public void testResourcesFilteringAccess() { setUpMockExpectations(requestPath, true, requestMethod); EasyMock.replay(req, request, authorizerMapper); - Assert.assertTrue(((AbstractResourceFilter) resourceFilter.getRequestFilter()).isApplicable(requestPath)); resourceFilter.getRequestFilter().filter(request); EasyMock.verify(req, request, authorizerMapper); } @@ -115,7 +114,6 @@ public void testResourcesFilteringNoAccess() { setUpMockExpectations(requestPath, false, requestMethod); EasyMock.replay(req, request, authorizerMapper); - Assert.assertTrue(((AbstractResourceFilter) resourceFilter.getRequestFilter()).isApplicable(requestPath)); try { resourceFilter.getRequestFilter().filter(request); Assert.fail(); @@ -125,13 +123,4 @@ public void testResourcesFilteringNoAccess() } EasyMock.verify(req, request, authorizerMapper); } - - @Test - public void testResourcesFilteringBadPath() - { - EasyMock.replay(req, request, authorizerMapper); - final String badRequestPath = WORD.matcher(requestPath).replaceAll("droid"); - Assert.assertFalse(((AbstractResourceFilter) resourceFilter.getRequestFilter()).isApplicable(badRequestPath)); - EasyMock.verify(req, request, authorizerMapper); - } } From d86baf484ec4f6d513ea1e655ce227cc5024216c Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Fri, 30 Nov 2018 12:00:05 -0800 Subject: [PATCH 2/2] cleanup --- .../overlord/http/OverlordResourceTest.java | 35 ++++++++----------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordResourceTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordResourceTest.java index 0dd9c2348d42..4558de2863ef 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordResourceTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordResourceTest.java @@ -126,20 +126,10 @@ public Access authorize(AuthenticationResult authenticationResult, Resource reso ); } - private void expectAuthorizationTokenCheck() + @After + public void tearDown() { - AuthenticationResult authenticationResult = new AuthenticationResult("druid", "druid", null, null); - EasyMock.expect(req.getAttribute(AuthConfig.DRUID_ALLOW_UNSECURED_PATH)).andReturn(null).anyTimes(); - EasyMock.expect(req.getAttribute(AuthConfig.DRUID_AUTHORIZATION_CHECKED)).andReturn(null).atLeastOnce(); - EasyMock.expect(req.getAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT)) - .andReturn(authenticationResult) - .atLeastOnce(); - - req.setAttribute(AuthConfig.DRUID_AUTHORIZATION_CHECKED, false); - EasyMock.expectLastCall().anyTimes(); - - req.setAttribute(AuthConfig.DRUID_AUTHORIZATION_CHECKED, true); - EasyMock.expectLastCall().anyTimes(); + EasyMock.verify(taskRunner, taskMaster, taskStorageQueryAdapter, indexerMetadataStorageAdapter, req); } @Test @@ -917,7 +907,6 @@ public void testGetTaskStatus() throws Exception Assert.assertEquals(new TaskStatusResponse("othertask", null), taskStatusResponse2); } - @Test public void testShutdownTask() { @@ -943,7 +932,6 @@ public void testShutdownTask() Assert.assertEquals("id_1", response.get("task")); } - @Test public void testShutdownAllTasks() { @@ -987,12 +975,20 @@ public void testShutdownAllTasks() Assert.assertEquals("datasource", response.get("dataSource")); } + private void expectAuthorizationTokenCheck() + { + AuthenticationResult authenticationResult = new AuthenticationResult("druid", "druid", null, null); + EasyMock.expect(req.getAttribute(AuthConfig.DRUID_ALLOW_UNSECURED_PATH)).andReturn(null).anyTimes(); + EasyMock.expect(req.getAttribute(AuthConfig.DRUID_AUTHORIZATION_CHECKED)).andReturn(null).atLeastOnce(); + EasyMock.expect(req.getAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT)) + .andReturn(authenticationResult) + .atLeastOnce(); + req.setAttribute(AuthConfig.DRUID_AUTHORIZATION_CHECKED, false); + EasyMock.expectLastCall().anyTimes(); - @After - public void tearDown() - { - EasyMock.verify(taskRunner, taskMaster, taskStorageQueryAdapter, indexerMetadataStorageAdapter, req); + req.setAttribute(AuthConfig.DRUID_AUTHORIZATION_CHECKED, true); + EasyMock.expectLastCall().anyTimes(); } private Task getTaskWithIdAndDatasource(String id, String datasource) @@ -1048,5 +1044,4 @@ public String getDataSource() } } - }