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

elikatsis · 2021-03-11T19:54:08Z

Description of your changes:
The changes of this PR and their rationale are described in detail in #5138 (comment).

In a nutshell:

Introduce the TokenCredentials abstract class, as described in sdk/client/auth - refactor GCP auth into a module #4683
~~Introduce the ServiceAccountTokenVolumeCredentials class to authenticate using ServiceAccountTokens, which is part of Add authentication with ServiceAccountToken #5138~~
Extend the Client to (optionally) expect a credentials object
~~If the user provides no credentials and the client is running inside a pod, attempt to use ServiceAccountTokenVolumeCredentials~~

/assign @Bobgy
/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.

Do you want this pull request (PR) cherry-picked into the current release branch?

Learn more about cherry-picking updates into the release branch.

Bobgy · 2021-03-19T02:55:00Z

/assign @chensun @Bobgy

Bobgy · 2021-03-19T02:56:11Z

This does not block the release, but we should better start reviewing and reaching a consensus

Bobgy

I don't like the idea of depending on serviceaccount projection very much, because the SDK would depend on external configuration for each pod it runs in.

I wonder if you ever considered using TokenRequest API directly, that will make the SDK usable anywhere. As long as the service account has RBAC permission of TokenRequest API, which seems to be a reasonable default for both KFP pipelines and notebooks.

Bobgy · 2021-03-24T08:32:11Z

sdk/python/kfp/_client.py

+    elif credentials:
+      token = credentials.get_token()
+      config.refresh_api_key_hook = credentials.refresh_api_key_hook


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.

elikatsis · 2021-03-24T09:44:59Z

I wonder if you ever considered using TokenRequest API directly, that will make the SDK usable anywhere. As long as the service account has RBAC permission of TokenRequest API, which seems to be a reasonable default for both KFP pipelines and notebooks.

We have and we've used it in the past. However, projected volumes are native to K8s and they don't require any API call. Also they are available for kubernetes >= 12.0.0, so we need to hack our way to it.

I don't like the idea of depending on serviceaccount projection very much, because the SDK would depend on external configuration for each pod it runs in.

We can definitely implement both and have a heuristic to use one or another by default (e.g.: If ML_PIPELINE_SA_TOKEN_ENV is set and a file under that path exists, use projected volume creds. Otherwise, use TokenRequest creds).

Bobgy · 2021-03-24T23:26:06Z

I wonder if you ever considered using TokenRequest API directly, that will make the SDK usable anywhere. As long as the service account has RBAC permission of TokenRequest API, which seems to be a reasonable default for both KFP pipelines and notebooks.

We have and we've used it in the past. However, projected volumes are native to K8s and they don't require any API call. Also they are available for kubernetes >= 12.0.0, so we need to hack our way to it.

My understanding is that kubelet make the API calls for us if they are configured. So I don't see a big difference.

The kubernetes>=12.0.0 requirement is a good point, maybe we can add an version detection error message in this case. We've already updated dependency range to allow 12.0.0 just recently.

I don't like the idea of depending on serviceaccount projection very much, because the SDK would depend on external configuration for each pod it runs in.

We can definitely implement both and have a heuristic to use one or another by default (e.g.: If ML_PIPELINE_SA_TOKEN_ENV is set and a file under that path exists, use projected volume creds. Otherwise, use TokenRequest creds).

That makes sense to me, can you come up with a plan? What do you like to implement first?

nit: my personal opinion is to get rid of ml pipeline wherever we can, let's stick to kubeflow pipelines branding.

andreyvelich · 2021-04-03T02:08:11Z

Thank you so much for implementing this @elikatsis.
I was able to run namespace isolated Pipeline from the Kubeflow Notebook in the Kubeflow 1.3 release with some modifications:

I made few changes in the KFP SDK from this PR. @Bobgy Any chance that we merge this PR in the KFP 1.5.0 release ?
I added env variable TOKEN_REVIEW_AUDIENCE: ml-pipeline to the KFP API Server YAML. Do we want to add this modification to the upstream ? Then, users can deploy PodDefault which @elikatsis mentioned here: Add authentication with ServiceAccountToken #5138 (comment) and execute namespaced Pipelines from the Kubeflow Notebooks.

/cc @yanniszark

Ark-kun · 2021-04-09T03:15:40Z

sdk/python/kfp/__init__.py

@@ -25,3 +25,4 @@
 from ._config import *
 from ._local_client import LocalClient
 from ._runners import *
+from ._credentials import *


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

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

elikatsis · 2021-04-13T20:15:23Z

We found out that a bug occurs when read_sa_token() returns None: kubeflow-kale/kale#276

This happens when the path does not exist as well. It seems that open("...", "r") doesn't raise an error when the path does not exist, so we will need to check.

This should be inline with #5287 (comment)

Bobgy · 2021-04-23T09:08:31Z

@elikatsis with Kubeflow 1.3 manifests released, I think we will have more time to finish this.

What do you think about separating the PR into 2?

build the auth module interface
build one implementation of the auth module that you prefer

with 1. merged, we can parallely implement what we prefer, e.g. I'd go with #5287 (review), because we want KFP to run as independently as possible. We do not like depending on pod defaults webhook.

elikatsis · 2021-05-06T16:09:12Z

@Bobgy, hi there! Sorry for the late reply, I had full cycles and after that some public holidays.

Sure I will split the PR in two per your suggestion and I'll ping you.

elikatsis · 2021-05-06T19:24:23Z

@Bobgy

Let's use this PR to introduce the abstract class for credentials. I updated the first message scratching off the items that we don't tackle in this PR.

After this, we will PR the credentials using projected tokens (that is, tokens that find themselves in the container's filesystem). We can then discuss heuristics to choose between the implementation you propose (TokenRequest API) and the projected one, for the client to use some credentials by default for the in-cluster case.

Feel free to suggest different structure.

Bobgy · 2021-05-07T13:46:11Z

/assign @chensun
Do you want to have a look besides my review?

chensun · 2021-05-08T05:00:38Z

sdk/python/kfp/_credentials.py

@@ -0,0 +1,37 @@
+# Copyright 2021 Arrikto Inc.


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.

@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?)

SGTM, we can do it separately too

sdk/python/kfp/_client.py

chensun · 2021-05-08T05:20:39Z

sdk/python/kfp/_credentials.py

+
+class TokenCredentials(object):
+
+    __metaclass__ = abc.ABCMeta


__metaclass__= ... is the Python2 style, and IIRC, it's actually ignored in Python3.

In Python3, you should use this instead:

class TokenCredentials(metaclass=abc.ABCMeta): ...

Ah! Thanks for catching this, it had totally slipped.

We need an abstract class here, not a metaclass. The proper way to define an abstract class, correct me if I'm wrong, is class TokenCredentials(abc.ABC), so let's do this.

Yes, both syntax work the same for defining an abstract class. abc.ABC is a helper class that is new in Python 3.4, which is fine here.
https://docs.python.org/3/library/abc.html#abc.ABC

sdk/python/kfp/_credentials.py

chensun · 2021-05-08T05:27:26Z

sdk/python/kfp/_credentials.py

+]
+
+
+class TokenCredentials(object):


nit: What do you think about naming it TokenCredentialsProvider or TokenCredentialsBase or TokenCredentialsMeta?

Providing more options, WDUT about AuthProvider?

From the first suggestions TokenCredentialsBase sounds good.

AuthProvider is more of a whole backend service that performs authentication, rather than a client-side component that ensures the requests contain the required credentials to authenticate against the provider.

Let's go with TokenCredentialsBase, but ofc feel free to elaborate!

I don't have strong opinions, TokenCredentialsBase SGTM

chensun · 2021-05-08T05:30:40Z

/assign @chensun
Do you want to have a look besides my review?

Thank you @Bobgy for bringing this to my attention. I left a couple comments.

Bobgy · 2021-05-08T10:46:23Z

sdk/python/kfp/_credentials.py

@@ -0,0 +1,37 @@
+# Copyright 2021 Arrikto Inc.


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.

Bobgy · 2021-05-08T10:48:14Z

sdk/python/kfp/_credentials.py

+]
+
+
+class TokenCredentials(object):


Providing more options, WDUT about AuthProvider?

Bobgy · 2021-05-08T10:55:29Z

sdk/python/kfp/_credentials.py

+    @abc.abstractmethod
+    def get_token(self):
+        raise NotImplementedError()


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.

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

@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?

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.

Bobgy · 2021-05-10T09:26:12Z

/retest

As presented in kubeflow#4683, in this commit we introduce a TokenCredentials abstract class which encapsulates the retrieval and refresh of token credentials. The reason we are using a class for the credentials is the fact that usually tokens are short-lived and the client needs to refresh them. All subclasses should define a 'get_token()' method responsible for fetching and refreshing (if needed) a token for authentication.

Bobgy

/lgtm

leaving to @chensun for a final approval

Bobgy · 2021-05-12T06:34:48Z

sdk/python/kfp/_credentials.py

@@ -0,0 +1,37 @@
+# Copyright 2021 Arrikto Inc.


SGTM, we can do it separately too

chensun · 2021-05-18T07:30:05Z

/lgtm
/approve

Thanks!

google-oss-robot · 2021-05-18T07:30:15Z

[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 and elikatsis Mar 11, 2021

google-oss-robot requested review from StefanoFioravanzo and yanniszark March 11, 2021 19:54

google-cla bot added the cla: yes label Mar 11, 2021

google-oss-robot added the size/M label Mar 11, 2021

elikatsis mentioned this pull request Mar 11, 2021

Add authentication with ServiceAccountToken #5138

Closed

elikatsis force-pushed the feature-client-credentials branch from 0ae9d39 to ae48463 Compare March 11, 2021 22:45

google-oss-robot added size/L and removed size/M labels Mar 11, 2021

elikatsis mentioned this pull request Mar 12, 2021

Katib x Kale x Multi-User Kubeflow doesn't support userid injection kubeflow/katib#1454

Closed

google-oss-robot assigned chensun Mar 19, 2021

Bobgy reviewed Mar 24, 2021

View reviewed changes

andreyvelich mentioned this pull request Apr 6, 2021

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

Merged

2 tasks

Ark-kun reviewed Apr 9, 2021

View reviewed changes

elikatsis force-pushed the feature-client-credentials branch from ae48463 to f99a4b7 Compare May 6, 2021 19:14

google-oss-robot added size/M and removed size/L labels May 6, 2021

elikatsis changed the title ~~feat(sdk): Support in-cluster authentication using credentials. Part of #4683, #5138~~ feat(sdk): Introduce the token credentials interface. Part of #4683 May 7, 2021

chensun reviewed May 8, 2021

View reviewed changes

Bobgy reviewed May 8, 2021

View reviewed changes

elikatsis added 2 commits May 10, 2021 18:43

Configure credentials when initializing KFP client

a7ca540

elikatsis force-pushed the feature-client-credentials branch from f99a4b7 to a7ca540 Compare May 10, 2021 15:43

Bobgy reviewed May 12, 2021

View reviewed changes

sdk/python/kfp/_credentials.py

@@ -0,0 +1,37 @@

# Copyright 2021 Arrikto Inc.

Copy link

Contributor

Bobgy May 12, 2021

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

google-oss-robot added the lgtm label May 12, 2021

yanniszark mentioned this pull request May 17, 2021

Release 1.3.1 kubeflow/manifests#1890

Closed

16 tasks

elikatsis mentioned this pull request May 17, 2021

No user identity in the header kubeflow-kale/kale#325

Closed

google-oss-robot added the approved label May 18, 2021

google-oss-robot merged commit 8936431 into kubeflow:master May 18, 2021

elikatsis deleted the feature-client-credentials branch May 18, 2021 14:46

elikatsis mentioned this pull request May 18, 2021

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

Merged

1 task

andreyvelich mentioned this pull request May 21, 2021

Validate Kubeflow GCP deployment using E2E example in https://github.com/kubeflow/pipelines/pull/5433 GoogleCloudPlatform/kubeflow-distribution#271

Open

Bobgy mentioned this pull request Jul 5, 2021

feat(sdk): Add interface to support basic auth and kerberos SPNEGO reverse proxy #5945

Closed

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

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

Conversation

elikatsis commented Mar 11, 2021 • edited Loading

Bobgy commented Mar 19, 2021

Bobgy commented Mar 19, 2021

Bobgy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elikatsis commented Mar 24, 2021

Bobgy commented Mar 24, 2021

andreyvelich commented Apr 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elikatsis commented Apr 13, 2021

Bobgy commented Apr 23, 2021 • edited Loading

elikatsis commented May 6, 2021

elikatsis commented May 6, 2021

Bobgy commented May 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chensun commented May 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bobgy May 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bobgy commented May 10, 2021

Bobgy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chensun commented May 18, 2021

google-oss-robot commented May 18, 2021

elikatsis commented Mar 11, 2021 •

edited

Loading

Bobgy left a comment •

edited

Loading

andreyvelich commented Apr 3, 2021 •

edited

Loading

Bobgy commented Apr 23, 2021 •

edited

Loading

Bobgy May 10, 2021 •

edited

Loading