Flytekit Auth system overhaul and pretty printing upgrade #1458

kumare3 · 2023-02-08T06:05:36Z

TL;DR

This PR introduces a new Oauth2 handling system in flytekit, that can be used in non flytekit python libraries that wish to use a standardized flow for flytekit. The standard flow is as defined in the RFC and a few additional supported methods.

The system uses client side - grpc.Interceptors. for fine control on the auth flow. It also introduces exception wrapping and retrying interceptor.

As a last time, it improves the terminal output when using pyflyte. All the output is not distilled and the error is traced correctly. Eventually we could link to docs for the errors.

Type

Bug Fix
Feature
Plugin

Are all requirements met?

- Reuse local keyring better - use grpc based auth system Signed-off-by: Ketan Umare <[email protected]>

retrying Signed-off-by: Ketan Umare <[email protected]>

Signed-off-by: Ketan Umare <[email protected]>

kumare3 · 2023-02-08T06:09:09Z

Help wanted.
cc @bethebunny / @kazesberger / @ByronHsu / @flixr / @honnix if some of you can test.
Especially @honnix you have a command based auth system

EngHabu

Thank you!

EngHabu · 2023-02-08T07:05:38Z

flytekit/clients/auth/auth_client.py

+                </head>
+                <body>
+                    <h1>Log in successful to {self.server.remote_metadata.endpoint}</h1>
+                </body></html>""",


nit: add please close this window or something like that..

will do, i will make it a configurable thing, also add Flyte icon

EngHabu · 2023-02-08T07:09:40Z

flytekit/clients/auth/authenticator.py

+        self._creds = Credentials(output.stdout.strip())
+
+
+class ClientCredentialsAuthenticator(Authenticator):


We should also add Device flow at some point...

EngHabu · 2023-02-08T07:11:55Z

flytekit/clients/grpc_utils/wrap_exception_interceptor.py

+    @staticmethod
+    def _raise_if_exc(request: typing.Any, e: Union[grpc.Call, grpc.Future]):
+        if e.code() == grpc.StatusCode.UNAUTHENTICATED:
+            raise FlyteAuthenticationException() from e


Am I missing something, where is this handled?

this is only an exception beautifier. This is not handled anywhere, this is once all auth tries fail

honnix · 2023-02-08T08:12:34Z

flytekit/clis/sdk_in_container/pyflyte.py

+        click.secho(f"Underlying Exception: {e.__cause__}", dim=True)
+        return
+
+    if isinstance(e, grpc._channel._InactiveRpcError):  # noqa


grpc._channel needs to be imported.

honnix · 2023-02-08T08:31:33Z

flytekit/clients/auth/authenticator.py

+        self._cmd = command
+        if not self._cmd:
+            raise ValueError("Command cannot be empty for command authenticator")
+        super().__init__(header_key)


Suggested change

super().__init__(header_key)

super().__init__(None, header_key)

thank you for the catch, updating it

honnix · 2023-02-08T09:20:06Z

I did a quick test. I managed to run a task via FlyteRemote and I verified that auth token was created and propagated correctly.

Signed-off-by: Ketan Umare <[email protected]>

goyalankit · 2023-02-14T02:48:56Z

flytekit/clients/auth/auth_client.py

+            server_process.terminate()
+
+    def refresh_access_token(self, credentials: Credentials) -> Credentials:
+        if credentials.refresh_token is None:
            raise ValueError("no refresh token available with which to refresh authorization credentials")

        resp = _requests.post(
            url=self._token_endpoint,


We should consider passing verify switch to request calls as in: #1509

let me incorporate your change

Signed-off-by: Ketan Umare <[email protected]>

…e the image smaller Signed-off-by: Yee Hing Tong <[email protected]>

Signed-off-by: Ketan Umare <[email protected]>

eapolinario · 2023-02-23T01:44:32Z

I was able to repro the test errors in a docker container (e.g. docker run --rm -it -v $(pwd):/root python:3.10-buster "bash" and cd /root && make setup && make unit_test).

Installing keyrings.alt makes the tests pass. That said, I'm not sure yet why we should require that package.

Signed-off-by: Yee Hing Tong <[email protected]>

Signed-off-by: Ketan Umare <[email protected]>

flytekit/clis/sdk_in_container/pyflyte.py

Signed-off-by: Ketan Umare <[email protected]>

Signed-off-by: Eduardo Apolinario <[email protected]>

codecov · 2023-02-23T21:24:03Z

Codecov Report

Merging #1458 (9d9feaa) into master (b3ad158) will decrease coverage by 0.05%.
The diff coverage is 53.21%.

@@            Coverage Diff             @@
##           master    #1458      +/-   ##
==========================================
- Coverage   69.32%   69.28%   -0.05%     
==========================================
  Files         305      315      +10     
  Lines       28671    28845     +174     
  Branches     2718     2741      +23     
==========================================
+ Hits        19877    19986     +109     
- Misses       8276     8341      +65     
  Partials      518      518

Impacted Files	Coverage Δ
flytekit/clients/auth/exceptions.py	`0.00% <0.00%> (ø)`
flytekit/configuration/internal.py	`16.21% <0.00%> (-0.23%)`	⬇️
flytekit/exceptions/user.py	`15.55% <0.00%> (-2.40%)`	⬇️
flytekit/remote/remote.py	`41.68% <0.00%> (+0.28%)`	⬆️
tests/flytekit/unit/cli/pyflyte/test_run.py	`99.31% <ø> (ø)`
...t/clients/grpc_utils/wrap_exception_interceptor.py	`2.70% <2.70%> (ø)`
flytekit/clients/grpc_utils/auth_interceptor.py	`2.77% <2.77%> (ø)`
flytekit/clients/auth/auth_client.py	`22.15% <14.28%> (ø)`
flytekit/clients/raw.py	`7.62% <33.33%> (-18.22%)`	⬇️
flytekit/configuration/__init__.py	`36.60% <33.33%> (-0.04%)`	⬇️
... and 14 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

* [wip] New authentication system - Reuse local keyring better - use grpc based auth system Signed-off-by: Ketan Umare <[email protected]> * Better error handling and printing, better exception handling and retrying Signed-off-by: Ketan Umare <[email protected]> * Delete legacy files Signed-off-by: Ketan Umare <[email protected]> * add missing None Signed-off-by: Ketan Umare <[email protected]> * keyring removed Signed-off-by: Ketan Umare <[email protected]> * added insecure_skip_verify Signed-off-by: Ketan Umare <[email protected]> * test fixed Signed-off-by: Ketan Umare <[email protected]> * Test fixed Signed-off-by: Ketan Umare <[email protected]> * Auth update Signed-off-by: Ketan Umare <[email protected]> * updated test Signed-off-by: Ketan Umare <[email protected]> * updated Signed-off-by: Ketan Umare <[email protected]> * flush buffer instead of closing, was getting a weird stack trace. make the image smaller Signed-off-by: Yee Hing Tong <[email protected]> * updated ca-cert logic Signed-off-by: Ketan Umare <[email protected]> * Fixed unit tests Signed-off-by: Ketan Umare <[email protected]> * updated Signed-off-by: Ketan Umare <[email protected]> * test fix Signed-off-by: Ketan Umare <[email protected]> * updated Signed-off-by: Ketan Umare <[email protected]> * nest raise if exc Signed-off-by: Yee Hing Tong <[email protected]> * added keyring.alt for tests Signed-off-by: Ketan Umare <[email protected]> * updated Signed-off-by: Ketan Umare <[email protected]> * updated Signed-off-by: Ketan Umare <[email protected]> * Lint Signed-off-by: Eduardo Apolinario <[email protected]> --------- Signed-off-by: Ketan Umare <[email protected]> Signed-off-by: Ketan Umare <[email protected]> Signed-off-by: Ketan Umare <[email protected]> Signed-off-by: Ketan Umare <[email protected]> Signed-off-by: Yee Hing Tong <[email protected]> Signed-off-by: Eduardo Apolinario <[email protected]> Co-authored-by: Ketan Umare <[email protected]> Co-authored-by: Ketan Umare <[email protected]> Co-authored-by: Ketan Umare <[email protected]> Co-authored-by: Yee Hing Tong <[email protected]> Co-authored-by: Eduardo Apolinario <[email protected]> Signed-off-by: Eduardo Apolinario <[email protected]>

…pgrade (#1458) (#1525) * Flytekit Auth system overhaul and pretty printing upgrade (#1458) * [wip] New authentication system - Reuse local keyring better - use grpc based auth system Signed-off-by: Ketan Umare <[email protected]> * Better error handling and printing, better exception handling and retrying Signed-off-by: Ketan Umare <[email protected]> * Delete legacy files Signed-off-by: Ketan Umare <[email protected]> * add missing None Signed-off-by: Ketan Umare <[email protected]> * keyring removed Signed-off-by: Ketan Umare <[email protected]> * added insecure_skip_verify Signed-off-by: Ketan Umare <[email protected]> * test fixed Signed-off-by: Ketan Umare <[email protected]> * Test fixed Signed-off-by: Ketan Umare <[email protected]> * Auth update Signed-off-by: Ketan Umare <[email protected]> * updated test Signed-off-by: Ketan Umare <[email protected]> * updated Signed-off-by: Ketan Umare <[email protected]> * flush buffer instead of closing, was getting a weird stack trace. make the image smaller Signed-off-by: Yee Hing Tong <[email protected]> * updated ca-cert logic Signed-off-by: Ketan Umare <[email protected]> * Fixed unit tests Signed-off-by: Ketan Umare <[email protected]> * updated Signed-off-by: Ketan Umare <[email protected]> * test fix Signed-off-by: Ketan Umare <[email protected]> * updated Signed-off-by: Ketan Umare <[email protected]> * nest raise if exc Signed-off-by: Yee Hing Tong <[email protected]> * added keyring.alt for tests Signed-off-by: Ketan Umare <[email protected]> * updated Signed-off-by: Ketan Umare <[email protected]> * updated Signed-off-by: Ketan Umare <[email protected]> * Lint Signed-off-by: Eduardo Apolinario <[email protected]> --------- Signed-off-by: Ketan Umare <[email protected]> Signed-off-by: Ketan Umare <[email protected]> Signed-off-by: Ketan Umare <[email protected]> Signed-off-by: Ketan Umare <[email protected]> Signed-off-by: Yee Hing Tong <[email protected]> Signed-off-by: Eduardo Apolinario <[email protected]> Co-authored-by: Ketan Umare <[email protected]> Co-authored-by: Ketan Umare <[email protected]> Co-authored-by: Ketan Umare <[email protected]> Co-authored-by: Yee Hing Tong <[email protected]> Co-authored-by: Eduardo Apolinario <[email protected]> Signed-off-by: Eduardo Apolinario <[email protected]> * Remove requirements files (#1511) * Remove mentions to spark Signed-off-by: Eduardo Apolinario <[email protected]> * Remove mentions to requirements.txt and dev-requirements.txt Signed-off-by: Eduardo Apolinario <[email protected]> * Remove plugins requirements. Signed-off-by: Eduardo Apolinario <[email protected]> * Remove all_requirements target from plugins makefile Signed-off-by: Eduardo Apolinario <[email protected]> * Use dev-requirements.in in lint gh action job Signed-off-by: Eduardo Apolinario <[email protected]> * Revert "Remove all_requirements target from plugins makefile" This reverts commit 50cbb4d. Signed-off-by: Eduardo Apolinario <[email protected]> * Revert "Remove plugins requirements." This reverts commit eae945c. Signed-off-by: Eduardo Apolinario <[email protected]> --------- Signed-off-by: Eduardo Apolinario <[email protected]> Co-authored-by: Eduardo Apolinario <[email protected]> Signed-off-by: Eduardo Apolinario <[email protected]> * Remove requirements files (#1511) * Remove mentions to spark Signed-off-by: Eduardo Apolinario <[email protected]> * Remove mentions to requirements.txt and dev-requirements.txt Signed-off-by: Eduardo Apolinario <[email protected]> * Remove plugins requirements. Signed-off-by: Eduardo Apolinario <[email protected]> * Remove all_requirements target from plugins makefile Signed-off-by: Eduardo Apolinario <[email protected]> * Use dev-requirements.in in lint gh action job Signed-off-by: Eduardo Apolinario <[email protected]> * Revert "Remove all_requirements target from plugins makefile" This reverts commit 50cbb4d. Signed-off-by: Eduardo Apolinario <[email protected]> * Revert "Remove plugins requirements." This reverts commit eae945c. Signed-off-by: Eduardo Apolinario <[email protected]> --------- Signed-off-by: Eduardo Apolinario <[email protected]> Co-authored-by: Eduardo Apolinario <[email protected]> Signed-off-by: Eduardo Apolinario <[email protected]> Signed-off-by: Eduardo Apolinario <[email protected]> --------- Signed-off-by: Ketan Umare <[email protected]> Signed-off-by: Ketan Umare <[email protected]> Signed-off-by: Ketan Umare <[email protected]> Signed-off-by: Ketan Umare <[email protected]> Signed-off-by: Yee Hing Tong <[email protected]> Signed-off-by: Eduardo Apolinario <[email protected]> Co-authored-by: Ketan Umare <[email protected]> Co-authored-by: Ketan Umare <[email protected]> Co-authored-by: Ketan Umare <[email protected]> Co-authored-by: Ketan Umare <[email protected]> Co-authored-by: Yee Hing Tong <[email protected]> Co-authored-by: Eduardo Apolinario <[email protected]>

kumare3 added 2 commits February 5, 2023 22:39

[wip] New authentication system

36bb637

- Reuse local keyring better - use grpc based auth system Signed-off-by: Ketan Umare <[email protected]>

Better error handling and printing, better exception handling and

ec3e8d0

retrying Signed-off-by: Ketan Umare <[email protected]>

kumare3 requested review from wild-endeavor, eapolinario, pingsutw and cosmicBboy as code owners February 8, 2023 06:05

Delete legacy files

f3df707

Signed-off-by: Ketan Umare <[email protected]>

EngHabu previously approved these changes Feb 8, 2023

View reviewed changes

honnix reviewed Feb 8, 2023

View reviewed changes

Ketan Umare added 2 commits February 12, 2023 22:10

Merge branch 'master' into auth-update

3251e8b

add missing None

332a912

Signed-off-by: Ketan Umare <[email protected]>

kumare3 dismissed EngHabu’s stale review via 332a912 February 13, 2023 06:16

kumare3 mentioned this pull request Feb 14, 2023

Propagate ca_cert_file_path and verify settings to auth client #1509

Closed

8 tasks

goyalankit reviewed Feb 14, 2023

View reviewed changes

Ketan Umare and others added 10 commits February 13, 2023 22:00

keyring removed

7444543

Signed-off-by: Ketan Umare <[email protected]>

added insecure_skip_verify

703346b

Signed-off-by: Ketan Umare <[email protected]>

test fixed

e10a3bf

Signed-off-by: Ketan Umare <[email protected]>

Test fixed

8bf6528

Signed-off-by: Ketan Umare <[email protected]>

Auth update

fca797a

Signed-off-by: Ketan Umare <[email protected]>

updated test

5c3cb8f

Signed-off-by: Ketan Umare <[email protected]>

updated

9994129

Signed-off-by: Ketan Umare <[email protected]>

flush buffer instead of closing, was getting a weird stack trace. mak…

20dd899

…e the image smaller Signed-off-by: Yee Hing Tong <[email protected]>

updated ca-cert logic

4a433ab

Signed-off-by: Ketan Umare <[email protected]>

Fixed unit tests

6fc56a6

Signed-off-by: Ketan Umare <[email protected]>

kumare3 changed the title ~~[WIP] Flytekit Auth system overhaul and pretty printing upgrade~~ Flytekit Auth system overhaul and pretty printing upgrade Feb 22, 2023

kumare3 added 2 commits February 21, 2023 22:32

Merge branch 'master' into auth-update

171be7c

updated

a9ebd4b

Signed-off-by: Ketan Umare <[email protected]>

test fix

7097db4

Signed-off-by: Ketan Umare <[email protected]>

wild-endeavor previously approved these changes Feb 23, 2023

View reviewed changes

updated

8831fe4

Signed-off-by: Ketan Umare <[email protected]>

kumare3 dismissed wild-endeavor’s stale review via 8831fe4 February 23, 2023 01:02

nest raise if exc

e8cdf58

Signed-off-by: Yee Hing Tong <[email protected]>

wild-endeavor previously approved these changes Feb 23, 2023

View reviewed changes

added keyring.alt for tests

96c3f60

Signed-off-by: Ketan Umare <[email protected]>

kumare3 dismissed wild-endeavor’s stale review via 96c3f60 February 23, 2023 06:23

kumare3 added 2 commits February 22, 2023 22:41

updated

6127100

Signed-off-by: Ketan Umare <[email protected]>

Merge branch 'master' into auth-update

9c7dc63

eapolinario reviewed Feb 23, 2023

View reviewed changes

flytekit/clis/sdk_in_container/pyflyte.py Show resolved Hide resolved

kumare3 and others added 2 commits February 23, 2023 11:38

updated

2c25466

Signed-off-by: Ketan Umare <[email protected]>

Lint

9d9feaa

Signed-off-by: Eduardo Apolinario <[email protected]>

eapolinario approved these changes Feb 23, 2023

View reviewed changes

eapolinario merged commit d682410 into master Feb 23, 2023

eapolinario mentioned this pull request Feb 28, 2023

Backport to 1.2 - Flytekit Auth system overhaul and pretty printing upgrade (#1458) #1525

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flytekit Auth system overhaul and pretty printing upgrade #1458

Flytekit Auth system overhaul and pretty printing upgrade #1458

kumare3 commented Feb 8, 2023 •

edited

Loading

kumare3 commented Feb 8, 2023

EngHabu left a comment

EngHabu Feb 8, 2023

kumare3 Feb 13, 2023

EngHabu Feb 8, 2023

kumare3 Feb 13, 2023

EngHabu Feb 8, 2023

kumare3 Feb 13, 2023

honnix Feb 8, 2023

honnix Feb 8, 2023

kumare3 Feb 13, 2023

honnix commented Feb 8, 2023

goyalankit Feb 14, 2023

kumare3 Feb 14, 2023

eapolinario commented Feb 23, 2023 •

edited

Loading

codecov bot commented Feb 23, 2023 •

edited

Loading

		self._creds = Credentials(output.stdout.strip())


		class ClientCredentialsAuthenticator(Authenticator):

	super().__init__(header_key)
	super().__init__(None, header_key)

Flytekit Auth system overhaul and pretty printing upgrade #1458

Flytekit Auth system overhaul and pretty printing upgrade #1458

Conversation

kumare3 commented Feb 8, 2023 • edited Loading

TL;DR

Type

Are all requirements met?

kumare3 commented Feb 8, 2023

EngHabu left a 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

honnix commented Feb 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eapolinario commented Feb 23, 2023 • edited Loading

codecov bot commented Feb 23, 2023 • edited Loading

Codecov Report

kumare3 commented Feb 8, 2023 •

edited

Loading

eapolinario commented Feb 23, 2023 •

edited

Loading

codecov bot commented Feb 23, 2023 •

edited

Loading