paul-rogers commented on code in PR #13989:
URL: https://github.com/apache/druid/pull/13989#discussion_r1151107789
##########
extensions-core/s3-extensions/src/test/java/org/apache/druid/catalog/model/table/S3InputSourceDefnTest.java:
##########
@@ -345,6 +345,7 @@ public void testAdHocUri()
CatalogUtils.stringListToUriList(uris),
s3InputSource.getUris()
);
+ assertEquals(Collections.singleton(S3StorageDruidModule.SCHEME),
externSpec.inputSourceTypes);
Review Comment:
While this is OK, it muddies the semantics. There should be only one input
source type, not a list. Yes, a list with one item is the same as a single
value, but the list suggests that there could be more values, which there
cannot be.
Second, the input source type should be the value returned from
`typeValue()` that happens to be the same as the `SCHEME` constant, but that is
an implementation detail.
Actually, the core problem here may be the lack of a `TYPE_KEY` constant in
`S3InputSourceDefn`. Go ahead and add one, then use it here:
```java
public class S3InputSourceDefn extends FormattedInputSourceDefn
{
public static final String TYPE_KEY = S3StorageDruidModule.SCHEME;
...
@Override
public String typeValue()
{
return TYPE_KEY;
}
`
##########
server/src/main/java/org/apache/druid/server/security/AuthConfig.java:
##########
@@ -97,6 +97,9 @@ public AuthConfig()
@JsonProperty
private final Set<String> securedContextKeys;
+ @JsonProperty
+ private final boolean inputSourceTypeSecurityEnabled;
Review Comment:
Maybe `enableInputSourceSecurity` shorter. Starts with a verb.
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteInsertDmlTest.java:
##########
@@ -351,6 +373,94 @@ public void testInsertFromExternalWithSchema()
.verify();
}
+ @Test
+ public void testInsertFromExternalWithSchemaWithInputsourceSecurity()
+ {
+ String extern;
+ ObjectMapper queryJsonMapper = queryFramework().queryJsonMapper();
+ try {
+ extern = StringUtils.format(
+ "TABLE(extern(%s, %s))",
+ Calcites.escapeStringLiteral(
+ queryJsonMapper.writeValueAsString(
+ new InlineInputSource("a,b,1\nc,d,2\n")
+ )
+ ),
+ Calcites.escapeStringLiteral(
+ queryJsonMapper.writeValueAsString(
+ new CsvInputFormat(ImmutableList.of("x", "y", "z"), null,
false, false, 0)
+ )
+ )
+ );
+ }
+ catch (JsonProcessingException e) {
+ throw new RuntimeException(e);
+ }
+ testIngestionQuery()
+ .sql("INSERT INTO dst SELECT * FROM %s\n" +
+ " (x VARCHAR, y VARCHAR, z BIGINT)\n" +
+ "PARTITIONED BY ALL TIME",
+ extern
+ )
+ .authentication(CalciteTests.SUPER_USER_AUTH_RESULT)
+
.authConfig(AuthConfig.newBuilder().setInputSourceTypeSecurityEnabled(true).build())
+ .expectTarget("dst", externalDataSource.getSignature())
+ .expectResources(dataSourceWrite("dst"), externalRead("inline"))
+ .expectQuery(
+ newScanQueryBuilder()
+ .dataSource(externalDataSource)
+ .intervals(querySegmentSpec(Filtration.eternity()))
+ .columns("x", "y", "z")
+ .context(PARTITIONED_BY_ALL_TIME_QUERY_CONTEXT)
+ .build()
+ )
+ .expectLogicalPlanFrom("insertFromExternal")
+ .verify();
+ }
+
+ @Test
+ public void
testInsertFromExternalFunctionalStyleWithSchemaWithInputsourceSecurity()
+ {
+ String extern;
+ ObjectMapper queryJsonMapper = queryFramework().queryJsonMapper();
+ try {
+ extern = StringUtils.format(
+ "TABLE(extern("
+ + "inputSource => '%s',"
+ + "inputFormat => '%s'))",
+ queryJsonMapper.writeValueAsString(
+ new InlineInputSource("a,b,1\nc,d,2\n")
+ ),
+ queryJsonMapper.writeValueAsString(
+ new CsvInputFormat(ImmutableList.of("x", "y", "z"), null, false,
false, 0)
+ )
+ );
+ }
+ catch (JsonProcessingException e) {
+ throw new RuntimeException(e);
+ }
+ testIngestionQuery()
+ .sql("INSERT INTO dst SELECT * FROM %s\n" +
+ " (x VARCHAR, y VARCHAR, z BIGINT)\n" +
+ "PARTITIONED BY ALL TIME",
+ extern
+ )
+ .authentication(CalciteTests.SUPER_USER_AUTH_RESULT)
+
.authConfig(AuthConfig.newBuilder().setInputSourceTypeSecurityEnabled(true).build())
+ .expectTarget("dst", externalDataSource.getSignature())
+ .expectResources(dataSourceWrite("dst"), externalRead("inline"))
+ .expectQuery(
+ newScanQueryBuilder()
+ .dataSource(externalDataSource)
+ .intervals(querySegmentSpec(Filtration.eternity()))
+ .columns("x", "y", "z")
+ .context(PARTITIONED_BY_ALL_TIME_QUERY_CONTEXT)
+ .build()
+ )
+ .expectLogicalPlanFrom("insertFromExternal")
Review Comment:
Should we include a test in which authorization fails?
##########
server/src/main/java/org/apache/druid/catalog/model/table/BaseInputSourceDefn.java:
##########
@@ -151,7 +152,8 @@ protected ExternalTableSpec convertArgsToTable(
return new ExternalTableSpec(
convertArgsToSource(args, jsonMapper),
convertArgsToFormat(args, columns, jsonMapper),
- Columns.convertSignature(columns)
+ Columns.convertSignature(columns),
+ Collections.singleton(typeValue())
Review Comment:
Again, there can only ever be a single value.
##########
sql/src/main/java/org/apache/druid/sql/calcite/external/DruidExternTableMacro.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.druid.sql.calcite.external;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.JsonNode;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlCharStringLiteral;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.util.NlsString;
+import org.apache.druid.server.security.Action;
+import org.apache.druid.server.security.Resource;
+import org.apache.druid.server.security.ResourceAction;
+import org.apache.druid.server.security.ResourceType;
+import org.apache.druid.sql.calcite.table.DruidTable;
+
+import javax.validation.constraints.NotNull;
+import java.util.Collections;
+import java.util.Set;
+
+/**
+ * Used by {@link ExternalOperatorConversion} to generate a {@link DruidTable}
+ * that references an {@link ExternalDataSource}.
+ */
+public class DruidExternTableMacro extends DruidUserDefinedTableMacro
+{
+ public DruidExternTableMacro(DruidTableMacro macro)
+ {
+ super(macro);
+ }
+
+ @Override
+ public Set<ResourceAction> computeResources(final SqlCall call, boolean
inputSourceTypeSecurityEnabled)
+ {
+ if (!inputSourceTypeSecurityEnabled) {
+ return Collections.singleton(Externals.EXTERNAL_RESOURCE_ACTION);
+ }
+ String inputSourceStr = getInputSourceArgument(call);
+
+ try {
+ JsonNode jsonNode = ((DruidTableMacro)
macro).getJsonMapper().readTree(inputSourceStr);
+ return Collections.singleton(new ResourceAction(new Resource(
+ ResourceType.EXTERNAL,
+ jsonNode.get("type").asText()
+ ), Action.READ));
+ }
+ catch (JsonProcessingException e) {
+ // this shouldn't happen, the input source paraemeter should have been
validated before this
+ throw new RuntimeException(e);
+ }
+ }
+
+ @NotNull
+ private String getInputSourceArgument(final SqlCall call)
+ {
+ // this covers case where parameters are used positionally
+ if (call.getOperandList().size() > 0) {
+ if (call.getOperandList().get(0) instanceof SqlCharStringLiteral) {
+ return ((SqlCharStringLiteral) call.getOperandList().get(0)).toValue();
+ }
+ }
+
+ // this covers case where named parameters are used.
+ for (SqlNode sqlNode : call.getOperandList()) {
+ if (sqlNode instanceof SqlCall) {
+ String argumentName = ((SqlCall) sqlNode).getOperandList().size() > 1 ?
+ ((SqlCall)
sqlNode).getOperandList().get(1).toString()
+ : null;
+ if
(ExternalOperatorConversion.INPUT_SOURCE_PARAM.equals(argumentName)) {
+ return ((NlsString) ((SqlCharStringLiteral) ((SqlCall)
call.getOperandList().get(0))
+ .getOperandList()
+ .get(0))
+ .getValue())
+ .getValue();
+ }
+ }
+ }
+ // this shouldn't happen, as the sqlCall should have been validated by
this point,
+ // and be guarenteed to have this parameter.
+ throw new RuntimeException("inputSource paramter not found in extern
function");
Review Comment:
paramter -> parameter
##########
server/src/main/java/org/apache/druid/catalog/model/table/ExternalTableSpec.java:
##########
@@ -36,14 +37,17 @@
public final InputSource inputSource;
public final InputFormat inputFormat;
@Nullable public final RowSignature signature;
+ public final Set<String> inputSourceTypes;
Review Comment:
Again, there can only ever be a single value.
##########
sql/src/main/java/org/apache/druid/sql/calcite/external/ExternalTableMacro.java:
##########
Review Comment:
Please remove it if it is not used.
##########
sql/src/main/java/org/apache/druid/sql/calcite/external/Externals.java:
##########
@@ -290,15 +294,19 @@ public static ExternalTable
buildExternalTable(ExternalTableSpec spec, ObjectMap
+ "Please change the column name to something other than
__time");
}
- return toExternalTable(spec, jsonMapper);
+ return toExternalTable(spec, jsonMapper, spec.inputSourceTypes);
}
public static ResourceAction externalRead(String name)
{
return new ResourceAction(new Resource(name, ResourceType.EXTERNAL),
Action.READ);
}
- public static ExternalTable toExternalTable(ExternalTableSpec spec,
ObjectMapper jsonMapper)
+ public static ExternalTable toExternalTable(
+ ExternalTableSpec spec,
+ ObjectMapper jsonMapper,
+ Set<String> inputSourceTypes
Review Comment:
There will only ever be one input source type.
--
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]
