From e423f8793ed1f51f1ff4dd5fd3124edd5a4eeb38 Mon Sep 17 00:00:00 2001 From: Jonathan Wei Date: Mon, 19 Mar 2018 12:55:47 -0700 Subject: [PATCH] Fix supervisor tombstone auth handling (#5504) --- .../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 5c1eecc38ff3..00f2c92c4b75 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())); } 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 348c40a58ab4..09196df401c3 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 @@ -48,6 +48,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; @@ -66,7 +67,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 c22dde9eb1f4..8415b919b721 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; @@ -295,7 +296,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 43283997031d..3c0a2bd1585c 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; @@ -295,6 +296,10 @@ public void testSpecGetAllHistory() throws Exception new VersionedSupervisorSpec( new TestSupervisorSpec("id1", null, Arrays.asList("datasource1")), "v2" + ), + new VersionedSupervisorSpec( + new NoopSupervisorSpec(null, Arrays.asList("datasource1")), + "tombstone" ) ); List versions2 = ImmutableList.of( @@ -305,11 +310,42 @@ public void testSpecGetAllHistory() throws Exception 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); @@ -352,6 +388,10 @@ public void testSpecGetAllHistoryWithAuthFailureFiltering() throws Exception new VersionedSupervisorSpec( new TestSupervisorSpec("id1", null, Arrays.asList("datasource1")), "v2" + ), + new VersionedSupervisorSpec( + new NoopSupervisorSpec(null, Arrays.asList("datasource1")), + "tombstone" ) ); List versions2 = ImmutableList.of( @@ -362,12 +402,62 @@ public void testSpecGetAllHistoryWithAuthFailureFiltering() throws Exception 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); @@ -387,6 +477,32 @@ public void testSpecGetAllHistoryWithAuthFailureFiltering() throws Exception 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()); @@ -410,6 +526,10 @@ public void testSpecGetHistory() throws Exception 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" @@ -420,6 +540,10 @@ public void testSpecGetHistory() 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" @@ -472,6 +596,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" @@ -482,6 +610,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" @@ -492,9 +624,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(); @@ -529,6 +677,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() @@ -582,6 +746,23 @@ public void testReset() throws Exception 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()); } }