You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "eason-yuchen-liu (via GitHub)" <gi...@apache.org> on 2024/05/29 17:25:33 UTC
[PR] [SPARK-48447][SS] Check state store provider class before invoking the constructor [spark]
eason-yuchen-liu opened a new pull request, #46791:
URL: https://github.com/apache/spark/pull/46791
### What changes were proposed in this pull request?
This PR adds a validity check to the StateStoreProvider class before calling its constructor. A new runtime exception was created to report this issue.
### Why are the changes needed?
This is a security improvement to a user-facing API. Users are only allowed to use class that extends `org.apache.spark.sql.execution.streaming.state.StateStoreProvider` as StateStoreProvider.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
This PR comes with a new test to test that invalid class cannot pass the security check. The fact that other tests in StreamingQuerySuite can pass implies that valid class can pass the check.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
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-48447][SS] Check state store provider class before invoking the constructor [spark]
Posted by "anishshri-db (via GitHub)" <gi...@apache.org>.
anishshri-db commented on code in PR #46791:
URL: https://github.com/apache/spark/pull/46791#discussion_r1619401726
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala:
##########
@@ -289,6 +289,10 @@ class InvalidUnsafeRowException(error: String)
"among restart. For the first case, you can try to restart the application without " +
s"checkpoint or use the legacy Spark version to process the streaming state.\n$error", null)
+class InvalidStateStoreProvider
+ extends RuntimeException("The streaming query failed because the given StateStoreProvider " +
+ "does not extend org.apache.spark.sql.execution.streaming.state.StateStoreProvider")
Review Comment:
Can we make sure that we are using the new error class framework here ?
cc - @HeartSaVioR , @WweiL
--
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-48447][SS] Check state store provider class before invoking the constructor [spark]
Posted by "anishshri-db (via GitHub)" <gi...@apache.org>.
anishshri-db commented on code in PR #46791:
URL: https://github.com/apache/spark/pull/46791#discussion_r1619400480
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala:
##########
@@ -289,6 +289,10 @@ class InvalidUnsafeRowException(error: String)
"among restart. For the first case, you can try to restart the application without " +
s"checkpoint or use the legacy Spark version to process the streaming state.\n$error", null)
+class InvalidStateStoreProvider
Review Comment:
nit: indent
--
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-48447][SS] Check state store provider class before invoking the constructor [spark]
Posted by "eason-yuchen-liu (via GitHub)" <gi...@apache.org>.
eason-yuchen-liu commented on code in PR #46791:
URL: https://github.com/apache/spark/pull/46791#discussion_r1619496663
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -2798,6 +2798,11 @@
],
"sqlState" : "42601"
},
+ "INVALID_STATE_STORE_PROVIDER" : {
+ "message" : [
+ "The given State Store Provider <inputClass> does not extend org.apache.spark.sql.execution.streaming.state.StateStoreProvider."
Review Comment:
Yep. Done. I choose this state:
```
"42K06": {
"description": "Invalid type for options",
"origin": "Spark",
"standard": "N",
"usedBy": ["Spark"]
}
```
--
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-48447][SS] Check state store provider class before invoking the constructor [spark]
Posted by "anishshri-db (via GitHub)" <gi...@apache.org>.
anishshri-db commented on code in PR #46791:
URL: https://github.com/apache/spark/pull/46791#discussion_r1619401726
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala:
##########
@@ -289,6 +289,10 @@ class InvalidUnsafeRowException(error: String)
"among restart. For the first case, you can try to restart the application without " +
s"checkpoint or use the legacy Spark version to process the streaming state.\n$error", null)
+class InvalidStateStoreProvider
+ extends RuntimeException("The streaming query failed because the given StateStoreProvider " +
+ "does not extend org.apache.spark.sql.execution.streaming.state.StateStoreProvider")
Review Comment:
Can we make sure that we are using the new error class framework here ?
cc - @HeartSaVioR
--
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-48447][SS] Check state store provider class before invoking the constructor [spark]
Posted by "anishshri-db (via GitHub)" <gi...@apache.org>.
anishshri-db commented on PR #46791:
URL: https://github.com/apache/spark/pull/46791#issuecomment-2138156757
@eason-yuchen-liu - seems like the CI tests are not running here ?
--
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-48447][SS] Check state store provider class before invoking the constructor [spark]
Posted by "eason-yuchen-liu (via GitHub)" <gi...@apache.org>.
eason-yuchen-liu commented on code in PR #46791:
URL: https://github.com/apache/spark/pull/46791#discussion_r1619491508
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala:
##########
@@ -396,6 +396,12 @@ object StateStoreProvider {
*/
def create(providerClassName: String): StateStoreProvider = {
val providerClass = Utils.classForName(providerClassName)
+ if (!classOf[StateStoreProvider].isAssignableFrom(providerClass)) {
+ throw new SparkException(
+ errorClass = "INVALID_STATE_STORE_PROVIDER",
Review Comment:
Sure.
--
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-48447][SS] Check state store provider class before invoking the constructor [spark]
Posted by "anishshri-db (via GitHub)" <gi...@apache.org>.
anishshri-db commented on code in PR #46791:
URL: https://github.com/apache/spark/pull/46791#discussion_r1619491956
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -2798,6 +2798,11 @@
],
"sqlState" : "42601"
},
+ "INVALID_STATE_STORE_PROVIDER" : {
+ "message" : [
+ "The given State Store Provider <inputClass> does not extend org.apache.spark.sql.execution.streaming.state.StateStoreProvider."
Review Comment:
I believe u also need to define some `sqlState` ?
--
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-48447][SS] Check state store provider class before invoking the constructor [spark]
Posted by "eason-yuchen-liu (via GitHub)" <gi...@apache.org>.
eason-yuchen-liu commented on PR #46791:
URL: https://github.com/apache/spark/pull/46791#issuecomment-2138285956
@anishshri-db Thank you for the suggestions. I have incorporated the new exception framework.
--
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-48447][SS] Check state store provider class before invoking the constructor [spark]
Posted by "anishshri-db (via GitHub)" <gi...@apache.org>.
anishshri-db commented on code in PR #46791:
URL: https://github.com/apache/spark/pull/46791#discussion_r1619400480
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala:
##########
@@ -289,6 +289,10 @@ class InvalidUnsafeRowException(error: String)
"among restart. For the first case, you can try to restart the application without " +
s"checkpoint or use the legacy Spark version to process the streaming state.\n$error", null)
+class InvalidStateStoreProvider
Review Comment:
nit: indent
--
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-48447][SS] Check state store provider class before invoking the constructor [spark]
Posted by "anishshri-db (via GitHub)" <gi...@apache.org>.
anishshri-db commented on code in PR #46791:
URL: https://github.com/apache/spark/pull/46791#discussion_r1619400858
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala:
##########
@@ -289,6 +289,10 @@ class InvalidUnsafeRowException(error: String)
"among restart. For the first case, you can try to restart the application without " +
s"checkpoint or use the legacy Spark version to process the streaming state.\n$error", null)
+class InvalidStateStoreProvider
Review Comment:
Also - prob rename as `InvalidStateStoreProviderException` ?
--
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-48447][SS] Check state store provider class before invoking the constructor [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on PR #46791:
URL: https://github.com/apache/spark/pull/46791#issuecomment-2144228814
Thanks! Merging to master.
--
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-48447][SS] Check state store provider class before invoking the constructor [spark]
Posted by "anishshri-db (via GitHub)" <gi...@apache.org>.
anishshri-db commented on code in PR #46791:
URL: https://github.com/apache/spark/pull/46791#discussion_r1622705112
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala:
##########
@@ -396,6 +396,12 @@ object StateStoreProvider {
*/
def create(providerClassName: String): StateStoreProvider = {
val providerClass = Utils.classForName(providerClassName)
+ if (!classOf[StateStoreProvider].isAssignableFrom(providerClass)) {
+ throw new SparkException(
+ errorClass = "STATE_STORE_INVALID_PROVIDER",
+ messageParameters = Map("inputClass" -> providerClassName),
+ cause = null)
Review Comment:
We can remove this line if its optional ?
--
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-48447][SS] Check state store provider class before invoking the constructor [spark]
Posted by "anishshri-db (via GitHub)" <gi...@apache.org>.
anishshri-db commented on code in PR #46791:
URL: https://github.com/apache/spark/pull/46791#discussion_r1619491270
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala:
##########
@@ -396,6 +396,12 @@ object StateStoreProvider {
*/
def create(providerClassName: String): StateStoreProvider = {
val providerClass = Utils.classForName(providerClassName)
+ if (!classOf[StateStoreProvider].isAssignableFrom(providerClass)) {
+ throw new SparkException(
+ errorClass = "INVALID_STATE_STORE_PROVIDER",
Review Comment:
Lets use the `STATE_STORE_` prefix here
so maybe rename as `STATE_STORE_INVALID_PROVIDER`
--
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-48447][SS] Check state store provider class before invoking the constructor [spark]
Posted by "eason-yuchen-liu (via GitHub)" <gi...@apache.org>.
eason-yuchen-liu commented on code in PR #46791:
URL: https://github.com/apache/spark/pull/46791#discussion_r1622718684
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala:
##########
@@ -396,6 +396,12 @@ object StateStoreProvider {
*/
def create(providerClassName: String): StateStoreProvider = {
val providerClass = Utils.classForName(providerClassName)
+ if (!classOf[StateStoreProvider].isAssignableFrom(providerClass)) {
+ throw new SparkException(
+ errorClass = "STATE_STORE_INVALID_PROVIDER",
+ messageParameters = Map("inputClass" -> providerClassName),
+ cause = null)
Review Comment:
`cause` is not optional sadly when using together with `messageParameters`.
--
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-48447][SS] Check state store provider class before invoking the constructor [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR closed pull request #46791: [SPARK-48447][SS] Check state store provider class before invoking the constructor
URL: https://github.com/apache/spark/pull/46791
--
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