-
Notifications
You must be signed in to change notification settings - Fork 35
feat(aicore): transparent TLS mode and reactive credential reload #256
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,16 @@ | |
| re-raising as :class:`ContentFilteredError` so callers can rely on a | ||
| single exception type for "filter blocked you." | ||
|
|
||
| Credential rotation handling | ||
| ---------------------------- | ||
| When a credential (client_secret or mTLS certificate) is rotated while the | ||
| pod is running, LiteLLM's cached token becomes invalid and the next token | ||
| refresh attempt raises ``litellm.AuthenticationError``. The wrappers | ||
| intercept this error, reload credentials from the mounted secret volume via | ||
| :func:`reload_aicore_credentials`, and retry the call once. The caller is | ||
| unaffected — rotation is transparent. If the retry also fails, the | ||
| ``AuthenticationError`` propagates normally. | ||
|
|
||
| Usage:: | ||
|
|
||
| from sap_cloud_sdk.aicore import completion, ContentFilteredError | ||
|
|
@@ -39,12 +49,31 @@ | |
|
|
||
| from __future__ import annotations | ||
|
|
||
| import logging | ||
| from typing import Any | ||
|
|
||
| import litellm | ||
|
|
||
| from .filtering.filters import _parse_input_filter_error | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def reload_aicore_credentials() -> None: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we creating a new method that only calls other?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. reload_aicore_credentials() serves two purposes: it's called automatically by completion()/acompletion() on AuthenticationError (reactive reload on credential rotation), and it's also exposed as a public API for callers that need to trigger a manual reload. Keeping it as a named function makes the automatic behavior explicit and gives callers a stable surface without coupling them to set_aicore_config() internals.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still disagree in having an new function just to wrapper with a new nomenclature. If in future you believe more will be needed, it's ok. |
||
| """Re-read AI Core credentials from the mounted secret volume. | ||
|
|
||
| Called automatically by :func:`completion` and :func:`acompletion` when | ||
| LiteLLM raises ``AuthenticationError`` — covers credential rotation | ||
| (client_secret or mTLS certificate) without requiring a pod restart. | ||
|
|
||
| Safe to call manually if the application needs to force a reload, e.g. | ||
| after a deliberate secret rotation triggered by the operator. | ||
| """ | ||
| # Import here to avoid a circular import: completion ← __init__ ← completion | ||
| from sap_cloud_sdk.aicore import set_aicore_config | ||
| logger.info("AI Core credentials reloading after authentication failure") | ||
| set_aicore_config() | ||
|
|
||
|
|
||
| def _maybe_translate_filter_error(exc: BaseException) -> BaseException: | ||
| """Return a :class:`ContentFilteredError` if ``exc`` is a wrapped | ||
|
|
@@ -60,19 +89,18 @@ def _maybe_translate_filter_error(exc: BaseException) -> BaseException: | |
|
|
||
|
|
||
| def completion(*args: Any, **kwargs: Any) -> Any: | ||
| """Wrapper around :func:`litellm.completion` that normalises filter errors. | ||
|
|
||
| Forwards every argument unchanged. The only difference from calling | ||
| ``litellm.completion`` directly is that an input-filter rejection | ||
| (which litellm wraps in ``APIConnectionError``) is re-raised as | ||
| :class:`ContentFilteredError`. Output-filter rejections already | ||
| surface as :class:`ContentFilteredError` via the SDK's transport patch | ||
| and pass through unchanged. | ||
| """Wrapper around :func:`litellm.completion` that normalises filter errors | ||
| and handles credential rotation transparently. | ||
|
|
||
| All other exceptions surface verbatim. | ||
| On ``AuthenticationError`` (e.g. rotated client_secret or mTLS cert), | ||
| reloads credentials from the mounted secret volume and retries once. | ||
| All other exceptions surface verbatim after the filter-error translation. | ||
| """ | ||
| try: | ||
| return litellm.completion(*args, **kwargs) | ||
| except litellm.AuthenticationError: | ||
| reload_aicore_credentials() | ||
| return litellm.completion(*args, **kwargs) | ||
| except Exception as exc: | ||
| translated = _maybe_translate_filter_error(exc) | ||
| if translated is exc: | ||
|
|
@@ -83,15 +111,18 @@ def completion(*args: Any, **kwargs: Any) -> Any: | |
| async def acompletion(*args: Any, **kwargs: Any) -> Any: | ||
| """Async wrapper around :func:`litellm.acompletion`. | ||
|
|
||
| Same translation semantics as :func:`completion`. | ||
| Same translation and credential-rotation semantics as :func:`completion`. | ||
| """ | ||
| try: | ||
| return await litellm.acompletion(*args, **kwargs) | ||
| except litellm.AuthenticationError: | ||
| reload_aicore_credentials() | ||
| return await litellm.acompletion(*args, **kwargs) | ||
| except Exception as exc: | ||
| translated = _maybe_translate_filter_error(exc) | ||
| if translated is exc: | ||
| raise | ||
| raise translated from exc | ||
|
|
||
|
|
||
| __all__ = ["completion", "acompletion"] | ||
| __all__ = ["completion", "acompletion", "reload_aicore_credentials"] | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we understand if we can have a single variable to set transparent proxy usage and not specific by module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably could be related to secrets resolver refactor.