Fix for keyring errors when initializing Flyte for_sandbox config client #2962
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ent (needs verification) Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
taieeuu
requested review from
wild-endeavor,
kumare3,
eapolinario,
pingsutw,
cosmicBboy,
samhita-alla,
thomasjpfan and
Future-Outlier
as code owners
|
Thank you for opening this pull request! 🙌 These tips will help get your PR across the finish line:
|
Signed-off-by: taieeuu <[email protected]>
2 tasks
Comment on lines
70
to
71
| 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) |
There was a problem hiding this comment.
add comments about response 401...
There was a problem hiding this comment.
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!
|
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]>
taieeuu
changed the title
Comment on lines
93
to
102
| 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) |
There was a problem hiding this comment.
should we move import under try: ...?
why this work for Newton?
this shouldn't work.
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2962 +/- ##
===========================================
+ Coverage 47.02% 76.14% +29.12%
===========================================
Files 201 201
Lines 21203 21301 +98
Branches 2732 2740 +8
===========================================
+ Hits 9970 16220 +6250
+ Misses 10744 4279 -6465
- Partials 489 802 +313 ☔ View full report in Codecov by Sentry. |
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Code Review Agent Run #00ff28Actionable Suggestions - 1
Additional Suggestions - 2
Review Details
AI Code Review powered by |
Changelist by BitoThis pull request implements the following key changes.
|
flyte-bot
reviewed
Dec 28, 2024
Comment on lines
+62
to
+72
| base_channel = get_channel(cfg, options=options) | ||
|
|
||
| if self.check_grpc_health_with_authentication(base_channel): | ||
| self._channel = wrap_exceptions_channel(cfg, base_channel) | ||
| else: | ||
| self._channel = wrap_exceptions_channel( | ||
| cfg, | ||
| upgrade_channel_to_authenticated( | ||
| cfg, upgrade_channel_to_proxy_authenticated(cfg, get_channel(cfg, options=options)) | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Consider caching channel creation
Consider caching the base_channel creation to avoid redundant channel creation. Currently, get_channel() is called twice when authentication fails.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| base_channel = get_channel(cfg, options=options) | |
| if self.check_grpc_health_with_authentication(base_channel): | |
| self._channel = wrap_exceptions_channel(cfg, base_channel) | |
| else: | |
| self._channel = wrap_exceptions_channel( | |
| cfg, | |
| upgrade_channel_to_authenticated( | |
| cfg, upgrade_channel_to_proxy_authenticated(cfg, get_channel(cfg, options=options)) | |
| ), | |
| ) | |
| base_channel = get_channel(cfg, options=options) | |
| if self.check_grpc_health_with_authentication(base_channel): | |
| self._channel = wrap_exceptions_channel(cfg, base_channel) | |
| else: | |
| self._channel = wrap_exceptions_channel( | |
| cfg, | |
| upgrade_channel_to_authenticated( | |
| cfg, upgrade_channel_to_proxy_authenticated(cfg, base_channel) | |
| ), | |
| ) |
Code Review Run #00ff28
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Code Review Agent Run #10536fActionable Suggestions - 0Review Details
AI Code Review powered by |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix for keyring errors when initializing Flyte for_sandbox config client (needs verification)
Tracking issue
flyteorg/flyte#4354
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
Summary by Bito
Implementation of keyring error handling for Flyte sandbox config client, featuring gRPC health checking and improved authentication flow. Restructured grpc health module imports by moving health_pb2 and health_pb2_grpc imports from global scope to client method level. The solution introduces service availability verification before authentication attempts and includes exception handling for gRPC status codes, with a fallback mechanism from unauthenticated to authenticated channels.Unit tests added: False
Estimated effort to review (1-5, lower is better): 2