add login, logout functionality for OpenShift cluster

[MichaelClifford]

This PR allows users of the SDK to login and out of an OpenShift cluster as part of their "cluster.up()" call by adding an Authentication object to their cluster config.

This includes a new file auth that contains a number of Authentication classes that can be used depending on the users specific authentication needs.

[MichaelClifford]

closes #31

[MichaelClifford]

@Maxusmusti @atinsood @erikerlandson

Re-did the authentication and login set up. Take a look and let me know if this is good to go. Thanks for the initial feedback :)

[atinsood]

@MichaelClifford yep, this looks 💯 , 3 questions

• is there a reason why you implemented the logout method in the base authentication and not keep it generic. the assumption is that in some cases we might not be using oc and there might be a different way of implementing auth logout
• I think we can make the Authentication class an python abstract class rather than a concrete class https://docs.python.org/3/library/abc.html
• I was thinking if we can make the login/logout operate within context manager https://docs.python.org/3/library/contextlib.html so rather than doing login/logout as 2 different steps, we can simply do with authentication() or something like that.

but all those are nitpicks, the first pass LGTM

[MichaelClifford]

• is there a reason why you implemented the logout method in the base authentication and not keep it generic. the assumption is that in some cases we might not be using oc and there might be a different way of implementing auth logout

This is a good question, that I thought about it a bit and I think gets to a larger question: Would we want to support cases were OpenShift is not the target platform? If so, we can make it more generic. Otherwise, no matter how you login to the cluster you can always log out the same way, so no need to re-implement.

• I think we can make the Authentication class an python abstract class rather than a concrete class https://docs.python.org/3/library/abc.html

Sure, I can make it an abstract class. But what exactly would be the benefit of doing that for our use case?

• I was thinking if we can make the login/logout operate within context manager https://docs.python.org/3/library/contextlib.html so rather than doing login/logout as 2 different steps, we can simply do with authentication() or something like that.

Will that work in an interactive notebook environment if your work spans multiple cells? I'll look into it as an option for job submission workflows. I think that would be cool. But I don't think it would work for notebook/interactive workflows (but let me know if im wrong 😄 )

[Maxusmusti]

Looks great, just needs a rebase (will bring in the format checking), and make to add docstrings/type hints to the new files/classes/functions

[atinsood]

This is a good question, that I thought about it a bit and I think gets to a larger question: Would we want to support cases were OpenShift is not the target platform? If so, we can make it more generic. Otherwise, no matter how you login to the cluster you can always log out the same way, so no need to re-implement.

so I was thinking of a use case when you don't have direct access to the oc client but the login/out is happening remotely. or say if the underlying client uses a different binary than oc

[atinsood]

Sure, I can make it an abstract class. But what exactly would be the benefit of doing that for our use case?

I think its more of a signal to external contributors going forward that they are supposed to implement their own authentication.

[atinsood]

Will that work in an interactive notebook environment if your work spans multiple cells? I'll look into it as an option for job submission workflows. I think that would be cool. But I don't think it would work for notebook/interactive workflows (but let me know if im wrong 😄 )

I think you should be ok. we are already doing something liek this https://github.com/project-codeflare/codeflare-sdk/pull/39/files#diff-5706a96c5346dd54a22370211260411363c4ed41d798fddf240eb8983b1366eaR99

so I am assuming it will be simialr to that

[Maxusmusti]

Looks great, no architectural suggestions or anything, just a couple minor nits

[Maxusmusti]

LGTM!

[atinsood]

In my opinion, I don't think that we want to login and out on every single call. This is not how a user would naturally interact with a cluster, and I'm not sure of the benefits of doing it that way.

Also, on logout() your token can expire, which would break subsequent login in calls.

It seems reasonable to me that you would want to login at the start of a sessions (on cluster.up()) and logout at the end (on cluster.down())

yeah I think its a fair point. lets go with what we have.

[MichaelClifford]

Awesome. Thanks for the feedback all. 😃