dongjoon-hyun commented on code in PR #58604:
URL: https://github.com/apache/spark/pull/58604#discussion_r3955464252
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -2676,6 +2676,18 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ val RESTRICTED_MODE_ENABLED = buildConf("spark.sql.restrictedMode.enabled")
+ .internal()
+ .doc("When true, SQL features that load or execute externally provided
code, scripts, or " +
+ "jars from the query itself are disabled: the reflect and java_method
functions and the " +
+ "TRANSFORM ... USING clause are rejected. This is an opt-in profile for
endpoints that run " +
+ "queries from many users; it composes with the individual data-source
option controls " +
+ "(for example spark.sql.kafka.disallowedOptions and the avro schema-URL
scheme allowlist). " +
Review Comment:
`spark.sql.kafka.disallowedOptions` does not exist anywhere in the
repository -- this doc string is the only occurrence. I could not find an "avro
schema-URL scheme allowlist" either.
Could you drop these examples, or point at configs that actually exist?
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/RestrictedModeSuite.scala:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.spark.sql.catalyst.analysis
+
+import org.apache.spark.sql.AnalysisException
+import org.apache.spark.sql.catalyst.expressions.{AttributeReference,
CallMethodViaReflection, Literal}
+import org.apache.spark.sql.catalyst.plans.logical.{ScriptInputOutputSchema,
ScriptTransformation}
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.types.StringType
+
+class RestrictedModeSuite extends AnalysisTest {
+
+ private val restricted = SQLConf.RESTRICTED_MODE_ENABLED.key
+
+ private def reflectCall: CallMethodViaReflection =
+ CallMethodViaReflection(Seq(Literal("java.lang.Math"), Literal("abs"),
Literal(-1)))
+
+ private def transformPlan: ScriptTransformation =
+ ScriptTransformation(
+ "cat",
+ Seq(AttributeReference("value", StringType)()),
+ TestRelations.testRelation,
+ ScriptInputOutputSchema(Nil, Nil, None, None, Nil, Nil, None, None,
schemaLess = false))
+
+ test("reflect/java_method are rejected only when restricted mode is
enabled") {
+ withSQLConf(restricted -> "true") {
+ val e = intercept[AnalysisException](reflectCall.checkInputDataTypes())
+ assert(e.getMessage.contains("restrictedMode.enabled"))
Review Comment:
A substring match on the message verifies neither the new error condition
name nor its parameters -- it would still pass if the error came from an
unrelated code path that happened to mention the config. `checkError` is the
convention here (82 uses in this test directory alone):
```scala
checkError(
exception = e,
condition = "UNSUPPORTED_FEATURE.SQL_RESTRICTED_MODE",
parameters = Map(
"feature" -> "...",
"config" -> "\"spark.sql.restrictedMode.enabled\""))
```
Same for the `TRANSFORM` assertion below.
Two cases also look worth adding: `try_reflect` (blocked via the
`TryReflect` replacement, but untested), and an end-to-end SQL test for `SELECT
TRANSFORM ... USING` -- the current test builds the `ScriptTransformation` node
directly, so it would not catch a plan shape that reaches execution without
passing through this `CheckAnalysis` traversal.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -375,6 +375,10 @@ trait CheckAnalysis extends LookupCatalog with
QueryErrorsBase with PlanToString
plan.foreachUp {
case p if p.analyzed => // Skip already analyzed sub-plans
+ case _: ScriptTransformation if SQLConf.get.restrictedModeEnabled =>
Review Comment:
This case sits below `case p if p.analyzed => // Skip already analyzed
sub-plans`, and `AnalysisHelper.setAnalyzed()` marks children recursively:
```scala
private[sql] def setAnalyzed(): Unit = {
...
children.foreach(_.setAnalyzed())
}
```
So a `ScriptTransformation` that lives inside an already-analyzed sub-plan
never reaches this case. That covers DataFrame-backed temp views
(`createOrReplaceTempView` stores the analyzed plan) and reused `Dataset`s, so
a view created while the mode was off can still be queried after the mode is
turned on.
The `reflect` gate has the analogous problem: `checkInputDataTypes()` is
reached through the `Expression.resolved` lazy val, so the conf value is
memoized at first resolution.
If the mode is meant to be a boundary rather than a hint, the check probably
needs to run somewhere that pre-analyzed plans cannot skip. At minimum it is
worth calling out the limitation.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CallMethodViaReflection.scala:
##########
@@ -84,6 +84,10 @@ case class CallMethodViaReflection(
override def prettyName: String =
getTagValue(FunctionRegistry.FUNC_ALIAS).getOrElse("reflect")
override def checkInputDataTypes(): TypeCheckResult = {
+ if (SQLConf.get.restrictedModeEnabled) {
+ throw QueryCompilationErrors.restrictedModeFeatureError(
+ "The reflect and java_method functions")
Review Comment:
Two issues with this string.
**Grammar.** Substituted into `"<feature> is not allowed because <config> is
enabled."` and prefixed by the `UNSUPPORTED_FEATURE` parent message, users see:
> The feature is not supported: The reflect and java_method function**s**
**is** not allowed because "spark.sql.restrictedMode.enabled" is enabled.
**`try_reflect` is missing.** `TryReflect` replaces itself with
`CallMethodViaReflection(params, failOnError = false)`, so `try_reflect` is
blocked by this check too, but the message never mentions it.
Using `prettyName` fixes both at once, since `prettyName` already resolves
to `reflect` / `java_method` / `try_reflect` via `FUNC_ALIAS`:
```scala
throw QueryCompilationErrors.restrictedModeFeatureError(
s"The ${toSQLId(prettyName)} function")
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -2676,6 +2676,18 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ val RESTRICTED_MODE_ENABLED = buildConf("spark.sql.restrictedMode.enabled")
Review Comment:
This should very likely be `buildStaticConf` rather than `buildConf`.
The doc below describes this as "an opt-in profile for endpoints that run
queries from many users", but as a runtime `SQLConf` any of those users can
disable the whole thing with
```sql
SET spark.sql.restrictedMode.enabled=false;
```
`internal()` only hides the config from the docs and `SET -v`; it does not
make it read-only. Spark already uses `buildStaticConf` for exactly this kind
of setting -- see
`spark.sql.legacy.hive.thriftServer.allowSettingSystemProperties` and
`spark.sql.hive.thriftServer.http.sniHostCheckEnabled` in
`StaticSQLConf.scala`. With a static conf, `RuntimeConfig.requireNonStaticConf`
rejects the `SET`.
As written, the gate can be turned off by the very users it is meant to
constrain, which undercuts the rest of the PR.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -329,6 +329,14 @@ private[sql] object QueryCompilationErrors extends
QueryErrorsBase with Compilat
messageParameters = Map.empty)
}
+ def restrictedModeFeatureError(feature: String): Throwable = {
+ new AnalysisException(
+ errorClass = "UNSUPPORTED_FEATURE.SQL_RESTRICTED_MODE",
+ messageParameters = Map(
+ "feature" -> feature,
+ "config" -> toSQLConfVal(SQLConf.RESTRICTED_MODE_ENABLED.key)))
Review Comment:
This is a config *key*, so it should be `toSQLConf`, not `toSQLConfVal`. The
convention is spelled out in `QueryErrorsBase`'s own header comment:
> 5. SQL configs ... For example: "spark.sql.ansi.enabled".
> 6. Any values of datasource options or SQL configs shall be double quoted.
For example: "true", "CORRECTED".
`Count.scala` shows the correct pairing:
```scala
toSQLConf(SQLConf.ALLOW_PARAMETERLESS_COUNT.key),
toSQLConfVal(true.toString))
```
Both helpers happen to emit the same double quotes today, so this is not a
behavior change -- but the two names mean opposite things.
Separately, on the error condition itself: `<feature>` taking free-form
English prose is the only such parameter in `error-conditions.json`. Splitting
into per-feature sub-conditions would match how the rest of the file is written.
--
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]
