Fix for keyring errors when initializing Flyte for_sandbox config client #2962

taieeuu · 2024-11-27T16:00:04Z

Fix for keyring errors when initializing Flyte for_sandbox config client (needs verification)

Tracking issue

What changes were proposed in this pull request?

First, I started by analyzing the issue reporter's code and modified the Config.for_sandbox() method to add a new value AuthType.No_Auth to auth_mode. Additionally, I updated the AuthUnaryInterceptor class to handle gRPC responses. If a 401 error (i.e., grpc.StatusCode.UNAUTHENTICATED) is encountered, it will trigger the creation of a PKCE authenticator.

How was this patch tested?

I am having difficulty replicating the issue reporter's environment, as it seems to require a public domain setup and GNOME keyring installed. However, I followed the suggestions from the discussion forum and implemented this fix.

Check all the applicable boxes

I updated the documentation accordingly.
All new and existing tests passed.
All commits are signed-off.

Summary by Bito

Implementation of keyring error handling for Flyte sandbox config client with lazy authentication initialization using factory pattern. The changes include gRPC health checking, improved authentication flow, and prevention of immediate keyring access during client initialization. The solution features service availability verification, exception handling for gRPC status codes, and enhanced authentication interceptor functionality for credential refresh and failure handling.

Unit tests added: False

Estimated effort to review (1-5, lower is better): 2

…ent (needs verification) Signed-off-by: taieeuu <[email protected]>

Signed-off-by: taieeuu <[email protected]>

welcome · 2024-11-27T16:00:08Z

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
Sign off your commits (Reference: DCO Guide).

Signed-off-by: taieeuu <[email protected]>

Future-Outlier · 2024-11-29T16:05:08Z

flytekit/clients/grpc_utils/auth_interceptor.py

            if e.code() == grpc.StatusCode.UNAUTHENTICATED or e.code() == grpc.StatusCode.UNKNOWN:
-                self._authenticator.refresh_credentials()
-                updated_call_details = self._call_details_with_auth_metadata(client_call_details)
-                return continuation(updated_call_details, request)
+                return self._handle_unauthenticated_error(fut, client_call_details, request)


add comments about response 401...

Future-Outlier

Maybe this log provided by @Tom-Newton can help.

tomnewton@ben-nevis:~/WayveCode/wayve/ai/nvs/services/workflow$ python /home/tomnewton/Documents/reproduce_key_vault_error.py
/usr/lib/python3/dist-packages/paramiko/transport.py:219: CryptographyDeprecationWarning: Blowfish has been deprecated
"class": algorithms.Blowfish,
╭──────────────────────────────────────────────────────────────────────────────────────────────────────────── Traceback (most recent call last) ─────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ /home/tomnewton/Documents/reproduce_key_vault_error.py:6 in <module>                                                                                                                                                                                       │
│                                                                                                                                                                                                                                                            │
│ ❱ 6 remote.client                                                                                                                                                                                                                                          │
│                                                                                                                                                                                                                                                            │
│ /home/tomnewton/.local/lib/python3.8/site-packages/flytekit/remote/remote.py:205 in client                                                                                                                                                                 │
│                                                                                                                                                                                                                                                            │
│ ❱  205 │   │   │   self._client = SynchronousFlyteClient(self.config.platform, **self._kwargs)                                                                                                                                                             │
│                                                                                                                                                                                                                                                            │
│ /home/tomnewton/.local/lib/python3.8/site-packages/flytekit/clients/raw.py:44 in __init__                                                                                                                                                                  │
│                                                                                                                                                                                                                                                            │
│ ❱  44 │   │   self._channel = wrap_exceptions_channel(cfg, upgrade_channel_to_authenticated(cf                                                                                                                                                             │
│                                                                                                                                                                                                                                                            │
│ /home/tomnewton/.local/lib/python3.8/site-packages/flytekit/clients/auth_helper.py:111 in upgrade_channel_to_authenticated                                                                                                                                 │
│                                                                                                                                                                                                                                                            │
│ ❱ 111 │   authenticator = get_authenticator(cfg, RemoteClientConfigStore(in_channel))                                                                                                                                                                      │
│                                                                                                                                                                                                                                                            │
│ /home/tomnewton/.local/lib/python3.8/site-packages/flytekit/clients/auth_helper.py:69 in get_authenticator                                                                                                                                                 │
│                                                                                                                                                                                                                                                            │
│ ❱  69 │   │   return PKCEAuthenticator(cfg.endpoint, cfg_store, verify=verify)                                                                                                                                                                             │
│                                                                                                                                                                                                                                                            │
│ /home/tomnewton/.local/lib/python3.8/site-packages/flytekit/clients/auth/authenticator.py:102 in __init__                                                                                                                                                  │
│                                                                                                                                                                                                                                                            │
│ ❱ 102 │   │   super().__init__(endpoint, header_key, KeyringStore.retrieve(endpoint), verify=v                                                                                                                                                             │
│                                                                                                                                                                                                                                                            │
│ /home/tomnewton/.local/lib/python3.8/site-packages/flytekit/clients/auth/keyring.py:49 in retrieve                                                                                                                                                         │
│                                                                                                                                                                                                                                                            │
│ ❱ 49 │   │   │   refresh_token = _keyring.get_password(for_endpoint, KeyringStore._refresh_to                                                                                                                                                              │
│                                                                                                                                                                                                                                                            │
│ /home/tomnewton/.local/lib/python3.8/site-packages/keyring/core.py:55 in get_password                                                                                                                                                                      │
│                                                                                                                                                                                                                                                            │
│ ❱  55 │   return get_keyring().get_password(service_name, username)                                                                                                                                                                                        │
│                                                                                                                                                                                                                                                            │
│ /home/tomnewton/.local/lib/python3.8/site-packages/keyring/backends/chainer.py:49 in get_password                                                                                                                                                          │
│                                                                                                                                                                                                                                                            │
│ ❱ 49 │   │   │   password = keyring.get_password(service, username)                                                                                                                                                                                        │
│                                                                                                                                                                                                                                                            │
│ /home/tomnewton/.local/lib/python3.8/site-packages/keyring/backends/SecretService.py:78 in get_password                                                                                                                                                    │
│                                                                                                                                                                                                                                                            │
│ ❱  78 │   │   collection = self.get_preferred_collection()                                                                                                                                                                                                 │
│                                                                                                                                                                                                                                                            │
│ /home/tomnewton/.local/lib/python3.8/site-packages/keyring/backends/SecretService.py:67 in get_preferred_collection                                                                                                                                        │
│                                                                                                                                                                                                                                                            │
│ ❱  67 │   │   │   │   raise KeyringLocked("Failed to unlock the collection!")                                                                                                                                                                              │
╰────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
KeyringLocked: Failed to unlock the collection!

Tom-Newton · 2024-12-02T22:33:46Z

I just tested a couple of scenarios where I previously had problems and it seems to be working now. Thanks for working on this 🙌.

Signed-off-by: taieeuu <[email protected]>

Future-Outlier · 2024-12-06T17:10:01Z

flytekit/clients/grpc_utils/auth_interceptor.py

+        try:
+            if isinstance(self._authenticator, Authenticator) and not isinstance(
+                self._authenticator, PKCEAuthenticator
+            ):
+                logging.info("Current authenticator is 'None', switching to PKCEAuthenticator")
+
+                from flytekit.clients.auth.authenticator import PKCEAuthenticator
+                from flytekit.clients.auth_helper import get_session
+
+                session = get_session(self._cfg)


should we move import under try: ...?
why this work for Newton?
this shouldn't work.

Signed-off-by: taieeuu <[email protected]>

flyte-bot · 2024-12-28T07:44:16Z

Code Review Agent Run #00ff28

Actionable Suggestions - 1

flytekit/clients/raw.py - 1
- Consider caching channel creation · Line 62-72

Additional Suggestions - 2

dev-requirements.in - 1
- Consider consolidating gRPC dependency declarations · Line 64-66
flytekit/clients/raw.py - 1
- Consider explicit health check returns · Line 89-92

Review Details

Files reviewed - 5 · Commit Range: 21ebec7..569558e
- dev-requirements.in
- dev-requirements.txt
- flytekit/clients/raw.py
- tests/flytekit/unit/clients/test_friendly.py
- tests/flytekit/unit/clients/test_raw.py
Files skipped - 2
- .github/workflows/monodocs_build.yml - Reason: Filter setting
- .github/workflows/pythonbuild.yml - Reason: Filter setting
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

flyte-bot · 2024-12-28T07:49:27Z

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
Feature Improvement - Enhanced gRPC Authentication and Health Checking	- `auth_helper.py` - Refactored authentication logic to use factory pattern for authenticator creation - `auth_interceptor.py` - Improved auth interceptor with lazy authenticator initialization and credential refresh - `raw.py` - Added gRPC health checking functionality with authentication support - `dev-requirements.in` - Added gRPC health checking dependencies - `dev-requirements.txt` - Updated dependencies with grpcio-health-checking package
Testing - Updated Test Cases for Authentication Changes	- `test_auth_helper.py` - Updated auth helper tests to use new authenticator property - `test_friendly.py` - Added health check mocking to project-related tests - `test_raw.py` - Added health check verification tests for raw client

flytekit/clients/raw.py

Signed-off-by: taieeuu <[email protected]>

flyte-bot · 2024-12-28T09:34:17Z

Code Review Agent Run #10536f

Actionable Suggestions - 0

Review Details

Files reviewed - 1 · Commit Range: 569558e..3d142c3
- flytekit/clients/raw.py
Files skipped - 1
- .github/workflows/pythonbuild.yml - Reason: Filter setting
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

Signed-off-by: taieeuu <[email protected]>

flyte-bot · 2025-01-25T07:17:25Z

Code Review Agent Run #5371e4

Actionable Suggestions - 2

flytekit/clients/auth_helper.py - 2
- Consider caching authenticator instance · Line 126-127
- Consider caching authenticator instance · Line 144-145

Review Details

Files reviewed - 4 · Commit Range: 3d142c3..e207a64
- flytekit/clients/auth_helper.py
- flytekit/clients/grpc_utils/auth_interceptor.py
- flytekit/clients/raw.py
- tests/flytekit/unit/clients/test_auth_helper.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

flyte-bot · 2025-01-25T07:22:42Z

flytekit/clients/auth_helper.py

+    def authenticator_factory():
+        return get_proxy_authenticator(cfg)


Consider caching authenticator instance

Consider caching the authenticator instance instead of creating a new one on each call to authenticator_factory(). This could improve performance by avoiding unnecessary object creation.

Code suggestion

Check the AI-generated fix before applying

Suggested change

def authenticator_factory():

return get_proxy_authenticator(cfg)

_cached_authenticator = None

def authenticator_factory():

nonlocal _cached_authenticator

if _cached_authenticator is None:

_cached_authenticator = get_proxy_authenticator(cfg)

return _cached_authenticator

Code Review Run #5371e4

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

flyte-bot · 2025-01-25T07:22:43Z

flytekit/clients/auth_helper.py

+    def authenticator_factory():
+        return get_authenticator(cfg, RemoteClientConfigStore(in_channel))


Consider caching authenticator instance

Consider caching the authenticator instance instead of creating a new one on every call to authenticator_factory(). This could improve performance since authentication configuration is unlikely to change during runtime.

Code suggestion

Check the AI-generated fix before applying

Suggested change

def authenticator_factory():

return get_authenticator(cfg, RemoteClientConfigStore(in_channel))

authenticator = None

def authenticator_factory():

nonlocal authenticator

if authenticator is None:

authenticator = get_authenticator(cfg, RemoteClientConfigStore(in_channel))

return authenticator

Code Review Run #5371e4

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

Signed-off-by: taieeuu <[email protected]>

flyte-bot · 2025-01-25T09:37:24Z

Code Review Agent Run Status

Limitations and other issues: ❌ Failure - Bito Code Review Agent didn't review this pull request automatically because it exceeded the size limit. No action is needed if you didn't intend for the agent to review it. Otherwise, you can initiate the review by typing /review in a comment below.

wild-endeavor

thanks! could we add one more unit test though please 🙏 ? mock.patch("flytekit.clients.auth.authenticator.KeyringStore") and set retrieve function of the mock to raise an exception, and then call upgrade_channel_to_authenticated and make sure there's no exception. mock whatever else you need too.

Signed-off-by: taieeuu <[email protected]>

flyte-bot · 2025-02-04T16:34:25Z

Code Review Agent Run Status

Limitations and other issues: ❌ Failure - Bito Code Review Agent didn't review this pull request automatically because it exceeded the size limit. No action is needed if you didn't intend for the agent to review it. Otherwise, you can initiate the review by typing /review in a comment below.

taieeuu added 4 commits November 25, 2024 22:47

Fix for keyring errors when initializing Flyte for_sandbox config cli…

21ebec7

…ent (needs verification) Signed-off-by: taieeuu <[email protected]>

no_msg

f358477

Signed-off-by: taieeuu <[email protected]>

no_msg

77bc862

Signed-off-by: taieeuu <[email protected]>

Merge branch 'master' into issue_4354

5c9bed0

taieeuu requested review from wild-endeavor, kumare3, eapolinario, pingsutw, cosmicBboy, samhita-alla, thomasjpfan and Future-Outlier as code owners November 27, 2024 16:00

fix: run linting on codebase

b42a90f

Signed-off-by: taieeuu <[email protected]>

Future-Outlier mentioned this pull request Nov 29, 2024

[BUG] Keyring errors when intialising flyte remote client flyteorg/flyte#4354

Open

2 tasks

Future-Outlier reviewed Nov 29, 2024

View reviewed changes

add grpc 401 comments

6cb2d3f

Signed-off-by: taieeuu <[email protected]>

taieeuu changed the title ~~Fix for keyring errors when initializing Flyte for_sandbox config client (needs verification)~~ Fix for keyring errors when initializing Flyte for_sandbox config client Dec 5, 2024

Future-Outlier reviewed Dec 6, 2024

View reviewed changes

taieeuu added 3 commits December 7, 2024 09:12

fix: import

2214e3d

Signed-off-by: taieeuu <[email protected]>

no_msg

a82cd3a

Signed-off-by: taieeuu <[email protected]>

Merge branch 'master' into issue_4354

4475cc5

Future-Outlier self-assigned this Dec 9, 2024

taieeuu added 5 commits December 22, 2024 13:56

fix: update method for initializing authenticator

8f28333

Signed-off-by: taieeuu <[email protected]>

fix: update the unit_test

db7cb2f

Signed-off-by: taieeuu <[email protected]>

fix: add grpc's health check to requirements.txt

48600a7

Signed-off-by: taieeuu <[email protected]>

no_msg

2542435

Signed-off-by: taieeuu <[email protected]>

no_msg

3e27029

Signed-off-by: taieeuu <[email protected]>

taieeuu added 13 commits December 27, 2024 21:11

no_msg

89cf6f6

Signed-off-by: taieeuu <[email protected]>

no_msg

17c840f

Signed-off-by: taieeuu <[email protected]>

Merge branch 'master' into issue_4354

27a6527

no_msg

857db92

Signed-off-by: taieeuu <[email protected]>

no_msg

1e56ac7

Signed-off-by: taieeuu <[email protected]>

no_msg

34f4aba

Signed-off-by: taieeuu <[email protected]>

no_msg

3bbbf41

Signed-off-by: taieeuu <[email protected]>

no_msg

9b3916c

Signed-off-by: taieeuu <[email protected]>

no_msg

0028436

Signed-off-by: taieeuu <[email protected]>

no_msg

d3ea8e4

Signed-off-by: taieeuu <[email protected]>

no_msg

5c19192

Signed-off-by: taieeuu <[email protected]>

no_msg

cf442e6

Signed-off-by: taieeuu <[email protected]>

no_msg

569558e

Signed-off-by: taieeuu <[email protected]>

flyte-bot reviewed Dec 28, 2024

View reviewed changes

flytekit/clients/raw.py Outdated Show resolved Hide resolved

taieeuu added 2 commits December 28, 2024 16:25

no_msg

c4817cc

Signed-off-by: taieeuu <[email protected]>

no_msg

3d142c3

Signed-off-by: taieeuu <[email protected]>

Future-Outlier removed their assignment Jan 13, 2025

Add lazy loading to AuthUnaryInterceptor.

e207a64

Signed-off-by: taieeuu <[email protected]>

flyte-bot reviewed Jan 25, 2025

View reviewed changes

taieeuu added 2 commits January 25, 2025 16:28

Merge master branch

3f989f7

Signed-off-by: taieeuu <[email protected]>

remove not use

1f62483

Signed-off-by: taieeuu <[email protected]>

wild-endeavor reviewed Jan 31, 2025

View reviewed changes

feat: implements test for keyring_exception

eb43e3a

Signed-off-by: taieeuu <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for keyring errors when initializing Flyte for_sandbox config client #2962

Fix for keyring errors when initializing Flyte for_sandbox config client #2962

taieeuu commented Nov 27, 2024 •

edited by flyte-bot

Loading

welcome bot commented Nov 27, 2024

Future-Outlier Nov 29, 2024

Future-Outlier left a comment

Tom-Newton commented Dec 2, 2024

Future-Outlier Dec 6, 2024

flyte-bot commented Dec 28, 2024 •

edited

Loading

Code Review Agent Run #00ff28

flyte-bot commented Dec 28, 2024 •

edited

Loading

Changelist by Bito

flyte-bot commented Dec 28, 2024 •

edited

Loading

Code Review Agent Run #10536f

flyte-bot commented Jan 25, 2025 •

edited

Loading

Code Review Agent Run #5371e4

flyte-bot Jan 25, 2025

flyte-bot Jan 25, 2025

flyte-bot commented Jan 25, 2025 •

edited

Loading

wild-endeavor left a comment

flyte-bot commented Feb 4, 2025 •

edited

Loading

		def authenticator_factory():
		return get_proxy_authenticator(cfg)

-    def authenticator_factory():
-        return get_proxy_authenticator(cfg)
+    _cached_authenticator = None
+    def authenticator_factory():
+        nonlocal _cached_authenticator
+        if _cached_authenticator is None:
+            _cached_authenticator = get_proxy_authenticator(cfg)
+        return _cached_authenticator

		def authenticator_factory():
		return get_authenticator(cfg, RemoteClientConfigStore(in_channel))

Fix for keyring errors when initializing Flyte for_sandbox config client #2962

Are you sure you want to change the base?

Fix for keyring errors when initializing Flyte for_sandbox config client #2962

Conversation

taieeuu commented Nov 27, 2024 • edited by flyte-bot Loading

Tracking issue

What changes were proposed in this pull request?

How was this patch tested?

Check all the applicable boxes

Summary by Bito

welcome bot commented Nov 27, 2024

Future-Outlier Nov 29, 2024

Choose a reason for hiding this comment

Future-Outlier left a comment

Choose a reason for hiding this comment

Tom-Newton commented Dec 2, 2024

Future-Outlier Dec 6, 2024

Choose a reason for hiding this comment

flyte-bot commented Dec 28, 2024 • edited Loading

Code Review Agent Run #00ff28

flyte-bot commented Dec 28, 2024 • edited Loading

Changelist by Bito

flyte-bot commented Dec 28, 2024 • edited Loading

Code Review Agent Run #10536f

flyte-bot commented Jan 25, 2025 • edited Loading

Code Review Agent Run #5371e4

flyte-bot Jan 25, 2025

Choose a reason for hiding this comment

flyte-bot Jan 25, 2025

Choose a reason for hiding this comment

flyte-bot commented Jan 25, 2025 • edited Loading

Code Review Agent Run Status

wild-endeavor left a comment

Choose a reason for hiding this comment

flyte-bot commented Feb 4, 2025 • edited Loading

Code Review Agent Run Status

taieeuu commented Nov 27, 2024 •

edited by flyte-bot

Loading

flyte-bot commented Dec 28, 2024 •

edited

Loading

flyte-bot commented Dec 28, 2024 •

edited

Loading

flyte-bot commented Dec 28, 2024 •

edited

Loading

flyte-bot commented Jan 25, 2025 •

edited

Loading

flyte-bot commented Jan 25, 2025 •

edited

Loading

flyte-bot commented Feb 4, 2025 •

edited

Loading