Github user vanzin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/21669#discussion_r223871862
  
    --- Diff: 
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/KerberosConfDriverFeatureStep.scala
 ---
    @@ -0,0 +1,179 @@
    +/*
    + * 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 io.fabric8.kubernetes.api.model.HasMetadata
    +
    +import org.apache.spark.deploy.SparkHadoopUtil
    +import org.apache.spark.deploy.k8s.{KubernetesConf, KubernetesUtils, 
SparkPod}
    +import org.apache.spark.deploy.k8s.Config._
    +import org.apache.spark.deploy.k8s.Constants._
    +import org.apache.spark.deploy.k8s.KubernetesDriverSpecificConf
    +import org.apache.spark.deploy.k8s.features.hadooputils._
    +import org.apache.spark.internal.Logging
    +
    + /**
    +  * Runs the necessary Hadoop-based logic based on Kerberos configs and 
the presence of the
    +  * HADOOP_CONF_DIR. This runs various bootstrap methods defined in 
HadoopBootstrapUtil.
    +  */
    +private[spark] class KerberosConfDriverFeatureStep(
    +   kubernetesConf: KubernetesConf[KubernetesDriverSpecificConf])
    +   extends KubernetesFeatureConfigStep with Logging {
    +
    +   require(kubernetesConf.hadoopConfSpec.isDefined,
    +     "Ensure that HADOOP_CONF_DIR is defined either via env or a 
pre-defined ConfigMap")
    +   private val hadoopConfDirSpec = kubernetesConf.hadoopConfSpec.get
    +   private val conf = kubernetesConf.sparkConf
    +   private val principal = 
conf.get(org.apache.spark.internal.config.PRINCIPAL)
    +   private val keytab = conf.get(org.apache.spark.internal.config.KEYTAB)
    +   private val existingSecretName = 
conf.get(KUBERNETES_KERBEROS_DT_SECRET_NAME)
    +   private val existingSecretItemKey = 
conf.get(KUBERNETES_KERBEROS_DT_SECRET_ITEM_KEY)
    +   private val krb5File = conf.get(KUBERNETES_KERBEROS_KRB5_FILE)
    +   private val krb5CMap = conf.get(KUBERNETES_KERBEROS_KRB5_CONFIG_MAP)
    +   private val kubeTokenManager = kubernetesConf.tokenManager(conf,
    +     SparkHadoopUtil.get.newConfiguration(conf))
    +   private val isKerberosEnabled =
    +     (hadoopConfDirSpec.hadoopConfDir.isDefined && 
kubeTokenManager.isSecurityEnabled) ||
    +       (hadoopConfDirSpec.hadoopConfigMapName.isDefined &&
    +         (krb5File.isDefined || krb5CMap.isDefined))
    +
    +   require(keytab.isEmpty || isKerberosEnabled,
    +     "You must enable Kerberos support if you are specifying a Kerberos 
Keytab")
    +
    +   require(existingSecretName.isEmpty || isKerberosEnabled,
    +     "You must enable Kerberos support if you are specifying a Kerberos 
Secret")
    +
    +   require((krb5File.isEmpty || krb5CMap.isEmpty) || isKerberosEnabled,
    +     "You must specify either a krb5 file location or a ConfigMap with a 
krb5 file")
    --- End diff --
    
    I'm not sure I understand this condition. What's it testing really?
    
    - if either a file or map is defined, it passes
    - if neither a file nor map is defined, it passes if kerberos is enabled
    
    So either the condition is a bit off or the error message is wrong?
    
    Also on the subject of these two options, is your goal to enforce that one 
of them is defined if kerberos is enabled? I think we should not enforce that, 
since I'd expect admins to create images with a pre-defined krb5.conf instead 
of letting users futz with that config.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to