Skip to content
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

sdk/client/auth - refactor GCP auth into a module #4683

Closed
Bobgy opened this issue Oct 28, 2020 · 5 comments
Closed

sdk/client/auth - refactor GCP auth into a module #4683

Bobgy opened this issue Oct 28, 2020 · 5 comments
Labels
area/authentication area/sdk/client lifecycle/stale The issue / pull request is stale, any activities remove this label. priority/p2

Comments

@Bobgy
Copy link
Contributor

Bobgy commented Oct 28, 2020

There are many types of auth features in KFP SDK, they are injected kind of ad-hoc in code.

I think we need to refactor it into auth modules, so that

  1. it's cleanly separated
  2. it's easy for other providers to add auth modules

Another common proposal is to make the auth parameters explicit, right now we are doing the auth part automatically based on host url regex matching, but users may set up their own dns name. We should try to make it more controllable.

@yanniszark
Copy link
Contributor

yanniszark commented Oct 30, 2020

Hi @Bobgy! We find this issue interesting because we have actually implemented something like this for our purposes.
As you said, the current client is not extensible with regards to authentication methods, as authentication methods are hardcoded inside the client's __init__.

What we have done in our own fork of the KFP client, is to add a TokenCredentials class, which encapsulates the retrieval and refresh of token credentials. The existing token argument is not enough, because we need to be able to refresh the token as well.

To make this concrete, this is the abstract class we used:

class TokenCredentials(object):
    __metaclass__ = abc.ABCMeta

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

        This is a helper function for registering token refresh with swagger
        generated clients.
        """
        config.api_key["authorization"] = self.get_token()

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

It has a get_token function which is responsible for fetching and refreshing (if needed) a token for authentication.
In addition, it has a helper refresh_api_key_hook function, which is designed to be used together with swagger-generated clients like so:

# config is the swagger client config
credentials = MyCredentialClass()
config.refresh_api_key_hook = credentials.refresh_api_key_hook

Finally, add an extra argument to the client's __init__ to be able to pass credentials this way.

In the GCP example, you could implement different credential classes for different GCP auth methods (GCP ServiceAccount, Default App Credentials, IAP). What do you think?

@Bobgy
Copy link
Contributor Author

Bobgy commented Nov 3, 2020

Thanks @yanniszark! I think that sounds like a clean and abstract interface. Will you be willing to contribute?

/cc @chensun @numerology @Ark-kun
What do you think about this?

@yanniszark
Copy link
Contributor

@Bobgy sure, we could open a PR for it.

@stale
Copy link

stale bot commented Feb 8, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the lifecycle/stale The issue / pull request is stale, any activities remove this label. label Feb 8, 2021
elikatsis added a commit to arrikto/kubeflow-pipelines that referenced this issue Mar 11, 2021
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.
elikatsis added a commit to arrikto/kubeflow-pipelines that referenced this issue May 6, 2021
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.
elikatsis added a commit to arrikto/kubeflow-pipelines that referenced this issue May 10, 2021
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.
google-oss-robot pushed a commit that referenced this issue May 18, 2021
…5287)

* Introduce TokenCredentials abstract class

As presented in #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.

* Configure credentials when initializing KFP client
@stale
Copy link

stale bot commented Jun 22, 2021

This issue has been automatically closed because it has not had recent activity. Please comment "/reopen" to reopen it.

@stale stale bot closed this as completed Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/authentication area/sdk/client lifecycle/stale The issue / pull request is stale, any activities remove this label. priority/p2
Projects
None yet
Development

No branches or pull requests

2 participants