You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/02/02 02:03:30 UTC
[GitHub] [druid] jihoonson opened a new pull request #10830: Allow only HTTP and HTTPS protocols for the HTTP inputSource
jihoonson opened a new pull request #10830:
URL: https://github.com/apache/druid/pull/10830
### Description
The HTTP inputSource should allow only HTTP and HTTPS protocols.
<hr>
This PR has:
- [x] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/licenses.yaml)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10830: HTTP inputSource and firehose should support only HTTP and HTTPS; HDFS inputSource and firehose should support only HDFS
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10830:
URL: https://github.com/apache/druid/pull/10830#discussion_r571301390
##########
File path: extensions-core/hdfs-storage/src/main/java/org/apache/druid/inputsource/hdfs/HdfsInputSource.java
##########
@@ -101,20 +102,25 @@ public HdfsInputSource(
return paths;
}
- public static Collection<Path> getPaths(List<String> inputPaths, Configuration configuration) throws IOException
+ public static Collection<Path> getPaths(List<String> inputPathStrings, Configuration configuration) throws IOException
{
- if (inputPaths.isEmpty()) {
+ if (inputPathStrings.isEmpty()) {
return Collections.emptySet();
}
// Use FileInputFormat to read splits. To do this, we need to make a fake Job.
Job job = Job.getInstance(configuration);
// Add paths to the fake JobContext.
- for (String inputPath : inputPaths) {
+ for (String inputPath : inputPathStrings) {
FileInputFormat.addInputPaths(job, inputPath);
}
+ final Path[] inputPaths = FileInputFormat.getInputPaths(job);
+ if (Arrays.stream(inputPaths).anyMatch(path -> !"hdfs".equalsIgnoreCase(path.toUri().getScheme()))) {
+ throw new IllegalArgumentException("Input paths must be the HDFS path");
+ }
+
Review comment:
@suneet-s @a2l007 good point. I realized that this is a [documented feature](https://druid.apache.org/docs/0.20.0/ingestion/native-batch.html#hdfs-input-source) that should be supported. The concern here is that users can use whatever protocol they want if it's supported by the hdfs client. Instead of restricting it to only hdfs, I added new configs for http and hdfs inputSources so that users can configure what protocols they want to allow.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10830: Add configurations for allowed protocols for HTTP and HDFS inputSources/firehoses
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10830:
URL: https://github.com/apache/druid/pull/10830#discussion_r575509818
##########
File path: docs/ingestion/native-batch.md
##########
@@ -1064,7 +1064,7 @@ Sample specs:
"type": "index_parallel",
"inputSource": {
"type": "hdfs",
- "paths": "hdfs://foo/bar/", "hdfs://bar/foo"
+ "paths": "hdfs:/foo/bar/", "hdfs:/bar/foo"
Review comment:
That sounds even better. I will update it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] techdocsmith commented on pull request #10830: Add configurations for allowed protocols for HTTP and HDFS inputSources/firehoses
Posted by GitBox <gi...@apache.org>.
techdocsmith commented on pull request #10830:
URL: https://github.com/apache/druid/pull/10830#issuecomment-776956394
@suneet-s , I did an editorial pass on the affected d`.md`. Let me know if I've missed something.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #10830: Allow only HTTP and HTTPS protocols for the HTTP inputSource
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10830:
URL: https://github.com/apache/druid/pull/10830#discussion_r569137229
##########
File path: core/src/main/java/org/apache/druid/data/input/impl/HttpInputSource.java
##########
@@ -69,6 +70,13 @@ public HttpInputSource(
this.config = config;
}
+ public static void throwIfInvalidProtocols(List<URI> uris)
Review comment:
nit:
```suggestion
private static void throwIfInvalidProtocols(List<URI> uris)
```
##########
File path: extensions-core/hdfs-storage/src/main/java/org/apache/druid/inputsource/hdfs/HdfsInputSource.java
##########
@@ -101,20 +102,25 @@ public HdfsInputSource(
return paths;
}
- public static Collection<Path> getPaths(List<String> inputPaths, Configuration configuration) throws IOException
+ public static Collection<Path> getPaths(List<String> inputPathStrings, Configuration configuration) throws IOException
{
- if (inputPaths.isEmpty()) {
+ if (inputPathStrings.isEmpty()) {
return Collections.emptySet();
}
// Use FileInputFormat to read splits. To do this, we need to make a fake Job.
Job job = Job.getInstance(configuration);
// Add paths to the fake JobContext.
- for (String inputPath : inputPaths) {
+ for (String inputPath : inputPathStrings) {
FileInputFormat.addInputPaths(job, inputPath);
}
+ final Path[] inputPaths = FileInputFormat.getInputPaths(job);
+ if (Arrays.stream(inputPaths).anyMatch(path -> !"hdfs".equalsIgnoreCase(path.toUri().getScheme()))) {
+ throw new IllegalArgumentException("Input paths must be the HDFS path");
+ }
+
Review comment:
It appears that `FileInputFormat#addInputPath` already has it's own scheme validation, so I don't believe we need this check. See `org.apache.hadoop.fs.FileSystem#checkPath`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #10830: Allow only HTTP and HTTPS protocols for the HTTP inputSource
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #10830:
URL: https://github.com/apache/druid/pull/10830#issuecomment-771841826
Do the docs need to be updated as well? Right now it just says "URIs of the input files." https://druid.apache.org/docs/latest/ingestion/native-batch.html#http-input-source
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10830: Add configurations for allowed protocols for HTTP and HDFS inputSources/firehoses
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10830:
URL: https://github.com/apache/druid/pull/10830#discussion_r586151214
##########
File path: docs/ingestion/native-batch.md
##########
@@ -1064,7 +1064,7 @@ Sample specs:
"type": "index_parallel",
"inputSource": {
"type": "hdfs",
- "paths": "hdfs://foo/bar/", "hdfs://bar/foo"
+ "paths": "hdfs:/foo/bar/", "hdfs:/bar/foo"
Review comment:
I updated the doc.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10830: Allow only HTTP and HTTPS protocols for the HTTP inputSource
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10830:
URL: https://github.com/apache/druid/pull/10830#discussion_r569145789
##########
File path: core/src/main/java/org/apache/druid/data/input/impl/HttpInputSource.java
##########
@@ -69,6 +70,13 @@ public HttpInputSource(
this.config = config;
}
+ public static void throwIfInvalidProtocols(List<URI> uris)
Review comment:
It should be public since this method is now used by `HttpFirehoseFactory` as well.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10830: Add configurations for allowed protocols for HTTP and HDFS inputSources/firehoses
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10830:
URL: https://github.com/apache/druid/pull/10830#discussion_r574986869
##########
File path: docs/ingestion/native-batch.md
##########
@@ -1553,6 +1557,11 @@ Note that prefetching or caching isn't that useful in the Parallel task.
|fetchTimeout|Timeout for fetching each file.|60000|
|maxFetchRetry|Maximum number of retries for fetching each file.|3|
+You can also ingest from other storage using the HDFS firehose if the HDFS client supports that storage.
+However, if you want to ingest from cloud storage, consider using the proper input sources for them.
Review comment:
Same here.
##########
File path: docs/ingestion/native-batch.md
##########
@@ -1127,9 +1127,10 @@ Sample specs:
|type|This should be `hdfs`.|None|yes|
|paths|HDFS paths. Can be either a JSON array or comma-separated string of paths. Wildcards like `*` are supported in these paths. Empty files located under one of the given paths will be skipped.|None|yes|
-You can also ingest from cloud storage using the HDFS input source.
-However, if you want to read from AWS S3 or Google Cloud Storage, consider using
-the [S3 input source](#s3-input-source) or the [Google Cloud Storage input source](#google-cloud-storage-input-source) instead.
+You can also ingest from other storage using the HDFS input source if the HDFS client supports that storage.
+However, if you want to ingest from cloud storage, consider using the proper input sources for them.
Review comment:
You can read from not only cloud storage but any storage if it's supported by the HDFS client. I changed to `the service-specific input source for your data storage`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] a2l007 commented on a change in pull request #10830: Add configurations for allowed protocols for HTTP and HDFS inputSources/firehoses
Posted by GitBox <gi...@apache.org>.
a2l007 commented on a change in pull request #10830:
URL: https://github.com/apache/druid/pull/10830#discussion_r575262001
##########
File path: docs/ingestion/native-batch.md
##########
@@ -1064,7 +1064,7 @@ Sample specs:
"type": "index_parallel",
"inputSource": {
"type": "hdfs",
- "paths": "hdfs://foo/bar/", "hdfs://bar/foo"
+ "paths": "hdfs:/foo/bar/", "hdfs:/bar/foo"
Review comment:
I think `hdfs:/foo/bar` may only work if the namenode is configured in hadoop. For general cases, the URI format is `scheme://authority/path` (https://hadoop.apache.org/docs/r2.4.1/hadoop-project-dist/hadoop-common/FileSystemShell.html#Overview)
Do you think it would be clearer to change this to `"hdfs://namenodehost/foo/bar/"` instead?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #10830: Add configurations for allowed protocols for HTTP and HDFS inputSources/firehoses
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10830:
URL: https://github.com/apache/druid/pull/10830#issuecomment-789474295
@a2l007 @suneet-s do you have more comments?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] a2l007 commented on a change in pull request #10830: Add configurations for allowed protocols for HTTP and HDFS inputSources/firehoses
Posted by GitBox <gi...@apache.org>.
a2l007 commented on a change in pull request #10830:
URL: https://github.com/apache/druid/pull/10830#discussion_r573073247
##########
File path: extensions-core/hdfs-storage/src/main/java/org/apache/druid/firehose/hdfs/HdfsFirehoseFactory.java
##########
@@ -44,21 +45,25 @@
{
private final List<String> inputPaths;
private final Configuration conf;
+ private final HdfsInputSourceConfig inputSourceConfig;
@JsonCreator
public HdfsFirehoseFactory(
- @JacksonInject @Hdfs Configuration conf,
@JsonProperty("paths") Object inputPaths,
@JsonProperty("maxCacheCapacityBytes") Long maxCacheCapacityBytes,
@JsonProperty("maxFetchCapacityBytes") Long maxFetchCapacityBytes,
@JsonProperty("prefetchTriggerBytes") Long prefetchTriggerBytes,
@JsonProperty("fetchTimeout") Long fetchTimeout,
- @JsonProperty("maxFetchRetry") Integer maxFetchRetry
+ @JsonProperty("maxFetchRetry") Integer maxFetchRetry,
+ @JacksonInject @Hdfs Configuration conf,
+ @JacksonInject HdfsInputSourceConfig inputSourceConfig
)
{
super(maxCacheCapacityBytes, maxFetchCapacityBytes, prefetchTriggerBytes, fetchTimeout, maxFetchRetry);
- this.inputPaths = HdfsInputSource.coerceInputPathsToList(inputPaths, "inputPaths");
+ this.inputPaths = HdfsInputSource.coerceInputPathsToList(inputPaths, "paths");
Review comment:
👍
##########
File path: docs/ingestion/native-batch.md
##########
@@ -1064,7 +1064,7 @@ Sample specs:
"type": "index_parallel",
"inputSource": {
"type": "hdfs",
- "paths": "hdfs://foo/bar/", "hdfs://bar/foo"
+ "paths": "hdfs:/foo/bar/", "hdfs:/bar/foo"
Review comment:
nit: Is this change needed?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10830: Allow only HTTP and HTTPS protocols for the HTTP inputSource
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10830:
URL: https://github.com/apache/druid/pull/10830#discussion_r569147174
##########
File path: extensions-core/hdfs-storage/src/main/java/org/apache/druid/inputsource/hdfs/HdfsInputSource.java
##########
@@ -101,20 +102,25 @@ public HdfsInputSource(
return paths;
}
- public static Collection<Path> getPaths(List<String> inputPaths, Configuration configuration) throws IOException
+ public static Collection<Path> getPaths(List<String> inputPathStrings, Configuration configuration) throws IOException
{
- if (inputPaths.isEmpty()) {
+ if (inputPathStrings.isEmpty()) {
return Collections.emptySet();
}
// Use FileInputFormat to read splits. To do this, we need to make a fake Job.
Job job = Job.getInstance(configuration);
// Add paths to the fake JobContext.
- for (String inputPath : inputPaths) {
+ for (String inputPath : inputPathStrings) {
FileInputFormat.addInputPaths(job, inputPath);
}
+ final Path[] inputPaths = FileInputFormat.getInputPaths(job);
+ if (Arrays.stream(inputPaths).anyMatch(path -> !"hdfs".equalsIgnoreCase(path.toUri().getScheme()))) {
+ throw new IllegalArgumentException("Input paths must be the HDFS path");
+ }
+
Review comment:
`FileSystem` has a couple of implementations such as `DistributedFileSystem`, `S3AFileSystem`, etc which support different schemes. The default file system is `LocalFileSystem` unless you set `fs.default.name`. I think we should not allow anything else but only `hdfs` and let users use the right inputSource to read from other storage (e.g., `S3InputSource to read from s3).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10830: Add configurations for allowed protocols for HTTP and HDFS inputSources/firehoses
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10830:
URL: https://github.com/apache/druid/pull/10830#discussion_r574989227
##########
File path: docs/ingestion/native-batch.md
##########
@@ -1064,7 +1064,7 @@ Sample specs:
"type": "index_parallel",
"inputSource": {
"type": "hdfs",
- "paths": "hdfs://foo/bar/", "hdfs://bar/foo"
+ "paths": "hdfs:/foo/bar/", "hdfs:/bar/foo"
Review comment:
`hdfs://foo/bar` seems strange to me because it refers the `/bar` path on the host `hdfs://foo` (https://en.wikipedia.org/wiki/Uniform_Resource_Identifier). I don't think it was intentional.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson merged pull request #10830: Add configurations for allowed protocols for HTTP and HDFS inputSources/firehoses
Posted by GitBox <gi...@apache.org>.
jihoonson merged pull request #10830:
URL: https://github.com/apache/druid/pull/10830
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #10830: Allow only HTTP and HTTPS protocols for the HTTP inputSource
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #10830:
URL: https://github.com/apache/druid/pull/10830#issuecomment-771841826
Do the docs need to be updated as well? Right now it just says "URIs of the input files." https://druid.apache.org/docs/latest/ingestion/native-batch.html#http-input-source
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #10830: Add configurations for allowed protocols for HTTP and HDFS inputSources/firehoses
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #10830:
URL: https://github.com/apache/druid/pull/10830#issuecomment-776930225
Overall, the defaults look reasonable to me. I looked through other InputSources to see if we need a similar change, and didn't find any. I haven't read through the code in fine detail - will defer to @a2l007
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #10830: HTTP inputSource and firehose should support only HTTP and HTTPS; HDFS inputSource and firehose should support only HDFS
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10830:
URL: https://github.com/apache/druid/pull/10830#discussion_r569543791
##########
File path: extensions-core/hdfs-storage/src/main/java/org/apache/druid/inputsource/hdfs/HdfsInputSource.java
##########
@@ -101,20 +102,25 @@ public HdfsInputSource(
return paths;
}
- public static Collection<Path> getPaths(List<String> inputPaths, Configuration configuration) throws IOException
+ public static Collection<Path> getPaths(List<String> inputPathStrings, Configuration configuration) throws IOException
{
- if (inputPaths.isEmpty()) {
+ if (inputPathStrings.isEmpty()) {
return Collections.emptySet();
}
// Use FileInputFormat to read splits. To do this, we need to make a fake Job.
Job job = Job.getInstance(configuration);
// Add paths to the fake JobContext.
- for (String inputPath : inputPaths) {
+ for (String inputPath : inputPathStrings) {
FileInputFormat.addInputPaths(job, inputPath);
}
+ final Path[] inputPaths = FileInputFormat.getInputPaths(job);
+ if (Arrays.stream(inputPaths).anyMatch(path -> !"hdfs".equalsIgnoreCase(path.toUri().getScheme()))) {
+ throw new IllegalArgumentException("Input paths must be the HDFS path");
+ }
+
Review comment:
Are there users that rely on this behavior? Would restricting this to just hdfs mean it's not possible for some users to ingest files from these locations any more? If that's the case, should we introduce a config to allow server admins to specify which schemes are supported?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #10830: Allow only HTTP and HTTPS protocols for the HTTP inputSource
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10830:
URL: https://github.com/apache/druid/pull/10830#issuecomment-772206529
> Do the docs need to be updated as well? Right now it just says "URIs of the input files." https://druid.apache.org/docs/latest/ingestion/native-batch.html#http-input-source
The linked section starts with `The HTTP input source is to support reading files directly from remote sites via HTTP`. I think this implies the URIs must be HTTP URIs, but guess it doesn't harm to make it clear. I updated the doc.
I also fixed the HDFS inputSource to allow only `hdfs` paths. All other inputSources already have a scheme check if they accept URIs.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #10830: HTTP inputSource and firehose should support only HTTP and HTTPS; HDFS inputSource and firehose should support only HDFS
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10830:
URL: https://github.com/apache/druid/pull/10830#discussion_r569529717
##########
File path: core/src/main/java/org/apache/druid/data/input/impl/HttpInputSource.java
##########
@@ -69,6 +70,13 @@ public HttpInputSource(
this.config = config;
}
+ public static void throwIfInvalidProtocols(List<URI> uris)
Review comment:
😢
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #10830: Add configurations for allowed protocols for HTTP and HDFS inputSources/firehoses
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #10830:
URL: https://github.com/apache/druid/pull/10830#issuecomment-776925188
@techdocsmith FYI
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #10830: Add configurations for allowed protocols for HTTP and HDFS inputSources/firehoses
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10830:
URL: https://github.com/apache/druid/pull/10830#issuecomment-792038984
@a2l007 @techdocsmith @suneet-s thanks for the review :+1:
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] techdocsmith commented on a change in pull request #10830: Add configurations for allowed protocols for HTTP and HDFS inputSources/firehoses
Posted by GitBox <gi...@apache.org>.
techdocsmith commented on a change in pull request #10830:
URL: https://github.com/apache/druid/pull/10830#discussion_r573989831
##########
File path: docs/configuration/index.md
##########
@@ -515,6 +515,36 @@ This deep storage is used to interface with Cassandra. Note that the `druid-cas
|`druid.storage.keyspace`|Cassandra key space.|none|
+### Ingestion Security Configuration
+
+#### HDFS input source
+
+You can set the following property to control what protocols are allowed for
+the [HDFS input source](../ingestion/native-batch.md#hdfs-input-source) and the [HDFS firehose](../ingestion/native-batch.md#hdfsfirehose).
+
+|Property|Possible Values|Description|Default|
+|--------|---------------|-----------|-------|
+|`druid.ingestion.hdfs.allowedProtocols`|List of protocols|Allowed protocols that HDFS input source and HDFS firehose can use.|["hdfs"]|
Review comment:
```suggestion
|`druid.ingestion.hdfs.allowedProtocols`|List of protocols|Allowed protocols for the HDFS input source and HDFS firehose.|["hdfs"]|
```
##########
File path: docs/configuration/index.md
##########
@@ -515,6 +515,36 @@ This deep storage is used to interface with Cassandra. Note that the `druid-cas
|`druid.storage.keyspace`|Cassandra key space.|none|
+### Ingestion Security Configuration
+
+#### HDFS input source
+
+You can set the following property to control what protocols are allowed for
Review comment:
```suggestion
You can set the following property to specify permissible protocols for
```
##########
File path: docs/configuration/index.md
##########
@@ -515,6 +515,36 @@ This deep storage is used to interface with Cassandra. Note that the `druid-cas
|`druid.storage.keyspace`|Cassandra key space.|none|
+### Ingestion Security Configuration
+
+#### HDFS input source
+
+You can set the following property to control what protocols are allowed for
+the [HDFS input source](../ingestion/native-batch.md#hdfs-input-source) and the [HDFS firehose](../ingestion/native-batch.md#hdfsfirehose).
+
+|Property|Possible Values|Description|Default|
+|--------|---------------|-----------|-------|
+|`druid.ingestion.hdfs.allowedProtocols`|List of protocols|Allowed protocols that HDFS input source and HDFS firehose can use.|["hdfs"]|
+
+
+#### HTTP input source
+
+You can set the following property to control what protocols are allowed for
+the [HTTP input source](../ingestion/native-batch.md#http-input-source) and the [HTTP firehose](../ingestion/native-batch.md#httpfirehose).
+
+|Property|Possible Values|Description|Default|
+|--------|---------------|-----------|-------|
+|`druid.ingestion.http.allowedProtocols`|List of protocols|Allowed protocols that HTTP input source and HTTP firehose can use.|["http", "https"]|
+
+The following properties are to control what domains native batch tasks can access to using
Review comment:
```suggestion
The following properties control the domains native batch tasks can access using
```
##########
File path: docs/configuration/index.md
##########
@@ -515,6 +515,36 @@ This deep storage is used to interface with Cassandra. Note that the `druid-cas
|`druid.storage.keyspace`|Cassandra key space.|none|
+### Ingestion Security Configuration
+
+#### HDFS input source
+
+You can set the following property to control what protocols are allowed for
+the [HDFS input source](../ingestion/native-batch.md#hdfs-input-source) and the [HDFS firehose](../ingestion/native-batch.md#hdfsfirehose).
+
+|Property|Possible Values|Description|Default|
+|--------|---------------|-----------|-------|
+|`druid.ingestion.hdfs.allowedProtocols`|List of protocols|Allowed protocols that HDFS input source and HDFS firehose can use.|["hdfs"]|
+
+
+#### HTTP input source
+
+You can set the following property to control what protocols are allowed for
+the [HTTP input source](../ingestion/native-batch.md#http-input-source) and the [HTTP firehose](../ingestion/native-batch.md#httpfirehose).
+
+|Property|Possible Values|Description|Default|
+|--------|---------------|-----------|-------|
+|`druid.ingestion.http.allowedProtocols`|List of protocols|Allowed protocols that HTTP input source and HTTP firehose can use.|["http", "https"]|
+
+The following properties are to control what domains native batch tasks can access to using
+the [HTTP input source](../ingestion/native-batch.md#http-input-source).
+
+|Property|Possible Values|Description|Default|
+|--------|---------------|-----------|-------|
+|`druid.ingestion.http.allowListDomains`|List of domains|Allowed domains from which ingestion will be allowed. Only one of allowList or denyList can be set.|empty list|
Review comment:
```suggestion
|`druid.ingestion.http.allowListDomains`|List of domains|Domains allowed to use as an ingestion source. You cannot use both `allowList` and `denyList`.|empty list|
```
##########
File path: docs/configuration/index.md
##########
@@ -515,6 +515,36 @@ This deep storage is used to interface with Cassandra. Note that the `druid-cas
|`druid.storage.keyspace`|Cassandra key space.|none|
+### Ingestion Security Configuration
+
+#### HDFS input source
+
+You can set the following property to control what protocols are allowed for
+the [HDFS input source](../ingestion/native-batch.md#hdfs-input-source) and the [HDFS firehose](../ingestion/native-batch.md#hdfsfirehose).
+
+|Property|Possible Values|Description|Default|
+|--------|---------------|-----------|-------|
+|`druid.ingestion.hdfs.allowedProtocols`|List of protocols|Allowed protocols that HDFS input source and HDFS firehose can use.|["hdfs"]|
+
+
+#### HTTP input source
+
+You can set the following property to control what protocols are allowed for
Review comment:
```suggestion
You can set the following property to specify permissible protocols for
```
##########
File path: docs/ingestion/native-batch.md
##########
@@ -1127,9 +1127,10 @@ Sample specs:
|type|This should be `hdfs`.|None|yes|
|paths|HDFS paths. Can be either a JSON array or comma-separated string of paths. Wildcards like `*` are supported in these paths. Empty files located under one of the given paths will be skipped.|None|yes|
-You can also ingest from cloud storage using the HDFS input source.
-However, if you want to read from AWS S3 or Google Cloud Storage, consider using
-the [S3 input source](#s3-input-source) or the [Google Cloud Storage input source](#google-cloud-storage-input-source) instead.
+You can also ingest from other storage using the HDFS input source if the HDFS client supports that storage.
+However, if you want to ingest from cloud storage, consider using the proper input sources for them.
+If you want to use a non-hdfs protocol with the HDFS input source, you need to include the protocol you want
Review comment:
```suggestion
If you want to use a non-hdfs protocol with the HDFS input source, include the protocol
```
##########
File path: docs/ingestion/native-batch.md
##########
@@ -1553,6 +1557,11 @@ Note that prefetching or caching isn't that useful in the Parallel task.
|fetchTimeout|Timeout for fetching each file.|60000|
|maxFetchRetry|Maximum number of retries for fetching each file.|3|
+You can also ingest from other storage using the HDFS firehose if the HDFS client supports that storage.
+However, if you want to ingest from cloud storage, consider using the proper input sources for them.
Review comment:
```suggestion
However, if you want to ingest from cloud storage, consider using the service-specific input source for your cloud storage.
```
##########
File path: docs/configuration/index.md
##########
@@ -515,6 +515,36 @@ This deep storage is used to interface with Cassandra. Note that the `druid-cas
|`druid.storage.keyspace`|Cassandra key space.|none|
+### Ingestion Security Configuration
+
+#### HDFS input source
+
+You can set the following property to control what protocols are allowed for
+the [HDFS input source](../ingestion/native-batch.md#hdfs-input-source) and the [HDFS firehose](../ingestion/native-batch.md#hdfsfirehose).
+
+|Property|Possible Values|Description|Default|
+|--------|---------------|-----------|-------|
+|`druid.ingestion.hdfs.allowedProtocols`|List of protocols|Allowed protocols that HDFS input source and HDFS firehose can use.|["hdfs"]|
+
+
+#### HTTP input source
+
+You can set the following property to control what protocols are allowed for
+the [HTTP input source](../ingestion/native-batch.md#http-input-source) and the [HTTP firehose](../ingestion/native-batch.md#httpfirehose).
+
+|Property|Possible Values|Description|Default|
+|--------|---------------|-----------|-------|
+|`druid.ingestion.http.allowedProtocols`|List of protocols|Allowed protocols that HTTP input source and HTTP firehose can use.|["http", "https"]|
+
+The following properties are to control what domains native batch tasks can access to using
+the [HTTP input source](../ingestion/native-batch.md#http-input-source).
+
+|Property|Possible Values|Description|Default|
+|--------|---------------|-----------|-------|
+|`druid.ingestion.http.allowListDomains`|List of domains|Allowed domains from which ingestion will be allowed. Only one of allowList or denyList can be set.|empty list|
+|`druid.ingestion.http.denyListDomains`|List of domains|Blacklisted domains from which ingestion will NOT be allowed. Only one of allowList or denyList can be set. |empty list|
Review comment:
```suggestion
|`druid.ingestion.http.denyListDomains`|List of domains|Domains not be allowed for use as an ingestion source. You cannot use both `denyList` and `allowList`. |empty list|
```
##########
File path: docs/ingestion/native-batch.md
##########
@@ -1590,6 +1599,9 @@ A sample HTTP Firehose spec is shown below:
}
```
+The protocols that the HTTP firehose can use is restricted by `druid.ingestion.http.allowedProtocols`.
Review comment:
```suggestion
You can only use protocols listed in the `druid.ingestion.http.allowedProtocols` property as HTTP firehose input sources.
```
##########
File path: docs/configuration/index.md
##########
@@ -515,6 +515,36 @@ This deep storage is used to interface with Cassandra. Note that the `druid-cas
|`druid.storage.keyspace`|Cassandra key space.|none|
+### Ingestion Security Configuration
+
+#### HDFS input source
+
+You can set the following property to control what protocols are allowed for
+the [HDFS input source](../ingestion/native-batch.md#hdfs-input-source) and the [HDFS firehose](../ingestion/native-batch.md#hdfsfirehose).
+
+|Property|Possible Values|Description|Default|
+|--------|---------------|-----------|-------|
+|`druid.ingestion.hdfs.allowedProtocols`|List of protocols|Allowed protocols that HDFS input source and HDFS firehose can use.|["hdfs"]|
+
+
+#### HTTP input source
+
+You can set the following property to control what protocols are allowed for
+the [HTTP input source](../ingestion/native-batch.md#http-input-source) and the [HTTP firehose](../ingestion/native-batch.md#httpfirehose).
+
+|Property|Possible Values|Description|Default|
+|--------|---------------|-----------|-------|
+|`druid.ingestion.http.allowedProtocols`|List of protocols|Allowed protocols that HTTP input source and HTTP firehose can use.|["http", "https"]|
Review comment:
```suggestion
|`druid.ingestion.http.allowedProtocols`|List of protocols|Allowed protocols for the HTTP input source and HTTP firehose.|["http", "https"]|
```
##########
File path: docs/ingestion/native-batch.md
##########
@@ -1203,10 +1204,13 @@ You can also use the other existing Druid PasswordProviders. Here is an example
|property|description|default|required?|
|--------|-----------|-------|---------|
|type|This should be `http`|None|yes|
-|uris|URIs of the input files.|None|yes|
+|uris|URIs of the input files. See below for the protocols allowed for URIs.|None|yes|
|httpAuthenticationUsername|Username to use for authentication with specified URIs. Can be optionally used if the URIs specified in the spec require a Basic Authentication Header.|None|no|
|httpAuthenticationPassword|PasswordProvider to use with specified URIs. Can be optionally used if the URIs specified in the spec require a Basic Authentication Header.|None|no|
+The protocols that the HTTP input source can use is restricted by `druid.ingestion.http.allowedProtocols`.
Review comment:
```suggestion
You can only use protocols listed in the `druid.ingestion.http.allowedProtocols` property as HTTP input sources.
```
##########
File path: docs/ingestion/native-batch.md
##########
@@ -1127,9 +1127,10 @@ Sample specs:
|type|This should be `hdfs`.|None|yes|
|paths|HDFS paths. Can be either a JSON array or comma-separated string of paths. Wildcards like `*` are supported in these paths. Empty files located under one of the given paths will be skipped.|None|yes|
-You can also ingest from cloud storage using the HDFS input source.
-However, if you want to read from AWS S3 or Google Cloud Storage, consider using
-the [S3 input source](#s3-input-source) or the [Google Cloud Storage input source](#google-cloud-storage-input-source) instead.
+You can also ingest from other storage using the HDFS input source if the HDFS client supports that storage.
+However, if you want to ingest from cloud storage, consider using the proper input sources for them.
Review comment:
```suggestion
However, if you want to ingest from cloud storage, consider using the service-specific input source for your cloud storage.
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #10830: HTTP inputSource and firehose should support only HTTP and HTTPS; HDFS inputSource and firehose should support only HDFS
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #10830:
URL: https://github.com/apache/druid/pull/10830#issuecomment-772622689
Added `Release Notes` as there is a subtle change in behavior that users should be aware of when they upgrade.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] a2l007 commented on a change in pull request #10830: HTTP inputSource and firehose should support only HTTP and HTTPS; HDFS inputSource and firehose should support only HDFS
Posted by GitBox <gi...@apache.org>.
a2l007 commented on a change in pull request #10830:
URL: https://github.com/apache/druid/pull/10830#discussion_r571154198
##########
File path: extensions-core/hdfs-storage/src/main/java/org/apache/druid/inputsource/hdfs/HdfsInputSource.java
##########
@@ -101,20 +102,25 @@ public HdfsInputSource(
return paths;
}
- public static Collection<Path> getPaths(List<String> inputPaths, Configuration configuration) throws IOException
+ public static Collection<Path> getPaths(List<String> inputPathStrings, Configuration configuration) throws IOException
{
- if (inputPaths.isEmpty()) {
+ if (inputPathStrings.isEmpty()) {
return Collections.emptySet();
}
// Use FileInputFormat to read splits. To do this, we need to make a fake Job.
Job job = Job.getInstance(configuration);
// Add paths to the fake JobContext.
- for (String inputPath : inputPaths) {
+ for (String inputPath : inputPathStrings) {
FileInputFormat.addInputPaths(job, inputPath);
}
+ final Path[] inputPaths = FileInputFormat.getInputPaths(job);
+ if (Arrays.stream(inputPaths).anyMatch(path -> !"hdfs".equalsIgnoreCase(path.toUri().getScheme()))) {
+ throw new IllegalArgumentException("Input paths must be the HDFS path");
+ }
+
Review comment:
@jihoonson Technically `WebHdfsFileSystem` did work with this inputsource before, and so it could break ingestion pipelines for operators relying on HDFS Inputsource with `webhdfs` scheme. Could you please comment on what is the motivation behind restricting to only `hdfs`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10830: Allow only HTTP and HTTPS protocols for the HTTP inputSource
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10830:
URL: https://github.com/apache/druid/pull/10830#discussion_r568440331
##########
File path: core/src/main/java/org/apache/druid/data/input/impl/HttpInputSource.java
##########
@@ -69,6 +70,13 @@ public HttpInputSource(
this.config = config;
}
+ private static void throwIfInvalidProtocols(List<URI> uris)
+ {
+ if (uris.stream().anyMatch(uri -> !"http".equals(uri.getScheme()) && !"https".equals(uri.getScheme()))) {
Review comment:
```suggestion
if (uris.stream().anyMatch(uri -> !"http".equalsIgnoreCase(uri.getScheme()) && !"https".equalsIgnoreCase(uri.getScheme()))) {
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #10830: Add configurations for allowed protocols for HTTP and HDFS inputSources/firehoses
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10830:
URL: https://github.com/apache/druid/pull/10830#issuecomment-777975206
@techdocsmith thanks for the review :+1:
> Overall, the defaults look reasonable to me. I looked through other InputSources to see if we need a similar change, and didn't find any.
@suneet-s I agree. I don't think other inputSources have the same issue.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10830: Allow only HTTP and HTTPS protocols for the HTTP inputSource
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10830:
URL: https://github.com/apache/druid/pull/10830#discussion_r568440331
##########
File path: core/src/main/java/org/apache/druid/data/input/impl/HttpInputSource.java
##########
@@ -69,6 +70,13 @@ public HttpInputSource(
this.config = config;
}
+ private static void throwIfInvalidProtocols(List<URI> uris)
+ {
+ if (uris.stream().anyMatch(uri -> !"http".equals(uri.getScheme()) && !"https".equals(uri.getScheme()))) {
Review comment:
```suggestion
if (uris.stream().anyMatch(uri -> !"http".equalsIgnoreCase(uri.getScheme()) && !"https".equalsIgnoreCase(uri.getScheme()))) {
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org