You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "hit2024 (via GitHub)" <gi...@apache.org> on 2026/05/16 04:32:52 UTC
[PR] [SPARK-56999][CONNECT] Propagate Spark Connect session user as a SparkContext local property [spark]
hit2024 opened a new pull request, #55917:
URL: https://github.com/apache/spark/pull/55917
### What changes were proposed in this pull request?
Propagating Spark Connect session user as a SparkContext local property. Currently user is added to the job description and not as a separate SparkContext property.
### Why are the changes needed?
We use SparkConnect along with Ranger for RBAC (org.apache.kyuubi.plugin.spark.authz.ranger.RangerSparkExtension), which requires the user (currently it checks for "kyuubi.session.user", will add a fallback to check for "spark.connect.session.user")
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Tested it on local where the RangerSparkExtension was able to extract the user as expected. No tests were added because its a small, trivial and non-destructive, backward compatible new feature.
### 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-56891][CONNECT] Propagate Spark Connect session user as a SparkContext local property [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #55917:
URL: https://github.com/apache/spark/pull/55917#discussion_r3638634551
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/execution/ExecuteThreadRunner.scala:
##########
@@ -219,6 +219,12 @@ private[connect] class ExecuteThreadRunner(executeHolder: ExecuteHolder) extends
s"Spark Connect - ${Utils.abbreviate(debugString, 128)}")
session.sparkContext.setInterruptOnCancel(true)
+ val sessionUser = executeHolder.sessionHolder.userId
+ if (sessionUser != null && sessionUser.nonEmpty) {
+ session.sparkContext.setLocalProperty(
Review Comment:
If you move the `set` into `withSession` per the other comment, please pair it with a try/finally that restores the previous value. `sc.localProperties` is an `InheritableThreadLocal`, and analyze/config run on reused gRPC handler threads, so a set-without-restore lets one user's `spark.connect.session.user` linger into the next request on that thread — Ranger could then authorize under the stale identity. The current code happens to dodge this only because execute runs on a throwaway `ExecutionThread` (the comment at lines 49-51 also flags that thread-pooling would need thread-local handling), but a shared entry point requires explicit cleanup.
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/execution/ExecuteThreadRunner.scala:
##########
@@ -219,6 +219,12 @@ private[connect] class ExecuteThreadRunner(executeHolder: ExecuteHolder) extends
s"Spark Connect - ${Utils.abbreviate(debugString, 128)}")
session.sparkContext.setInterruptOnCancel(true)
+ val sessionUser = executeHolder.sessionHolder.userId
Review Comment:
This property is only set in `ExecuteThreadRunner`, but the analyze path (`SparkConnectAnalyzeHandler`) also goes through `sessionHolder.withSession` and runs plan analysis plus `persist`/`unpersist`, yet it never gets `spark.connect.session.user`. Since the goal is RBAC via Ranger, analyze-style operations need the user identity too — wiring only the execute path leaves the feature half-covered. `userId` is already a field of `SessionHolder`, and `withSession` is the shared entry point for execute/analyze/config, so would it be cleaner to set the property inside `SessionHolder.withSession`? That covers every path at once and avoids having to remember an extra line each time a new path is added. Note this needs a matching restore of the previous value once moved.
Amd this property is an external contract that the Ranger RBAC side depends on, so it's more worth protecting than debug-only info like callSite. Consider a tiny test: after an execute, assert `sc.getLocalProperty(SPARK_CONNECT_SESSION_USER_KEY)` equals the expected user. It's cheap and guards against a later refactor silently dropping it. I realize the local properties in this module (callSite, etc.) aren't individually tested, so this isn't a merge blocker; it's just that this key is an external integration point, which is a bit different in nature.
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectService.scala:
##########
@@ -314,6 +314,7 @@ class SparkConnectService(debug: Boolean) extends AsyncService with BindableServ
*/
object SparkConnectService extends Logging {
+ private[connect] val SPARK_CONNECT_SESSION_USER_KEY: String = "spark.connect.session.user"
Review Comment:
This key lives under the `spark.connect.session.*` prefix, which is currently the namespace for the static confs in `Connect.scala` (planCache, planCompression, manager.*, all `buildStaticConf`/`buildConf`), yet it's actually a runtime local property. I get that the name can't change (the PR says Kyuubi's Ranger plugin will fall back to reading `spark.connect.session.user`, so it's an agreed contract), so I'd suggest a scaladoc line on this constant stating "this is a SparkContext local property, not a SQLConf; Ranger/Kyuubi depends on this key", so nobody later mistakes it for something settable via `spark.conf.set`. It currently sits right next to the heavily-commented keepalive constant but has no comment of its own.
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/execution/ExecuteThreadRunner.scala:
##########
@@ -219,6 +219,12 @@ private[connect] class ExecuteThreadRunner(executeHolder: ExecuteHolder) extends
s"Spark Connect - ${Utils.abbreviate(debugString, 128)}")
session.sparkContext.setInterruptOnCancel(true)
+ val sessionUser = executeHolder.sessionHolder.userId
+ if (sessionUser != null && sessionUser.nonEmpty) {
Review Comment:
+1
--
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-56891][CONNECT] Propagate Spark Connect session user as a SparkContext local property [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on code in PR #55917:
URL: https://github.com/apache/spark/pull/55917#discussion_r3624122169
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/execution/ExecuteThreadRunner.scala:
##########
@@ -219,6 +219,12 @@ private[connect] class ExecuteThreadRunner(executeHolder: ExecuteHolder) extends
s"Spark Connect - ${Utils.abbreviate(debugString, 128)}")
session.sparkContext.setInterruptOnCancel(true)
+ val sessionUser = executeHolder.sessionHolder.userId
Review Comment:
Can identity spoofing happen here? For example, clients could select another user ID and inherit that user’s permissions. So if Ranger treats this property as the authenticated user, then the client may receive admin permissions.
--
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-56891][CONNECT] Propagate Spark Connect session user as a SparkContext local property [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on code in PR #55917:
URL: https://github.com/apache/spark/pull/55917#discussion_r3624090692
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/execution/ExecuteThreadRunner.scala:
##########
@@ -219,6 +219,12 @@ private[connect] class ExecuteThreadRunner(executeHolder: ExecuteHolder) extends
s"Spark Connect - ${Utils.abbreviate(debugString, 128)}")
session.sparkContext.setInterruptOnCancel(true)
+ val sessionUser = executeHolder.sessionHolder.userId
+ if (sessionUser != null && sessionUser.nonEmpty) {
Review Comment:
The null check here is dead noise, because `SessionHolder.userId` is a non-nullable Scala String from proto `getUserId` (defaults to "", never `null`).
--
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-56891][CONNECT] Propagate Spark Connect session user as a SparkContext local property [spark]
Posted by "j1wonpark (via GitHub)" <gi...@apache.org>.
j1wonpark commented on PR #55917:
URL: https://github.com/apache/spark/pull/55917#issuecomment-6216321703
Hi, we also encountered a related issue: Spark Connect sessions have a user ID, but SQL functions such as `current_user()` return the server user.
I’m planning a focused PR to propagate the session user through `CurrentUserContext` in `SessionHolder.withSession`, following the suggestions in this review, with tests covering execute/analyze paths and context restoration.
The scope would be SQL user-context propagation, without adding a new local property or authentication behavior. I’ll link back to this PR and its discussion. Please let me know if you’d prefer to coordinate this work 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-56891][CONNECT] Propagate Spark Connect session user as a SparkContext local property [spark]
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on PR #55917:
URL: https://github.com/apache/spark/pull/55917#issuecomment-5037816805
Ranger extension could access `CurrentUserContext.getCurrentUserOrEmpty`... but how is that extension even running on the ExecuteThreadRunner thread, which is a thread that is used for query execution? Where is that extension injected?
--
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-56891][CONNECT] Propagate Spark Connect session user as a SparkContext local property [spark]
Posted by "hit2024 (via GitHub)" <gi...@apache.org>.
hit2024 commented on PR #55917:
URL: https://github.com/apache/spark/pull/55917#issuecomment-5036599818
@LuciferYang Could you check this and help me
--
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-56891][CONNECT] Propagate Spark Connect session user as a SparkContext local property [spark]
Posted by "hit2024 (via GitHub)" <gi...@apache.org>.
hit2024 commented on PR #55917:
URL: https://github.com/apache/spark/pull/55917#issuecomment-4633211026
<img width="1723" height="787" alt="Screenshot 2026-06-05 at 9 11 23 PM" src="https://github.com/user-attachments/assets/43604f65-23f0-429d-9c10-29f218edd111" />
@LuciferYang I have enabled it but they are stuck in Queued State
--
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-56891][CONNECT] Propagate Spark Connect session user as a SparkContext local property [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #55917:
URL: https://github.com/apache/spark/pull/55917#issuecomment-4628988721
<img width="2526" height="1588" alt="image" src="https://github.com/user-attachments/assets/4f75f359-aece-4754-a271-89c24fd9a866" />
Could you enable GHA first?
--
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-56891][CONNECT] Propagate Spark Connect session user as a SparkContext local property [spark]
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on code in PR #55917:
URL: https://github.com/apache/spark/pull/55917#discussion_r3638721023
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectService.scala:
##########
@@ -314,6 +314,7 @@ class SparkConnectService(debug: Boolean) extends AsyncService with BindableServ
*/
object SparkConnectService extends Logging {
+ private[connect] val SPARK_CONNECT_SESSION_USER_KEY: String = "spark.connect.session.user"
Review Comment:
Could we use CurrentUserContext instead of SparkContext local property, the way Hive Thriftserver does?
--
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-56891][CONNECT] Propagate Spark Connect session user as a SparkContext local property [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on code in PR #55917:
URL: https://github.com/apache/spark/pull/55917#discussion_r3624055674
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/execution/ExecuteThreadRunner.scala:
##########
@@ -219,6 +219,12 @@ private[connect] class ExecuteThreadRunner(executeHolder: ExecuteHolder) extends
s"Spark Connect - ${Utils.abbreviate(debugString, 128)}")
session.sparkContext.setInterruptOnCancel(true)
+ val sessionUser = executeHolder.sessionHolder.userId
+ if (sessionUser != null && sessionUser.nonEmpty) {
+ session.sparkContext.setLocalProperty(
+ SparkConnectService.SPARK_CONNECT_SESSION_USER_KEY, sessionUser)
+ }
+
Review Comment:
Please add a regression test, e.g. a connect execution test case verifying spark.connect.session.user is visible from the execution thread, and contains the session user.
--
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-56891][CONNECT] Propagate Spark Connect session user as a SparkContext local property [spark]
Posted by "hit2024 (via GitHub)" <gi...@apache.org>.
hit2024 commented on PR #55917:
URL: https://github.com/apache/spark/pull/55917#issuecomment-4633573513
@LuciferYang It was enabled, but the workflow was stuck in a queued state. After syncing with master, I was able to trigger the Run Benchmarks workflow. However, the remaining checks were skipped, and I couldn't find an option to rerun them. I deleted the workflow run, but the build hasn't started again. Could you please guide me on how to get the GitHub checks running? Thanks!
--
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-56891][CONNECT] Propagate Spark Connect session user as a SparkContext local property [spark]
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on PR #55917:
URL: https://github.com/apache/spark/pull/55917#issuecomment-5037791063
A precedent would be how Hive Thriftserver does it, which is set it to the `CURRENT_USER` (https://github.com/apache/spark/blob/e3eda671de6595e6d3071f3a9563f12ba0ea71cd/sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkOperation.scala#L114), which then also works with the `current_user` function.
As @uros-b mentioned https://github.com/apache/spark/pull/55917#discussion_r3624122169 this by itself does not prevent the client from setting anything they want as username, authentication/verification of that is a different layer not provided by 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-56891][CONNECT] Propagate Spark Connect session user as a SparkContext local property [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on code in PR #55917:
URL: https://github.com/apache/spark/pull/55917#discussion_r3624080637
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectService.scala:
##########
@@ -314,6 +314,7 @@ class SparkConnectService(debug: Boolean) extends AsyncService with BindableServ
*/
object SparkConnectService extends Logging {
+ private[connect] val SPARK_CONNECT_SESSION_USER_KEY: String = "spark.connect.session.user"
Review Comment:
Is this intentionally private? This way, it can't be imported by external consumers (Kyuubi/Ranger).
--
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