mayankshriv commented on code in PR #18660:
URL: https://github.com/apache/pinot/pull/18660#discussion_r3352345382
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/FileIngestionHelper.java:
##########
@@ -176,14 +179,45 @@ 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(allowLocalFileSystem ||
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)) {
Review Comment:
Minor: `PluginManager.get().loadClass(pluginName, rawClassName)` will
trigger static initializers for whatever class name the caller provides in
`input.fs.className` before the `isAssignableFrom` check rejects it. Consider a
fast string-based pre-check (e.g., class name ends with `LocalPinotFS`) to
avoid loading attacker-controlled classes unnecessarily.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFSFactory.java:
##########
@@ -83,6 +83,14 @@ public static boolean isSchemeSupported(String scheme) {
return PINOT_FS_MAP.containsKey(scheme);
}
+ public static boolean isSchemeRegisteredWith(String scheme, Class<? extends
PinotFS> pinotFSClass) {
+ PinotFS pinotFS = PINOT_FS_MAP.get(scheme);
Review Comment:
Nit: an explicit null guard (`if (pinotFS == null) return false;`) before
the `instanceof` check would make the intent clearer, even though
`isInstance(null)` returns false.
##########
pinot-controller/src/test/java/org/apache/pinot/controller/util/FileIngestionHelperTest.java:
##########
@@ -0,0 +1,113 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller.util;
+
+import java.io.File;
+import java.net.URI;
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.filesystem.LocalPinotFS;
+import org.apache.pinot.spi.filesystem.PinotFSFactory;
+import org.apache.pinot.spi.ingestion.batch.BatchConfigProperties;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.expectThrows;
+
+
+@Test(groups = "stateless")
+public class FileIngestionHelperTest {
+ private static final File DEST_FILE = new File("unused");
+
+ @Test
+ public void testRejectsLocalFileUriWhenLocalFileSystemIsDisabled() {
+ IllegalArgumentException exception =
expectThrows(IllegalArgumentException.class,
+ () -> FileIngestionHelper.copyURIToLocal(new HashMap<>(),
URI.create("file:///tmp/input.csv"), DEST_FILE,
+ false));
+
+ assertTrue(exception.getMessage().contains("Local filesystem URIs are not
allowed"));
+ }
+
+ @Test
+ public void testRejectsSchemeLessUriWhenLocalFileSystemIsDisabled() {
+ IllegalArgumentException exception =
expectThrows(IllegalArgumentException.class,
+ () -> FileIngestionHelper.copyURIToLocal(new HashMap<>(),
URI.create("/tmp/input.csv"), DEST_FILE, false));
+
+ assertTrue(exception.getMessage().contains("Source URI must include a
scheme"));
+ }
+
+ @Test
+ public void testPreservesSchemeLessUriHandlingWhenLocalFileSystemIsEnabled()
{
+ IllegalArgumentException exception =
expectThrows(IllegalArgumentException.class,
+ () -> FileIngestionHelper.copyURIToLocal(new HashMap<>(),
URI.create("/tmp/input.csv"), DEST_FILE, true));
+
+ assertTrue(exception.getMessage().contains("Must provide
input.fs.className"));
+ }
+
+ @Test
+ public void
testRejectsLocalFileSystemClassForCustomSchemeWhenLocalFileSystemIsDisabled() {
+ Map<String, String> batchConfigMap = new HashMap<>();
+ batchConfigMap.put(BatchConfigProperties.INPUT_FS_CLASS,
LocalPinotFS.class.getName());
+
+ IllegalArgumentException exception =
expectThrows(IllegalArgumentException.class,
+ () -> FileIngestionHelper.copyURIToLocal(batchConfigMap,
URI.create("customlocalfstest:///tmp/input.csv"),
+ DEST_FILE, false));
+
+ assertTrue(exception.getMessage().contains("Local filesystem
implementation is not allowed"));
+ }
+
+ @Test
+ public void
testRejectsLegacyLocalFileSystemClassForCustomSchemeWhenLocalFileSystemIsDisabled()
{
+ Map<String, String> batchConfigMap = new HashMap<>();
Review Comment:
Nice coverage of the rejection paths. One thing missing: a positive test
that `file:///` URIs actually work end-to-end when `allowLocalFileSystem=true`
(the existing `PinotIngestionRestletResourceStatelessTest` covers it
indirectly, but a unit-level happy-path test here would be more explicit).
--
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]
