sunchao commented on code in PR #57350: URL: https://github.com/apache/spark/pull/57350#discussion_r3693005821
########## resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/DriverUIServiceFeatureStep.scala: ########## @@ -0,0 +1,167 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.spark.deploy.k8s.features + +import scala.jdk.CollectionConverters._ + +import io.fabric8.kubernetes.api.model.{HasMetadata, ServiceBuilder} + +import org.apache.spark.deploy.k8s.{KubernetesDriverConf, SparkPod} +import org.apache.spark.deploy.k8s.Config.{ + KUBERNETES_DRIVER_SERVICE_IP_FAMILIES, + KUBERNETES_DRIVER_SERVICE_IP_FAMILY_POLICY, + KUBERNETES_DRIVER_UI_SERVICE_ENABLED, + KUBERNETES_DRIVER_UI_SERVICE_NAME, + KUBERNETES_DRIVER_UI_SERVICE_TYPE +} +import org.apache.spark.deploy.k8s.Constants._ +import org.apache.spark.internal.{config, Logging} + +/** + * Optionally provisions a dedicated Kubernetes Service exposing only the Spark driver's Web UI + * port. + * + * When the user requests a random UI port (`spark.ui.port=0`), the actual port is unknown at + * submission time, so the Service's `targetPort` uses a placeholder. To avoid routing traffic + * to the wrong endpoint during that window (in `hostNetwork` mode a pod endpoint is the node IP, + * so the placeholder `targetPort` could reach an unrelated driver co-located on the same node), + * the Service is created *without* a selector, leaving it endpointless. Once the driver's Jetty + * server has bound, `K8sDriverUIServicePatcher` patches the selector and the actual `targetPort` + * together. When the port is fixed up front, the Service is created with its selector and final + * `targetPort` immediately. + */ +private[spark] class DriverUIServiceFeatureStep(kubernetesConf: KubernetesDriverConf) + extends KubernetesFeatureConfigStep with Logging { + import DriverUIServiceFeatureStep._ + + private val enabled = kubernetesConf.get(KUBERNETES_DRIVER_UI_SERVICE_ENABLED) + private lazy val serviceType = kubernetesConf.get(KUBERNETES_DRIVER_UI_SERVICE_TYPE) + private lazy val configuredUIPort = kubernetesConf.get(config.UI.UI_PORT) + + /** + * Port value used when building the Service. When the user has requested a random UI port + * (`spark.ui.port=0`), the actual port is only known after the driver's Jetty server binds, + * so we substitute the default UI port (typically 4040) purely as a placeholder to satisfy + * Kubernetes' Service port validation (must be > 0). After the driver JVM starts, + * [[org.apache.spark.scheduler.cluster.k8s.K8sDriverUIServicePatcher]] updates the Service's + * `targetPort` to the real bound port. + */ + private lazy val servicePort: Int = if (configuredUIPort == 0) { + config.UI.UI_PORT.defaultValue.get + } else { + configuredUIPort + } + + private lazy val serviceName: String = kubernetesConf.get(KUBERNETES_DRIVER_UI_SERVICE_NAME) Review Comment: [P1] Reject custom Service names already owned by another application A static `spark.kubernetes.driver.ui.service.name` can be reused by two live Spark applications, but this override is accepted without checking whether the Service already exists or who owns it. During submission, `KubernetesClientApplication` calls `addOwnerReference(createdDriverPod, ...)` and then `.forceConflicts().serverSideApply()`; `addOwnerReference` replaces the owner references with the new driver Pod. Consequently, the second submission can forcibly transfer the existing UI Service's selector, application labels, and controller ownership to itself, disrupting the first application and causing the shared Service to be garbage-collected with the wrong Pod. Please use create-only semantics or verify existing ownership before applying, reject collisions with this application's mandatory driver Service, and add cross-application collision coverage. ########## resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/DriverUIServiceFeatureStep.scala: ########## @@ -0,0 +1,167 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.spark.deploy.k8s.features + +import scala.jdk.CollectionConverters._ + +import io.fabric8.kubernetes.api.model.{HasMetadata, ServiceBuilder} + +import org.apache.spark.deploy.k8s.{KubernetesDriverConf, SparkPod} +import org.apache.spark.deploy.k8s.Config.{ + KUBERNETES_DRIVER_SERVICE_IP_FAMILIES, + KUBERNETES_DRIVER_SERVICE_IP_FAMILY_POLICY, + KUBERNETES_DRIVER_UI_SERVICE_ENABLED, + KUBERNETES_DRIVER_UI_SERVICE_NAME, + KUBERNETES_DRIVER_UI_SERVICE_TYPE +} +import org.apache.spark.deploy.k8s.Constants._ +import org.apache.spark.internal.{config, Logging} + +/** + * Optionally provisions a dedicated Kubernetes Service exposing only the Spark driver's Web UI + * port. + * + * When the user requests a random UI port (`spark.ui.port=0`), the actual port is unknown at + * submission time, so the Service's `targetPort` uses a placeholder. To avoid routing traffic + * to the wrong endpoint during that window (in `hostNetwork` mode a pod endpoint is the node IP, + * so the placeholder `targetPort` could reach an unrelated driver co-located on the same node), + * the Service is created *without* a selector, leaving it endpointless. Once the driver's Jetty + * server has bound, `K8sDriverUIServicePatcher` patches the selector and the actual `targetPort` + * together. When the port is fixed up front, the Service is created with its selector and final + * `targetPort` immediately. + */ +private[spark] class DriverUIServiceFeatureStep(kubernetesConf: KubernetesDriverConf) + extends KubernetesFeatureConfigStep with Logging { + import DriverUIServiceFeatureStep._ + + private val enabled = kubernetesConf.get(KUBERNETES_DRIVER_UI_SERVICE_ENABLED) + private lazy val serviceType = kubernetesConf.get(KUBERNETES_DRIVER_UI_SERVICE_TYPE) + private lazy val configuredUIPort = kubernetesConf.get(config.UI.UI_PORT) + + /** + * Port value used when building the Service. When the user has requested a random UI port + * (`spark.ui.port=0`), the actual port is only known after the driver's Jetty server binds, + * so we substitute the default UI port (typically 4040) purely as a placeholder to satisfy + * Kubernetes' Service port validation (must be > 0). After the driver JVM starts, + * [[org.apache.spark.scheduler.cluster.k8s.K8sDriverUIServicePatcher]] updates the Service's + * `targetPort` to the real bound port. + */ + private lazy val servicePort: Int = if (configuredUIPort == 0) { + config.UI.UI_PORT.defaultValue.get + } else { + configuredUIPort + } + + private lazy val serviceName: String = kubernetesConf.get(KUBERNETES_DRIVER_UI_SERVICE_NAME) + .getOrElse(kubernetesConf.driverUIServiceName) + + // The UI Service reuses the driver Service IP family settings to keep the same IP family. + private lazy val ipFamilyPolicy = kubernetesConf.get(KUBERNETES_DRIVER_SERVICE_IP_FAMILY_POLICY) + private lazy val ipFamilies = + kubernetesConf.get(KUBERNETES_DRIVER_SERVICE_IP_FAMILIES).split(",").toList.asJava + + override def configurePod(pod: SparkPod): SparkPod = pod + + override def getAdditionalPodSystemProperties(): Map[String, String] = { + // These properties exist solely to drive the runtime patch, which only happens for a random + // port. A fixed-port Service is already complete, so nothing needs to reach the driver. + if (enabled && configuredUIPort == 0) { + Map( + KUBERNETES_DRIVER_UI_SERVICE_NAME_INTERNAL -> serviceName, + KUBERNETES_DRIVER_UI_SERVICE_PORT_INTERNAL -> servicePort.toString, + KUBERNETES_DRIVER_UI_SERVICE_SELECTOR_INTERNAL -> encodeSelector(kubernetesConf.labels)) + } else { + Map.empty + } + } + + override def getAdditionalKubernetesResources(): Seq[HasMetadata] = { + if (!enabled) return Seq.empty + + val spec = new ServiceBuilder() + .withNewMetadata() + .withName(serviceName) + .addToAnnotations(kubernetesConf.serviceAnnotations.asJava) + .addToLabels(SPARK_APP_ID_LABEL, kubernetesConf.appId) + .addToLabels(kubernetesConf.serviceLabels.asJava) + .endMetadata() + .withNewSpec() + .withType(serviceType) + .withIpFamilyPolicy(ipFamilyPolicy) + .withIpFamilies(ipFamilies) + .addNewPort() + .withName(UI_PORT_NAME) + .withPort(servicePort) + .withNewTargetPort(servicePort) Review Comment: [P2] Target the HTTPS connector when UI TLS is enabled For `spark.ui.port=4040` with `spark.ssl.ui.enabled=true`, Jetty binds HTTPS on `spark.ssl.ui.port` or 4440 by default, and port 4040 becomes an HTTP-to-HTTPS redirect connector. This Service nevertheless targets and exposes only 4040; TLS clients therefore reach a plaintext connector, and HTTP clients are redirected to HTTPS port 4440/8443, which the Service does not expose. `SparkUI.boundPort` already resolves to the actual secure connector, but the current implementation only consults it when the configured UI port equals zero. Please reconcile fixed-port Services against `SparkUI.boundPort` too, and cover both default and explicitly configured TLS ports. ########## resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/DriverUIServiceFeatureStep.scala: ########## @@ -0,0 +1,167 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.spark.deploy.k8s.features + +import scala.jdk.CollectionConverters._ + +import io.fabric8.kubernetes.api.model.{HasMetadata, ServiceBuilder} + +import org.apache.spark.deploy.k8s.{KubernetesDriverConf, SparkPod} +import org.apache.spark.deploy.k8s.Config.{ + KUBERNETES_DRIVER_SERVICE_IP_FAMILIES, + KUBERNETES_DRIVER_SERVICE_IP_FAMILY_POLICY, + KUBERNETES_DRIVER_UI_SERVICE_ENABLED, + KUBERNETES_DRIVER_UI_SERVICE_NAME, + KUBERNETES_DRIVER_UI_SERVICE_TYPE +} +import org.apache.spark.deploy.k8s.Constants._ +import org.apache.spark.internal.{config, Logging} + +/** + * Optionally provisions a dedicated Kubernetes Service exposing only the Spark driver's Web UI + * port. + * + * When the user requests a random UI port (`spark.ui.port=0`), the actual port is unknown at + * submission time, so the Service's `targetPort` uses a placeholder. To avoid routing traffic + * to the wrong endpoint during that window (in `hostNetwork` mode a pod endpoint is the node IP, + * so the placeholder `targetPort` could reach an unrelated driver co-located on the same node), + * the Service is created *without* a selector, leaving it endpointless. Once the driver's Jetty + * server has bound, `K8sDriverUIServicePatcher` patches the selector and the actual `targetPort` + * together. When the port is fixed up front, the Service is created with its selector and final + * `targetPort` immediately. + */ +private[spark] class DriverUIServiceFeatureStep(kubernetesConf: KubernetesDriverConf) + extends KubernetesFeatureConfigStep with Logging { + import DriverUIServiceFeatureStep._ + + private val enabled = kubernetesConf.get(KUBERNETES_DRIVER_UI_SERVICE_ENABLED) + private lazy val serviceType = kubernetesConf.get(KUBERNETES_DRIVER_UI_SERVICE_TYPE) + private lazy val configuredUIPort = kubernetesConf.get(config.UI.UI_PORT) + + /** + * Port value used when building the Service. When the user has requested a random UI port + * (`spark.ui.port=0`), the actual port is only known after the driver's Jetty server binds, + * so we substitute the default UI port (typically 4040) purely as a placeholder to satisfy + * Kubernetes' Service port validation (must be > 0). After the driver JVM starts, + * [[org.apache.spark.scheduler.cluster.k8s.K8sDriverUIServicePatcher]] updates the Service's + * `targetPort` to the real bound port. + */ + private lazy val servicePort: Int = if (configuredUIPort == 0) { + config.UI.UI_PORT.defaultValue.get + } else { + configuredUIPort + } + + private lazy val serviceName: String = kubernetesConf.get(KUBERNETES_DRIVER_UI_SERVICE_NAME) + .getOrElse(kubernetesConf.driverUIServiceName) + + // The UI Service reuses the driver Service IP family settings to keep the same IP family. + private lazy val ipFamilyPolicy = kubernetesConf.get(KUBERNETES_DRIVER_SERVICE_IP_FAMILY_POLICY) + private lazy val ipFamilies = + kubernetesConf.get(KUBERNETES_DRIVER_SERVICE_IP_FAMILIES).split(",").toList.asJava + + override def configurePod(pod: SparkPod): SparkPod = pod + + override def getAdditionalPodSystemProperties(): Map[String, String] = { + // These properties exist solely to drive the runtime patch, which only happens for a random + // port. A fixed-port Service is already complete, so nothing needs to reach the driver. + if (enabled && configuredUIPort == 0) { Review Comment: [P1] Reconcile fixed UI ports after bind collisions too A nonzero `spark.ui.port` is not guaranteed to be the actual bound port: `JettyUtils.startJettyServer` delegates to `Utils.startServiceOnPort`, which retries 4040 at 4041 and higher after a collision. With two `hostNetwork` drivers requesting 4040, the second driver can bind 4041, but this condition omits all runtime patch metadata and the new Service permanently selects `nodeIP:4040`—the first application's UI. `NodePort`/`LoadBalancer` therefore exposes the wrong application's UI, while ordinary pod networking leaves the second UI unreachable. Please emit reconciliation metadata for fixed ports as well and install the selector only after `SparkUI.boundPort` is known. ########## resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/DriverUIServiceFeatureStep.scala: ########## @@ -0,0 +1,167 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.spark.deploy.k8s.features + +import scala.jdk.CollectionConverters._ + +import io.fabric8.kubernetes.api.model.{HasMetadata, ServiceBuilder} + +import org.apache.spark.deploy.k8s.{KubernetesDriverConf, SparkPod} +import org.apache.spark.deploy.k8s.Config.{ + KUBERNETES_DRIVER_SERVICE_IP_FAMILIES, + KUBERNETES_DRIVER_SERVICE_IP_FAMILY_POLICY, + KUBERNETES_DRIVER_UI_SERVICE_ENABLED, + KUBERNETES_DRIVER_UI_SERVICE_NAME, + KUBERNETES_DRIVER_UI_SERVICE_TYPE +} +import org.apache.spark.deploy.k8s.Constants._ +import org.apache.spark.internal.{config, Logging} + +/** + * Optionally provisions a dedicated Kubernetes Service exposing only the Spark driver's Web UI + * port. + * + * When the user requests a random UI port (`spark.ui.port=0`), the actual port is unknown at + * submission time, so the Service's `targetPort` uses a placeholder. To avoid routing traffic + * to the wrong endpoint during that window (in `hostNetwork` mode a pod endpoint is the node IP, + * so the placeholder `targetPort` could reach an unrelated driver co-located on the same node), + * the Service is created *without* a selector, leaving it endpointless. Once the driver's Jetty + * server has bound, `K8sDriverUIServicePatcher` patches the selector and the actual `targetPort` + * together. When the port is fixed up front, the Service is created with its selector and final + * `targetPort` immediately. + */ +private[spark] class DriverUIServiceFeatureStep(kubernetesConf: KubernetesDriverConf) + extends KubernetesFeatureConfigStep with Logging { + import DriverUIServiceFeatureStep._ + + private val enabled = kubernetesConf.get(KUBERNETES_DRIVER_UI_SERVICE_ENABLED) Review Comment: [P1] Do not expose a UI Service when the Spark UI is disabled This only checks `spark.kubernetes.driver.ui.service.enabled`; it never checks `spark.ui.enabled`. With `spark.ui.enabled=false`, `spark.ui.port=4040`, this feature enabled, and `hostNetwork=true`, Spark still creates an immediately selectable Service targeting the node's port 4040 even though `SparkContext` never starts a UI. If another application on that node owns 4040, this application's Service routes to that unrelated UI, and `NodePort`/`LoadBalancer` can publish it externally. The runtime cannot repair this because `sc.ui` is `None`. Please suppress the Service or reject this configuration whenever the Spark UI is disabled. -- 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]
