paul-rogers commented on code in PR #14003:
URL: https://github.com/apache/druid/pull/14003#discussion_r1156401854
##########
extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/KafkaIndexTask.java:
##########
@@ -132,6 +136,20 @@ public String getType()
return TYPE;
}
+ @JsonIgnore
+ @Nullable
+ @Override
+ public Set<String> getInputSourceTypes()
+ {
+ return Collections.singleton("kafka");
Review Comment:
Is there a Kafka input source? If so, perhaps use the `TYPE_KEY` constant
from that class (adding one if one does not already exist.)
##########
processing/src/main/java/org/apache/druid/data/input/impl/CombiningInputSource.java:
##########
@@ -61,6 +63,19 @@ public CombiningInputSource(
this.delegates = delegates;
}
+ @Override
+ public Set<String> getTypes()
+ {
+ Set<String> types = new HashSet<>();
+ for (InputSource delegate : delegates) {
+ Set<String> delegateTypes = delegate.getTypes();
+ if (null != delegateTypes) {
+ types.addAll(delegateTypes);
+ }
+ }
Review Comment:
The above would be easier if we didn't allow nulls:
```java
Set<String> types = new HashSet<>();
for (InputSource delegate : delegates) {
types.addAll(delegate.getTypes());
}
```
Or, the streaming alternative, if IntelliJ inspections doesn't like the
above loop.
##########
server/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorSpec.java:
##########
@@ -71,6 +73,15 @@ default boolean isSuspended()
*/
String getType();
+ @Nullable
+ default Set<String> getInputSourceTypes() {
+ return null;
+ }
+
+ default boolean usesFirehose() {
+ return false;
+ }
Review Comment:
Perhaps add comments to explain these method. Especially the
`usesFirehose()` method: I gather that firehose doesn't fit into the input
security model? Why not? An explanation will help future readers.
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/PartialDimensionDistributionTask.java:
##########
@@ -174,6 +176,22 @@ public String getType()
return TYPE;
}
+ @JsonIgnore
+ @Nullable
+ @Override
+ public Set<String> getInputSourceTypes()
+ {
+ return getIngestionSchema().getIOConfig().getInputSource() != null ?
Review Comment:
This code appears again and again. Can it be promoted to a base class?
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/AppenderatorDriverRealtimeIndexTask.java:
##########
@@ -213,6 +215,20 @@ public String getType()
return "index_realtime_appenderator";
}
+ @JsonIgnore
+ @Nullable
+ @Override
+ public Set<String> getInputSourceTypes()
+ {
+ return null;
Review Comment:
Is this the right answer? Or, should it be `ImmutableSet.of()` so it is
non-null? Here and below.
##########
processing/src/main/java/org/apache/druid/data/input/impl/InlineInputSource.java:
##########
@@ -48,6 +50,12 @@ public InlineInputSource(@JsonProperty("data") String data)
this.data = data;
}
+ @Override
+ public Set<String> getTypes()
+ {
+ return Collections.singleton(TYPE_KEY);
+ }
Review Comment:
Move to the base class since all input sources supported thus far have only
one type. (The catalog and table function stuff depend on this fact.)
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordResource.java:
##########
@@ -188,14 +194,36 @@ public OverlordResource(
public Response taskPost(final Task task, @Context final HttpServletRequest
req)
{
final String dataSource = task.getDataSource();
- final ResourceAction resourceAction = new ResourceAction(
- new Resource(dataSource, ResourceType.DATASOURCE),
- Action.WRITE
- );
+ final Set<ResourceAction> resourceActions = new HashSet<>();
+ resourceActions.add(new ResourceAction(new Resource(dataSource,
ResourceType.DATASOURCE), Action.WRITE));
+ if (authConfig.isEnableInputSourceSecurity()) {
+ if (task.usesFirehose()) {
+ return Response.status(Response.Status.BAD_REQUEST)
+ .entity(
+ ImmutableMap.of(
+ "error",
+ StringUtils.format(
+ "Input source based security cannot be performed for
Task[%s] because it uses firehose."
+ + "Change the tasks configuration, or disable
`isEnableInputSourceSecurity`",
+ task.getId()
+ )
+ )
+ )
+ .build();
+ }
+ if (null != task.getInputSourceTypes()) {
Review Comment:
Nit: we're computing the input source types twice.
```
Set<String> inputSourceTypes = task.getInputSourceTypes();
if (inputSourceTypes != null) {
...
```
Then, the reason we return a `Set` and not a single key is that some input
sources are compound. If we allow the input source type method to return null,
then those methods that are compound have to include code to handle null.
But, if we say the method must return a non-null set, then it is easy to
combine child input sources without null checks.
##########
processing/src/main/java/org/apache/druid/data/input/InputSource.java:
##########
@@ -87,4 +88,10 @@ InputSourceReader reader(
@Nullable InputFormat inputFormat,
File temporaryDirectory
);
+
+ @Nullable
+ default Set<String> getTypes()
+ {
+ return null;
+ }
Review Comment:
Would be good to add a comment explaining why we return a set, not a single
key. Also, see the note above regarding `null` vs. and empty set.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
