From 2a6efb49b0338318e70ff9aa53e227070fa342de Mon Sep 17 00:00:00 2001 From: jon-wei Date: Fri, 16 Mar 2018 18:02:04 -0700 Subject: [PATCH] Fix supervisor tombstone auth handling --- .../supervisor/SupervisorManager.java | 5 +- .../supervisor/SupervisorResource.java | 3 +- .../supervisor/SupervisorManagerTest.java | 3 +- .../supervisor/SupervisorResourceTest.java | 181 ++++++++++++++++++ .../supervisor/NoopSupervisorSpec.java | 52 ++++- 5 files changed, 238 insertions(+), 6 deletions(-) diff --git a/indexing-service/src/main/java/io/druid/indexing/overlord/supervisor/SupervisorManager.java b/indexing-service/src/main/java/io/druid/indexing/overlord/supervisor/SupervisorManager.java index ccaf0d5a19cb..870c01d7a93f 100644 --- a/indexing-service/src/main/java/io/druid/indexing/overlord/supervisor/SupervisorManager.java +++ b/indexing-service/src/main/java/io/druid/indexing/overlord/supervisor/SupervisorManager.java @@ -71,6 +71,7 @@ public boolean createOrUpdateAndStartSupervisor(SupervisorSpec spec) Preconditions.checkState(started, "SupervisorManager not started"); Preconditions.checkNotNull(spec, "spec"); Preconditions.checkNotNull(spec.getId(), "spec.getId()"); + Preconditions.checkNotNull(spec.getDataSources(), "spec.getDatasources()"); synchronized (lock) { Preconditions.checkState(started, "SupervisorManager not started"); @@ -197,7 +198,7 @@ private boolean possiblyStopAndRemoveSupervisorInternal(String id, boolean write } if (writeTombstone) { - metadataSupervisorManager.insert(id, new NoopSupervisorSpec()); // where NoopSupervisorSpec is a tombstone + metadataSupervisorManager.insert(id, new NoopSupervisorSpec(null, pair.rhs.getDataSources())); // where NoopSupervisorSpec is a tombstone } pair.lhs.stop(true); supervisors.remove(id); @@ -232,7 +233,7 @@ private boolean createAndStartSupervisorInternal(SupervisorSpec spec, boolean pe catch (Exception e) { // Supervisor creation or start failed write tombstone only when trying to start a new supervisor if (persistSpec) { - metadataSupervisorManager.insert(id, new NoopSupervisorSpec()); + metadataSupervisorManager.insert(id, new NoopSupervisorSpec(null, spec.getDataSources())); } throw Throwables.propagate(e); } diff --git a/indexing-service/src/main/java/io/druid/indexing/overlord/supervisor/SupervisorResource.java b/indexing-service/src/main/java/io/druid/indexing/overlord/supervisor/SupervisorResource.java index a258e8df2dca..7a7d535da41e 100644 --- a/indexing-service/src/main/java/io/druid/indexing/overlord/supervisor/SupervisorResource.java +++ b/indexing-service/src/main/java/io/druid/indexing/overlord/supervisor/SupervisorResource.java @@ -47,6 +47,7 @@ import javax.ws.rs.core.Context; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; +import java.util.ArrayList; import java.util.Collection; import java.util.List; import java.util.Map; @@ -65,7 +66,7 @@ public class SupervisorResource return null; } if (supervisorSpec.getSpec().getDataSources() == null) { - return null; + return new ArrayList<>(); } return Iterables.transform( supervisorSpec.getSpec().getDataSources(), diff --git a/indexing-service/src/test/java/io/druid/indexing/overlord/supervisor/SupervisorManagerTest.java b/indexing-service/src/test/java/io/druid/indexing/overlord/supervisor/SupervisorManagerTest.java index 0a428c3aa2f8..be75fb1e9cf4 100644 --- a/indexing-service/src/test/java/io/druid/indexing/overlord/supervisor/SupervisorManagerTest.java +++ b/indexing-service/src/test/java/io/druid/indexing/overlord/supervisor/SupervisorManagerTest.java @@ -35,6 +35,7 @@ import org.junit.rules.ExpectedException; import org.junit.runner.RunWith; +import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -288,7 +289,7 @@ public Supervisor createSupervisor() @Override public List getDataSources() { - return null; + return new ArrayList<>(); } } diff --git a/indexing-service/src/test/java/io/druid/indexing/overlord/supervisor/SupervisorResourceTest.java b/indexing-service/src/test/java/io/druid/indexing/overlord/supervisor/SupervisorResourceTest.java index 189953eaef65..b1947e3f202e 100644 --- a/indexing-service/src/test/java/io/druid/indexing/overlord/supervisor/SupervisorResourceTest.java +++ b/indexing-service/src/test/java/io/druid/indexing/overlord/supervisor/SupervisorResourceTest.java @@ -19,6 +19,7 @@ package io.druid.indexing.overlord.supervisor; +import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.base.Optional; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -287,6 +288,10 @@ public void testSpecGetAllHistory() new VersionedSupervisorSpec( new TestSupervisorSpec("id1", null, Arrays.asList("datasource1")), "v2" + ), + new VersionedSupervisorSpec( + new NoopSupervisorSpec(null, Arrays.asList("datasource1")), + "tombstone" ) ); List versions2 = ImmutableList.of( @@ -297,11 +302,42 @@ public void testSpecGetAllHistory() new VersionedSupervisorSpec( new TestSupervisorSpec("id2", null, Arrays.asList("datasource2")), "v2" + ), + new VersionedSupervisorSpec( + new NoopSupervisorSpec(null, Arrays.asList("datasource2")), + "tombstone" + ), + new VersionedSupervisorSpec( + new TestSupervisorSpec("id2", null, Arrays.asList("datasource2")), + "v3" + ) + ); + List versions3 = ImmutableList.of( + new VersionedSupervisorSpec( + new TestSupervisorSpec("id2", null, Arrays.asList("datasource3")), + "v1" + ), + new VersionedSupervisorSpec( + new NoopSupervisorSpec(null, null), + "tombstone" + ), + new VersionedSupervisorSpec( + new TestSupervisorSpec("id2", null, Arrays.asList("datasource3")), + "v2" + ), + new VersionedSupervisorSpec( + new NoopSupervisorSpec(null, null), + "tombstone" + ), + new VersionedSupervisorSpec( + new TestSupervisorSpec("id2", null, Arrays.asList("datasource3")), + "v3" ) ); Map> history = Maps.newHashMap(); history.put("id1", versions1); history.put("id2", versions2); + history.put("id3", versions3); EasyMock.expect(taskMaster.getSupervisorManager()).andReturn(Optional.of(supervisorManager)).times(2); EasyMock.expect(supervisorManager.getSupervisorHistory()).andReturn(history); @@ -344,6 +380,10 @@ public void testSpecGetAllHistoryWithAuthFailureFiltering() new VersionedSupervisorSpec( new TestSupervisorSpec("id1", null, Arrays.asList("datasource1")), "v2" + ), + new VersionedSupervisorSpec( + new NoopSupervisorSpec(null, Arrays.asList("datasource1")), + "tombstone" ) ); List versions2 = ImmutableList.of( @@ -354,12 +394,62 @@ public void testSpecGetAllHistoryWithAuthFailureFiltering() new VersionedSupervisorSpec( new TestSupervisorSpec("id2", null, Arrays.asList("datasource2")), "v2" + ), + new VersionedSupervisorSpec( + new NoopSupervisorSpec(null, Arrays.asList("datasource2")), + "tombstone" + ) + ); + List versions3 = ImmutableList.of( + new VersionedSupervisorSpec( + new TestSupervisorSpec("id2", null, Arrays.asList("datasource2")), + "v1" + ), + new VersionedSupervisorSpec( + new TestSupervisorSpec("id2", null, Arrays.asList("datasource2")), + "v2" + ), + new VersionedSupervisorSpec( + new NoopSupervisorSpec(null, Arrays.asList("datasource2")), + "tombstone" + ), + new VersionedSupervisorSpec( + new TestSupervisorSpec("id3", null, Arrays.asList("datasource3")), + "v1" + ), + new VersionedSupervisorSpec( + new NoopSupervisorSpec(null, Arrays.asList("datasource3")), + "tombstone" + ) + ); + List versions4 = ImmutableList.of( + new VersionedSupervisorSpec( + new TestSupervisorSpec("id2", null, Arrays.asList("datasource2")), + "v1" + ), + new VersionedSupervisorSpec( + new NoopSupervisorSpec(null, null), + "tombstone" + ), + new VersionedSupervisorSpec( + new TestSupervisorSpec("id2", null, Arrays.asList("datasource2")), + "v2" + ), + new VersionedSupervisorSpec( + new NoopSupervisorSpec(null, null), + "tombstone" + ), + new VersionedSupervisorSpec( + new TestSupervisorSpec("id2", null, Arrays.asList("datasource2")), + "v3" ) ); Map> history = Maps.newHashMap(); history.put("id1", versions1); history.put("id2", versions2); + history.put("id3", versions3); + history.put("id4", versions4); EasyMock.expect(taskMaster.getSupervisorManager()).andReturn(Optional.of(supervisorManager)).times(2); EasyMock.expect(supervisorManager.getSupervisorHistory()).andReturn(history); @@ -379,6 +469,32 @@ public void testSpecGetAllHistoryWithAuthFailureFiltering() Map> filteredHistory = Maps.newHashMap(); filteredHistory.put("id1", versions1); + filteredHistory.put( + "id3", + ImmutableList.of( + new VersionedSupervisorSpec( + new TestSupervisorSpec("id3", null, Arrays.asList("datasource3")), + "v1" + ), + new VersionedSupervisorSpec( + new NoopSupervisorSpec(null, Arrays.asList("datasource3")), + "tombstone" + ) + ) + ); + filteredHistory.put( + "id4", + ImmutableList.of( + new VersionedSupervisorSpec( + new NoopSupervisorSpec(null, null), + "tombstone" + ), + new VersionedSupervisorSpec( + new NoopSupervisorSpec(null, null), + "tombstone" + ) + ) + ); Assert.assertEquals(200, response.getStatus()); Assert.assertEquals(filteredHistory, response.getEntity()); @@ -402,6 +518,10 @@ public void testSpecGetHistory() new TestSupervisorSpec("id1", null, Arrays.asList("datasource1")), "v1" ), + new VersionedSupervisorSpec( + new NoopSupervisorSpec(null, Arrays.asList("datasource1")), + "tombstone" + ), new VersionedSupervisorSpec( new TestSupervisorSpec("id1", null, Arrays.asList("datasource1")), "v2" @@ -412,6 +532,10 @@ public void testSpecGetHistory() new TestSupervisorSpec("id2", null, Arrays.asList("datasource2")), "v1" ), + new VersionedSupervisorSpec( + new NoopSupervisorSpec(null, Arrays.asList("datasource2")), + "tombstone" + ), new VersionedSupervisorSpec( new TestSupervisorSpec("id2", null, Arrays.asList("datasource2")), "v2" @@ -464,6 +588,10 @@ public void testSpecGetHistoryWithAuthFailure() throws Exception new TestSupervisorSpec("id1", null, Arrays.asList("datasource1")), "v1" ), + new VersionedSupervisorSpec( + new NoopSupervisorSpec(null, Arrays.asList("datasource3")), + "tombstone" + ), new VersionedSupervisorSpec( new TestSupervisorSpec("id1", null, Arrays.asList("datasource1")), "v2" @@ -474,6 +602,10 @@ public void testSpecGetHistoryWithAuthFailure() throws Exception new TestSupervisorSpec("id2", null, Arrays.asList("datasource2")), "v1" ), + new VersionedSupervisorSpec( + new NoopSupervisorSpec(null, Arrays.asList("datasource2")), + "tombstone" + ), new VersionedSupervisorSpec( new TestSupervisorSpec("id2", null, Arrays.asList("datasource2")), "v2" @@ -484,9 +616,25 @@ public void testSpecGetHistoryWithAuthFailure() throws Exception new TestSupervisorSpec("id3", null, Arrays.asList("datasource3")), "v1" ), + new VersionedSupervisorSpec( + new NoopSupervisorSpec(null, null), + "tombstone" + ), new VersionedSupervisorSpec( new TestSupervisorSpec("id3", null, Arrays.asList("datasource2")), "v2" + ), + new VersionedSupervisorSpec( + new NoopSupervisorSpec(null, null), + "tombstone" + ), + new VersionedSupervisorSpec( + new TestSupervisorSpec("id3", null, Arrays.asList("datasource3")), + "v2" + ), + new VersionedSupervisorSpec( + new NoopSupervisorSpec(null, Arrays.asList("datasource3")), + "tombstone" ) ); Map> history = Maps.newHashMap(); @@ -521,6 +669,22 @@ public void testSpecGetHistoryWithAuthFailure() throws Exception new VersionedSupervisorSpec( new TestSupervisorSpec("id3", null, Arrays.asList("datasource3")), "v1" + ), + new VersionedSupervisorSpec( + new NoopSupervisorSpec(null, null), + "tombstone" + ), + new VersionedSupervisorSpec( + new NoopSupervisorSpec(null, null), + "tombstone" + ), + new VersionedSupervisorSpec( + new TestSupervisorSpec("id3", null, Arrays.asList("datasource3")), + "v2" + ), + new VersionedSupervisorSpec( + new NoopSupervisorSpec(null, Arrays.asList("datasource3")), + "tombstone" ) ), response.getEntity() @@ -574,6 +738,23 @@ public void testReset() verifyAll(); } + @Test + public void testNoopSupervisorSpecSerde() throws Exception + { + ObjectMapper mapper = new ObjectMapper(); + String oldSpec = "{\"type\":\"NoopSupervisorSpec\",\"id\":null,\"dataSources\":null}"; + NoopSupervisorSpec expectedSpec = new NoopSupervisorSpec(null, null); + NoopSupervisorSpec deserializedSpec = mapper.readValue(oldSpec, NoopSupervisorSpec.class); + Assert.assertEquals(expectedSpec, deserializedSpec); + + NoopSupervisorSpec spec1 = new NoopSupervisorSpec("abcd", Lists.newArrayList("defg")); + NoopSupervisorSpec spec2 = mapper.readValue( + mapper.writeValueAsBytes(spec1), + NoopSupervisorSpec.class + ); + Assert.assertEquals(spec1, spec2); + } + private static class TestSupervisorSpec implements SupervisorSpec { private final String id; diff --git a/server/src/main/java/io/druid/indexing/overlord/supervisor/NoopSupervisorSpec.java b/server/src/main/java/io/druid/indexing/overlord/supervisor/NoopSupervisorSpec.java index 401023fe032a..380861d9fb8f 100644 --- a/server/src/main/java/io/druid/indexing/overlord/supervisor/NoopSupervisorSpec.java +++ b/server/src/main/java/io/druid/indexing/overlord/supervisor/NoopSupervisorSpec.java @@ -19,20 +19,46 @@ package io.druid.indexing.overlord.supervisor; +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; import io.druid.indexing.overlord.DataSourceMetadata; import javax.annotation.Nullable; +import java.util.ArrayList; import java.util.List; +import java.util.Objects; /** * Used as a tombstone marker in the supervisors metadata table to indicate that the supervisor has been removed. */ public class NoopSupervisorSpec implements SupervisorSpec { + // NoopSupervisorSpec is used as a tombstone, added when a previously running supervisor is stopped. + // Inherit the datasources of the previous running spec, so that we can determine whether a user is authorized to see + // this tombstone (users can only see tombstones for datasources that they have access to). + @Nullable + @JsonProperty("dataSources") + private List datasources; + + @Nullable + @JsonProperty("id") + private String id; + + @JsonCreator + public NoopSupervisorSpec( + @Nullable @JsonProperty("id") String id, + @Nullable @JsonProperty("dataSources") List datasources + ) + { + this.id = id; + this.datasources = datasources == null ? new ArrayList<>() : datasources; + } + @Override + @JsonProperty public String getId() { - return null; + return id; } @Override @@ -68,8 +94,30 @@ public void checkpoint( } @Override + @Nullable + @JsonProperty("dataSources") public List getDataSources() { - return null; + return datasources; + } + + @Override + public boolean equals(Object o) + { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + NoopSupervisorSpec spec = (NoopSupervisorSpec) o; + return Objects.equals(datasources, spec.datasources) && + Objects.equals(getId(), spec.getId()); + } + + @Override + public int hashCode() + { + return Objects.hash(datasources, getId()); } }