Skip to content

feat(sdk): Introduce the token credentials interface. Part of #4683 #5287

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 18, 2021
Merged

feat(sdk): Introduce the token credentials interface. Part of #4683 #5287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
069d835
Introduce TokenCredentials abstract class
elikatsis Mar 11, 2021
a7ca540
Configure credentials when initializing KFP client
elikatsis Mar 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions sdk/python/kfp/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,4 @@
from ._config import *
from ._local_client import LocalClient
from ._runners import *
from ._credentials import *
Copy link
Contributor

Choose a reason for hiding this comment

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

The credentials seem Client-specific, so I'm not sure we should import them in the root kfp package.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @Ark-kun, I just saw this. Do you have some specific structure in mind?

12 changes: 9 additions & 3 deletions sdk/python/kfp/_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,9 @@ class Client(object):
proxy: HTTP or HTTPS proxy server
ssl_ca_cert: Cert for proxy
kube_context: String name of context within kubeconfig to use, defaults to the current-context set within kubeconfig.
credentials: A TokenCredentialsBase object which provides the logic to
populate the requests with credentials to authenticate against the API
server.
"""

# in-cluster DNS name of the pipeline service
Expand All @@ -126,7 +129,7 @@ class Client(object):
LOCAL_KFP_CONTEXT = os.path.expanduser('~/.config/kfp/context.json')

# TODO: Wrap the configurations for different authentication methods.
def __init__(self, host=None, client_id=None, namespace='kubeflow', other_client_id=None, other_client_secret=None, existing_token=None, cookies=None, proxy=None, ssl_ca_cert=None, kube_context=None):
def __init__(self, host=None, client_id=None, namespace='kubeflow', other_client_id=None, other_client_secret=None, existing_token=None, cookies=None, proxy=None, ssl_ca_cert=None, kube_context=None, credentials=None):
"""Create a new instance of kfp client.
"""
host = host or os.environ.get(KF_PIPELINES_ENDPOINT_ENV)
Expand All @@ -135,7 +138,7 @@ def __init__(self, host=None, client_id=None, namespace='kubeflow', other_client
other_client_id = other_client_id or os.environ.get(KF_PIPELINES_APP_OAUTH2_CLIENT_ID_ENV)
other_client_secret = other_client_secret or os.environ.get(KF_PIPELINES_APP_OAUTH2_CLIENT_SECRET_ENV)

config = self._load_config(host, client_id, namespace, other_client_id, other_client_secret, existing_token, proxy, ssl_ca_cert, kube_context)
config = self._load_config(host, client_id, namespace, other_client_id, other_client_secret, existing_token, proxy, ssl_ca_cert, kube_context, credentials)
# Save the loaded API client configuration, as a reference if update is
# needed.
self._load_context_setting_or_default()
Expand All @@ -160,7 +163,7 @@ def __init__(self, host=None, client_id=None, namespace='kubeflow', other_client
except FileNotFoundError:
logging.info('Failed to automatically set namespace.', exc_info=True)

def _load_config(self, host, client_id, namespace, other_client_id, other_client_secret, existing_token, proxy, ssl_ca_cert, kube_context):
def _load_config(self, host, client_id, namespace, other_client_id, other_client_secret, existing_token, proxy, ssl_ca_cert, kube_context, credentials):
config = kfp_server_api.configuration.Configuration()

if proxy:
Expand Down Expand Up @@ -215,6 +218,9 @@ def _load_config(self, host, client_id, namespace, other_client_id, other_client
elif self._is_inverse_proxy_host(host):
token = get_gcp_access_token()
self._is_refresh_token = False
elif credentials:
token = credentials.get_token()
config.refresh_api_key_hook = credentials.refresh_api_key_hook
Comment on lines +221 to +223
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can move this branch higher, probably only after existing_token. The other branches should be refactored to use the credentials interface afterwards.


if token:
config.api_key['authorization'] = token
Expand Down
44 changes: 44 additions & 0 deletions sdk/python/kfp/_credentials.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# Copyright 2021 Arrikto Inc.
Copy link
Member

Choose a reason for hiding this comment

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

Please use the standard header: "Copyright 2021 The Kubeflow Authors"

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, all Google LLC headers have been changed to "The Kubeflow Authors".
Not a blocker, if you can update existing Arrikto Inc headers also to "The Kubeflow Authors" in a separate PR, that would be even better.

Copy link
Member Author

Choose a reason for hiding this comment

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

@chensun @Bobgy this is a great initiative and we plan on changing all the copyrights as well.

However I think something is missing from this effort: an AUTHORS file. I've also left a comment in the issue: #5470 (comment)

Is it ok if we move forward like this and change the headers after we resolve this issue? (Or we have this file somewhere and I've missed it?)

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM, we can do it separately too

#
# Licensed 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.

import abc

from kubernetes.client.configuration import Configuration


__all__ = [
"TokenCredentialsBase",
]


class TokenCredentialsBase(abc.ABC):

def refresh_api_key_hook(self, config: Configuration):
"""Refresh the api key.

This is a helper function for registering token refresh with swagger
generated clients.

Args:
config (kubernetes.client.configuration.Configuration):
The configuration object that the client uses.

The Configuration object of the kubernetes client's is the same
with kfp_server_api.configuration.Configuration.
"""
config.api_key["authorization"] = self.get_token()

@abc.abstractmethod
def get_token(self):
raise NotImplementedError()
Comment on lines +42 to +44
Copy link
Contributor

Choose a reason for hiding this comment

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

Because we refresh api_key every time before sending a request, I feel like this method in interface is not necessary. It's enough having just refresh_api_key_hook.

Copy link
Contributor

@Bobgy Bobgy May 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We might want to rename that method, what do you think about "update_auth_token(api_config)"

Copy link
Member Author

Choose a reason for hiding this comment

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

@Bobgy

It's enough having just refresh_api_key_hook.

I think removing the get_token() method [in favor of a different unique method which unifies functionalities] is error prone.
We don't want users to be messing with the refresh_api_key_hook() method (unless they have a strong reason to do so). What it does (setting the key for the authorization header) is very specific and important for the functionality.

Users should only be declaring the logic with which the client can retrieve[/get/read/generate] the token. The rest is implementation detail (and following the codebase as described in my issue comment, the K8s client defines it strictly enough).

Plus, we have thought that this name is a good choice because the Configuation object calls it that way itself (configuration.refresh_api_key_hook).

Am I explaining it properly? Do the above make any sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want users to be messing with the refresh_api_key_hook() method (unless they have a strong reason to do so). What it does (setting the key for the authorization header) is very specific and important for the functionality.

I'm not fully confident to say so, e.g. https://www.kubeflow.org/docs/distributions/aws/pipeline/#authenticate-kubeflow-pipeline-using-sdk-inside-cluster needs a cookie to authenticate. Should that be avoided or sth potentially needed for other scenarios as well?

Rethinking about this, providing a default refresh_api_key_hook implementation while allowing people to override seems reasonable too, so I think I'm OK with this now.

The configuration object is auto-generated code, so we do not have a naming choice. We might consider change the auto-generator, so it's not necessarily a name we need to stick to.
Anyway, I feel like it's not a bad choice either.