apache-spark-on-k8s / spark

Apache Spark enhanced with native Kubernetes scheduler back-end: NOTE this repository is being ARCHIVED as all new development for the kubernetes scheduler back-end is now on https://github.com/apache/spark/
https://spark.apache.org/
Apache License 2.0
612 stars 118 forks source link

remove camel case naming in kerberos secret names #612

Closed aberey closed 6 years ago

aberey commented 6 years ago

What changes were proposed in this pull request?

This is a bugfix for PR #540 (Basic Secure HDFS Support).

The HadoopKerberosKeytabResolverStep currently uses the constant KERBEROS_DELEGEGATION_TOKEN_SECRET_NAME when storing the Kerberos delegation token into a Kubernetes secret - this however causes a KubernetesClientException stating the following as secret names are supposed to adhere to RFC 1123:

a DNS-1123 subdomain must consist of lower case alphanumeric characters,
'-' or '.', and must start and end with an alphanumeric character (e.g.
'example.com', regex used for validation is
'[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')

The code in question is https://github.com/apache-spark-on-k8s/spark/blob/branch-2.2-kubernetes/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/submitsteps/hadoopsteps/HadoopKerberosKeytabResolverStep.scala#L102

The problem is that the current constants are using camel case, so I have changed the values to not use any uppercase characters anymore.

How was this patch tested?

This was tested manually (against a Kubernetes cluster with version v1.8.5), since the currently implemented unit tests only use mocks and the above Exception is only thrown when the actual Kubernetes API is called.

liyinan926 commented 6 years ago

@aberey can you bring the PR up-to-date with the base branch?

ifilonenko commented 6 years ago

Thank you for this! @aberey. This should be caught in the integration tests in the integration tests that I am writing.

aberey commented 6 years ago

@liyinan926 I just rebased the PR on the latest branch-2.2-kubernetes - is there any other branch I should base this on?

liyinan926 commented 6 years ago

Thanks @aberey merging.