Skip to content

feat(aicore): reactive credential reload and proactive secret watcher - #256

Merged
tiagoek merged 13 commits into
mainfrom
feat/aicore-transparent-tls
Aug 31, 2026
Merged

feat(aicore): reactive credential reload and proactive secret watcher#256
tiagoek merged 13 commits into
mainfrom
feat/aicore-transparent-tls

Conversation

@tiagoek

@tiagoek tiagoek commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Description

Addresses credential rotation failures in long-running agent pods.

When BTP rotates a service binding, kubelet updates the mounted secret volume within ~2 minutes. However, agent pods are never restarted — LiteLLM's cached OAuth token (up to 12h lifetime) eventually expires and the refresh attempt fails because the process still holds the old client_secret. This PR makes credential rotation transparent to agents without requiring pod restarts.

1. Reactive credential reload on AuthenticationError

completion() and acompletion() intercept litellm.AuthenticationError, reload credentials from the mounted secret volume via set_aicore_config(), and retry the call once. If the retry also fails, the error propagates normally.

2. Proactive secret watcher (watch_aicore_config)

A daemon thread polls the secret directory mtime every 60 seconds. On change (kubelet performs an atomic symlink swap on rotation), calls set_aicore_config() proactively — before the cached OAuth token expires. Avoids the 401 entirely rather than recovering from it.

Typical usage:

from sap_cloud_sdk.aicore import set_aicore_config, watch_aicore_config

set_aicore_config()
watch_aicore_config()   # starts daemon thread — call once at startup

3. LangGraph / ChatLiteLLM compatibility (patch_litellm_for_credential_rotation)

LangGraph agents use ChatLiteLLM which calls litellm.completion directly, bypassing our completion() wrapper. patch_litellm_for_credential_rotation() wraps litellm.completion and litellm.acompletion at the module level so all callers get transparent reload on 401 — including ChatLiteLLM.

from sap_cloud_sdk.aicore import (
    set_aicore_config,
    patch_litellm_for_credential_rotation,
    watch_aicore_config,
)

set_aicore_config()
patch_litellm_for_credential_rotation()   # for LangGraph / ChatLiteLLM agents
watch_aicore_config()

Related Issues

Type of Change

  • New feature (non-breaking change that adds functionality)
  • Bug fix (non-breaking change that fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Code refactoring
  • Dependency update

How to Test

python -m pytest tests/aicore/unit/ -v
# 111 passed

Key test files:

  • tests/aicore/unit/test_aicore_watcher.py — proactive watcher (8 tests)
  • tests/aicore/unit/test_credential_rotation_flow.py — env update contract (6 tests)
  • tests/aicore/unit/test_completion.py — reactive reload (sync + async)
  • tests/aicore/unit/test_langgraph_compat.py — LangGraph / ChatLiteLLM compatibility (15 tests)

Checklist

  • I have read the Contributing Guidelines
  • I have verified that my changes solve the issue
  • I have added/updated automated tests to cover my changes
  • All tests pass locally
  • I have verified that my code follows the Code Guidelines
  • I have updated documentation (if applicable)
  • I have added type hints for all public APIs
  • My code does not contain sensitive information (credentials, tokens, etc.)
  • I have followed Conventional Commits for commit messages

Breaking Changes

None. All changes are additive:

  • watch_aicore_config() — new public function, opt-in
  • patch_litellm_for_credential_rotation() — new public function, opt-in, idempotent
  • Retry on AuthenticationError in completion() — same exception type propagates if retry also fails; no contract change

Comment thread src/sap_cloud_sdk/aicore/completion.py Outdated
Comment thread src/sap_cloud_sdk/aicore/__init__.py Outdated
tiagoek added a commit that referenced this pull request Aug 25, 2026
…ature

Address reviewer feedback on PR #256:

1. Remove reload_aicore_credentials() wrapper — inline set_aicore_config()
   directly in the except AuthenticationError blocks. The wrapper added a
   named function for a single call; inlining is simpler and clearer.

2. Remove transparent TLS feature (AICORE_TRANSPARENT_TLS env var,
   _is_transparent_tls(), conditional client_secret handling in
   set_aicore_config()). This feature is blocked on an upstream LiteLLM PR
   and is not needed for the credential rotation fix. Nicole flagged that
   it belongs in a future secrets-resolver refactor.

Behavior unchanged: AuthenticationError still triggers set_aicore_config()
+ retry, completely transparent to callers.
@tiagoek

tiagoek commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Production validation — proactive credential reload (watch_aicore_config)

Test suite (cloud-sdk-python)

All 70 unit tests pass on branch feat/aicore-transparent-tls:

tests/aicore/unit/test_aicore_watcher.py          8 passed
tests/aicore/unit/test_credential_rotation_flow.py 6 passed
tests/aicore/unit/test_aicore.py                  (existing, green)
tests/aicore/unit/test_completion.py              (existing, green)

Integration into autonomous-documentation-org

The vendor shim (vendor/sap_cloud_sdk_aicore/) was synced with watch_aicore_config + _get_secret_dir_mtime.

Finding (critical): Starlette 1.3.1 does NOT propagate lifespan events to mounted sub-apps — verified with a live ASGI simulation:

# Starlette 1.3.1 — Mount does not trigger FastAPI sub-app lifespan
outer = Starlette(routes=[Mount('/sub', app=fastapi_sub_app)])
# result: started = []  (sub-app _lifespan never called)

watch_aicore_config() was therefore moved to app/main.py at module level alongside set_aicore_config() — the same proven path that loads credentials on every boot.

Production proof (Kyma managed runtime, auto-doc-dev-eu12)

Thread count in PID 1 before and after deploying commit a72f5e0:

Pod Image Threads in PID 1
Pre-fix (6ad5b0854983) 0.3.3-20260825... (no watcher call) 14
Post-fix (1858f9060d95) 0.3.3-20260825200836_fd446f2+1 15

The extra thread is aicore-secret-watcher (daemon, polls /etc/secrets/appfnd/aicore/aicore-instance/ mtime every 30 s). It will call set_aicore_config() on the next kubelet symlink-swap rotation without requiring a pod restart.

autonomous-documentation-org unit tests: 286 passed, 1 skipped — coverage 90.51%

@tiagoek

tiagoek commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

E2E validation — reactive credential reload (live pod test)

Environment: Kyma managed runtime, namespace auto-doc-dev-eu12, pod autonomous-documentation-ee13df69-1858f9060d95 (image 0.3.3-20260825)

Test script

tests/integration/e2e_llm_reactive_reload.py — run via kubectl exec in the pod where the secret volume is mounted.

Execution output

[1] Real secret loaded from file: 263a99c5...
[2] Env poisoned with invalid secret
INFO sap_cloud_sdk_aicore.completion: AI Core credentials reloading after authentication failure
INFO sap_cloud_sdk_aicore: Loaded AICORE_CLIENT_SECRET from file: /etc/secrets/appfnd/aicore/aicore-instance/clientsecret
INFO sap_cloud_sdk_aicore: AI Core configuration has been set successfully
[3] Post-call secret: (cleared by _clear_client_secret — expected PR#257)
[4] LLM response: 'OK'
[OK] Test 1 PASSED — reactive reload intercepted the failure, retried, LLM responded
[5] LiteLLM aicore/client namespace attrs: [...in_memory_llm_clients_cache...]
[6] AICORE_CLIENT_SECRET in env: (absent — cleared after token acquisition)
[OK] Test 2 PASSED — os.environ is the sole credential store; no per-model client object
INFO sap_cloud_sdk_aicore.completion: AICORE_CLIENT_SECRET cleared from environment after token acquisition (AFSDK-4291)

What this proves

Scenario Result
Invalid AICORE_CLIENT_SECRET in env → LiteLLM call AuthenticationError caught by handler
reload_aicore_credentials() reads from mounted file All 5 AICORE_* vars reloaded from /etc/secrets/appfnd/aicore/aicore-instance/
Retry with restored credentials LLM responded 'OK'
PR#257 _clear_client_secret() behaviour Secret cleared from env after token acquisition — confirmed expected
LiteLLM in_memory_llm_clients_cache Memory cache keyed by model identifier, not by credential values — os.environ read fresh on every OAuth token request

@tiagoek
tiagoek marked this pull request as ready for review August 26, 2026 01:33
@tiagoek
tiagoek requested a review from a team as a code owner August 26, 2026 01:33
Comment thread src/sap_cloud_sdk/aicore/__init__.py Outdated
Comment thread src/sap_cloud_sdk/aicore/__init__.py Outdated
tiagoek added a commit that referenced this pull request Aug 26, 2026
…ature

Address reviewer feedback on PR #256:

1. Remove reload_aicore_credentials() wrapper — inline set_aicore_config()
   directly in the except AuthenticationError blocks. The wrapper added a
   named function for a single call; inlining is simpler and clearer.

2. Remove transparent TLS feature (AICORE_TRANSPARENT_TLS env var,
   _is_transparent_tls(), conditional client_secret handling in
   set_aicore_config()). This feature is blocked on an upstream LiteLLM PR
   and is not needed for the credential rotation fix. Nicole flagged that
   it belongs in a future secrets-resolver refactor.

Behavior unchanged: AuthenticationError still triggers set_aicore_config()
+ retry, completely transparent to callers.
@tiagoek
tiagoek force-pushed the feat/aicore-transparent-tls branch from 439f8d8 to ccadb06 Compare August 26, 2026 14:25

@pedro-hca pedro-hca left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Image

Introduces two security improvements for AI Core credential handling:

1. Transparent TLS mode (AICORE_TRANSPARENT_TLS=true): when active,
   set_aicore_config() skips writing AICORE_CLIENT_SECRET to os.environ
   and removes any stale value. The infrastructure sidecar proxy adds
   the mTLS certificate transparently on the SDK's behalf — no secret
   material needed in the agent process. Addresses HASI2026203 /
   SEC-309 (credentials exposed as env vars with excessive scope).

2. Reactive credential reload on AuthenticationError: completion() and
   acompletion() now intercept litellm.AuthenticationError, re-read
   credentials from the mounted secret volume, and retry once. Covers
   client_secret rotation and mTLS certificate rotation (cert-manager
   updates the volume file; the next failed token refresh triggers the
   reload) without requiring a pod restart.

Relates-to: AFSDK-4306
…ature

Address reviewer feedback on PR #256:

1. Remove reload_aicore_credentials() wrapper — inline set_aicore_config()
   directly in the except AuthenticationError blocks. The wrapper added a
   named function for a single call; inlining is simpler and clearer.

2. Remove transparent TLS feature (AICORE_TRANSPARENT_TLS env var,
   _is_transparent_tls(), conditional client_secret handling in
   set_aicore_config()). This feature is blocked on an upstream LiteLLM PR
   and is not needed for the credential rotation fix. Nicole flagged that
   it belongs in a future secrets-resolver refactor.

Behavior unchanged: AuthenticationError still triggers set_aicore_config()
+ retry, completely transparent to callers.
- Add watch_aicore_config() daemon thread that polls secret directory
  mtime every 30s; on change calls set_aicore_config() proactively
  before LiteLLM's cached OAuth token expires (avoids 401 entirely)
- Add _get_secret_dir_mtime() helper — returns 0.0 on OSError so
  missing dirs are handled safely
- Fix ruff format: add blank line after local imports inside except
  blocks in completion.py (sync and async paths)
- Add test_aicore_watcher.py (10 cases) and
  test_credential_rotation_flow.py (7 cases) covering watcher unit
  behavior and the LiteLLM env-update contract
- Remove __wrapped__ introspection in test_credential_rotation_flow
  that caused ty call-non-callable error; watcher call is already
  verified via reloaded.wait()
- Fix trailing blank lines in test_aicore.py (end-of-file-fixer)
- Bump version 0.38.0 → 0.41.0 (new public API: watch_aicore_config)
@tiagoek
tiagoek force-pushed the feat/aicore-transparent-tls branch from ccadb06 to 10bfdc8 Compare August 26, 2026 20:25
Comment thread tests/aicore/unit/test_completion.py
… agents

Agents using ChatLiteLLM (LangGraph) call litellm.completion directly,
bypassing the SDK completion() wrapper and its reactive 401 handler.

patch_litellm_for_credential_rotation() wraps litellm.completion/acompletion
at the module level so ALL callers get transparent credential reload on
AuthenticationError — including ChatLiteLLM — without any code changes to
the agent's LLM call patterns.

Recommended startup pattern for LangGraph agents:
    set_aicore_config()
    patch_litellm_for_credential_rotation()   # reactive reload for ChatLiteLLM
    watch_aicore_config()                     # proactive reload on rotation

Adds test_langgraph_compat.py (15 tests) covering: env sharing across callers,
reactive path scope documentation, watcher sufficiency, patch behaviour,
idempotency, and full startup pattern end-to-end.

Addresses review comment from thiagob on PR #256 re: LangGraph template compat.
@tiagoek tiagoek changed the title feat(aicore): transparent TLS mode and reactive credential reload feat(aicore): reactive credential reload and proactive secret watcher Aug 28, 2026
Comment thread src/sap_cloud_sdk/aicore/__init__.py
Comment thread src/sap_cloud_sdk/aicore/__init__.py
…tellm_for_credential_rotation

Address Betina's review comments on PR #256: both public API functions now
emit telemetry via @record_metrics, consistent with set_aicore_config and
all other SDK public functions. Adds AICORE_WATCH_CONFIG and
AICORE_PATCH_LITELLM to the Operation enum.
betinacosta
betinacosta previously approved these changes Aug 28, 2026
Documents watch_aicore_config, patch_litellm_for_credential_rotation,
and the recommended startup pattern for LangGraph/ChatLiteLLM agents.
betinacosta
betinacosta previously approved these changes Aug 28, 2026
betinacosta
betinacosta previously approved these changes Aug 28, 2026
Comment thread src/sap_cloud_sdk/aicore/user-guide.md Outdated
Comment thread src/sap_cloud_sdk/aicore/__init__.py
Comment thread src/sap_cloud_sdk/aicore/__init__.py Outdated
Comment thread src/sap_cloud_sdk/aicore/__init__.py Outdated
Replace the startup-only @record_metrics on watch_aicore_config with two
operation-level metrics that fire on every actual reload event:

- AICORE_PROACTIVE_RELOAD: emitted each time the watcher detects a secret
  volume change and calls set_aicore_config proactively
- AICORE_REACTIVE_RELOAD: emitted each time a 401 triggers a credential
  reload in completion(), acompletion(), or the litellm patch

Adds private _reload_proactive / _reload_reactive helpers decorated with
the new operations. Fixes user-guide double-call appearance in watcher
example. Updates operation count test to 161.
@tiagoek
tiagoek merged commit 1406fdb into main Aug 31, 2026
11 checks passed
@tiagoek
tiagoek deleted the feat/aicore-transparent-tls branch August 31, 2026 20:14
tiagoek added a commit that referenced this pull request Sep 1, 2026
…ature

Address reviewer feedback on PR #256:

1. Remove reload_aicore_credentials() wrapper — inline set_aicore_config()
   directly in the except AuthenticationError blocks. The wrapper added a
   named function for a single call; inlining is simpler and clearer.

2. Remove transparent TLS feature (AICORE_TRANSPARENT_TLS env var,
   _is_transparent_tls(), conditional client_secret handling in
   set_aicore_config()). This feature is blocked on an upstream LiteLLM PR
   and is not needed for the credential rotation fix. Nicole flagged that
   it belongs in a future secrets-resolver refactor.

Behavior unchanged: AuthenticationError still triggers set_aicore_config()
+ retry, completely transparent to callers.
tiagoek added a commit that referenced this pull request Sep 1, 2026
…A tests

Add @record_metrics to _configure_proxy_mode and _configure_destination_mode
(Operations.AICORE_PROXY_MODE / AICORE_DESTINATION_MODE) — fills the same
telemetry gap Jean flagged on PR #256 for the two new routing mode helpers.

Add 5 unit tests in TestProxyModeWithLangGraph covering:
- patch_litellm_for_credential_rotation + proxy mode: 401 reload preserves
  AICORE_PROXY_URL and does not inject AICORE_CLIENT_SECRET
- watcher reload re-enters proxy mode, litellm.api_base stays set
- A2A / ChatLiteLLM direct litellm.completion path via proxy gets reactive
  reload after patching
- destination mode: successive set_aicore_config() calls re-fetch credentials
  from Destination Service (mirrors AFSDK-4306 Val 3, auto-doc-dev-eu12)
- proxy mode never writes AICORE_CLIENT_SECRET to env, even after reload

Update test_operation.py count 161 → 163 (9 aicore ops).
tiagoek added a commit that referenced this pull request Sep 1, 2026
…back)

PY-TEL-07: module-level public functions in SDK packages should have
@record_metrics when sibling functions are already instrumented. Catches
the exact pattern Betina flagged on PR #256 (watch_aicore_config and
patch_litellm_for_credential_rotation missing @record_metrics).

PY-TEL-08: when new public SDK functions are added without a corresponding
change to operation.py, emit FLAG suggesting an Operation constant.
Implements Jean's suggestion from PR #256 for richer telemetry granularity.

Also extends client_files detection to all src/sap_cloud_sdk/**/*.py
(not just *Client.py) for broader PY-TEL-07 coverage.

5 new bats tests: FP-TEL-01 through FP-TEL-05. All 40 tests green.
tiagoek added a commit that referenced this pull request Sep 1, 2026
…ature

Address reviewer feedback on PR #256:

1. Remove reload_aicore_credentials() wrapper — inline set_aicore_config()
   directly in the except AuthenticationError blocks. The wrapper added a
   named function for a single call; inlining is simpler and clearer.

2. Remove transparent TLS feature (AICORE_TRANSPARENT_TLS env var,
   _is_transparent_tls(), conditional client_secret handling in
   set_aicore_config()). This feature is blocked on an upstream LiteLLM PR
   and is not needed for the credential rotation fix. Nicole flagged that
   it belongs in a future secrets-resolver refactor.

Behavior unchanged: AuthenticationError still triggers set_aicore_config()
+ retry, completely transparent to callers.
tiagoek added a commit that referenced this pull request Sep 1, 2026
…A tests

Add @record_metrics to _configure_proxy_mode and _configure_destination_mode
(Operations.AICORE_PROXY_MODE / AICORE_DESTINATION_MODE) — fills the same
telemetry gap Jean flagged on PR #256 for the two new routing mode helpers.

Add 5 unit tests in TestProxyModeWithLangGraph covering:
- patch_litellm_for_credential_rotation + proxy mode: 401 reload preserves
  AICORE_PROXY_URL and does not inject AICORE_CLIENT_SECRET
- watcher reload re-enters proxy mode, litellm.api_base stays set
- A2A / ChatLiteLLM direct litellm.completion path via proxy gets reactive
  reload after patching
- destination mode: successive set_aicore_config() calls re-fetch credentials
  from Destination Service (mirrors AFSDK-4306 Val 3, auto-doc-dev-eu12)
- proxy mode never writes AICORE_CLIENT_SECRET to env, even after reload

Update test_operation.py count 161 → 163 (9 aicore ops).
tiagoek added a commit that referenced this pull request Sep 2, 2026
…ature

Address reviewer feedback on PR #256:

1. Remove reload_aicore_credentials() wrapper — inline set_aicore_config()
   directly in the except AuthenticationError blocks. The wrapper added a
   named function for a single call; inlining is simpler and clearer.

2. Remove transparent TLS feature (AICORE_TRANSPARENT_TLS env var,
   _is_transparent_tls(), conditional client_secret handling in
   set_aicore_config()). This feature is blocked on an upstream LiteLLM PR
   and is not needed for the credential rotation fix. Nicole flagged that
   it belongs in a future secrets-resolver refactor.

Behavior unchanged: AuthenticationError still triggers set_aicore_config()
+ retry, completely transparent to callers.
tiagoek added a commit that referenced this pull request Sep 2, 2026
…A tests

Add @record_metrics to _configure_proxy_mode and _configure_destination_mode
(Operations.AICORE_PROXY_MODE / AICORE_DESTINATION_MODE) — fills the same
telemetry gap Jean flagged on PR #256 for the two new routing mode helpers.

Add 5 unit tests in TestProxyModeWithLangGraph covering:
- patch_litellm_for_credential_rotation + proxy mode: 401 reload preserves
  AICORE_PROXY_URL and does not inject AICORE_CLIENT_SECRET
- watcher reload re-enters proxy mode, litellm.api_base stays set
- A2A / ChatLiteLLM direct litellm.completion path via proxy gets reactive
  reload after patching
- destination mode: successive set_aicore_config() calls re-fetch credentials
  from Destination Service (mirrors AFSDK-4306 Val 3, auto-doc-dev-eu12)
- proxy mode never writes AICORE_CLIENT_SECRET to env, even after reload

Update test_operation.py count 161 → 163 (9 aicore ops).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants