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: [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]
