Copilot commented on code in PR #18660:
URL: https://github.com/apache/pinot/pull/18660#discussion_r3345447622
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/FileIngestionHelper.java:
##########
@@ -176,14 +179,39 @@ public SuccessResponse buildSegmentAndPush(DataPayload
payload)
*/
public static void copyURIToLocal(Map<String, String> batchConfigMap, URI
sourceFileURI, File destFile)
throws Exception {
+ copyURIToLocal(batchConfigMap, sourceFileURI, destFile, true);
+ }
+
+ public static void copyURIToLocal(Map<String, String> batchConfigMap, URI
sourceFileURI, File destFile,
+ boolean allowLocalFileSystem)
+ throws Exception {
String sourceFileURIScheme = sourceFileURI.getScheme();
+ Preconditions.checkArgument(StringUtils.isNotBlank(sourceFileURIScheme),
+ "Source URI must include a scheme: %s", sourceFileURI);
+ Preconditions.checkArgument(allowLocalFileSystem
+ ||
!PinotFSFactory.LOCAL_PINOT_FS_SCHEME.equalsIgnoreCase(sourceFileURIScheme),
+ "Local filesystem URIs are not allowed for /ingestFromURI: %s",
sourceFileURI);
if (!PinotFSFactory.isSchemeSupported(sourceFileURIScheme)) {
- PinotFSFactory.register(sourceFileURIScheme,
batchConfigMap.get(BatchConfigProperties.INPUT_FS_CLASS),
+ String inputFsClassName =
batchConfigMap.get(BatchConfigProperties.INPUT_FS_CLASS);
+ Preconditions.checkArgument(StringUtils.isNotBlank(inputFsClassName),
+ "Must provide %s for unsupported scheme: %s",
BatchConfigProperties.INPUT_FS_CLASS, sourceFileURIScheme);
+ Preconditions.checkArgument(allowLocalFileSystem ||
!isLocalPinotFSClass(inputFsClassName),
+ "Local filesystem implementation is not allowed for /ingestFromURI:
%s", inputFsClassName);
+ PinotFSFactory.register(sourceFileURIScheme, inputFsClassName,
IngestionConfigUtils.getInputFsProps(batchConfigMap));
}
PinotFSFactory.create(sourceFileURIScheme).copyToLocalFile(sourceFileURI,
destFile);
Review Comment:
When allowLocalFileSystem=false, the code only blocks the built-in local
filesystem by checking the URI scheme against "file" and by preventing *new*
registrations of LocalPinotFS for unsupported schemes. However, if a non-"file"
scheme is already registered to LocalPinotFS (e.g., via PinotFSFactory.init
config, or by a prior /ingestFromURI call when local FS was enabled that
registered a custom scheme), this path will still allow local file reads
because `isSchemeSupported()` short-circuits the LocalPinotFS class check. This
can defeat the intent of disabling local filesystem ingestion.
--
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]
