Repository navigation
fix(config): use api.{realm} CRUD host and add O11y API URL override (SAO-18181) #304
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 |
|---|---|---|
|
|
@@ -3,12 +3,13 @@ | |
| import os | ||
| from dataclasses import dataclass | ||
| from enum import StrEnum | ||
| from urllib.parse import urlsplit | ||
|
|
||
| from pydantic import SecretStr | ||
|
|
||
| from splunk_ao.shared.exceptions import AmbiguousConfigurationError, MissingConfigurationError | ||
| from splunk_ao.shared.exceptions import AmbiguousConfigurationError, ConfigurationError, MissingConfigurationError | ||
|
|
||
| _O11Y_ENV_VARS = ("SPLUNK_AO_REALM", "SPLUNK_AO_O11Y_TOKEN", "SPLUNK_AO_O11Y_API_TOKEN") | ||
| _O11Y_ENV_VARS = ("SPLUNK_AO_REALM", "SPLUNK_AO_O11Y_TOKEN", "SPLUNK_AO_O11Y_API_TOKEN", "SPLUNK_AO_O11Y_API_URL") | ||
| _STANDALONE_ENV_VARS = ("SPLUNK_AO_API_KEY", "SPLUNK_AO_CONSOLE_URL", "SPLUNK_AO_API_URL") | ||
|
|
||
|
|
||
|
|
@@ -46,13 +47,34 @@ def resolve_deployment() -> DeploymentMode: | |
| ) | ||
|
|
||
|
|
||
| def _normalize_api_origin(value: str) -> str: | ||
| """Validate an API origin override and return it as ``https://host[:port]``.""" | ||
| try: | ||
| parts = urlsplit(value.strip()) | ||
| port = parts.port | ||
| except ValueError: | ||
| raise ConfigurationError("SPLUNK_AO_O11Y_API_URL is not a valid URL.") from None | ||
| if parts.scheme != "https" or not parts.hostname: | ||
| raise ConfigurationError("SPLUNK_AO_O11Y_API_URL must be an https:// URL.") | ||
| if "@" in parts.netloc or parts.query or parts.fragment or parts.path.strip("/"): | ||
| raise ConfigurationError( | ||
| "SPLUNK_AO_O11Y_API_URL must be an origin only (scheme://host[:port]) without credentials, path, " | ||
| "query, or fragment." | ||
| ) | ||
| if any(ch.isspace() for ch in parts.netloc): | ||
| raise ConfigurationError("SPLUNK_AO_O11Y_API_URL has an invalid host.") | ||
| host = f"[{parts.hostname}]" if ":" in parts.hostname else parts.hostname | ||
| return f"https://{host}:{port}" if port is not None else f"https://{host}" | ||
|
|
||
|
|
||
| @dataclass | ||
| class O11yConfig: | ||
| """Configuration for a Splunk Observability Cloud deployment.""" | ||
|
|
||
| realm: str | ||
| o11y_token: SecretStr | None = None | ||
| o11y_api_token: SecretStr | None = None | ||
| api_url: str | None = None | ||
|
|
||
| def __post_init__(self) -> None: | ||
| missing = [] | ||
|
|
@@ -63,6 +85,11 @@ def __post_init__(self) -> None: | |
| if missing: | ||
| raise MissingConfigurationError(f"O11y deployment requires {' and '.join(missing)} to be set.") | ||
|
|
||
| if self.api_url: | ||
| self.api_url = _normalize_api_origin(self.api_url) | ||
|
Comment on lines
+88
to
+89
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. 🟡 minor (design): An invalid override is validated eagerly in
Scenario: a telemetry-only user sets Options:
Option 1 is simpler and catches typos early. It needs the doc fix and the test. 🤖 Generated by the Astra agent |
||
| else: | ||
| self.api_url = None | ||
|
|
||
| if self.o11y_token is not None and not isinstance(self.o11y_token, SecretStr): | ||
| self.o11y_token = SecretStr(self.o11y_token) | ||
| if self.o11y_api_token is not None and not isinstance(self.o11y_api_token, SecretStr): | ||
|
|
@@ -78,6 +105,7 @@ def from_env(cls) -> "O11yConfig": | |
| realm=realm or "", | ||
| o11y_token=SecretStr(o11y_token) if o11y_token else None, | ||
| o11y_api_token=SecretStr(o11y_api_token) if o11y_api_token else None, | ||
| api_url=_env("SPLUNK_AO_O11Y_API_URL"), | ||
| ) | ||
|
|
||
| @property | ||
|
|
@@ -107,11 +135,13 @@ def require_ingest_token(self) -> SecretStr: | |
|
|
||
| @property | ||
| def api_root(self) -> str: | ||
| """Return the realm-derived application origin used by the AO API.""" | ||
| return f"https://app.{self.realm}.observability.splunkcloud.com" | ||
| """Return the explicit API origin override, or the realm-derived origin used by the AO API.""" | ||
| if self.api_url: | ||
| return self.api_url | ||
| return f"https://api.{self.realm}.observability.splunkcloud.com" | ||
|
|
||
| def require_api_url(self) -> str: | ||
| """Return the realm-derived AO CRUD API URL.""" | ||
| """Return the AO CRUD API URL from the override origin or the realm-derived origin.""" | ||
| return f"{self.api_root}/ao/api/" | ||
|
|
||
| def require_console_url(self) -> str: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,13 +7,14 @@ | |
| import pytest | ||
|
|
||
| from splunk_ao.config import SplunkAOConfig | ||
| from splunk_ao.deployment import DeploymentMode, O11yConfig, StandaloneConfig | ||
| from splunk_ao.shared.exceptions import AmbiguousConfigurationError, MissingConfigurationError | ||
| from splunk_ao.deployment import DeploymentMode, O11yConfig, StandaloneConfig, resolve_deployment | ||
| from splunk_ao.shared.exceptions import AmbiguousConfigurationError, ConfigurationError, MissingConfigurationError | ||
|
|
||
| _DETECTION_ENV_VARS = ( | ||
| "SPLUNK_AO_REALM", | ||
| "SPLUNK_AO_O11Y_TOKEN", | ||
| "SPLUNK_AO_O11Y_API_TOKEN", | ||
| "SPLUNK_AO_O11Y_API_URL", | ||
| "SPLUNK_AO_API_KEY", | ||
| "SPLUNK_AO_CONSOLE_URL", | ||
| "SPLUNK_AO_API_URL", | ||
|
|
@@ -176,14 +177,79 @@ def test_require_ingest_token_rejects_crud_only_config() -> None: | |
|
|
||
| def test_api_root_derives_from_realm() -> None: | ||
| cfg = O11yConfig(realm="lab0", o11y_token="tok") | ||
| assert cfg.api_root == "https://app.lab0.observability.splunkcloud.com" | ||
| assert cfg.api_root == "https://api.lab0.observability.splunkcloud.com" | ||
|
|
||
|
|
||
| def test_require_api_url_derives_from_realm() -> None: | ||
| cfg = O11yConfig(realm="lab0", o11y_token="tok") | ||
| assert cfg.require_api_url() == "https://app.lab0.observability.splunkcloud.com/ao/api/" | ||
| assert cfg.require_api_url() == "https://api.lab0.observability.splunkcloud.com/ao/api/" | ||
| assert cfg.require_api_url() == f"{cfg.api_root}/ao/api/" | ||
| assert cfg.require_api_url() == f"{cfg.require_console_url()}ao/api/" | ||
|
|
||
|
|
||
| def test_api_url_override_replaces_api_root_only() -> None: | ||
| # Given: an explicit API origin override | ||
| cfg = O11yConfig(realm="lab0", o11y_token="tok", api_url="https://api.example.com/") | ||
| # Then: CRUD uses the override while ingest and console stay realm-derived | ||
| assert cfg.api_root == "https://api.example.com" | ||
| assert cfg.require_api_url() == "https://api.example.com/ao/api/" | ||
| assert cfg.otlp_endpoint == "https://ingest.lab0.observability.splunkcloud.com/v2/trace/otlp" | ||
| assert cfg.require_console_url() == "https://app.lab0.observability.splunkcloud.com/" | ||
|
|
||
|
|
||
| def test_api_url_override_read_from_env() -> None: | ||
| with env( | ||
| SPLUNK_AO_REALM="lab0", SPLUNK_AO_O11Y_TOKEN="tok", SPLUNK_AO_O11Y_API_URL="https://api.lab0.signalfx.com" | ||
| ): | ||
| assert O11yConfig.from_env().api_root == "https://api.lab0.signalfx.com" | ||
|
|
||
|
|
||
| def test_empty_api_url_override_uses_realm_default() -> None: | ||
| cfg = O11yConfig(realm="lab0", o11y_token="tok", api_url="") | ||
| assert cfg.api_root == "https://api.lab0.observability.splunkcloud.com" | ||
|
|
||
|
|
||
| def test_api_url_override_counts_as_o11y_and_conflicts_with_standalone() -> None: | ||
| with env(SPLUNK_AO_O11Y_API_URL="https://api.lab0.signalfx.com"): | ||
| assert resolve_deployment() == DeploymentMode.O11Y | ||
| with env(SPLUNK_AO_O11Y_API_URL="https://api.lab0.signalfx.com", SPLUNK_AO_API_KEY="key"): | ||
| with pytest.raises(AmbiguousConfigurationError): | ||
| resolve_deployment() | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "value", | ||
| [ | ||
| "http://api.lab0.signalfx.com", | ||
| "http://localhost:8080", | ||
| "api.lab0.signalfx.com", | ||
| "https://user:[email protected]", | ||
| "https://api.lab0.signalfx.com/ao/api/", | ||
| "https://api.lab0.signalfx.com?x=1", | ||
| "https://api.lab0.signalfx.com#frag", | ||
| "https://[::1", | ||
| "https://@api.lab0.signalfx.com", | ||
| "https://api.lab0.signalfx.com:abc", | ||
| "https://api.lab0.signalfx.com:99999", | ||
| "https://api.lab0 .signalfx.com", | ||
| ], | ||
| ) | ||
| def test_api_url_override_rejects_invalid_values(value: str) -> None: | ||
| with pytest.raises(ConfigurationError) as exc_info: | ||
| O11yConfig(realm="lab0", o11y_token="tok", api_url=value) | ||
| assert "secret" not in str(exc_info.value) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| ("value", "expected"), | ||
| [ | ||
| ("https://api.lab0.signalfx.com:", "https://api.lab0.signalfx.com"), | ||
| ("https://api.lab0.signalfx.com:8443/", "https://api.lab0.signalfx.com:8443"), | ||
| (" https://API.lab0.signalfx.com ", "https://api.lab0.signalfx.com"), | ||
| ("https://[::1]:8443", "https://[::1]:8443"), | ||
| ], | ||
| ) | ||
| def test_api_url_override_is_rebuilt_from_host_and_port(value: str, expected: str) -> None: | ||
| assert O11yConfig(realm="lab0", o11y_token="tok", api_url=value).api_root == expected | ||
|
|
||
|
|
||
| def test_require_console_url_derives_from_realm() -> None: | ||
|
|
||
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.
🟡 minor (bug): The host check rejects only whitespace.
urlsplitaccepts and keeps other invalid host characters. I checked these values with stdliburlsplit:https://h.com;xgives hostnameh.com;x.https://h.com\evilgives hostnameh.com\evil.https://h%2e.comgives hostnameh%2e.com.https://h:0gives port0. The SDK accepts it ashttps://h:0.urlsplitalso silently strips embedded\nand\t, sohttps://api.exa\nmple.combecomesapi.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.hostnameagainst an allowlist pattern (LDH labels, or an IPv6 literal). Reject port0. If the user enters a Unicode IDN, encode it withidnafirst, or reject it with a message.🤖 Generated by the Astra agent