Conversation
|
|
||
| def get_configuration( | ||
| self, | ||
| tenant_context: TenantContext, |
There was a problem hiding this comment.
if tenant_context is required for all request, can we move it to client_level? Similar to agent gateway.
Also, can't we infer it from what is created during provisioning / spii?
There was a problem hiding this comment.
Thanks for the feedback. You're right that binding tenant context at client level is the better pattern — it's exactly what agentgateway does with tenant_subdomain: str | Callable[[], str]. The callable form is the key: one injected client instance, and the callable reads from whatever auth context the consumer maintains at call time (e.g. a request-scoped context var populated during SPII handling).
The TenantContext | Callable[[], TenantContext] signature handles both agent deployment modes:
- Single-tenant: tenant IDs are fixed at startup → pass a
TenantContextvalue directly. - Multi-tenant: tenant IDs vary per request → pass a callable that reads from the incoming request context.
One nuance worth aligning on: CBC requires two IDs per call — cbcTenantId (for URL subdomain routing) and appTenantId (query param). The app tenant is extractable from the auth context, but cbcTenantId comes from a separate mapping populated during SPII provisioning.
On inferring from SPII: the client can't own or infer that mapping — the SPII callback handler stores it wherever the consumer decides (a cache, a DB, a context var), and the client has no business coupling to that store. The consumer extracts both IDs and supplies them via the callable. The proposal would be:
client = create_client(
tenant_context=lambda: TenantContext(
cbc_tenant_id=get_cbc_tid(auth_ctx.tenant_id),
app_tenant_id=auth_ctx.tenant_id,
)
)
# then all call sites become:
config = client.get_configuration()Does that match your expectation? If so I'll update DefaultClient.__init__ to accept tenant_context: TenantContext | Callable[[], TenantContext] and remove it from the public method signatures — same pattern as agw's tenant_subdomain.
There was a problem hiding this comment.
I would expect something like create_client(tenant=<tenant>) where tenant is the subscriber agent sub-account id. Would it be possible? And of course you can also have something like:
create_client(config=..., tenant=...) where you can add manually the configuration and override SPII.
There was a problem hiding this comment.
create_client(tenant=subscriber_subaccount_id) would not work for two reasons:
- The CBC API today requires both
app_tenant_id(subscriber subaccount ID) andcbc_tenant_id. How the consumer derivescbc_tenant_idfromapp_tenant_idis outside this SDK's scope — it might be a DB lookup, a cache populated during SPII handshake, a call to UMS, etc. CBC may relax this in future (internally resolvecbc_tenant_idfrom the subaccount ID), but that's not supported today. - For multi-tenant agents,
tenantalso needs to be request-scoped — soCallable[[], TenantContext]is necessary alongside the direct value form.
There was a problem hiding this comment.
Since we agreed on moving tenant_context to client level, I've implemented this in 0c61a7f — TenantContext | Callable[[], TenantContext] at construction time, removed from the public method signatures. The reasoning for the callable and the two IDs is covered in the discussion above.
There was a problem hiding this comment.
Follow-up: went with your suggestion - the caller just provides the subaccount id, no TenantContext type. There's no second tenant id anymore: the full CBC URL comes straight from the tenant-mapping Fragment created during provisioning / SPII handshake.
|
|
||
| Example (local mock):: | ||
|
|
||
| client = DefaultClient(base_url="http://localhost:8001") |
There was a problem hiding this comment.
We don't have a final decision about local mode and we would like to keep it consistent across module. Is this really needed on first version?
There was a problem hiding this comment.
You're right — on reflection this isn't needed. Quick context on why local mode exists: CBC rewrites the URL subdomain to the cbcTenantId on every request. When an agent tests against a locally running mock server (the CBC CLI's local cell command spins one up based on the agent's config object shapes), that rewrite silently corrupts the URL — http://localhost:8001 becomes http://<tenant-id>.localhost:8001, which won't resolve. Auto-detection was added to spare developers from having to know this.
But CLOUD_SDK_CBC_REPLACE_SUBDOMAIN=false alongside CLOUD_SDK_CBC_URL=http://localhost:8001 already handles it — so auto-detection is a convenience, not a necessity. Happy to remove it in v1 and revisit as part of the cross-module local mode decision. Shall I go ahead and remove it?
There was a problem hiding this comment.
Yes please, remove local mode for now if it not a must have in V1.
There was a problem hiding this comment.
Done in c62dfa8 — _is_local_url deleted, user-guide, docstrings, and tests updated accordingly.
| replace_subdomain: bool | None = None | ||
|
|
||
|
|
||
| def load_from_env() -> CBCConfig: |
There was a problem hiding this comment.
Why this is only loading from env? This is not being provisioned by managed runtime. My expectation is that it should work similar to agw, where we read fragments and destination created during provisioning.
There was a problem hiding this comment.
Fair point — I see that aicore supports both:
- Destination mode:
AICORE_DESTINATION_NAMEset → fetches URL + credentials from BTP Destination Service at startup. - Direct mode (fallback): reads from mounted K8s secret volume or env vars — used for local development where no Destination Service is available.
CBC credentials are stored in a BTP Destination entry, so we should support the same pattern: CLOUD_SDK_CBC_DESTINATION_NAME → fetch URL + cert from the destination, with env vars as the local fallback. I'll double check how the CBC URL and credentials are stored in the destination and implement this mirroring the aicore approach — destination mode when CLOUD_SDK_CBC_DESTINATION_NAME is set, env/file fallback otherwise. Does that sound right?
There was a problem hiding this comment.
Yes, this sounds good. We just need to ensure that we are relying on what is already created on sap-internal-sdk during SPII.
There was a problem hiding this comment.
Done. We now read provisioning via sap_cloud_sdk.destination the same way agw does - the CBC URL from the tenant-mapping Destination Fragment (create_fragment_client), and the mTLS provider certificate from the Destination Service (create_certificate_client, provider access strategy).
c3dc18a to
c62dfa8
Compare
Typed Python client for reading tenant-specific business configuration from SAP Central Business Configuration. Supports mTLS (production), local/mock (loopback auto-detection), and HTTPS mock servers via the CLOUD_SDK_CBC_REPLACE_SUBDOMAIN env var override. Public API: create_client(), CBCClient protocol, DefaultClient, CBCConfig, ConfigData / ConfigObject / EntityData / EntityContent, ConsumptionVersions, and a full CBC exception hierarchy.
…h params Replace the cert tuple parameter with symmetric cert_path/key_path params. Add CLOUD_SDK_CBC_CERT / CLOUD_SDK_CBC_KEY env vars so PEM values can be supplied directly (e.g. from K8s secrets) without writing to disk first — create_client() handles the temp-file lifecycle automatically.
…nts, version bump - Bump version to 0.54.0 (required by CI for src/ changes) - Fix ruff format violations in _models.py and client.py - Fix ty errors: conftest fixture return type CBCClient, test_models assert-not-None before .version - Update test_module (15→16) and test_operation (161→163) counts for CBC module/operations
- Soften "do not instantiate" to "prefer create_client" - Replace contradictory direct-instantiation examples with create_client usage - Reference BTP Destination Service and env vars as credential sources - Add tmp/ to .gitignore
`_is_local_url` auto-disabled subdomain replacement for loopback URLs. Replace with an explicit opt-out: `replace_subdomain` now defaults to `True`; consumers set `CLOUD_SDK_CBC_REPLACE_SUBDOMAIN=false` when pointing at a local mock server. - Remove `_is_local_url` from `_http.py` - Default `replace_subdomain` to `True` in `DefaultClient.__init__` - Update `config.py` and `user-guide.md` to document the env-var escape hatch - Remove `TestDefaultClientLocalMode` and related tests
Binds `TenantContext | Callable[[], TenantContext]` at construction time instead of per-call. The callable form supports multi-tenant agents where the tenant varies per request (e.g. read from a request-scoped context var). - `DefaultClient.__init__` and `create_client` gain `tenant_context` param - `get_consumption_versions` and `get_configuration` drop the param - `CBCClient` Protocol updated to match - Integration conftest bakes tenant into the client fixture - Tests cover callable invocation count and missing-tenant error
0c61a7f to
33f6e35
Compare
Redesign the CBC client around a generic core and a platform adapter layered strictly on top of it. Core (client.py): base_url and app_tenant_id are per-request callables; the only credential input is an ssl.SSLContext. create_client is a thin factory. Drops TenantContext, config.py, _http.py, and all env/cert-file machinery. Platform adapter (client_adapter.py): ships the SAP application-platform provisioning defaults. The SDK owns two ContextVars (app_tenant_id_var, tenant_subdomain_var) that the app populates; create_agent_client resolves base_url from the tenant-mapping Destination Fragment (listing the subaccount and matching on appTenantId, pre-PR-SAP#79 shape) and loads the provider mTLS cert from the Destination Service. Every default is overridable via args or CLOUD_SDK_CBC_* env vars. CBC unit coverage 98% (adapter 100%).
get_configuration previously re-invoked the base_url callable on every HTTP request (consumption versions + config objects + one per entity), so a single call triggered 2+N tenant-mapping fragment lookups in the platform adapter. Resolve base_url once at the top of the public call and thread the resolved string into the internal methods (_get_configuration_objects, _fetch_entity_data, _configurations_url), mirroring how app_tenant_id is already threaded. One operation now does one base_url resolution. Also fix incoherent unit-test data: agent-config no longer holds restaurant/contact/hours entities; replaced with tax-config / tax-category / tax-rate to match the finance-domain examples used elsewhere.
The adapter's _resolve_base_url and _load_ssl_context call Destination Service APIs that can raise DestinationError, which leaked past the documented CBCConfigError contract. Wrap both in try/except, re-raising as CBCConfigError with a descriptive message and chained cause.
The adapter loaded the mTLS certificate once in create_agent_client and baked it into the httpx.Client at construction, so a rotated or expired certificate killed a long-lived client until the process restarted — violating the "Credential Binding Rotation" guideline that every module reading credentials must recover from rotation. Make the core reload reactively. ssl_context becomes a Callable[[], ssl.SSLContext] | None, resolved once at construction and re-invoked only when a request fails the TLS handshake. On such a failure the client rebuilds its httpx.Client from a fresh context under a lock (guarding against a thundering herd) and retries the one request once; a second failure, a non-TLS transport error, or a failing rebuild (the cert loader raises CBCConfigError) propagates cleanly, leaving the previous working client in place. TLS failures are detected by walking the exception's __cause__/__context__ chain for ssl.SSLError, since httpx surfaces an expired client cert as ReadError wrapping ssl.SSLError two levels down. Mirrors objectstore's _execute_with_retry rotation pattern. create_agent_client drops its ssl_context parameter and now owns cert resolution end-to-end, passing a lambda: load_ssl_context(...) factory to the core. Expose the three platform resolvers as public composition helpers (resolve_base_url, resolve_app_tenant_id, load_ssl_context) at the top level, so a caller can keep most of the platform preset but override a single axis via create_client instead of reimplementing the resolvers. Also: - resolve base_url + app_tenant_id once per get_configuration on the auto-version path (extract _get_consumption_versions taking resolved values), avoiding a double resolve that could also disagree if the request context changed between the two. - rename _build_client/_rebuild_client/self._client to the _http_client forms, so names disambiguate the httpx client from the CBC client. - build the client with verify=ctx if ctx is not None else True, making it explicit that TLS verification is never disabled.
The pre-commit ty hook checks tests/ (unlike an src-only ty run), which surfaced 10 diagnostics in the CBC test suite. test_client_adapter.py: create_agent_client() returns the CBCClient Protocol, which has no private members, so accessing _ssl_factory / _base_url / _resolve_app_tenant_id failed. Narrow to DefaultClient with an isinstance assert before touching privates, and guard the optional _ssl_factory before calling it. test_client.py: orig_build = client._build_http_client is typed () -> httpx.Client, so orig_build() was Client, not MagicMock, breaking the .request wiring and the -> MagicMock return annotations. Align the build helpers' return types with the method they replace (httpx.Client), cast() the mock where .request is wired, and replace the ineffective mypy # type: ignore[method-assign] with the repo's ty idiom # ty: ignore[invalid-assignment].
Replace the three loose keyword args on create_agent_client (destination_instance, cbc_cert_name, p12_password) with a single CBCDestinationConfig settings object, addressing the PR review request for a config object. The object lives in a new cbc/config.py, matching the dedicated-config.py convention of the agw, adms, print, and destination modules. Per-field env overrides are unchanged: a value set on the config wins over its CLOUD_SDK_CBC_* env var, which in turn falls back to the platform default. Scoped to the adapter layer only — the core create_client and its hardcoded timeout are untouched; a core config object is deferred. Also rename the private _ClientConfig (API-path holder) to _ApiPaths so it no longer reads as a near-homonym of the new public config class.
This branch has not been deployed
2 participants
Description
Adds
sap_cloud_sdk.cbc— a typed Python client for reading tenant-specific business configuration from SAP Central Business Configuration (CBC). The client is built as two layers: a generic core (client.py) wherebase_urlandapp_tenant_idare per-request callables and mTLS is supplied as aCallable[[], ssl.SSLContext]factory, and a platform adapter (client_adapter.py) that ships the SAP application-platform provisioning defaults on top of the core. The mTLS certificate is reloaded automatically when a request fails the TLS handshake (certificate rotation/expiry), so a long-lived client recovers without being recreated. Includes a full exception hierarchy, Pydantic-backed API models, and aCBCClientProtocol for test doubles.Related Issue
Closes #280
Type of Change
How to Test
Unit tests (no external service required):
Integration tests (requires a CBC server or mock):
Against a plain-HTTP mock (
CLOUD_SDK_CBC_URLmust have the CBC tenant id baked in — it is used verbatim):Against a real mTLS server, add the client cert and key paths:
CLOUD_SDK_CBC_URL,CLOUD_SDK_CBC_CERT_PATH, andCLOUD_SDK_CBC_KEY_PATHare integration-harness env vars (read only by the test conftest), not part of the SDK's public API. The cert/key are optional — supply both for mTLS, omit for a plain-HTTP mock.Expected result: 62 unit tests pass; integration tests skip automatically when env vars are absent (CI-safe).
Checklist
Breaking Changes
None. This is a new module with no existing public API.
Additional Notes
Module structure follows the repo convention (
client.py,client_adapter.py,exceptions.py,_models.py,py.typed,user-guide.md).Key design decisions:
create_clientis generic and knows nothing about the platform —base_urlandapp_tenant_idareCallable[[], str]invoked per request (both vary per tenant in a multi-tenant agent), and the credential input is aCallable[[], ssl.SSLContext]factory. The platform adapter (create_agent_client) layers strictly on top and supplies the SAP application-platform defaults.ContextVars (app_tenant_id_var,tenant_subdomain_var) that the app populates per request; it resolvesbase_urlfrom the tenant-mapping Destination Fragment (listing the subaccount and matching onappTenantId) and loads the provider-level mTLS certificate from the Destination Service. Every default is overridable via theCBCDestinationConfigobject passed tocreate_agent_client, orCLOUD_SDK_CBC_*env vars.create_agent_clienttakes a singleCBCDestinationConfigsettings object (which Destination Serviceinstance, certificate name, and keystore password to read) rather than loose keyword args — the config object lives incbc/config.py, matching the dedicated-config.pyconvention of the agw, adms, print, and destination modules. A value set on the config wins over itsCLOUD_SDK_CBC_*env var, which in turn falls back to the platform default.ssl_contextfactory is resolved once at construction and re-invoked only when a request fails the TLS handshake (an expired/rotated client cert surfaces asReadErrorwrappingssl.SSLError, detected by walking the exception cause chain). On such a failure the client rebuilds itshttpx.Clientfrom a fresh context under a lock and retries the request once; a second failure, a non-TLS transport error, or a failing rebuild propagates cleanly, leaving the previous working client in place. A long-livedcreate_agent_client()singleton therefore recovers from rotation on its own. Mirrors the objectstore_execute_with_retryrotation pattern.resolve_base_url,resolve_app_tenant_id,load_ssl_context) are public, exported at the top level.create_agent_clientstays a fixed preset wiring all three; a caller who wants to keep most of the preset but override a single axis composescreate_clientwith the public resolvers plus their own callable for that axis, instead of reimplementing the resolvers.get_configurationreads the config objects from the CBCconfigurationObjectsAPI — which already returns each config object with its child entities — so consumers work with the authored config-object vocabulary without any client-side grouping. The per-requestbase_url/app_tenant_idcallables are each resolved once per public call and threaded into the internal requests, so a singleget_configurationtriggers onebase_urlresolution (one tenant-mapping fragment lookup in the adapter), not one per entity.EntityData,ConfigObject,ConfigDataare plain@dataclass(not Pydantic) — they are constructed in client code, never parsed from JSON.@record_metrics; internal helpers do not, to avoid double-counting a single user operation.Test evidence:
62 passed, 1 warning
Integration: 5 passed in 19.10s (real CBC server)
Sample ConfigData response (real CBC server)
{ "consumption_version": "a0392d4f-...", "app_tenant_id": "<app-tenant-id>", "config_objects": [ { "config_object_id": "payment-config", "entities": [ { "entity_id": "payment-mode", "data": [ { "paymentModeCode": "CASH", "name": "Cash", "isOnline": false }, { "paymentModeCode": "CARD", "name": "Credit / Debit Card", "isOnline": false }, { "paymentModeCode": "DIGITAL_WALLET","name": "Digital Wallet", "isOnline": true } ] } ] }, { "config_object_id": "tax-config", "entities": [ { "entity_id": "tax-category", "data": [ { "code": "STD", "ratePercent": 8.5, "isDefault": true }, { "code": "REDUCED", "ratePercent": 5, "isDefault": false }, { "code": "ZERO", "ratePercent": 0, "isDefault": false } ] } ] } ] }