gianm commented on code in PR #14003:
URL: https://github.com/apache/druid/pull/14003#discussion_r1157781418
##########
server/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorSpec.java:
##########
@@ -71,6 +74,25 @@ default boolean isSuspended()
*/
String getType();
+ /**
+ * @return The types of {@link org.apache.druid.data.input.InputSource} that
the task uses. Empty set is returned if
+ * the task does not use any. Users can be given permission to access
particular types of
+ * input sources but not others, using the
+ * {@link
org.apache.druid.server.security.AuthConfig#enableInputSourceSecurity} config.
+ */
+ default Set<String> getInputSourceTypes() {
+ return ImmutableSet.of();
+ }
+
+ /**
+ * @return Whether the task uses {@link
org.apache.druid.data.input.Firehose}. If the
+ * {@link
org.apache.druid.server.security.AuthConfig#enableInputSourceSecurity} config is
+ * enabled, then tasks that use firehose cannot be used.
+ */
+ default boolean usesFirehose() {
Review Comment:
We should change this to what callers care about. The caller isn't
interested in whether a task uses firehoses: it's interested in whether the
`getInputSourceTypes` method can be used for authorization. (Consider a case
where a task doesn't use firehoses, but still also doesn't support input source
authorization.)
The default is also problematic: security stuff must always fail-secure.
Doing `return false` here fails insecure: it means that a task that doesn't
implement any of this stuff would be allowed. So we should flip that. Putting
these together: I'd consider changing this to `boolean
canUseInputSourceTypeAuthorization()` and having the default be `return false`.
Or, another option would be eliminating this method, and having
`getInputSourceTypes()` (or `getInputSourceResources()`) throw an exception if
the task doesn't support input source authorization. The default implementation
would need to throw that exception.
(Same comment for `Task`, btw.)
##########
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:
It's better here to return a `Set<Resource>` rather than `Set<String>`. It
makes it clearer that this is used for security purposes, since `Resource` is a
security-specific thing.
##########
server/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorSpec.java:
##########
@@ -71,6 +74,25 @@ default boolean isSuspended()
*/
String getType();
+ /**
+ * @return The types of {@link org.apache.druid.data.input.InputSource} that
the task uses. Empty set is returned if
+ * the task does not use any. Users can be given permission to access
particular types of
+ * input sources but not others, using the
+ * {@link
org.apache.druid.server.security.AuthConfig#enableInputSourceSecurity} config.
+ */
+ default Set<String> getInputSourceTypes() {
Review Comment:
Similar to the comment on `InputSource`: it's better for this to be
`Set<Resource>`, so it's clear it's security-related. Method name would be
`getInputSourceResources()` in that case.
--
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]
