Updated authentication for Kubernetes

[Bobbins228]

Update authentication for Kubernetes

references #185
#146
A user can now authenticate using their own k8s config file.

A user can authenticate with their token and server address.

All methods that used config.load_kube_config() have been updated to check if a user has already specified their own config file so it isn't overridden.

How to test the new authentication methods

git pull https://github.com/Bobbins228/codeflare-sdk.git
git checkout update-authentication
Run poetry build within the codeflare-sdk folder to build your own wheel.
Install the sdk with pip install codeflare_sdk-0.0.0.dev0-py3-none-any.whl
Import the necessary classes.

# Import pieces from codeflare-sdk
from codeflare_sdk.cluster.cluster import Cluster, ClusterConfiguration
from codeflare_sdk.cluster.auth import TokenAuthentication, KubeConfigFileAuthentication

Loading a user's own config file

auth = KubeConfigFileAuthentication(
            kube_config_path="/path/to/config"
        )
auth.load_kube_config()

Authenticating with token + server

Authenticating with certificate

auth = TokenAuthentication(
    token = "TOKEN",
    server = "SERVER",
    skip_tls=False,
    ca_cert_path="path/to/ca_bundle.crt"
)
auth.login()

Authenticating by skipping tls verification

auth = TokenAuthentication(
    token = "TOKEN",
    server = "SERVER",
    skip_tls=True
)
auth.login()

Logging out
auth.logout()

[Bobbins228]

I have changed how the login works by using the kubernetes API to handle an authenticated user. This is independent of whether a user has a loaded config file already.

When a user logs out the default config file is loaded again.

[Maxusmusti]

Looking good, just a few questions and nits to start

[carsonmh]

Tested this out and works as expected. LGTM!

[Maxusmusti]

Quick issue I noticed when testing. This is the current behavior when namespace cannot be found:

• get_current_namespace() returns: "Unable to find current namespace please specify with namespace=<your_current_namespace>"
• ClusterConfiguration() (without setting namespace) works and generates yaml when passed into cluster with namespace=Unable to find current namespace please specify with namespace=<your_current_namespace>

What we want is probably something more along the lines of:

• get_current_namespace() returns None in that case and prints a message like "Unable to find current namespace"
• Cluster(ClusterConfiguration() fails if the namespace in the configuration is set to None, and tells the user "Unable to find current namespace, please specify in your ClusterConfiguration with namespace=<your_current_namespace>"

[Maxusmusti]

So like after line 71 in cluster.py just check if namespace is still None, and if so then fail with that message

[MichaelClifford]

Overall looks very good 👍

One thing though that is something of a simple change but, will impact a lot of the implementation. 😬

Can we make api_config_handler() and config_check() generic auth functions instead of class methods? It looks odd to invoke both authentication type classes on every call, especially when a users should only ever be using one anyways.

Is there any reason we can't separate these from the classes?

[MichaelClifford]

Thanks for the changes! :)

LGTM

[Maxusmusti]

Everything seemed to work great, except one critical edge case:

• When doing a token login, I can just pass in no token at all (or no server), and it leads to all sorts of broken behavior (those should not have defaults)
• Also, when I pass in an incorrect token, it will still tell me I've "logged in", only to fail later down the line

Also, how are you obtaining/generating the ca cert that you are passing in when testing the Token approach? I'd like to test that as well, have just been testing with tls-skip for now.

EDIT: Realized if you're on RHEL the default path is probably where your ca cert bundle is, not sure how to do it on mac os though...

[Bobbins228]

Everything seemed to work great, except one critical edge case:

• When doing a token login, I can just pass in no token at all (or no server), and it leads to all sorts of broken behavior (those should not have defaults)
• Also, when I pass in an incorrect token, it will still tell me I've "logged in", only to fail later down the line

Also, how are you obtaining/generating the ca cert that you are passing in when testing the Token approach? I'd like to test that as well, have just been testing with tls-skip for now.

EDIT: Realized if you're on RHEL the default path is probably where your ca cert bundle is, not sure how to do it on mac os though...

That was a great catch about default values for the token. I'll update the PR.
Yeah the default path is where the ca cert bundle is for RHEL and Openshift.

[Bobbins228]

Looking into how we can pass an incorrect token and catch that and there doesn't seem to be any errors for when you set up the configuration even with bad credentials. Only when the api instance is used will an error be thrown e.g.cluster.up().

I found this, we can call get_api_group() to test if the credentials are correct and add in an error if that fails.

[Maxusmusti]

@Bobbins228 the get_api_group solution seems great to me!

[Maxusmusti]

LGTM