You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "HyukjinKwon (via GitHub)" <gi...@apache.org> on 2026/09/08 12:44:33 UTC
[PR] [SPARK-59329][SQL] Add an allowlist of URI schemes for the Avro schemaUrl option [spark]
HyukjinKwon opened a new pull request, #58614:
URL: https://github.com/apache/spark/pull/58614
### What changes were proposed in this pull request?
Add an opt-in `spark.sql.avro.schemaUrlAllowedSchemes` (internal, default empty = no
restriction). When non-empty, an `avroSchemaUrl` whose resolved URI scheme is not in the
allowlist is rejected before the schema is opened. A scheme-less path is resolved against the
default file system before its scheme is checked, so it can be permitted by allowing that file
system's scheme.
### Why are the changes needed?
Gives operators an optional control over which URI schemes are accepted for the `avroSchemaUrl`
option. Empty by default, so there is no behavior change unless configured.
### Does this PR introduce _any_ user-facing change?
No by default. When `spark.sql.avro.schemaUrlAllowedSchemes` is set, an `avroSchemaUrl` whose
scheme is not listed is rejected.
### How was this patch tested?
New `AvroSuite` tests: an allowed scheme is permitted (including a scheme-less local path that
resolves to `file`), a scheme not in the allowlist is rejected, and the empty default imposes no
restriction.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 4.8
This pull request and its description were written by Isaac.
Co-authored-by: Isaac <no...@databricks.com>
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59329][SQL] Add an allowlist of URI schemes for the Avro schemaUrl option [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #58614:
URL: https://github.com/apache/spark/pull/58614#discussion_r3994452810
##########
connector/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala:
##########
@@ -1255,6 +1255,64 @@ abstract class AvroSuite
assertExceptionMsg[FileNotFoundException](e, "File not_exists.avsc does not exist")
}
+ // spark.sql.avro.schemaUrlAllowedSchemes is a static SQL config, so it cannot be set with
Review Comment:
Done in 9655f9e. Added `AvroSchemaUrlAllowlistSuite` (`QueryTest with SharedSparkSession`, overriding `sparkConf` to set the static allowlist) that drives a real `spark.read ... load()`: an allowed scheme is permitted, a disallowed scheme is rejected, and a session cannot relax it (`CANNOT_MODIFY_STATIC_CONFIG` via both `spark.conf.set` and SQL `SET`). Kept the direct-`AvroOptions` cases in `AvroSuite` on top of it.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -7016,6 +7016,20 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ val AVRO_SCHEMA_URL_ALLOWED_SCHEMES =
+ buildStaticConf("spark.sql.avro.schemaUrlAllowedSchemes")
Review Comment:
Kept it with the other `spark.sql.avro.*` entries and added a comment noting it is static; `buildStaticConf` registers the static key wherever it is declared. 9655f9e.
##########
connector/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala:
##########
@@ -1255,6 +1255,64 @@ abstract class AvroSuite
assertExceptionMsg[FileNotFoundException](e, "File not_exists.avsc does not exist")
}
+ // spark.sql.avro.schemaUrlAllowedSchemes is a static SQL config, so it cannot be set with
+ // withSQLConf; these drive AvroOptions directly under a SQLConf provided via withExistingConf.
+ // A scheme-less local path resolves to the default file system ("file").
+ test("SPARK-59329: avroSchemaUrl scheme allowlist permits an allowed scheme") {
+ val avroSchemaUrl = testFile("test_sub.avsc")
+ val hadoopConf = spark.sessionState.newHadoopConf()
+ val conf = new SQLConf()
+ conf.setConf(SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES, Seq("file"))
+ SQLConf.withExistingConf(conf) {
+ val options = new AvroOptions(Map("avroSchemaUrl" -> avroSchemaUrl), hadoopConf)
+ assert(options.schema.isDefined)
+ }
+ }
+
+ test("SPARK-59329: avroSchemaUrl allowlist rejects a disallowed scheme " +
+ "before opening the file system") {
+ // An explicit non-"file" scheme is rejected by the allowlist check, which runs before the
+ // file system for the URL is instantiated -- so this surfaces the clean allowlist error
+ // rather than a lower-level failure from trying to load the s3a file system.
+ val hadoopConf = spark.sessionState.newHadoopConf()
+ val conf = new SQLConf()
+ conf.setConf(SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES, Seq("file"))
+ SQLConf.withExistingConf(conf) {
+ val e = intercept[AnalysisException] {
+ new AvroOptions(Map("avroSchemaUrl" -> "s3a://bucket/user.avsc"), hadoopConf)
Review Comment:
Done in 9655f9e. The rejection test now uses `S3A://bucket/user.avsc` and asserts the message contains `The scheme 's3a'`, so dropping the `.toLowerCase(Locale.ROOT)` on the scheme fails 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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59329][SQL] Add an allowlist of URI schemes for the Avro schemaUrl option [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #58614:
URL: https://github.com/apache/spark/pull/58614#discussion_r3978941730
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -7016,6 +7016,19 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ val AVRO_SCHEMA_URL_ALLOWED_SCHEMES =
+ buildConf("spark.sql.avro.schemaUrlAllowedSchemes")
Review Comment:
**Finding 1.** This is `buildConf`, so the allowlist is a runtime SQL conf. Any session can turn it off with `SET spark.sql.avro.schemaUrlAllowedSchemes=` or `spark.conf.set(...)`, and the PR's own tests set it exactly that way through `withSQLConf`. So it does not constrain a user who can set the `avroSchemaUrl` option in the first place, which is the user that "allowlist" / "permitted" / "rejected" implies it is protecting against.
Two ways out, and which one is right depends on the threat model you have in mind:
- If the point is to constrain what users may reference, this needs `buildStaticConf`, so it can only be set when the `SparkSession` is created. The cost is that the tests can no longer use `withSQLConf` and would have to build a session per case.
- If the point is a guardrail against accidental misconfiguration rather than a boundary, the `.doc` should say that outright. An operator reading "rejected before it is opened" will deploy it as a boundary.
##########
sql/core/src/main/scala/org/apache/spark/sql/avro/AvroOptions.scala:
##########
@@ -76,6 +76,25 @@ private[sql] class AvroOptions(
parameters.get(AVRO_SCHEMA).map(AvroUtils.parseAvroSchema).orElse({
val avroUrlSchema = parameters.get(AVRO_SCHEMA_URL).map(url => {
log.debug("loading avro schema from url: " + url)
+ // Optional operator-configured allowlist of URI schemes for avroSchemaUrl. Empty by
+ // default, which permits any scheme and leaves the file-system resolution below unchanged.
+ // When set, the scheme is resolved and checked before the file system for the URL is
+ // instantiated, so a disallowed scheme is rejected with a clear error rather than a
+ // lower-level failure while opening it. A scheme-less URL takes the default file system's
+ // scheme, so it can be permitted by allowing that scheme.
+ val allowedSchemes = SQLConf.get.getConf(SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES)
Review Comment:
**Finding 3.** The control is per-option, so an operator who sets it still has the same shape open in XML. `rowValidationXSDPath` (`sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlOptions.scala:128`) is a user-supplied path that `StaxXmlParser` and `XmlInferSchema` hand to `ValidatorUtil.openSchemaFile`, which does `xsdPath.getFileSystem(SparkHadoopUtil.get.conf)` and `fs.open(xsdPath)` with no scheme check (`sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/ValidatorUtil.scala:51`).
I am not asking you to fix XML here. But if the intent is an operator-level restriction on which URIs a data source option may open, an Avro-only internal conf is a partial answer, and the next source will want its own conf under its own key. Worth a line in the description saying whether a shared helper is the direction, or whether Avro is deliberately the only case in scope.
##########
sql/core/src/main/scala/org/apache/spark/sql/avro/AvroOptions.scala:
##########
@@ -76,6 +76,25 @@ private[sql] class AvroOptions(
parameters.get(AVRO_SCHEMA).map(AvroUtils.parseAvroSchema).orElse({
val avroUrlSchema = parameters.get(AVRO_SCHEMA_URL).map(url => {
log.debug("loading avro schema from url: " + url)
+ // Optional operator-configured allowlist of URI schemes for avroSchemaUrl. Empty by
+ // default, which permits any scheme and leaves the file-system resolution below unchanged.
+ // When set, the scheme is resolved and checked before the file system for the URL is
+ // instantiated, so a disallowed scheme is rejected with a clear error rather than a
+ // lower-level failure while opening it. A scheme-less URL takes the default file system's
+ // scheme, so it can be permitted by allowing that scheme.
+ val allowedSchemes = SQLConf.get.getConf(SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES)
+ .map(_.toLowerCase(Locale.ROOT))
+ if (allowedSchemes.nonEmpty) {
+ val scheme = Option(new URI(url).getScheme)
Review Comment:
**Finding 4.** The `getOrElse("")` is unreachable. `FileSystem.getDefaultUri` throws `IllegalArgumentException("No scheme in default FS: ...")` rather than returning a scheme-less URI, so the `Option(...)` around it is never `None` (checked against the Hadoop 3.3.0 `FileSystem.java` sources; the method reads the same in later releases). While you are here, `new URI(url)` is built again at line 98 for `FileSystem.get`.
```scala
val uri = new URI(url)
...
val scheme = Option(uri.getScheme)
.getOrElse(FileSystem.getDefaultUri(conf).getScheme)
.toLowerCase(Locale.ROOT)
...
val fs = FileSystem.get(uri, conf)
```
##########
connector/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala:
##########
@@ -1255,6 +1255,65 @@ abstract class AvroSuite
assertExceptionMsg[FileNotFoundException](e, "File not_exists.avsc does not exist")
}
+ test("SPARK-59329: avroSchemaUrl scheme allowlist permits an allowed scheme") {
+ val avroSchemaUrl = testFile("test_sub.avsc")
+ // A scheme-less local path resolves to the default file system ("file"), so allowing
+ // "file" lets it through.
+ withSQLConf(SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES.key -> "file") {
+ val result = spark.read.option("avroSchemaUrl", avroSchemaUrl)
+ .format("avro")
+ .load(testAvro)
+ .collect()
+ val expected = spark.read.format("avro").load(testAvro).select("string").collect()
+ assert(result.sameElements(expected))
+ }
+ }
+
+ test("SPARK-59329: avroSchemaUrl scheme allowlist rejects a scheme not listed") {
+ val avroSchemaUrl = testFile("test_sub.avsc")
+ withSQLConf(SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES.key -> "s3a") {
+ val e = intercept[AnalysisException] {
+ spark.read.option("avroSchemaUrl", avroSchemaUrl)
+ .format("avro")
+ .load(testAvro)
+ .collect()
+ }
+ assert(e.getCondition == "STDS_INVALID_OPTION_VALUE.WITH_MESSAGE")
+ assert(e.getMessage.contains("avroSchemaUrl"))
+ assert(e.getMessage.contains("not in the allowlist"))
+ }
+ }
+
+ test("SPARK-59329: avroSchemaUrl allowlist rejects an explicit disallowed scheme " +
+ "before opening the file system") {
+ // An explicit non-"file" scheme is rejected by the allowlist check, which runs before the
+ // file system for the URL is instantiated -- so this surfaces the clean allowlist error
+ // rather than a lower-level failure from trying to load the s3a file system.
+ withSQLConf(SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES.key -> "file") {
+ val e = intercept[AnalysisException] {
+ spark.read.option("avroSchemaUrl", "s3a://bucket/user.avsc")
+ .format("avro")
+ .load(testAvro)
+ .collect()
+ }
+ assert(e.getCondition == "STDS_INVALID_OPTION_VALUE.WITH_MESSAGE")
+ assert(e.getMessage.contains("avroSchemaUrl"))
+ assert(e.getMessage.contains("not in the allowlist"))
+ assert(e.getMessage.contains("s3a"))
+ }
+ }
+
+ test("SPARK-59329: avroSchemaUrl scheme allowlist is disabled by default") {
Review Comment:
**Finding 2.** This is the existing `SPARK-34416: support user provided avro schema url` at `connector/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala:1212` with a comment added. Same `testFile("test_sub.avsc")`, same read, same `expected`, same assertion, and it passes on base for the same reason that one does.
The added code that nothing covers is the case folding of the conf value at `sql/core/src/main/scala/org/apache/spark/sql/avro/AvroOptions.scala:86`. Drop the `.map(_.toLowerCase(Locale.ROOT))` there and every test in this PR still passes. Swapping this test for that case covers the new code instead of repeating an old test:
```scala
test("SPARK-59329: avroSchemaUrl scheme allowlist is case-insensitive") {
val avroSchemaUrl = testFile("test_sub.avsc")
withSQLConf(SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES.key -> "FILE") {
val result = spark.read.option("avroSchemaUrl", avroSchemaUrl)
.format("avro").load(testAvro).collect()
val expected = spark.read.format("avro").load(testAvro).select("string").collect()
assert(result.sameElements(expected))
}
}
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59329][SQL] Add an allowlist of URI schemes for the Avro schemaUrl option [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #58614:
URL: https://github.com/apache/spark/pull/58614#discussion_r4001014258
##########
connector/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala:
##########
@@ -1255,6 +1255,66 @@ abstract class AvroSuite
assertExceptionMsg[FileNotFoundException](e, "File not_exists.avsc does not exist")
}
+ // spark.sql.avro.schemaUrlAllowedSchemes is a static SQL config, so it cannot be set with
+ // withSQLConf; these drive AvroOptions directly under a SQLConf provided via withExistingConf.
+ // A scheme-less local path resolves to the default file system ("file").
Review Comment:
Right -- `testFile` returns a `file:` URL, so nothing reached the default-FS branch. Added `SPARK-59329: avroSchemaUrl allowlist resolves a scheme-less path against the default file system` in 69dd3b5 (strips the scheme; permitted when `file` is allowed, rejected reporting `The scheme 'file'` otherwise), and corrected the misleading "scheme-less" comments and the description.
##########
sql/core/src/main/scala/org/apache/spark/sql/avro/AvroOptions.scala:
##########
@@ -76,7 +76,29 @@ private[sql] class AvroOptions(
parameters.get(AVRO_SCHEMA).map(AvroUtils.parseAvroSchema).orElse({
val avroUrlSchema = parameters.get(AVRO_SCHEMA_URL).map(url => {
log.debug("loading avro schema from url: " + url)
- val fs = FileSystem.get(new URI(url), conf)
+ val uri = new URI(url)
+ // Optional operator-configured allowlist of URI schemes for avroSchemaUrl. Empty by
+ // default, which permits any scheme and leaves the file-system resolution below unchanged.
+ // When set, the scheme is resolved and checked before the file system for the URL is
+ // instantiated, so a disallowed scheme is rejected with a clear error rather than a
+ // lower-level failure while opening it. A scheme-less URL takes the default file system's
+ // scheme, so it can be permitted by allowing that scheme.
+ val allowedSchemes = SQLConf.get.getConf(SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES)
+ .map(_.toLowerCase(Locale.ROOT))
+ if (allowedSchemes.nonEmpty) {
+ // FileSystem.getDefaultUri always carries a scheme (it throws otherwise), so a
+ // scheme-less URL resolves to the default file system's scheme.
+ val scheme = Option(uri.getScheme)
+ .getOrElse(FileSystem.getDefaultUri(conf).getScheme)
+ .toLowerCase(Locale.ROOT)
+ if (!allowedSchemes.contains(scheme)) {
+ throw QueryCompilationErrors.avroOptionsException(
+ AVRO_SCHEMA_URL,
+ s"The scheme '$scheme' of avroSchemaUrl '$url' is not in the allowlist " +
+ s"configured by ${SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES.key}.")
Review Comment:
Applied the suggestion in 69dd3b5 -- the message now echoes the parsed allowlist, e.g. `... is not in the allowlist [file://] configured by ...`. Both existing assertions still match.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59329][SQL] Add an allowlist of URI schemes for the Avro schemaUrl option [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on code in PR #58614:
URL: https://github.com/apache/spark/pull/58614#discussion_r3966420187
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -7016,6 +7016,19 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ val AVRO_SCHEMA_URL_ALLOWED_SCHEMES =
+ buildConf("spark.sql.avro.schemaUrlAllowedSchemes")
+ .internal()
+ .doc("A comma-separated allowlist of URI schemes permitted for the 'avroSchemaUrl' Avro " +
+ "option. Empty by default, which permits any scheme and preserves the previous behavior; " +
+ "when non-empty, an avroSchemaUrl whose scheme is not listed is rejected before it is " +
+ "opened.")
+ .version("4.3.0")
Review Comment:
Ty!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59329][SQL] Add an allowlist of URI schemes for the Avro schemaUrl option [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on code in PR #58614:
URL: https://github.com/apache/spark/pull/58614#discussion_r3959694379
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -7016,6 +7016,19 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ val AVRO_SCHEMA_URL_ALLOWED_SCHEMES =
+ buildConf("spark.sql.avro.schemaUrlAllowedSchemes")
+ .internal()
+ .doc("A comma-separated allowlist of URI schemes permitted for the 'avroSchemaUrl' Avro " +
+ "option. Empty by default, which permits any scheme and preserves the previous behavior; " +
+ "when non-empty, an avroSchemaUrl whose scheme is not listed is rejected before it is " +
+ "opened.")
+ .version("4.3.0")
Review Comment:
Just to double check, is this intended for 4.3?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59329][SQL] Add an allowlist of URI schemes for the Avro schemaUrl option [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #58614: [SPARK-59329][SQL] Add an allowlist of URI schemes for the Avro schemaUrl option
URL: https://github.com/apache/spark/pull/58614
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59329][SQL] Add an allowlist of URI schemes for the Avro schemaUrl option [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #58614:
URL: https://github.com/apache/spark/pull/58614#discussion_r4004067690
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -7016,6 +7016,22 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ // Kept here with the other `spark.sql.avro.*` entries rather than in `StaticSQLConf`, though it
+ // is static: `buildStaticConf` registers the static key wherever it is declared.
+ val AVRO_SCHEMA_URL_ALLOWED_SCHEMES =
+ buildStaticConf("spark.sql.avro.schemaUrlAllowedSchemes")
+ .internal()
+ .doc("A comma-separated allowlist of URI schemes permitted for the 'avroSchemaUrl' Avro " +
+ "option. Empty by default, which permits any scheme and preserves the previous behavior; " +
+ "when non-empty, an avroSchemaUrl whose scheme is not listed is rejected before it is " +
+ "opened. This is a static configuration fixed when the SparkSession is created and not " +
+ "modifiable at runtime, so it is an operator-level boundary that a session cannot relax.")
Review Comment:
Fixed. Took your suggested framing: the `.doc` (d325505) now says the allowlist restricts the scheme an `avroSchemaUrl` may name, not which file system serves it -- that is decided by `fs.<scheme>.impl`, which a session can still set. Dropped the "a session cannot relax it" claim from both the doc and the PR description. Separately, per finding 6, I moved the conf into `StaticSQLConf` next to the other 4.3.0 static allowlists (cb6ac21); `AvroSchemaUrlAllowlistSuite`'s `CANNOT_MODIFY_STATIC_CONFIG` case confirms the static registration still holds.
##########
connector/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala:
##########
@@ -1255,6 +1255,91 @@ abstract class AvroSuite
assertExceptionMsg[FileNotFoundException](e, "File not_exists.avsc does not exist")
}
+ // spark.sql.avro.schemaUrlAllowedSchemes is a static SQL config, so it cannot be set with
+ // withSQLConf; these drive AvroOptions directly under a SQLConf provided via withExistingConf.
+ // testFile returns a "file:" URL, so its scheme is an explicit "file"; the scheme-less path that
+ // resolves against the default file system is covered by its own test below.
+ test("SPARK-59329: avroSchemaUrl scheme allowlist permits an allowed scheme") {
+ val avroSchemaUrl = testFile("test_sub.avsc")
+ val hadoopConf = spark.sessionState.newHadoopConf()
+ val conf = new SQLConf()
+ conf.setConf(SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES, Seq("file"))
+ SQLConf.withExistingConf(conf) {
+ val options = new AvroOptions(Map("avroSchemaUrl" -> avroSchemaUrl), hadoopConf)
+ assert(options.schema.isDefined)
+ }
+ }
+
+ test("SPARK-59329: avroSchemaUrl allowlist rejects a disallowed scheme " +
+ "before opening the file system") {
+ // An explicit non-"file" scheme is rejected by the allowlist check, which runs before the
+ // file system for the URL is instantiated -- so this surfaces the clean allowlist error
+ // rather than a lower-level failure from trying to load the s3a file system. The URL uses an
+ // upper-case "S3A" scheme so the lower-case "s3a" in the message pins the scheme-side case
+ // folding: dropping the fold on the scheme leaves no lower-case "s3a" in the message.
+ val hadoopConf = spark.sessionState.newHadoopConf()
+ val conf = new SQLConf()
+ conf.setConf(SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES, Seq("file"))
+ SQLConf.withExistingConf(conf) {
+ val e = intercept[AnalysisException] {
+ new AvroOptions(Map("avroSchemaUrl" -> "S3A://bucket/user.avsc"), hadoopConf)
+ }
+ assert(e.getCondition == "STDS_INVALID_OPTION_VALUE.WITH_MESSAGE")
+ assert(e.getMessage.contains("avroSchemaUrl"))
+ assert(e.getMessage.contains("not in the allowlist"))
+ assert(e.getMessage.contains("The scheme 's3a'"))
Review Comment:
Added in d325505: `AvroSuite` "SPARK-59329: the allowlist rejection echoes the parsed allowlist" uses a `file://` entry (which parses to one entry that matches nothing) and asserts the message contains both `The scheme 'file'` and `not in the allowlist [file://]`, so dropping the parsed-allowlist echo fails it. Runs green on head; the second assertion is exactly what pins the mutation you measured.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59329][SQL] Add an allowlist of URI schemes for the Avro schemaUrl option [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #58614:
URL: https://github.com/apache/spark/pull/58614#issuecomment-5672439374
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/5839ba0b095d10eb86438ace057691024cb9d074
- merged into branch-4.x https://github.com/apache/spark/commit/3dcbaa4c5698303b16979cd5dc7555cf4caff038
- merged into branch-4.3 https://github.com/apache/spark/commit/c025759c4c95ef4f14494e32ea8b1affd326bec9
*Posted by `merge_spark_pr.py`*
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59329][SQL] Add an allowlist of URI schemes for the Avro schemaUrl option [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #58614:
URL: https://github.com/apache/spark/pull/58614#discussion_r4003648578
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -7016,6 +7016,22 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ // Kept here with the other `spark.sql.avro.*` entries rather than in `StaticSQLConf`, though it
+ // is static: `buildStaticConf` registers the static key wherever it is declared.
+ val AVRO_SCHEMA_URL_ALLOWED_SCHEMES =
+ buildStaticConf("spark.sql.avro.schemaUrlAllowedSchemes")
+ .internal()
+ .doc("A comma-separated allowlist of URI schemes permitted for the 'avroSchemaUrl' Avro " +
+ "option. Empty by default, which permits any scheme and preserves the previous behavior; " +
+ "when non-empty, an avroSchemaUrl whose scheme is not listed is rejected before it is " +
+ "opened. This is a static configuration fixed when the SparkSession is created and not " +
+ "modifiable at runtime, so it is an operator-level boundary that a session cannot relax.")
Review Comment:
**Finding 11.** The `.doc` ends with "an operator-level boundary that a session cannot relax". `buildStaticConf` pins the allowlist *value*, and that part holds. It does not pin which file system an allowed scheme resolves to, and that is caller-controlled:
- `SessionState.newHadoopConf` copies every SQL conf into the Hadoop conf (`sql/core/src/main/scala/org/apache/spark/sql/internal/SessionState.scala:149`), so `spark.conf.set("fs.s3a.impl", ...)` on a live session lands there.
- `newHadoopConfWithOptions` adds every read option except `path` and `paths` (`sql/core/src/main/scala/org/apache/spark/sql/internal/SessionState.scala:121`), so `.option("fs.s3a.impl", ...)` lands there too.
- Hadoop's `FileSystem.getFileSystemClass` prefers `fs.<scheme>.impl` from that conf over the service-loaded implementation (`FileSystem.java:3376`, 3.3.0 sources).
That is the same `conf` this `AvroOptions` holds. So the scheme string the check compares and the class that opens the URL are two separate decisions.
Measured on this head. Allowlist `s3a`, data path local so only the schema URL is affected, and `fs.<scheme>.impl.disable.cache=true` in every arm - `FileSystem.CACHE` keys on scheme and authority rather than on the conf, so without it the first arm's instance serves the rest.
| arm | `avroSchemaUrl` | remap | result |
|---|---|---|---|
| A | `s3a://127.0.0.11:2121/user.avsc` | `.option("fs.s3a.impl", "org.apache.hadoop.fs.ftp.FTPFileSystem")` | `SocketTimeoutException: Connect timed out` at `java.net.Socket.connect` |
| B | `s3a://127.0.0.12:2121/user.avsc` | `spark.conf.set("fs.s3a.impl", ...)` at runtime | same |
| C | `s3a://127.0.0.13:2121/user.avsc` | none | `NoAuthWithAWSException` at `org.apache.hadoop.fs.s3a.S3AFileSystem.s3GetFileStatus` |
| D | `ftp://127.0.0.14:2121/user.avsc` | `fs.ftp.impl` set | the allowlist error, thrown at `AvroOptions.scala:99` |
C is the control: without a remap the allowed scheme does go to the real S3A. A and B open an FTP connection to a host the caller picked, through a scheme the allowlist permits. B is the one the wording turns on - no read option, one `spark.conf.set` on a running session. `FTPFileSystem` ships inside `hadoop-client-api`, so nothing extra has to be on the classpath.
D says the scheme name itself is enforced, so I would fix the sentence rather than the check:
```suggestion
"modifiable at runtime. It restricts the scheme an avroSchemaUrl may name. It does not " +
"restrict which file system serves that scheme. That is decided by fs.<scheme>.impl, " +
"which a session can still set.")
```
Making it a real boundary would mean also rejecting when `fs.<scheme>.impl` for the resolved scheme differs from the value in `sparkContext.hadoopConfiguration`. That is a much bigger change than this PR wants.
##########
connector/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala:
##########
@@ -1255,6 +1255,91 @@ abstract class AvroSuite
assertExceptionMsg[FileNotFoundException](e, "File not_exists.avsc does not exist")
}
+ // spark.sql.avro.schemaUrlAllowedSchemes is a static SQL config, so it cannot be set with
+ // withSQLConf; these drive AvroOptions directly under a SQLConf provided via withExistingConf.
+ // testFile returns a "file:" URL, so its scheme is an explicit "file"; the scheme-less path that
+ // resolves against the default file system is covered by its own test below.
+ test("SPARK-59329: avroSchemaUrl scheme allowlist permits an allowed scheme") {
+ val avroSchemaUrl = testFile("test_sub.avsc")
+ val hadoopConf = spark.sessionState.newHadoopConf()
+ val conf = new SQLConf()
+ conf.setConf(SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES, Seq("file"))
+ SQLConf.withExistingConf(conf) {
+ val options = new AvroOptions(Map("avroSchemaUrl" -> avroSchemaUrl), hadoopConf)
+ assert(options.schema.isDefined)
+ }
+ }
+
+ test("SPARK-59329: avroSchemaUrl allowlist rejects a disallowed scheme " +
+ "before opening the file system") {
+ // An explicit non-"file" scheme is rejected by the allowlist check, which runs before the
+ // file system for the URL is instantiated -- so this surfaces the clean allowlist error
+ // rather than a lower-level failure from trying to load the s3a file system. The URL uses an
+ // upper-case "S3A" scheme so the lower-case "s3a" in the message pins the scheme-side case
+ // folding: dropping the fold on the scheme leaves no lower-case "s3a" in the message.
+ val hadoopConf = spark.sessionState.newHadoopConf()
+ val conf = new SQLConf()
+ conf.setConf(SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES, Seq("file"))
+ SQLConf.withExistingConf(conf) {
+ val e = intercept[AnalysisException] {
+ new AvroOptions(Map("avroSchemaUrl" -> "S3A://bucket/user.avsc"), hadoopConf)
+ }
+ assert(e.getCondition == "STDS_INVALID_OPTION_VALUE.WITH_MESSAGE")
+ assert(e.getMessage.contains("avroSchemaUrl"))
+ assert(e.getMessage.contains("not in the allowlist"))
+ assert(e.getMessage.contains("The scheme 's3a'"))
Review Comment:
**Finding 12.** The echo landed with nothing pinning it. I removed `${allowedSchemes.mkString("[", ", ", "]")}` from `sql/core/src/main/scala/org/apache/spark/sql/avro/AvroOptions.scala:98` and re-ran: the five `SPARK-59329` cases here and the three in `AvroSchemaUrlAllowlistSuite` all stayed green. So the case the echo exists for - an operator writing `file://` where `file` was meant - has no test. That case came from my ask at [r3995617105](https://github.com/apache/spark/pull/58614#discussion_r3995617105).
This test is the only one of the nine that fails with the echo removed. I ran it both ways, green on this head and red under the mutation:
```scala
test("SPARK-59329: the allowlist rejection echoes the parsed allowlist") {
val hadoopConf = spark.sessionState.newHadoopConf()
// "file://" is the shape an operator is most likely to write by mistake: it parses to one
// entry that matches nothing, so the message has to show what it parsed to be readable.
val conf = new SQLConf()
conf.setConf(SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES, Seq("file://"))
SQLConf.withExistingConf(conf) {
val e = intercept[AnalysisException] {
new AvroOptions(Map("avroSchemaUrl" -> testFile("test_sub.avsc")), hadoopConf)
}
assert(e.getMessage.contains("The scheme 'file'"))
assert(e.getMessage.contains("not in the allowlist [file://]"))
}
}
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59329][SQL] Add an allowlist of URI schemes for the Avro schemaUrl option [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #58614:
URL: https://github.com/apache/spark/pull/58614#discussion_r3987277354
##########
connector/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala:
##########
@@ -1255,6 +1255,64 @@ abstract class AvroSuite
assertExceptionMsg[FileNotFoundException](e, "File not_exists.avsc does not exist")
}
+ // spark.sql.avro.schemaUrlAllowedSchemes is a static SQL config, so it cannot be set with
Review Comment:
**Finding 5.** The static conf does rule out `withSQLConf`, but not a read. These four tests hand a hand-built `SQLConf` to the check through `withExistingConf`, which is the one input that cannot be wrong in production. So the three properties the round-2 change is *for* are all unpinned:
- that the conf set on the `SparkSession` is visible at the `SQLConf.get` the check reads,
- that the check fires in a real `spark.read ... load()`,
- that a session cannot relax it, which is what `buildStaticConf` bought.
All three hold on this head - I ran them - so this is coverage, not a bug. A dedicated suite with `override protected def sparkConf` gets them, the same pattern `AvroV1Suite` uses at `connector/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala:3925`. This compiles and passes as written:
```scala
class AvroSchemaUrlAllowlistSuite extends QueryTest with SharedSparkSession {
override protected def sparkConf: SparkConf =
super.sparkConf.set(SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES.key, "file")
private val testAvro = testFile("test.avro")
test("SPARK-59329: an allowed scheme is permitted through a read") {
val result = spark.read.option("avroSchemaUrl", testFile("test_sub.avsc"))
.format("avro").load(testAvro).collect()
val expected = spark.read.format("avro").load(testAvro).select("string").collect()
assert(result.sameElements(expected))
}
test("SPARK-59329: a disallowed scheme is rejected through a read") {
val e = intercept[AnalysisException] {
spark.read.option("avroSchemaUrl", "s3a://bucket/user.avsc")
.format("avro").load(testAvro).collect()
}
assert(e.getCondition == "STDS_INVALID_OPTION_VALUE.WITH_MESSAGE")
assert(e.getMessage.contains("not in the allowlist"))
}
test("SPARK-59329: a session cannot relax the allowlist") {
val key = SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES.key
Seq[() => Unit](
() => spark.conf.set(key, "s3a"),
() => spark.sql(s"SET $key=s3a").collect()
).foreach { f =>
checkError(
exception = intercept[AnalysisException](f()),
condition = "CANNOT_MODIFY_STATIC_CONFIG",
parameters = Map("key" -> s""""$key""""))
}
}
}
```
Keep whichever of the direct-`AvroOptions` cases you still want in `AvroSuite` on top of that.
##########
connector/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala:
##########
@@ -1255,6 +1255,64 @@ abstract class AvroSuite
assertExceptionMsg[FileNotFoundException](e, "File not_exists.avsc does not exist")
}
+ // spark.sql.avro.schemaUrlAllowedSchemes is a static SQL config, so it cannot be set with
+ // withSQLConf; these drive AvroOptions directly under a SQLConf provided via withExistingConf.
+ // A scheme-less local path resolves to the default file system ("file").
+ test("SPARK-59329: avroSchemaUrl scheme allowlist permits an allowed scheme") {
+ val avroSchemaUrl = testFile("test_sub.avsc")
+ val hadoopConf = spark.sessionState.newHadoopConf()
+ val conf = new SQLConf()
+ conf.setConf(SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES, Seq("file"))
+ SQLConf.withExistingConf(conf) {
+ val options = new AvroOptions(Map("avroSchemaUrl" -> avroSchemaUrl), hadoopConf)
+ assert(options.schema.isDefined)
+ }
+ }
+
+ test("SPARK-59329: avroSchemaUrl allowlist rejects a disallowed scheme " +
+ "before opening the file system") {
+ // An explicit non-"file" scheme is rejected by the allowlist check, which runs before the
+ // file system for the URL is instantiated -- so this surfaces the clean allowlist error
+ // rather than a lower-level failure from trying to load the s3a file system.
+ val hadoopConf = spark.sessionState.newHadoopConf()
+ val conf = new SQLConf()
+ conf.setConf(SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES, Seq("file"))
+ SQLConf.withExistingConf(conf) {
+ val e = intercept[AnalysisException] {
+ new AvroOptions(Map("avroSchemaUrl" -> "s3a://bucket/user.avsc"), hadoopConf)
Review Comment:
**Finding 8.** The new test's comment says the entries and the URL scheme are both folded, but only the entry side is pinned - that half was finding 2. I deleted the `.toLowerCase(Locale.ROOT)` on the scheme at `sql/core/src/main/scala/org/apache/spark/sql/avro/AvroOptions.scala:93` and all four SPARK-59329 tests still passed. Every URL in them is scheme-less, so the scheme always arrives from `FileSystem.getDefaultUri` already lower-case.
One character here covers it. With an upper-case scheme the `assert(e.getMessage.contains("s3a"))` three lines below becomes the pin: the message reads `The scheme 's3a' of avroSchemaUrl 'S3A://bucket/user.avsc'` with the fold, and without it there is no lower-case `s3a` left anywhere in it. I ran it both ways - passes as the code stands, fails with the fold removed.
```suggestion
new AvroOptions(Map("avroSchemaUrl" -> "S3A://bucket/user.avsc"), hadoopConf)
```
Tightening that assertion to `contains("The scheme 's3a'")` makes the intent explicit if you prefer.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -7016,6 +7016,20 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ val AVRO_SCHEMA_URL_ALLOWED_SCHEMES =
+ buildStaticConf("spark.sql.avro.schemaUrlAllowedSchemes")
Review Comment:
**Finding 6.** This is the only `buildStaticConf` in `SQLConf.scala`. The other 39 SQL static confs live in `StaticSQLConf.scala`, and Hive's and Connect's live in their own module objects (`HiveUtils`, `Connect`). `StaticSQLConf` is where a reader goes to find out what cannot be set at runtime. Here the entry sits between `spark.sql.avro.filterPushdown.enabled` and `spark.sql.json.enablePartialResults`, with nothing but the builder name marking it apart from the runtime confs around it.
Moving it to `StaticSQLConf.scala` is the smaller surprise. If you would rather keep it with the other `spark.sql.avro.*` entries, a one-line comment above it saying it is static would do.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59329][SQL] Add an allowlist of URI schemes for the Avro schemaUrl option [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #58614:
URL: https://github.com/apache/spark/pull/58614#discussion_r3962605631
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -7016,6 +7016,19 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ val AVRO_SCHEMA_URL_ALLOWED_SCHEMES =
+ buildConf("spark.sql.avro.schemaUrlAllowedSchemes")
+ .internal()
+ .doc("A comma-separated allowlist of URI schemes permitted for the 'avroSchemaUrl' Avro " +
+ "option. Empty by default, which permits any scheme and preserves the previous behavior; " +
+ "when non-empty, an avroSchemaUrl whose scheme is not listed is rejected before it is " +
+ "opened.")
+ .version("4.3.0")
Review Comment:
Yes, this is intentionally targeting 4.3.0 rather than 4.4.0. The rationale is being discussed on a private channel, so I can't share the details here per ASF policy -- happy to follow up offline. Thanks for checking.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59329][SQL] Add an allowlist of URI schemes for the Avro schemaUrl option [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #58614:
URL: https://github.com/apache/spark/pull/58614#discussion_r3962605631
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -7016,6 +7016,19 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ val AVRO_SCHEMA_URL_ALLOWED_SCHEMES =
+ buildConf("spark.sql.avro.schemaUrlAllowedSchemes")
+ .internal()
+ .doc("A comma-separated allowlist of URI schemes permitted for the 'avroSchemaUrl' Avro " +
+ "option. Empty by default, which permits any scheme and preserves the previous behavior; " +
+ "when non-empty, an avroSchemaUrl whose scheme is not listed is rejected before it is " +
+ "opened.")
+ .version("4.3.0")
Review Comment:
Yes, this is intentionally targeting 4.3.0 rather than 4.4.0. The rationale is being discussed on a private channel, so I can't share the details here -- happy to follow up offline. Thanks for checking.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59329][SQL] Add an allowlist of URI schemes for the Avro schemaUrl option [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #58614:
URL: https://github.com/apache/spark/pull/58614#issuecomment-5642263300
Addressed the two description findings (7 and 3): the PR description is updated so "How was this patch tested?" reflects the four direct `AvroOptions` cases plus the new read-path `AvroSchemaUrlAllowlistSuite`, and states the config is static. Added a scope note that this deliberately targets Avro's `avroSchemaUrl`; options of the same shape such as XML's `rowValidationXSDPath` are out of scope here and there is no shared helper yet. (9655f9e)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59329][SQL] Add an allowlist of URI schemes for the Avro schemaUrl option [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #58614:
URL: https://github.com/apache/spark/pull/58614#discussion_r3995617103
##########
connector/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala:
##########
@@ -1255,6 +1255,66 @@ abstract class AvroSuite
assertExceptionMsg[FileNotFoundException](e, "File not_exists.avsc does not exist")
}
+ // spark.sql.avro.schemaUrlAllowedSchemes is a static SQL config, so it cannot be set with
+ // withSQLConf; these drive AvroOptions directly under a SQLConf provided via withExistingConf.
+ // A scheme-less local path resolves to the default file system ("file").
Review Comment:
**Finding 9.** This says a scheme-less local path resolves to the default file system, but `testFile` is `Thread.currentThread().getContextClassLoader.getResource(fileName).toString` (`sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala:508`), so `avroSchemaUrl` here is `file:/.../test_sub.avsc`. Its `getScheme` is `"file"`, and `Option(uri.getScheme)` is never `None` in any test on this PR.
So the `getOrElse(FileSystem.getDefaultUri(conf).getScheme)` at `sql/core/src/main/scala/org/apache/spark/sql/avro/AvroOptions.scala:92` has no coverage. I replaced it with a constant and all seven new tests still passed: the four here and the three in `AvroSchemaUrlAllowlistSuite`.
The same claim is at `AvroSuite.scala:4281` and in the description ("including a scheme-less local path that resolves to `file`"). It came from my round-2 comment at [r3987277368](https://github.com/apache/spark/pull/58614#discussion_r3987277368), where I said every test URL is scheme-less. That was wrong. The scheme-side fold is pinned correctly now, but not for the reason I gave.
This case covers the branch. I ran it both ways: green on this head, red with the fallback replaced.
```scala
test("SPARK-59329: avroSchemaUrl allowlist resolves a scheme-less path " +
"against the default file system") {
// testFile returns a "file:" URL, so strip the scheme to get a genuinely scheme-less path.
val avroSchemaUrl = new URI(testFile("test_sub.avsc")).getPath
assert(new URI(avroSchemaUrl).getScheme == null)
val hadoopConf = spark.sessionState.newHadoopConf()
// The default file system is "file", so allowing "file" permits the scheme-less path ...
val allowed = new SQLConf()
allowed.setConf(SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES, Seq("file"))
SQLConf.withExistingConf(allowed) {
assert(new AvroOptions(Map("avroSchemaUrl" -> avroSchemaUrl), hadoopConf).schema.isDefined)
}
// ... and an allowlist without it rejects the same path.
val disallowed = new SQLConf()
disallowed.setConf(SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES, Seq("s3a"))
SQLConf.withExistingConf(disallowed) {
val e = intercept[AnalysisException] {
new AvroOptions(Map("avroSchemaUrl" -> avroSchemaUrl), hadoopConf)
}
assert(e.getMessage.contains("The scheme 'file'"))
}
}
```
##########
sql/core/src/main/scala/org/apache/spark/sql/avro/AvroOptions.scala:
##########
@@ -76,7 +76,29 @@ private[sql] class AvroOptions(
parameters.get(AVRO_SCHEMA).map(AvroUtils.parseAvroSchema).orElse({
val avroUrlSchema = parameters.get(AVRO_SCHEMA_URL).map(url => {
log.debug("loading avro schema from url: " + url)
- val fs = FileSystem.get(new URI(url), conf)
+ val uri = new URI(url)
+ // Optional operator-configured allowlist of URI schemes for avroSchemaUrl. Empty by
+ // default, which permits any scheme and leaves the file-system resolution below unchanged.
+ // When set, the scheme is resolved and checked before the file system for the URL is
+ // instantiated, so a disallowed scheme is rejected with a clear error rather than a
+ // lower-level failure while opening it. A scheme-less URL takes the default file system's
+ // scheme, so it can be permitted by allowing that scheme.
+ val allowedSchemes = SQLConf.get.getConf(SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES)
+ .map(_.toLowerCase(Locale.ROOT))
+ if (allowedSchemes.nonEmpty) {
+ // FileSystem.getDefaultUri always carries a scheme (it throws otherwise), so a
+ // scheme-less URL resolves to the default file system's scheme.
+ val scheme = Option(uri.getScheme)
+ .getOrElse(FileSystem.getDefaultUri(conf).getScheme)
+ .toLowerCase(Locale.ROOT)
+ if (!allowedSchemes.contains(scheme)) {
+ throw QueryCompilationErrors.avroOptionsException(
+ AVRO_SCHEMA_URL,
+ s"The scheme '$scheme' of avroSchemaUrl '$url' is not in the allowlist " +
+ s"configured by ${SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES.key}.")
Review Comment:
**Finding 10.** The message names the key but not what it parsed to, so the mistake an operator is most likely to make reads as a contradiction. `spark.sql.avro.schemaUrlAllowedSchemes=file://` gives `Seq("file://")`, which matches nothing, and every read then fails with `The scheme 'file' of avroSchemaUrl '...' is not in the allowlist configured by spark.sql.avro.schemaUrlAllowedSchemes` while `file` is exactly what they wrote.
```suggestion
s"${allowedSchemes.mkString("[", ", ", "]")} configured by " +
s"${SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES.key}.")
```
That reads `... is not in the allowlist [file://] configured by ...`. Both existing assertions still match; I ran the four `AvroSuite` cases and `AvroSchemaUrlAllowlistSuite` with it.
The other route is #58613's: reject a malformed entry up front with `checkValue`. Either works.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org