feat(sdk): Add credentials to authenticate with ServiceAccountTokens. Part of #5138 #5676

elikatsis · 2021-05-18T15:44:29Z

Description of your changes:
In continuation of #5287, the changes of this PR and their rationale are described in detail in #5138 (comment). More specifically:

Introduce the ServiceAccountTokenVolumeCredentials class to authenticate using ServiceAccountTokens, which is described in detail in Add authentication with ServiceAccountToken #5138
If the user provides no credentials and the client is running inside a pod, attempt to use ServiceAccountTokenVolumeCredentials

This is part of #5138.

/assign @Bobgy
/assign @chensun
/assign @elikatsis
/cc @yanniszark
/cc @StefanoFioravanzo

Checklist:

The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.

sdk/python/kfp/_credentials.py

sdk/python/kfp/_client.py

chensun · 2021-05-19T00:01:40Z

sdk/python/kfp/_credentials.py

 __all__ = [
    "TokenCredentialsBase",
+    "ML_PIPELINE_SA_TOKEN_ENV",
+    "ML_PIPELINE_SA_TOKEN_PATH",


Do we really need to expose these two constants?

The two constants are interfaces to configure the SDK, so it seems reasonable to me to make them public.

nit: I'd probably suggest a different naming. what do you think about KF_PIPELINES_SA_TOKEN_ENV and KF_PIPELINES_SA_TOKEN_PATH? to align with

pipelines/sdk/python/kfp/_client.py

Line 84 in 8b97562

KF_PIPELINES_ENDPOINT_ENV = 'KF_PIPELINES_ENDPOINT'

Renaming done

I think you forgot to remove these, when the new env vars were added.

Bobgy · 2021-05-20T08:33:01Z

sdk/python/kfp/_client.py

+
+    try:
+        token = credentials.get_token()
+    except OSError as e:


credentials should be the interface, if there are ignorable errors, they should handle that by themselves.

We cannot catch exceptions thrown when refreshing credentials

That's a good thought. I moved the error handling back to the credentials, and it returns None.
This part, I'm not very fond of it. Now this method checks if not credentials.get_token() to abort setting default credentials. If you have some better idea, feel free to suggest

@elikatsis I'd prefer that we only call the credentials.refresh_api_key hook and avoid this get_token.

For example, if a auth implementation overrides refresh_api_key method and adds token to cookies, what should get_token do? Therefore, I think we should avoid get_token altogether, it's an implementation detail.

@Bobgy, I see what you mean. I can't tell if this solution works with cookies, because I don't know what it requires.

If you check the description of the code & the links I have included in #5138 (comment), search for get_api_key_with_prefix, all of this is tied to headers and especially the authorization header.

Thinking while writing, refresh_api_key_hook can modify any field of the Configuration as each implementation desires. So this can help with the cookie settings, as long as you also configure some authorization header or something?

I'll look into it a bit more and will comment back. I believe we can do as you propose, but I want to make sure it's solid.

@Bobgy I moved the abstraction to the refresh_api_key_hook method instead of get_token. Please tell me what you think.

Bobgy · 2021-05-20T08:38:27Z

sdk/python/kfp/_credentials.py

+    return token
+
+
+class ServiceAccountTokenVolumeCredentials(TokenCredentialsBase):


Can we move this class to a separate package? To make the separation clear between interface and implementation.

Also all implementations should be optional.

I split everything to distinct modules under the kfp.auth module.

I don't understand what it is that you're referring to by this:

Also all implementations should be optional

sdk/python/kfp/_client.py

Bobgy · 2021-05-31T13:21:03Z

sdk/python/kfp/_credentials.py

 __all__ = [
    "TokenCredentialsBase",
+    "ML_PIPELINE_SA_TOKEN_ENV",
+    "ML_PIPELINE_SA_TOKEN_PATH",


The two constants are interfaces to configure the SDK, so it seems reasonable to me to make them public.

nit: I'd probably suggest a different naming. what do you think about KF_PIPELINES_SA_TOKEN_ENV and KF_PIPELINES_SA_TOKEN_PATH? to align with

pipelines/sdk/python/kfp/_client.py

Line 84 in 8b97562

KF_PIPELINES_ENDPOINT_ENV = 'KF_PIPELINES_ENDPOINT'

sdk/python/kfp/_credentials.py

Bobgy · 2021-05-31T13:33:12Z

See #5138 (comment), I'm now leaning towards making this method the canonical way for authenticating inside a K8s cluster. Therefore, it'll be beneficial also adding a method like add_gcp_secret -- probably add_kfp_api_token to SDK.

/cc @chensun @neuromage what do you think about this?

elikatsis · 2021-06-02T17:44:28Z

/retest

davidspek · 2021-06-09T12:21:23Z

@elikatsis It looks like the errors causing the CI to fail are the same as what I am experiencing when importing the kfp package.

ImportError: cannot import name 'auth' from partially initialized module 'kfp' (most likely due to a circular import) (/opt/conda/lib/python3.8/site-packages/kfp/__init__.py)

I'll look into this a bit more after lunch.

davidspek · 2021-06-10T16:14:56Z

sdk/python/kfp/auth/_tokencredentialsbase.py

+        raise NotImplementedError()
+
+
+def read_token_from_file(path=None):


Can't this better be placed in _satvolumecredentials.py, since this is where it is used? This can then also be removed from the import in __init__.py.

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

Bobgy · 2021-06-21T06:33:09Z

/retest

Bobgy · 2021-06-21T06:34:48Z

Hi @elikatsis, as @davidspek pointed out, there's a circular import causing tests to fail.
Any updates?

davidspek · 2021-06-21T10:27:20Z

I've spent a few hours to try and figure out what was causing the circular import and fix it without any luck.

midhun1998 · 2021-06-21T16:13:43Z

Hi @davidspek . I think this line could be the reason for circular import link (line 18 in _satvolumecredentials.py). I was able to resolve it by removing imports from kfp/auth/__init__.py and manually importing TokenCredentialsBase and read_token_from_file in kfp/auth/_satvolumecredentials.py.

davidspek · 2021-06-23T09:57:09Z

@midhun1998 That was my first idea as well, but for some reason it didn't work for me. I'll try doing this again in a fresh environment.

davidspek · 2021-06-23T10:42:47Z

@midhun1998 I might be doing something wrong, but when I apply the changes you suggested I still get the following error when I run import kfp:

---------------------------------------------------------------------------
ImportError                               Traceback (most recent call last)
<ipython-input-1-ee5b3cc3ae38> in <module>
----> 1 import kfp

/opt/conda/lib/python3.8/site-packages/kfp/__init__.py in <module>
     22 from . import containers
     23 from . import dsl
---> 24 from . import auth
     25 from ._client import Client
     26 from ._config import *

ImportError: cannot import name 'auth' from partially initialized module 'kfp' (most likely due to a circular import) (/opt/conda/lib/python3.8/site-packages/kfp/__init__.py)

midhun1998 · 2021-06-23T16:38:37Z

@davidspek Pardon me I'm not an expert in this field. But this worked out for me. I rearranged import in auth/__init__.py to

KF_PIPELINES_SA_TOKEN_ENV = "KF_PIPELINES_SA_TOKEN_PATH"
KF_PIPELINES_SA_TOKEN_PATH = "/var/run/secrets/kubeflow/pipelines/token"

from ._tokencredentialsbase import TokenCredentialsBase, read_token_from_file
from ._satvolumecredentials import ServiceAccountTokenVolumeCredentials

and kfp/auth/_satvolumecredentials.py looks like this

from . import KF_PIPELINES_SA_TOKEN_ENV
from . import KF_PIPELINES_SA_TOKEN_PATH
from . import TokenCredentialsBase, read_token_from_file

I know there is redundancy in imports. But this is the only way I could overcome circular imports. Please ignore if this is not a valid suggestion.

midhun1998 · 2021-06-23T16:48:59Z

sdk/python/kfp/auth/_satvolumecredentials.py

+
+    def __init__(self, path=None):
+        self._token_path = (path
+                            or os.getenv(auth.KF_PIPELINES_SA_TOKEN_ENV)


Hi @elikatsis. Shouldn't this be os.getenv(KF_PIPELINES_SA_TOKEN_ENV) ?

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

Cool. Actually, I misunderstood KF_PIPELINES_SA_TOKEN_ENV to be pod environment variable but instead it is refering to the Path. I get that now. Thanks @elikatsis 🚀

neuromage · 2021-06-23T16:49:35Z

Can you use absolute imports? We're in the process of migrating all our imports to absolute ones in the SDK (e.g. #5891 ).

elikatsis · 2021-06-24T13:24:08Z

Sorry for the delayed answer. I can't seem to be able to reproduce this error :/

elikatsis · 2021-06-24T13:25:17Z

@davidspek @midhun1998 what Python version are you trying with? Maybe I can reproduce your env

davidspek · 2021-06-24T13:29:03Z

@elikatsis I'm using Python 3.8. I'll retest with the exact code mentioned above later today as well.

Part of kubeflow#5138 This is a subclass of TokenCredentials and implements the logic of retrieving a service account token provided via a ProjectedVolume. The 'get_token()' method reads and doesn't store the token as the kubelet is refreshing it quite often. Relevant docs: https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/#service-account-token-volume-projection

If the KFP client detects it's running inside a pod and the user hasn't provided any credentials, it now attempts to set up credentials based on a projected service account token.

Also change some names and values to avoid "ml-pipeline" references.

* Let ServiceAccountTokenVolumeCredentials handle OSErrors internally * Have the default-creds-setter only check if credentials provide a valid value * Use Configuration's 'refresh_api_key_hook' instead of having duplicate code

elikatsis · 2021-06-24T14:22:00Z

I managed to reproduce it. I had forgotten to add kfp.auth in the setup.py file as an exported package.
I believe I couldn't reproduce it because I've been working with editable installations.

andreyvelich · 2021-07-06T13:21:51Z

Thank you for implementing this @elikatsis!
I was able to use this SDK changes to run Kubeflow e2e mnist Pipeline: #5433 using this PodDefault:

apiVersion: kubeflow.org/v1alpha1
kind: PodDefault
metadata:
  name: access-kf-pipeline
  namespace: kubeflow-user-example-com
spec:
  desc: Allow access to KFP
  selector:
    matchLabels:
      access-kf-pipeline: "true"
  volumeMounts:
    - mountPath: /var/run/secrets/kubeflow/pipelines
      name: volume-kf-pipeline-token
      readOnly: true
  volumes:
    - name: volume-kf-pipeline-token
      projected:
        sources:
          - serviceAccountToken:
              path: token
              expirationSeconds: 7200
              audience: pipelines.kubeflow.org
  env:
    - name: KF_PIPELINES_SA_TOKEN_PATH
      value: /var/run/secrets/kubeflow/pipelines/token

@elikatsis @Bobgy Is there any chance that we merge these changes in the next KFP SDK version ?

Bobgy · 2021-07-07T05:37:21Z

Thanks for the update, I missed notification that @elikatsis has fixed tests.

Will do another round of review asap

Bobgy · 2021-07-07T07:43:10Z

/lgtm
Only one non blocking nit picking.

@chensun can you help the review too?

Bobgy

Again, thank you for the perseverance and contributing this feature! Really appreciate because this is the top one user ask!

Bobgy · 2021-07-07T07:37:05Z

sdk/python/kfp/_client.py

    elif credentials:
-      token = credentials.get_token()
+      config.api_key['authorization'] = 'placeholder'
+      config.api_key_prefix['authorization'] = 'Bearer'


Not a blocker for this PR, from #5945 context, we should not add bearer authorization either.

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

@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 when authorization is in api_key (source).

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.

chensun · 2021-07-08T23:01:27Z

/lgtm
/approve

google-oss-robot · 2021-07-08T23:01:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chensun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~sdk/OWNERS~~ [chensun]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

google-oss-robot assigned Bobgy, chensun and elikatsis May 18, 2021

google-oss-robot requested review from StefanoFioravanzo and yanniszark May 18, 2021 15:44

google-cla bot added the cla: yes label May 18, 2021

google-oss-robot added the size/M label May 18, 2021

neuromage reviewed May 18, 2021

View reviewed changes

sdk/python/kfp/_credentials.py Show resolved Hide resolved

sdk/python/kfp/_credentials.py Outdated Show resolved Hide resolved

sdk/python/kfp/_client.py Outdated Show resolved Hide resolved

chensun reviewed May 19, 2021

View reviewed changes

Bobgy reviewed May 20, 2021

View reviewed changes

Bobgy reviewed May 21, 2021

View reviewed changes

sdk/python/kfp/_client.py Outdated Show resolved Hide resolved

Bobgy mentioned this pull request May 31, 2021

Add authentication with ServiceAccountToken #5138

Closed

Bobgy reviewed May 31, 2021

View reviewed changes

elikatsis force-pushed the feature-client-creds-sa-token-volume branch from eb31a63 to 9b97849 Compare June 2, 2021 17:06

google-oss-robot added size/L and removed size/M labels Jun 2, 2021

andreyvelich mentioned this pull request Jun 3, 2021

fix(components): Refactor Kubeflow E2E mnist example #5433

Merged

2 tasks

johnugeorge mentioned this pull request Jun 8, 2021

Release 1.3.1 kubeflow/manifests#1890

Closed

16 tasks

davidspek reviewed Jun 10, 2021

View reviewed changes

midhun1998 reviewed Jun 23, 2021

View reviewed changes

elikatsis added 7 commits June 24, 2021 17:21

Attemp to use credentials when in-cluster

2ad9689

If the KFP client detects it's running inside a pod and the user hasn't provided any credentials, it now attempts to set up credentials based on a projected service account token.

Move credentials to kfp.auth module

f09ae02

Also change some names and values to avoid "ml-pipeline" references.

Import kfp.auth lazily during the Client init

f88f519

Have credentials handle errors internally

4cea89c

* Let ServiceAccountTokenVolumeCredentials handle OSErrors internally * Have the default-creds-setter only check if credentials provide a valid value * Use Configuration's 'refresh_api_key_hook' instead of having duplicate code

Remove dead code

c34bb2a

Add kfp.auth in the setup.py

ff0a51b

elikatsis force-pushed the feature-client-creds-sa-token-volume branch from 9b97849 to ff0a51b Compare June 24, 2021 14:21

Move abstraction to the 'refresh_api_key_hook' method

3589814

google-oss-robot added the lgtm label Jul 7, 2021

Bobgy reviewed Jul 7, 2021

View reviewed changes

google-oss-robot added the approved label Jul 8, 2021

google-oss-robot merged commit 52b0792 into kubeflow:master Jul 8, 2021

Bobgy mentioned this pull request Jul 30, 2021

[Multi User] failed to call kfp.Client().create_run_from_pipeline_func in in-cluster juypter notebook #4440

Closed

midhun1998 mentioned this pull request Aug 7, 2021

KFP CLIENT CALL #6254

Closed

elikatsis deleted the feature-client-creds-sa-token-volume branch October 4, 2021 12:25

		return token


		class ServiceAccountTokenVolumeCredentials(TokenCredentialsBase):

		raise NotImplementedError()


		def read_token_from_file(path=None):

feat(sdk): Add credentials to authenticate with ServiceAccountTokens. Part of #5138 #5676

feat(sdk): Add credentials to authenticate with ServiceAccountTokens. Part of #5138 #5676

Uh oh!

Conversation

elikatsis commented May 18, 2021 • edited by Bobgy Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Bobgy commented May 31, 2021

Uh oh!

elikatsis commented Jun 2, 2021

Uh oh!

davidspek commented Jun 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bobgy commented Jun 21, 2021

Uh oh!

Bobgy commented Jun 21, 2021

Uh oh!

davidspek commented Jun 21, 2021

Uh oh!

midhun1998 commented Jun 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidspek commented Jun 23, 2021

Uh oh!

davidspek commented Jun 23, 2021

Uh oh!

midhun1998 commented Jun 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neuromage commented Jun 23, 2021

Uh oh!

elikatsis commented Jun 24, 2021

Uh oh!

elikatsis commented Jun 24, 2021

Uh oh!

davidspek commented Jun 24, 2021

Uh oh!

elikatsis commented Jun 24, 2021

Uh oh!

andreyvelich commented Jul 6, 2021

Uh oh!

Bobgy commented Jul 7, 2021

elikatsis commented May 18, 2021 •

edited by Bobgy

Loading

midhun1998 commented Jun 21, 2021 •

edited

Loading