Copilot commented on code in PR #7710:
URL: https://github.com/apache/kyuubi/pull/7710#discussion_r3915655378
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/KubernetesApplicationOperation.scala:
##########
@@ -217,14 +217,26 @@ class KubernetesApplicationOperation extends
ApplicationOperation with Logging {
}
private[kyuubi] def getKubernetesClientInitializeInfo(
- kyuubiConf: KyuubiConf): Seq[KubernetesInfo] = {
- kyuubiConf.get(KyuubiConf.KUBERNETES_CLIENT_INITIALIZE_LIST).map { init =>
- val (context, namespace) = init.split(":") match {
- case Array(ctx, ns) => (Some(ctx).filterNot(_.isEmpty),
Some(ns).filterNot(_.isEmpty))
- case Array(ctx) => (Some(ctx).filterNot(_.isEmpty), None)
- case _ => (None, None)
+ kyuubiConf: KyuubiConf,
+ environment: Map[String, String] = sys.env): Seq[KubernetesInfo] = {
+ val configuredInitializeInfo =
+ kyuubiConf.get(KyuubiConf.KUBERNETES_CLIENT_INITIALIZE_LIST).map { init
=>
+ val (context, namespace) = init.split(":") match {
+ case Array(ctx, ns) => (Some(ctx).filterNot(_.isEmpty),
Some(ns).filterNot(_.isEmpty))
+ case Array(ctx) => (Some(ctx).filterNot(_.isEmpty), None)
+ case _ => (None, None)
+ }
+ KubernetesInfo(context, namespace)
}
- KubernetesInfo(context, namespace)
+ if (configuredInitializeInfo.nonEmpty) {
+ configuredInitializeInfo
+ } else if (environment.contains(KUBERNETES_SERVICE_HOST) &&
+ environment.contains(KUBERNETES_SERVICE_PORT)) {
Review Comment:
The in-cluster detection relies solely on the presence of
`KUBERNETES_SERVICE_HOST`/`KUBERNETES_SERVICE_PORT`. If these env vars are
injected/set in a non-Kubernetes runtime, Kyuubi will treat the environment as
in-cluster and attempt in-cluster initialization unexpectedly. A more robust
check is to also validate a standard in-cluster signal (e.g.,
presence/readability of the service account token/CA files under
`/var/run/secrets/kubernetes.io/serviceaccount/`) before enabling the fallback.
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -1427,7 +1427,10 @@ object KyuubiConf {
.doc("The kubernetes client initialize list to register kubernetes
resource informers" +
" during Kyuubi server startup. This ensure the Kyuubi server is
promptly informed for" +
" any Kubernetes resource changes after startup. It is highly
recommend to set it for" +
Review Comment:
Grammar fixes in the updated doc string: change 'This ensure' to 'This
ensures', and 'highly recommend' to 'highly recommended'. Since this text is
surfaced into generated docs, correcting it here will improve user-facing
documentation.
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/KubernetesApplicationOperation.scala:
##########
@@ -217,14 +217,26 @@ class KubernetesApplicationOperation extends
ApplicationOperation with Logging {
}
private[kyuubi] def getKubernetesClientInitializeInfo(
- kyuubiConf: KyuubiConf): Seq[KubernetesInfo] = {
- kyuubiConf.get(KyuubiConf.KUBERNETES_CLIENT_INITIALIZE_LIST).map { init =>
- val (context, namespace) = init.split(":") match {
- case Array(ctx, ns) => (Some(ctx).filterNot(_.isEmpty),
Some(ns).filterNot(_.isEmpty))
- case Array(ctx) => (Some(ctx).filterNot(_.isEmpty), None)
- case _ => (None, None)
+ kyuubiConf: KyuubiConf,
+ environment: Map[String, String] = sys.env): Seq[KubernetesInfo] = {
+ val configuredInitializeInfo =
+ kyuubiConf.get(KyuubiConf.KUBERNETES_CLIENT_INITIALIZE_LIST).map { init
=>
+ val (context, namespace) = init.split(":") match {
+ case Array(ctx, ns) => (Some(ctx).filterNot(_.isEmpty),
Some(ns).filterNot(_.isEmpty))
+ case Array(ctx) => (Some(ctx).filterNot(_.isEmpty), None)
+ case _ => (None, None)
+ }
+ KubernetesInfo(context, namespace)
}
- KubernetesInfo(context, namespace)
+ if (configuredInitializeInfo.nonEmpty) {
+ configuredInitializeInfo
+ } else if (environment.contains(KUBERNETES_SERVICE_HOST) &&
+ environment.contains(KUBERNETES_SERVICE_PORT)) {
+ Seq(KubernetesInfo(
+ None,
+ Some(kyuubiConf.get(KyuubiConf.KUBERNETES_NAMESPACE))))
Review Comment:
In the in-cluster fallback branch, the namespace is wrapped with `Some(...)`
without filtering empty strings. If `kyuubi.kubernetes.namespace` is set to an
empty string, this will produce `Some(\"\")`, which is inconsistent with the
explicit-list parsing (which filters empty values) and can lead to
invalid/undesired informer initialization. Consider applying the same
`filterNot(_.isEmpty)` logic here (or constructing the `Option` via filtering)
so the behavior is consistent.
--
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]
