Skip to content
This repository was archived by the owner on Jan 9, 2020. It is now read-only.

Conversation

@mccheah
Copy link

@mccheah mccheah commented Mar 8, 2017

Closes #179

@mccheah
Copy link
Author

mccheah commented Mar 8, 2017

@foxish @robert3005 for review.

I don't quite know how to test this. Does Minikube support authentication with OAuth tokens instead of certificate and key files? I need this for an internal application but it seems like a good idea to have this option in general.

The other question is whether or not the token contents should be provided directly in the Spark configuration or if the token configurations should be passed through a file. I chose the latter because it seems more secure. The SparkConf is presented on the Spark UI page and can be logged if the user sets spark.logConf to true.

@robert3005
Copy link

Hmm, setting this on spark conf sounds like asking for trouble to me. I am not certain if there's a better place though. There's some sanitisation available in current release but the users need to enable it.

@mccheah
Copy link
Author

mccheah commented Mar 8, 2017

Well in this change we're not passing the token itself, but only the location on the submitter's disk. That way the token itself isn't ever logged anywhere.

Copy link

@robert3005 robert3005 left a comment

Choose a reason for hiding this comment

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

Can we maybe send the file across to the pods maybe instead of sparkconf?

""".stripMargin)
.stringConf
.createOptional

Choose a reason for hiding this comment

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

remove whitespace

}
sparkConf.get(KUBERNETES_OAUTH_TOKEN_FILE).foreach { f =>
val oauthTokenFile = new File(f)
require(oauthTokenFile.isFile, s"OAuth token file provided at $f does not exist or is" +

Choose a reason for hiding this comment

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

Can we move the whole message to the new line and not break it?

@robert3005
Copy link

Right, sorry I misread some other comment. Looks good now

@mccheah
Copy link
Author

mccheah commented Mar 9, 2017

The more I think about it the more I think it makes sense to be allowed to pass the token value via Spark configuration instead of through a file. The reason is that it allows users of the SparkLauncher API to avoid having to push the token data down to disk.

We can erase the token value from the Spark configuration after we use it.

@mccheah
Copy link
Author

mccheah commented Mar 9, 2017

@foxish is this reasonable?

@foxish
Copy link
Member

foxish commented Mar 13, 2017

@mccheah, the fabric8 kubernetes-client has support for using OAuth tokens instead of client-certs which is what we use against GKE, if the .kube/config file is setup appropriately. Do we need this explicitly as an option to Spark submit if fabric8 handles it for us?

@mccheah
Copy link
Author

mccheah commented Mar 13, 2017

It is useful to be able to provide the token dynamically instead of relying on a static configuration file. Multiple applications being submitted from the same host but from different users is an example.

@foxish
Copy link
Member

foxish commented Mar 13, 2017

My .kube/config contains many clusters and a mechanism by which I am authorized to talk to them. The way I switch which cluster to talk to is using https://kubernetes.io/docs/user-guide/kubectl/kubectl_config_use-context/. If we expect a user to get stdout/stderr using kubectl get logs from the driver pod, they do need to switch to the context in which that SparkJob is running. In that case, do we gain a lot from this? It's a minor change however, so, if you have a good reason (like automation around job submission to different clusters), I'm okay with this change.

@mccheah
Copy link
Author

mccheah commented Mar 13, 2017

One example use case is that the application submitter does not itself have the credentials, but rather is provided the credentials by some other party and the submitter wishes to forward those credentials through to spark-submit.

@foxish
Copy link
Member

foxish commented Mar 13, 2017

That sounds reasonable. LGTM.

@mccheah
Copy link
Author

mccheah commented Mar 13, 2017

@robert3005 good to merge?

@robert3005
Copy link

robert3005 commented Mar 13, 2017 via email

@mccheah mccheah merged commit 1aba361 into k8s-support-alternate-incremental Mar 13, 2017
@foxish foxish deleted the oauth-token branch March 13, 2017 21:52
ash211 pushed a commit that referenced this pull request Mar 16, 2017
* Allow providing an OAuth token for authenticating against k8s

* Organize imports

* Fix style

* Remove extra newline

* Use OAuth token data instead of a file.

(cherry picked from commit 1aba361)
foxish pushed a commit that referenced this pull request Jul 24, 2017
* Allow providing an OAuth token for authenticating against k8s

* Organize imports

* Fix style

* Remove extra newline

* Use OAuth token data instead of a file.

(cherry picked from commit 1aba361)
ifilonenko pushed a commit to ifilonenko/spark that referenced this pull request Feb 25, 2019
…-spark-on-k8s#180)

* Allow providing an OAuth token for authenticating against k8s

* Organize imports

* Fix style

* Remove extra newline

* Use OAuth token data instead of a file.

(cherry picked from commit 1aba361)
(cherry picked from commit 35724a3)
ifilonenko pushed a commit to ifilonenko/spark that referenced this pull request Feb 25, 2019
puneetloya pushed a commit to puneetloya/spark that referenced this pull request Mar 11, 2019
…-spark-on-k8s#180)

* Allow providing an OAuth token for authenticating against k8s

* Organize imports

* Fix style

* Remove extra newline

* Use OAuth token data instead of a file.

(cherry picked from commit 1aba361)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants