fix(config): HYBIM-960 derive standalone API URLs from custom console hosts - #212
fix(config): HYBIM-960 derive standalone API URLs from custom console hosts#212pradystar wants to merge 2 commits into
Conversation
fercor-cisco
left a comment
There was a problem hiding this comment.
🤖 This review was generated by the Astra agent (claude-opus-5). It may contain mistakes.
Verdict: request_changes — The new resolver drops the ticket's explicit "no scheme" branch and now raises an unpacking ValueError for scheme-less console URLs (reachable via the unvalidated StandaloneConfig.console_url), and info.data["console_url"] replaces a graceful lookup with a KeyError when console URL validation fails.
General Comments
- 🟠 major (bug): The Jira ticket's proposed logic has four branches, one of which is explicitly for console URLs with no scheme:
elif "://" in console_url:
api_url = console_url.replace("://", "://api.", 1)
else:
api_url = f"api.{console_url}" # <-- this branchThe implementation dropped that last branch and instead does an unconditional base_url.split("://", 1) unpack into two names. That is not a no-op omission — it turns a previously-degraded-but-working input into a hard ValueError. See the line comment on deployment.py:59 for the concrete failure path.
This matters because the two callers of resolve_standalone_api_url give very different guarantees about the input:
SplunkAOConfig.set_api_urlpassesstr(info.data["console_url"]), whereconsole_urlis a pydanticUrland galileo-core'sensure_https_console_urlhas already prependedhttps://. A scheme is guaranteed.StandaloneConfig.console_urlis a plainstrdataclass field, andStandaloneConfig.from_env()assignsos.environ["SPLUNK_AO_CONSOLE_URL"]verbatim with no normalization at all.
So the shared helper is only safe on one of its two call sites. Since the stated goal of the PR is to make CRUD and OTLP agree, the helper should be the place that normalizes, not assume a caller already did.
Follow-ups
Suggested follow-up work that could be tracked as Jira tickets:
examples/agent/langgraph-otel/main.py:67-69: This example hand-rolls the old derivation (console_url.replace("://console.", "://api.").replace("://app.", "://api.")) and now diverges from the SDK: it will not add the newapi.prefix for a custom console host, so an example user withSPLUNK_AO_CONSOLE_URL=https://customer.example.composts spans tohttps://customer.example.com/otel/v1/traceswhile the SDK itself targetshttps://api.customer.example.com/.... Consider importingresolve_standalone_api_url(orStandaloneConfig.otlp_endpoint) here so the example cannot drift from the resolver again.src/splunk_ao/deployment.py:144-145:StandaloneConfig.console_urlis an unvalidatedstrwhileSplunkAOConfig.console_urlis a pydanticUrlthat gets a scheme prepended byensure_https_console_url. That asymmetry is the root cause of the scheme-less crash flagged in this review, and it will keep producing this class of bug for any future consumer of the raw string. Consider normalizing in__post_init__(prependhttps://when no scheme is present, strip the trailing slash) so both deployment configs offer the same guarantees to shared helpers.src/splunk_ao/deployment.py:57-63: The resolver does string surgery on the whole post-scheme remainder, so a console URL carrying a path is handled oddly:https://example.com/consoleyields host+pathexample.com/console, thestartswith("api.")guard tests against that combined string, and the result ishttps://api.example.com/console. Rewriting this on top ofurllib.parse.urlsplit/urlunsplitwould confine the prefix logic tonetloc, preserve port and path correctly, and remove the need for manual://handling entirely.
| base_url = console_url.rstrip("/") | ||
| base_url = base_url.replace("://console.", "://api.", 1).replace("://app.", "://api.", 1) | ||
| scheme, host = base_url.split("://", 1) | ||
| if not host.startswith("api."): | ||
| host = f"api.{host}" | ||
|
|
||
| return f"{scheme}://{host}" |
There was a problem hiding this comment.
🟠 major (bug): base_url.split("://", 1) unpacks into exactly two names, so any console_url without a scheme raises ValueError: not enough values to unpack (expected 2, got 1).
This is reachable, and it is a regression rather than a pre-existing wart:
StandaloneConfig.console_urlis an unvalidatedstr, andfrom_env()copiesSPLUNK_AO_CONSOLE_URLstraight through (deployment.py:165). Nothing prepends a scheme on this path.- With
SPLUNK_AO_CONSOLE_URL=customer.example.com,StandaloneConfig.from_env().otlp_endpointnow raises. That property is called duringSplunkAOLogger.__init__(logger/logger.py:394) andSplunkAOOTLPExporter.__init__(otel.py:139), so logger construction dies with an opaque unpacking error instead of exporting anything. - The previous implementation was
self.console_url.replace("://console.", "://api.", 1).replace("://app.", "://api.", 1), which left a scheme-less host untouched and producedcustomer.example.com/otel/v1/traces— wrong, but not an exception.
A scheme-less console URL is a realistic user input: several example .env files in this repo prompt for SPLUNK_AO_CONSOLE_URL=your-splunk-ao-console-url with no scheme, and the ticket's own pseudocode has a dedicated branch for it.
str.partition keeps this a one-line fix and defaults to https, matching what galileo-core's ensure_https_console_url does on the config path:
| base_url = console_url.rstrip("/") | |
| base_url = base_url.replace("://console.", "://api.", 1).replace("://app.", "://api.", 1) | |
| scheme, host = base_url.split("://", 1) | |
| if not host.startswith("api."): | |
| host = f"api.{host}" | |
| return f"{scheme}://{host}" | |
| base_url = console_url.rstrip("/") | |
| base_url = base_url.replace("://console.", "://api.", 1).replace("://app.", "://api.", 1) | |
| scheme, separator, host = base_url.partition("://") | |
| if not separator: | |
| scheme, host = "https", base_url | |
| if not host.startswith("api."): | |
| host = f"api.{host}" | |
| return f"{scheme}://{host}" |
🤖 Generated by the Astra agent
| console_url_value = str(info.data["console_url"]) | ||
| api_url_value = str(api_url) if api_url is not None else None | ||
| resolved_api_url = resolve_standalone_api_url(console_url_value, api_url_value) | ||
| return super().set_api_url(resolved_api_url, info) |
There was a problem hiding this comment.
🟠 major (bug): info.data["console_url"] raises KeyError when console_url fails its own validation. In pydantic v2, info.data contains only the fields validated successfully so far, and per-field validators for later fields still run — so a bad console URL means this subscript throws. A KeyError is not a ValueError/AssertionError, so pydantic does not collect it as a validation error; it propagates out of model construction and replaces the real diagnostic. The user typo'd their console URL and gets KeyError: 'console_url' with no mention of the actual problem.
The base implementation this replaced deliberately avoided that (base_config.py:185-190):
console_url = info.data.get("console_url")
if console_url is None:
raise ValueError(
"Console URL is required. Please set the environment variable "
"`GALILEO_CONSOLE_URL` to your Galileo console URL."
)Reachable via SplunkAOConfig.get(console_url="not a url") or SPLUNK_AO_CONSOLE_URL="not a url" — ensure_https_console_url prepends https://, the embedded space still fails Url validation, and this validator then runs without console_url in info.data.
Falling back to super() when console_url is absent keeps the base class's clear message and its Url-vs-str handling, and costs nothing on the happy path:
| console_url_value = str(info.data["console_url"]) | |
| api_url_value = str(api_url) if api_url is not None else None | |
| resolved_api_url = resolve_standalone_api_url(console_url_value, api_url_value) | |
| return super().set_api_url(resolved_api_url, info) | |
| console_url = info.data.get("console_url") | |
| if console_url is None: | |
| return super().set_api_url(api_url, info) | |
| api_url_value = str(api_url) if api_url is not None else None | |
| resolved_api_url = resolve_standalone_api_url(str(console_url), api_url_value) | |
| return super().set_api_url(resolved_api_url, info) |
🤖 Generated by the Astra agent
| if "localhost" in console_url or "127.0.0.1" in console_url: | ||
| return "http://localhost:8088" |
There was a problem hiding this comment.
🟡 minor (bug): The PR description says "Kept localhost development behavior and the /otel/v1/traces endpoint unchanged," but the OTLP endpoint for local dev did change. otlp_endpoint previously had no localhost branch at all — it only did the console./app. replacements — so the console port was carried through:
| console_url | otlp_endpoint before |
after |
|---|---|---|
http://localhost:8088 |
http://localhost:8088/otel/v1/traces |
unchanged |
http://localhost:3000 |
http://localhost:3000/otel/v1/traces |
http://localhost:8088/otel/v1/traces |
Routing to the API port rather than the console port is very likely the intended outcome, so I'm not asking you to revert it — but it should be stated in the description and the CHANGELOG rather than described as unchanged, and it deserves a test (see the tests/test_deployment.py comment). The hardcoded 8088 also silently discards a non-default local API port; anyone running the API elsewhere locally must now set SPLUNK_AO_API_URL, which is worth the same README note the custom-domain fallback got.
Separately, "localhost" in console_url is a substring test, so https://localhost.customer.example.com also collapses to http://localhost:8088. That over-broad match is inherited verbatim from galileo-core's set_api_url, so it is pre-existing on the CRUD path — but this change newly extends it to OTLP export.
🤖 Generated by the Astra agent
There was a problem hiding this comment.
🟡 minor (testing): The new test_resolve_standalone_api_url parametrization covers six happy-path shapes but none of the inputs that changed behavior in a risky way:
- No scheme (
"customer.example.com") — currently raisesValueError. The ticket called this case out explicitly; a parametrized case here would have caught it. - Explicit
api_urlargument — theif api_url: return api_urlshort-circuit is only exercised indirectly throughSplunkAOConfigintest_config.pyand throughStandaloneConfig.otlp_endpoint; the resolver's own override behavior is untested. otlp_endpointfor a localhost console on a non-8088 port — this is the one case whose output actually changed (see thedeployment.py:54comment), and nothing asserts the new value.
Cases 1 and 3 are both inside the problem this PR set out to solve, so they belong in this PR rather than a follow-up.
🤖 Generated by the Astra agent
|
Regressions in this PR The core fix is right: 1. console_url_value = str(info.data["console_url"])In pydantic v2
The user typo'd their console URL and gets no mention of it. The base implementation avoids this deliberately ( 2.
Routing to the API port rather than the console port is very likely intended, so I'm not asking for a revert — but the description states localhost behavior is "unchanged," and nothing tests the one output that actually moved. 3. Schemeless URLs — not a regression, but the error should be explicit Schemeless
No working behavior is lost, so this isn't a regression — but the new failure names neither the variable nor the problem. Since the ticket called this case out, reject it explicitly in Test gaps
|
|
fercor-cisco
left a comment
There was a problem hiding this comment.
🤖 This review was generated by the Astra agent (claude-opus-5). It may contain mistakes.
Verdict: request_changes — The new unconditional api. prefix breaks previously-working on-prem console URLs that are IP literals or single-label hosts (e.g. http://10.0.0.5:8088, http://ao-prod:8088), and the behavior change is filed as a bug fix with no migration note.
General Comments
- 🟡 minor (design): The resolver silently rewrites the API host, and when it guesses wrong the user's first symptom is an opaque failure far from the cause: for CRUD it's a healthcheck connection error inside
GalileoConfig.set_api_urlsurfaced throughConfiguration.connect()'s generic"Connection failed: ..."wrapper; for OTLP it's a silent export failure in a background span sink.
Since the derived host is now a guess about the deployment's DNS layout rather than a mechanical console.→api. substitution, consider emitting a debug/info log from resolve_standalone_api_url when the derived host differs from the console host, naming SPLUNK_AO_API_URL as the override. One line makes an otherwise invisible inference diagnosable:
if derived != console_host:
logger.debug(
f"Derived standalone API URL {derived} from console URL {console_url}. "
"Set SPLUNK_AO_API_URL if your deployment does not follow this convention."
)Follow-ups
Suggested follow-up work that could be tracked as Jira tickets:
examples/agent/langgraph-otel/main.py:62-69: This example hand-rolls the old derivation (console_url.replace("://console.", "://api.").replace("://app.", "://api.")) and now disagrees with the SDK for custom console hosts: forSPLUNK_AO_CONSOLE_URL=https://customer.example.comthe example posts tohttps://customer.example.com/otel/v1/traceswhile the SDK useshttps://api.customer.example.com/otel/v1/traces. Not blocking (the example is standalone and doesn't import the SDK resolver), but it now teaches the wrong convention. Consider importingresolve_standalone_api_url, or deriving the endpoint fromStandaloneConfig.from_env().otlp_endpoint, so the example cannot drift from the SDK again.src/splunk_ao/deployment.py:49-65:resolve_standalone_api_urlpasses an explicitapi_urlthrough completely unvalidated (line 51-52), on both the OTLP path (StandaloneConfig.from_env()copiesSPLUNK_AO_API_URLstraight from the env, line 167) and the CRUD path. A schemelessSPLUNK_AO_API_URL=backend.example.comtherefore reaches the exporter and fails at first export withrequests.MissingSchemarather than at construction. This is unchanged by this PR and correctly out of scope for the custom-console-host fix, but now that the resolver is the single funnel for both paths it is the natural place to normalize/reject explicit overrides the same way console URLs are handled.
| base_url = console_url.rstrip("/") | ||
| if "://" not in base_url: | ||
| base_url = f"https://{base_url}" | ||
| base_url = base_url.replace("://console.", "://api.", 1).replace("://app.", "://api.", 1) | ||
| scheme, host = base_url.split("://", 1) | ||
| if not host.startswith("api."): | ||
| host = f"api.{host}" | ||
|
|
||
| return f"{scheme}://{host}" |
There was a problem hiding this comment.
🟠 major (bug): The api. prefix is applied to any host that isn't already api.-prefixed, including hosts where an api. subdomain cannot exist. This breaks deployments that worked before this PR on both paths.
Concrete failures:
SPLUNK_AO_CONSOLE_URL |
before (CRUD + OTLP) | after |
|---|---|---|
http://10.0.0.5:8088 |
http://10.0.0.5:8088 |
http://api.10.0.0.5:8088 |
http://ao-prod:8088 |
http://ao-prod:8088 |
http://api.ao-prod:8088 |
IP-literal and single-label (k8s service / hostname) console URLs are the normal shape for on-prem single-ingress installs — the repo's own fixtures and docs use exactly this layout (http://localhost:8088 in splunk-ao-a2a/README.md:76, http://fake.test:8088 in tests/conftest.py:26). Pre-PR, galileo-core's console_url.replace("console", "api") was a no-op on these, so api_url fell back to the console host and worked when console and API share a host+port; otlp_endpoint did the same. After this change both resolve to a hostname that does not resolve in DNS, and the only recovery is setting SPLUNK_AO_API_URL.
127.0.0.1 is already special-cased above, which shows IP-shaped input was contemplated — but 10.0.0.5, 192.168.x.x, and ::1 are not, and neither is any dotless host. Prefixing only makes sense for a dotted DNS name, so guard on that:
Note this also fixes the sibling case where the host part carries a port or path into the startswith("api.") check.
| base_url = console_url.rstrip("/") | |
| if "://" not in base_url: | |
| base_url = f"https://{base_url}" | |
| base_url = base_url.replace("://console.", "://api.", 1).replace("://app.", "://api.", 1) | |
| scheme, host = base_url.split("://", 1) | |
| if not host.startswith("api."): | |
| host = f"api.{host}" | |
| return f"{scheme}://{host}" | |
| base_url = console_url.rstrip("/") | |
| if "://" not in base_url: | |
| base_url = f"https://{base_url}" | |
| base_url = base_url.replace("://console.", "://api.", 1).replace("://app.", "://api.", 1) | |
| scheme, host = base_url.split("://", 1) | |
| hostname = host.partition("/")[0].partition(":")[0] | |
| # Only a dotted DNS name can gain an `api.` label. IP literals and | |
| # single-label hosts (k8s services, bare hostnames) must be left alone — | |
| # `api.10.0.0.5` and `api.ao-prod` do not resolve. | |
| is_ip_literal = ":" in hostname or (bool(hostname) and all(part.isdigit() for part in hostname.split("."))) | |
| is_dotted_domain = "." in hostname and not is_ip_literal | |
| if is_dotted_domain and not hostname.startswith("api."): | |
| host = f"api.{host}" | |
| return f"{scheme}://{host}" |
🤖 Generated by the Astra agent
| - Standalone custom console domains now derive a consistent `api.` hostname | ||
| for both CRUD operations and OTLP trace export unless `SPLUNK_AO_API_URL` is | ||
| set explicitly. |
There was a problem hiding this comment.
🟠 major (documentation): This is filed under ### Fixed, but for a class of existing users it is a breaking change, not a fix. Any standalone deployment whose console host lacks a console./app. prefix previously had api_url fall back to the console host (galileo-core's replace("console", "api") was a no-op) and worked if console and API shared a host. Those deployments now point at api.<console-host> and stop working until SPLUNK_AO_API_URL is set.
The PR description does call this out under "Compatibility / risk", but the CHANGELOG is what users actually read on upgrade, and it currently reads as a pure improvement. Two other behavior changes are also unrecorded: otlp_endpoint for a localhost console on a non-8088 port now retargets to http://localhost:8088, and hosts containing console as a non-leading substring (customer-console.example.com) now derive api.customer-console.example.com instead of customer-api.example.com.
Please add a ### Changed entry with the migration step:
| - Standalone custom console domains now derive a consistent `api.` hostname | |
| for both CRUD operations and OTLP trace export unless `SPLUNK_AO_API_URL` is | |
| set explicitly. | |
| ### Changed | |
| - **Standalone API URL derivation.** Console hosts without a leading `console.` | |
| or `app.` label now derive an `api.`-prefixed API hostname instead of reusing | |
| the console host. Deployments that serve the console and API from the same | |
| host, or whose API hostname does not follow this convention, must now set | |
| `SPLUNK_AO_API_URL` explicitly. Two related changes: `otlp_endpoint` for a | |
| localhost console now targets the API port (`http://localhost:8088`) rather | |
| than the console port, and `console` is now matched only as a leading label, | |
| so hosts such as `customer-console.example.com` derive | |
| `api.customer-console.example.com`. | |
| ### Fixed | |
| - Standalone custom console domains now derive a consistent `api.` hostname | |
| for both CRUD operations and OTLP trace export unless `SPLUNK_AO_API_URL` is | |
| set explicitly. |
🤖 Generated by the Astra agent
| scheme, host = base_url.split("://", 1) | ||
| if not host.startswith("api."): | ||
| host = f"api.{host}" |
There was a problem hiding this comment.
🟡 minor (bug): host here is everything after ://, so any path on the console URL is carried into the derived API base. https://customer.example.com/ui yields https://api.customer.example.com/ui, and otlp_endpoint then produces https://api.customer.example.com/ui/otel/v1/traces.
The function's contract is to return an API base URL, so the console's path component should not survive. Pre-PR this was harmless (the host was unchanged, so the path was at least still valid for that origin); now both the host and the path are wrong. Deriving from the parsed hostname — as suggested in the comment on lines 57-65 — drops the path as a side effect. If you'd rather keep the string manipulation, strip the path explicitly:
host = host.partition("/")[0]A parametrized case such as ("https://customer.example.com/ui", None, "https://api.customer.example.com") would pin this.
🤖 Generated by the Astra agent
| if "localhost" in console_url or "127.0.0.1" in console_url: | ||
| return "http://localhost:8088" |
There was a problem hiding this comment.
🟡 minor (bug): "localhost" in console_url is an unanchored substring test, so a legitimate public host containing that label collapses to http://localhost:8088 — e.g. https://localhost.customer.example.com sends both CRUD traffic and OTLP spans to the developer's own machine.
This over-broad match is inherited verbatim from galileo-core's set_api_url, so on the CRUD path it is pre-existing — but this PR newly extends it to OTLP export, which previously had no localhost branch at all. Since you're already centralizing the logic here, matching the host rather than the whole string closes it:
hostname = urlsplit(base_url if "://" in console_url else f"https://{console_url}").hostname or ""
if hostname in ("localhost", "127.0.0.1", "::1") or hostname.endswith(".localhost"):
return "http://localhost:8088"At minimum this deserves a parametrized negative case (https://localhost.customer.example.com → https://api.localhost.customer.example.com) so the intent is recorded either way.
🤖 Generated by the Astra agent
| For standalone custom domains, the SDK derives the API hostname by replacing a | ||
| leading `console.` or `app.` label with `api.`, or by adding an `api.` prefix | ||
| when neither label is present. Set `SPLUNK_AO_API_URL` explicitly when your API | ||
| does not follow this convention. |
There was a problem hiding this comment.
🟡 minor (documentation): This paragraph documents the console./app. → api. mapping and the api. fallback, but omits the localhost branch directly above it in the resolver, which is the case on-prem developers hit first: any console URL containing localhost or 127.0.0.1 resolves to http://localhost:8088 regardless of the console port. Anyone running the API on a different local port must set SPLUNK_AO_API_URL, and nothing tells them so.
Worth one more sentence here, especially since splunk-ao-a2a/README.md:76 advertises http://localhost:8088 as the example console URL:
| For standalone custom domains, the SDK derives the API hostname by replacing a | |
| leading `console.` or `app.` label with `api.`, or by adding an `api.` prefix | |
| when neither label is present. Set `SPLUNK_AO_API_URL` explicitly when your API | |
| does not follow this convention. | |
| For standalone custom domains, the SDK derives the API hostname by replacing a | |
| leading `console.` or `app.` label with `api.`, or by adding an `api.` prefix | |
| when neither label is present. Local development console URLs (`localhost` or | |
| `127.0.0.1`) always resolve to `http://localhost:8088`. Set `SPLUNK_AO_API_URL` | |
| explicitly when your API does not follow these conventions — for example when | |
| the console and API share a hostname, or when a local API listens on a port | |
| other than 8088. |
🤖 Generated by the Astra agent
|
Triaged the review — responses below.
CHANGELOG
README localhost branch — not documenting; the intended audience is not running the API server locally. Two low-priority follow-ups filed: |
Summary
Correct standalone API URL derivation so CRUD operations and OTLP trace export use the same API host for custom console domains.
What changed
SPLUNK_AO_API_URLoverrides.console.andapp.hosts toapi.and preserving existingapi.hosts.api.prefix for custom console hosts without a recognized prefix.http://localhost:8088and the/otel/v1/tracesendpoint unchanged, .Testing
poetry check --lock, andgit diff --checkpassed.Compatibility / risk
This changes fallback behavior for standalone custom domains: a console host such as
customer.example.comnow derivesapi.customer.example.com. Deployments whose API does not follow this convention must setSPLUNK_AO_API_URLexplicitly. O11y behavior is unchanged.