Skip to content

[SPARK-41958][CORE][3.3] Disallow arbitrary custom classpath with proxy user in cluster mode #41428

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
degant wants to merge 3 commits into from
Closed

[SPARK-41958][CORE][3.3] Disallow arbitrary custom classpath with proxy user in cluster mode #41428

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bfba577
Backporting fix for SPARK-41958 to 3.3 branch from #39474
peter-toth Jan 10, 2023
d8a1794
Default to true and add migration guide
degant Jun 2, 2023
dede104
Add missing newline
degant Jun 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,10 @@ private[spark] class SparkSubmit extends Logging {
val isKubernetesClient = clusterManager == KUBERNETES && deployMode == CLIENT
val isKubernetesClusterModeDriver = isKubernetesClient &&
sparkConf.getBoolean("spark.kubernetes.submitInDriver", false)
val isCustomClasspathInClusterModeDisallowed =
!sparkConf.get(ALLOW_CUSTOM_CLASSPATH_BY_PROXY_USER_IN_CLUSTER_MODE) &&
args.proxyUser != null &&
(isYarnCluster || isMesosCluster || isStandAloneCluster || isKubernetesCluster)

if (!isMesosCluster && !isStandAloneCluster) {
// Resolve maven dependencies if there are any and add classpath to jars. Add them to py-files
Expand Down Expand Up @@ -870,6 +874,13 @@ private[spark] class SparkSubmit extends Logging {

sparkConf.set("spark.app.submitTime", System.currentTimeMillis().toString)

if (childClasspath.nonEmpty && isCustomClasspathInClusterModeDisallowed) {
childClasspath.clear()
logWarning(s"Ignore classpath ${childClasspath.mkString(", ")} with proxy user specified " +
s"in Cluster mode when ${ALLOW_CUSTOM_CLASSPATH_BY_PROXY_USER_IN_CLUSTER_MODE.key} is " +
s"disabled")
}

(childArgs.toSeq, childClasspath.toSeq, sparkConf, childMainClass)
}

Expand Down Expand Up @@ -923,6 +934,10 @@ private[spark] class SparkSubmit extends Logging {
logInfo(s"Classpath elements:\n${childClasspath.mkString("\n")}")
logInfo("\n")
}
assert(!(args.deployMode == "cluster" && args.proxyUser != null && childClasspath.nonEmpty) ||
sparkConf.get(ALLOW_CUSTOM_CLASSPATH_BY_PROXY_USER_IN_CLUSTER_MODE),
s"Classpath of spark-submit should not change in cluster mode if proxy user is specified " +
s"when ${ALLOW_CUSTOM_CLASSPATH_BY_PROXY_USER_IN_CLUSTER_MODE.key} is disabled")
val loader = getSubmitClassLoader(sparkConf)
for (jar <- childClasspath) {
addJarToClasspath(jar, loader)
Expand Down
7 changes: 7 additions & 0 deletions core/src/main/scala/org/apache/spark/internal/config/package.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2355,4 +2355,11 @@ package object config {
.version("3.3.0")
.intConf
.createWithDefault(5)

private[spark] val ALLOW_CUSTOM_CLASSPATH_BY_PROXY_USER_IN_CLUSTER_MODE =
ConfigBuilder("spark.submit.proxyUser.allowCustomClasspathInClusterMode")
.internal()
.version("3.3.3")
.booleanConf
.createWithDefault(false)
Copy link
Member

@HyukjinKwon HyukjinKwon Jun 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should at least put this in the migration guide. The problem is that this is a breaking change so we didn't backport IIRC.

Copy link
Author

@degant degant Jun 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HyukjinKwon is this the one you're referring to: https://github.com/apache/spark/blob/master/docs/core-migration-guide.md?

  • What if we default the setting spark.submit.proxyUser.allowCustomClasspathInClusterMode to true in 3.3.3? Then it won't be a breaking change anymore
  • Shouldn't we also mention this property in the above migration guide from 3.3 to 3.4?

Copy link
Member

@dongjoon-hyun dongjoon-hyun Jun 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we can keep the existing behavior by having the default value true here in branch-3.3. Then, it removes the breaking change issue and allows some users utilize this based on their situation. Could you change the default value, @degant ?

Copy link
Member

@dongjoon-hyun dongjoon-hyun Jun 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, the additional migration guide is always a welcome idea.

Copy link
Author

@degant degant Jun 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dongjoon-hyun I can default it to true here. And does 3.3.3 make sense? Is that the next release from branch-3.3?

Can you also share, what you're expecting in the migration guide? I can make the push the changes in the same PR. Only for 3.3 to 3.4? or also for 3.3?

Copy link
Author

@degant degant Jun 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set the default value to true and added a note in the migration guide for 3.3. Please review when you get a chance @dongjoon-hyun and @HyukjinKwon, thanks!

I will raise another PR to add the migration guide for 3.4

}