Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,14 @@ private[spark] object Config extends Logging {
.toSequence
.createWithDefault(Nil)

val KUBERNETES_DRIVER_POD_EXCLUDED_FEATURE_STEPS =
ConfigBuilder("spark.kubernetes.driver.pod.excludedFeatureSteps")
.doc("Class names to exclude from driver pod feature steps. Comma separated.")
Comment on lines +407 to +409
Copy link
Member

Choose a reason for hiding this comment

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

Can this be used to exclude Spark internal feature steps too?

A few questions.

Is there any feature step that are optional and users probably want to exclude?

Different to #30206 which is used to add extra feature step out from Spark internal feature steps. If we allow users to exclude any feature steps, won't it be a possible (security) issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for review, @viirya

Yes, this is designed to do that as I wrote in the PR description.

Can this be used to exclude Spark internal feature steps too?

There are two common cases.

Is there any feature step that are optional and users probably want to exclude?

  1. For built-in steps, a user can exclude KerberosConfDriverFeatureStep when they don't use Kerberos system. This is also useful
  2. For user-provided steps, when a production system provide many steps via spark.kubernetes.(driver|executor).pod.featureSteps, a user can selectively exclude one or two with this new configuration.

The steps provide credential-like information. So, it will be unable to access the backend system. It's a user-own risk.

If we allow users to exclude any feature steps, won't it be a possible (security) issue?

Copy link
Member

Choose a reason for hiding this comment

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

For built-in steps, a user can exclude KerberosConfDriverFeatureStep when they don't use Kerberos system. This is also useful
For user-provided steps, when a production system provide many steps via spark.kubernetes.(driver|executor).pod.featureSteps, a user can selectively exclude one or two with this new configuration.

I think it maybe only useful for internal steps. For user-provided steps, users can simply pull out unnecessary steps from spark.kubernetes.(driver|executor).pod.featureSteps. No need to add steps and exclude them using two lists.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you. You are right because it's logical. For the user-defined ones, the main case is the time when the platform team provides spark.kubernetes.(driver|executor).pod.featureSteps by default and a user wants to customize their jobs.

.version("4.1.0")
.stringConf
.toSequence
.createWithDefault(Nil)

val KUBERNETES_EXECUTOR_POD_FEATURE_STEPS =
ConfigBuilder("spark.kubernetes.executor.pod.featureSteps")
.doc("Class name of an extra executor pod feature step implementing " +
Expand All @@ -416,6 +424,14 @@ private[spark] object Config extends Logging {
.toSequence
.createWithDefault(Nil)

val KUBERNETES_EXECUTOR_POD_EXCLUDED_FEATURE_STEPS =
ConfigBuilder("spark.kubernetes.executor.pod.excludedFeatureSteps")
.doc("Class name to exclude from executor pod feature steps. Comma separated.")
.version("4.1.0")
.stringConf
.toSequence
.createWithDefault(Nil)

val KUBERNETES_EXECUTOR_DECOMMISSION_LABEL =
ConfigBuilder("spark.kubernetes.executor.decommissionLabel")
.doc("Label to apply to a pod which is being decommissioned." +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ class KubernetesDriverBuilder {
}
}

val features = Seq(
val allFeatures = Seq(
new BasicDriverFeatureStep(conf),
new DriverKubernetesCredentialsFeatureStep(conf),
new DriverServiceFeatureStep(conf),
Expand All @@ -85,6 +85,9 @@ class KubernetesDriverBuilder {
new PodTemplateConfigMapStep(conf),
new LocalDirsFeatureStep(conf)) ++ userFeatures

val features = allFeatures.filterNot(f =>
conf.get(Config.KUBERNETES_DRIVER_POD_EXCLUDED_FEATURE_STEPS).contains(f.getClass.getName))

val spec = KubernetesDriverSpec(
initialPod,
driverPreKubernetesResources = Seq.empty,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ private[spark] class KubernetesExecutorBuilder {
}
}

val features = Seq(
val allFeatures = Seq(
new BasicExecutorFeatureStep(conf, secMgr, resourceProfile),
new ExecutorKubernetesCredentialsFeatureStep(conf),
new MountSecretsFeatureStep(conf),
Expand All @@ -74,6 +74,9 @@ private[spark] class KubernetesExecutorBuilder {
new HadoopConfExecutorFeatureStep(conf),
new LocalDirsFeatureStep(conf)) ++ userFeatures

val features = allFeatures.filterNot(f =>
conf.get(Config.KUBERNETES_EXECUTOR_POD_EXCLUDED_FEATURE_STEPS).contains(f.getClass.getName))

val spec = KubernetesExecutorSpec(
initialPod,
executorKubernetesResources = Seq.empty)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ abstract class PodBuilderSuite extends SparkFunSuite {

protected def roleSpecificSchedulerNameConf: ConfigEntry[_]

protected def excludedFeatureStepsConf: ConfigEntry[_]

protected def userFeatureStepsConf: ConfigEntry[_]

protected def userFeatureStepWithExpectedAnnotation: (String, String)
Expand Down Expand Up @@ -91,6 +93,21 @@ abstract class PodBuilderSuite extends SparkFunSuite {
assert(pod.container.getVolumeMounts.asScala.exists(_.getName == "so_long_two"))
}

test("SPARK-52830: exclude a feature step") {
val client = mockKubernetesClient()
val sparkConf = baseConf.clone()
.set(excludedFeatureStepsConf.key,
"org.apache.spark.deploy.k8s.TestStepTwo")
.set(userFeatureStepsConf.key,
"org.apache.spark.deploy.k8s.TestStepTwo," +
"org.apache.spark.deploy.k8s.TestStep")
.set(templateFileConf.key, "template-file.yaml")
val pod = buildPod(sparkConf, client)
verifyPod(pod)
assert(pod.container.getVolumeMounts.asScala.exists(_.getName == "so_long"))
assert(!pod.container.getVolumeMounts.asScala.exists(_.getName == "so_long_two"))
}

test("SPARK-37145: configure a custom test step with base config") {
val client = mockKubernetesClient()
val sparkConf = baseConf.clone()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ class KubernetesDriverBuilderSuite extends PodBuilderSuite {
Config.KUBERNETES_DRIVER_SCHEDULER_NAME
}

override protected def excludedFeatureStepsConf: ConfigEntry[_] = {
Config.KUBERNETES_DRIVER_POD_EXCLUDED_FEATURE_STEPS
}

override protected def userFeatureStepsConf: ConfigEntry[_] = {
Config.KUBERNETES_DRIVER_POD_FEATURE_STEPS
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ class KubernetesExecutorBuilderSuite extends PodBuilderSuite {
Config.KUBERNETES_EXECUTOR_SCHEDULER_NAME
}

override protected def excludedFeatureStepsConf: ConfigEntry[_] = {
Config.KUBERNETES_EXECUTOR_POD_EXCLUDED_FEATURE_STEPS
}

override protected def userFeatureStepsConf: ConfigEntry[_] = {
Config.KUBERNETES_EXECUTOR_POD_FEATURE_STEPS
}
Expand Down