fix(config): use api.{realm} CRUD host and add O11y API URL override (SAO-18181) - #304
etserend wants to merge 2 commits into
Conversation
Sorry, something went wrong.
…(SAO-18181) O11y CRUD requests now target https://api.{realm}.observability.splunkcloud.com instead of https://app.{realm}.observability.splunkcloud.com. Add optional SPLUNK_AO_O11Y_API_URL to override the CRUD API origin: https only, origin only (no path, credentials, query or fragment), with the origin rebuilt from host and port. The override counts as O11y configuration and is reported by the auth guard when the realm is missing. The OTLP ingest endpoint and the console URL are unchanged. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
🤖 This review was generated by the Astra agent (claude-opus-5-5). It may contain mistakes.
Verdict: needs_discussion — The code is sound. The default CRUD host changes for every realm, but the PR verifies only three realms, and the public variable name is still an open item.
General Comments
- 🟡 minor (question): This change moves the default CRUD host from app.{realm} to api.{realm} for every O11y user. The testing table in the description lists only lab0, rc0 and us1.
If api.{realm} does not route /ao/api/ in another realm where AO is available, CRUD breaks there after upgrade. Examples are us0, eu0, jp0 and au0. The new override is the only way to recover.
Please confirm one of these:
- AO is only available in the verified realms.
- api.{realm} serves /ao/api/ in all realms where AO runs.
The description also leaves "Confirm the SPLUNK_AO_O11Y_API_URL name" unchecked. This is a public surface, so settle it before merge.
- 🟡 minor (testing): SPLUNK_AO_O11Y_API_URL now counts as an O11y detection variable (src/splunk_ao/deployment.py:12). The root fixture clears it (tests/conftest.py:177). The ADK test does not.
splunk-ao-adk/tests/test_observer.py:25 clears only the three old O11y variables. It then sets SPLUNK_AO_API_KEY and SPLUNK_AO_CONSOLE_URL.
If a developer or CI shell exports SPLUNK_AO_O11Y_API_URL, resolve_deployment() raises AmbiguousConfigurationError and the test fails. Add the new variable to that loop. A search for SPLUNK_AO_O11Y_API_TOKEN" in tests, splunk-ao-a2a and splunk-ao-adk found no other env-clearing loop that needs it.
Follow-ups
Suggested follow-up work that could be tracked as Jira tickets:
- splunk-ao-adk/README.md:65-66: The ADK and A2A READMEs (splunk-ao-a2a/README.md:86-87) list the O11y variables but not SPLUNK_AO_O11Y_API_URL. Add a row or link to the root README so integration users can find the override.
Sorry, something went wrong.
| if self.api_url: | ||
| self.api_url = _normalize_api_origin(self.api_url) |
There was a problem hiding this comment.
🟡 minor (design): An invalid override is validated eagerly in __post_init__. Every O11yConfig.from_env() call therefore raises on it, including the telemetry-only paths:
- src/splunk_ao/otel.py:137 (SplunkAOOTLPExporter)
- src/splunk_ao/logger/logger.py:419 (logger sink)
- src/splunk_ao/config.py:230 (_bridge_env_vars, run by SplunkAOConfig.get() at otel.py:133)
Scenario: a telemetry-only user sets SPLUNK_AO_O11Y_API_URL=https://api.lab0.signalfx.com/ao/api/ by mistake. OTLP exporter and logger construction then raise ConfigurationError. This contradicts CHANGELOG.md:18 ("OTLP ingest is not affected") and the "never OTLP ingest" note in AGENTS.md:121.
Options:
- Keep fail-fast. Then reword the docs to say an invalid value fails all SDK configuration. Add a test that SplunkAOConfig.get() raises the clean ConfigurationError and not a wrapped pydantic error.
- Validate lazily in api_root, so only CRUD paths fail.
Option 1 is simpler and catches typos early. It needs the doc fix and the test.
🤖 Generated by the Astra agent
Sorry, something went wrong.
| if any(ch.isspace() for ch in parts.netloc): | ||
| raise ConfigurationError("SPLUNK_AO_O11Y_API_URL has an invalid host.") |
There was a problem hiding this comment.
🟡 minor (bug): The host check rejects only whitespace. urlsplit accepts and keeps other invalid host characters. I checked these values with stdlib urlsplit:
- https://h.com;x gives hostname h.com;x.
- https://h.com\evil gives hostname h.com\evil.
- https://h%2e.com gives hostname h%2e.com.
- https://h:0 gives port 0. The SDK accepts it as https://h:0.
urlsplit also silently strips embedded \n and \t, so https://api.exa\nmple.com becomes api.example.com.
These values pass validation and then fail later at request time with an unclear transport error. That defeats the purpose of the clear error here.
Fix: check parts.hostname against an allowlist pattern (LDH labels, or an IPv6 literal). Reject port 0. If the user enters a Unicode IDN, encode it with idna first, or reject it with a message.
🤖 Generated by the Astra agent
Sorry, something went wrong.
SPLUNK_AO_O11Y_API_URL now counts as an O11y detection variable. The ADK autouse config fixture cleared no O11y variables, so exporting it (or SPLUNK_AO_REALM) broke every ADK test during setup. Clear all four O11y variables in the fixture, mirroring the root tests/conftest.py, and add the new variable to the observer test's own clearing loop. Co-Authored-By: Claude <noreply@anthropic.com>
opikalova
left a comment
There was a problem hiding this comment.
🤖 This review was generated by the Astra agent (claude-opus-5-5). It may contain mistakes.
Verdict: needs_discussion — The ADK isolation fix is correct. The two open deployment.py threads and the realm and name questions from the last review are still unanswered.
General Comments
- 🟡 minor (question): Commit e8e22a5 fixes the ADK test-isolation gap from the earlier review. The autouse fixture at splunk-ao-adk/tests/conftest.py:166-168 now clears all four O11y variables. test_observer.py:25 clears them too.
The earlier rollout question has no answer yet:
- The description still verifies api.{realm} only for lab0, rc0 and us1.
- The description still has "Confirm the SPLUNK_AO_O11Y_API_URL name (public surface)" unchecked.
The default CRUD host changes for every O11y user. Please confirm that api.{realm} serves /ao/api/ in every realm where AO runs, or that AO runs only in the verified realms. Then settle the variable name before merge.
The two open threads on src/splunk_ao/deployment.py are also unaddressed. git diff 8223cdc HEAD changes only the two ADK test files:
- Lines 88-89: eager validation breaks telemetry-only paths. This contradicts CHANGELOG.md:18.
- Lines 64-65: the host check accepts ;, \, % and port 0.
Please fix them or reply on each thread.
Sorry, something went wrong.
Summary
The SDK hardcoded app.{realm}.observability.splunkcloud.com for O11y CRUD requests with no way to override the endpoint. This change moves the default CRUD host and adds an optional override. Jira: SAO-18181.
Testing
Open items before merge
Not covered: an override for the OTLP ingest endpoint, and custom CA/TLS configuration.