-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(sdk): Add credentials to authenticate with ServiceAccountTokens. Part of #5138 #5676
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.
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
Changes from all commits
8dc6d26
2ad9689
f09ae02
f88f519
4cea89c
c34bb2a
ff0a51b
3589814
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
# Copyright 2021 Arrikto Inc. | ||
# | ||
# 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. | ||
|
||
from ._tokencredentialsbase import TokenCredentialsBase, read_token_from_file | ||
from ._satvolumecredentials import ServiceAccountTokenVolumeCredentials | ||
|
||
KF_PIPELINES_SA_TOKEN_ENV = "KF_PIPELINES_SA_TOKEN_PATH" | ||
KF_PIPELINES_SA_TOKEN_PATH = "/var/run/secrets/kubeflow/pipelines/token" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
# Copyright 2021 Arrikto Inc. | ||
# | ||
# 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 os | ||
import logging | ||
|
||
from kubernetes.client import configuration | ||
|
||
from kfp import auth | ||
|
||
|
||
class ServiceAccountTokenVolumeCredentials(auth.TokenCredentialsBase): | ||
"""Audience-bound ServiceAccountToken in the local filesystem. | ||
|
||
This is a credentials interface for audience-bound ServiceAccountTokens | ||
found in the local filesystem, that get refreshed by the kubelet. | ||
|
||
The constructor of the class expects a filesystem path. | ||
If not provided, it uses the path stored in the environment variable | ||
defined in ``auth.KF_PIPELINES_SA_TOKEN_ENV``. | ||
If the environment variable is also empty, it falls back to the path | ||
specified in ``auth.KF_PIPELINES_SA_TOKEN_PATH``. | ||
|
||
This method of authentication is meant for use inside a Kubernetes cluster. | ||
|
||
Relevant documentation: | ||
https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/#service-account-token-volume-projection | ||
""" | ||
|
||
def __init__(self, path=None): | ||
self._token_path = (path | ||
or os.getenv(auth.KF_PIPELINES_SA_TOKEN_ENV) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @elikatsis. Shouldn't this be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be like this based on the code you wrote above, but that's not the case in this branch. At least not yet There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool. Actually, I misunderstood |
||
or auth.KF_PIPELINES_SA_TOKEN_PATH) | ||
|
||
def _get_token(self): | ||
token = None | ||
try: | ||
token = auth.read_token_from_file(self._token_path) | ||
except OSError as e: | ||
logging.error("Failed to read a token from file '%s' (%s).", | ||
self._token_path, str(e)) | ||
raise | ||
return token | ||
|
||
def refresh_api_key_hook(self, config: configuration.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() |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,31 +14,34 @@ | |
|
||
import abc | ||
|
||
from kubernetes.client.configuration import Configuration | ||
|
||
|
||
__all__ = [ | ||
"TokenCredentialsBase", | ||
] | ||
from kubernetes.client import configuration | ||
|
||
|
||
class TokenCredentialsBase(abc.ABC): | ||
|
||
def refresh_api_key_hook(self, config: Configuration): | ||
@abc.abstractmethod | ||
def refresh_api_key_hook(self, config: configuration.Configuration): | ||
"""Refresh the api key. | ||
|
||
This is a helper function for registering token refresh with swagger | ||
generated clients. | ||
|
||
All classes that inherit from TokenCredentialsBase must implement this | ||
method to refresh the credentials. | ||
|
||
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() | ||
|
||
|
||
def read_token_from_file(path=None): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't this better be placed in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The plan is to use this function in multiple places and implementations of TokenCredentials. For instance, we do use it internally for another implementation of credentials we've got |
||
"""Read a token found in some file.""" | ||
token = None | ||
with open(path, "r") as f: | ||
token = f.read().strip() | ||
return token |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker for this PR, from #5945 context, we should not add bearer authorization either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, I think we should write a base class for bearer authorization.
Then e.g. others can write a base class for basic authentication
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Bobgy unfortunately I don't think we can do this.
If you check the links I've included in #5138 (comment) explaining the
Configuration
's codebase,refresh_api_key_hook
is only triggered whenauthorization
is inapi_key
(source).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great catch! I didn't know the limitation. Let's leave that use case in the backlog and get this PR merged first.