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

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

Open
wants to merge 43 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
21ebec7
Fix for keyring errors when initializing Flyte for_sandbox config cli…
taieeuu Nov 25, 2024
f358477
no_msg
taieeuu Nov 27, 2024
77bc862
no_msg
taieeuu Nov 27, 2024
5c9bed0
Merge branch 'master' into issue_4354
taieeuu Nov 27, 2024
b42a90f
fix: run linting on codebase
taieeuu Nov 27, 2024
6cb2d3f
add grpc 401 comments
taieeuu Dec 3, 2024
2214e3d
fix: import
taieeuu Dec 7, 2024
a82cd3a
no_msg
taieeuu Dec 7, 2024
4475cc5
Merge branch 'master' into issue_4354
taieeuu Dec 7, 2024
8f28333
fix: update method for initializing authenticator
taieeuu Dec 22, 2024
db7cb2f
fix: update the unit_test
taieeuu Dec 22, 2024
48600a7
fix: add grpc's health check to requirements.txt
taieeuu Dec 22, 2024
2542435
no_msg
taieeuu Dec 22, 2024
3e27029
no_msg
taieeuu Dec 22, 2024
61b35fd
fix: package dependency
taieeuu Dec 23, 2024
42509fb
fix: ci dependencies
taieeuu Dec 23, 2024
0a5afe0
fix: dependencies
taieeuu Dec 23, 2024
a50078e
no_msg
taieeuu Dec 24, 2024
456cd5d
fix: lint
taieeuu Dec 24, 2024
9253dc0
fix: add dependencies
taieeuu Dec 24, 2024
c5d9426
no_msg
taieeuu Dec 24, 2024
7bb5ef3
no_msg
taieeuu Dec 24, 2024
b7e5146
no_msg
taieeuu Dec 24, 2024
b97107b
no_msg
taieeuu Dec 24, 2024
89cf6f6
no_msg
taieeuu Dec 27, 2024
17c840f
no_msg
taieeuu Dec 27, 2024
27a6527
Merge branch 'master' into issue_4354
taieeuu Dec 27, 2024
857db92
no_msg
taieeuu Dec 27, 2024
1e56ac7
no_msg
taieeuu Dec 27, 2024
34f4aba
no_msg
taieeuu Dec 27, 2024
3bbbf41
no_msg
taieeuu Dec 27, 2024
9b3916c
no_msg
taieeuu Dec 27, 2024
0028436
no_msg
taieeuu Dec 27, 2024
d3ea8e4
no_msg
taieeuu Dec 28, 2024
5c19192
no_msg
taieeuu Dec 28, 2024
cf442e6
no_msg
taieeuu Dec 28, 2024
569558e
no_msg
taieeuu Dec 28, 2024
c4817cc
no_msg
taieeuu Dec 28, 2024
3d142c3
no_msg
taieeuu Dec 28, 2024
e207a64
Add lazy loading to AuthUnaryInterceptor.
taieeuu Jan 25, 2025
3f989f7
Merge master branch
taieeuu Jan 25, 2025
1f62483
remove not use
taieeuu Jan 25, 2025
eb43e3a
feat: implements test for keyring_exception
taieeuu Feb 4, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions flytekit/clients/auth_helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -122,9 +122,12 @@ def upgrade_channel_to_proxy_authenticated(cfg: PlatformConfig, in_channel: grpc
:param in_channel: grpc.Channel Precreated channel
:return: grpc.Channel. New composite channel
"""

def authenticator_factory():
return get_proxy_authenticator(cfg)
Comment on lines +126 to +127
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if cfg.proxy_command:
proxy_authenticator = get_proxy_authenticator(cfg)
return grpc.intercept_channel(in_channel, AuthUnaryInterceptor(proxy_authenticator))
return grpc.intercept_channel(in_channel, AuthUnaryInterceptor(authenticator_factory))
else:
return in_channel

Expand All @@ -137,8 +140,11 @@ def upgrade_channel_to_authenticated(cfg: PlatformConfig, in_channel: grpc.Chann
:param in_channel: grpc.Channel Precreated channel
:return: grpc.Channel. New composite channel
"""
authenticator = get_authenticator(cfg, RemoteClientConfigStore(in_channel))
return grpc.intercept_channel(in_channel, AuthUnaryInterceptor(authenticator))

def authenticator_factory():
return get_authenticator(cfg, RemoteClientConfigStore(in_channel))
Comment on lines +144 to +145
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return grpc.intercept_channel(in_channel, AuthUnaryInterceptor(authenticator_factory))


def get_authenticated_channel(cfg: PlatformConfig) -> grpc.Channel:
Expand Down
15 changes: 12 additions & 3 deletions flytekit/clients/grpc_utils/auth_interceptor.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,22 @@ class AuthUnaryInterceptor(grpc.UnaryUnaryClientInterceptor, grpc.UnaryStreamCli
is needed.
"""

def __init__(self, authenticator: Authenticator):
self._authenticator = authenticator
def __init__(self, get_authenticator: typing.Callable[[], Authenticator]):
self._get_authenticator = get_authenticator
self._authenticator = None

@property
def authenticator(self) -> Authenticator:
if self._authenticator is None:
self._authenticator = self._get_authenticator()
return self._authenticator

def _call_details_with_auth_metadata(self, client_call_details: grpc.ClientCallDetails) -> grpc.ClientCallDetails:
"""
Returns new ClientCallDetails with metadata added.
"""
metadata = client_call_details.metadata
auth_metadata = self._authenticator.fetch_grpc_call_auth_metadata()
auth_metadata = self.authenticator.fetch_grpc_call_auth_metadata()
if auth_metadata:
metadata = []
if client_call_details.metadata:
Expand Down Expand Up @@ -65,6 +72,7 @@ def intercept_unary_unary(
raise e
if e.code() == grpc.StatusCode.UNAUTHENTICATED or e.code() == grpc.StatusCode.UNKNOWN:
self._authenticator.refresh_credentials()
self.authenticator.refresh_credentials()
updated_call_details = self._call_details_with_auth_metadata(client_call_details)
return continuation(updated_call_details, request)
return fut
Expand All @@ -77,6 +85,7 @@ def intercept_unary_stream(self, continuation, client_call_details, request):
c: grpc.Call = continuation(updated_call_details, request)
if c.code() == grpc.StatusCode.UNAUTHENTICATED:
self._authenticator.refresh_credentials()
self.authenticator.refresh_credentials()
updated_call_details = self._call_details_with_auth_metadata(client_call_details)
return continuation(updated_call_details, request)
return c
40 changes: 40 additions & 0 deletions tests/flytekit/unit/clients/auth/test_keyring_store.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,15 @@

from flytekit.clients.auth.keyring import Credentials, KeyringStore

from flytekit.clients.auth_helper import upgrade_channel_to_authenticated, upgrade_channel_to_proxy_authenticated

from flytekit.configuration import PlatformConfig

import pytest

from flytekit.clients.auth.authenticator import CommandAuthenticator

from flytekit.clients.grpc_utils.auth_interceptor import AuthUnaryInterceptor

@patch("keyring.get_password")
def test_keyring_store_get(kr_get_password: MagicMock):
Expand All @@ -30,3 +39,34 @@ def test_keyring_store_set(kr_set_password: MagicMock):

kr_set_password.side_effect = NoKeyringError()
assert KeyringStore.retrieve("example2.com") is None

@patch("flytekit.clients.auth.authenticator.KeyringStore")
def test_upgrade_channel_to_authenticated_with_keyring_exception(mock_keyring_store):
mock_keyring_store.retrieve.side_effect = Exception("mock exception")

mock_channel = MagicMock()

platform_config = PlatformConfig()

try:
out_ch = upgrade_channel_to_authenticated(platform_config, mock_channel)
except Exception as e:
pytest.fail(f"upgrade_channel_to_authenticated Exception: {e}")

assert isinstance(out_ch._interceptor, AuthUnaryInterceptor)

@patch("flytekit.clients.auth.authenticator.KeyringStore")
def test_upgrade_channel_to_proxy_authenticated_with_keyring_exception(mock_keyring_store):
mock_keyring_store.retrieve.side_effect = Exception("mock exception")

mock_channel = MagicMock()

platform_config = PlatformConfig(auth_mode="Pkce", proxy_command=["echo", "foo-bar"])

try:
out_ch = upgrade_channel_to_proxy_authenticated(platform_config, mock_channel)
except Exception as e:
pytest.fail(f"upgrade_channel_to_proxy_authenticated Exception: {e}")

assert isinstance(out_ch._interceptor, AuthUnaryInterceptor)
assert isinstance(out_ch._interceptor.authenticator, CommandAuthenticator)
2 changes: 1 addition & 1 deletion tests/flytekit/unit/clients/test_auth_helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ def test_upgrade_channel_to_proxy_auth():
ch,
)
assert isinstance(out_ch._interceptor, AuthUnaryInterceptor)
assert isinstance(out_ch._interceptor._authenticator, CommandAuthenticator)
assert isinstance(out_ch._interceptor.authenticator, CommandAuthenticator)


def test_get_proxy_authenticated_session():
Expand Down
1 change: 0 additions & 1 deletion tests/flytekit/unit/clients/test_friendly.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
from flytekit.configuration import PlatformConfig
from flytekit.models.project import Project as _Project


@mock.patch("flytekit.clients.friendly._RawSynchronousFlyteClient.update_project")
def test_update_project(mock_raw_update_project):
client = _SynchronousFlyteClient(PlatformConfig.for_endpoint("a.b.com", True))
Expand Down
2 changes: 0 additions & 2 deletions tests/flytekit/unit/clients/test_raw.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
from flytekit.clients.raw import RawSynchronousFlyteClient
from flytekit.configuration import PlatformConfig


@mock.patch("flytekit.clients.raw._admin_service")
@mock.patch("flytekit.clients.raw.grpc.insecure_channel")
def test_update_project(mock_channel, mock_admin):
Expand All @@ -14,7 +13,6 @@ def test_update_project(mock_channel, mock_admin):
client.update_project(project)
mock_admin.AdminServiceStub().UpdateProject.assert_called_with(project, metadata=None)


@mock.patch("flytekit.clients.raw._admin_service")
@mock.patch("flytekit.clients.raw.grpc.insecure_channel")
def test_list_projects_paginated(mock_channel, mock_admin):
Expand Down
Loading