Skip to content

Commit 1fd2cf9

Browse files
dm36claude
andcommitted
refactor(AGX1-272): query egp-api-backend for FGAC_AGENT_API_KEYS_DUAL_WRITE
Per team discussion: rather than maintain a parallel env-var flag system in scale-agentex, route api_key dual-write flag checks through egp-api-backend's existing flag service. One source of truth across services, single flip surface for ops, fewer per-env env-var allowlists to keep in sync. Changes: - EnvVarKeys.EGP_API_BACKEND_URL — new env var for the egp-api-backend base URL. Used by the new HTTP-backed flag provider. - FeatureFlagProvider rewritten as an HTTP client of egp-api-backend's GET /feature-flag/{id} endpoint: * Forwards x-api-key / x-user-id / x-service-account-id / x-selected-account-id from the caller's principal_context so the endpoint's REQUIRE_IDENTITY_AND_OPTIONAL_ACCOUNT policy admits the request. * Coerces the response's `value` field to bool. * Fails closed to False on any error (config missing, no identity, non-2xx, transport failure, JSON parse failure) — the legacy no-Spark code path is the safe default. * `is_enabled` is now async (HTTP call). Signature is `is_enabled(name, *, principal_context, account_id)`. - AgentAPIKeysUseCase: both call sites now await is_enabled and pass principal_context. _deregister grabs principal_context from self.authorization_service. - Test fixtures: mock FeatureFlagProvider directly (Mock with is_enabled = AsyncMock(return_value=flag_on)) so dual-write tests stay hermetic. The pre-existing FeatureFlagProvider() no-arg constructions in test_agents_api_keys_use_case.py and integration_client.py now pass egp_api_backend_url=None (provider returns False without it, matching the prior "flag never enabled in unit tests" behavior). Out of scope: - Migrating Asher's FGAC_TASKS_DUAL_WRITE flag check off env vars. That's task-team-owned and we leave their existing pattern alone per the team discussion (new-work-only). - Caching the flag response. Each is_enabled is a fresh HTTP call. Egp-api-backend's flag endpoint is fast and the caller paths are already crossing the network for the actual register/deregister, so one extra round-trip is acceptable for now. Add caching later if load profiling shows it matters. Test plan: - uv run pytest agentex/tests/integration/services/test_agent_api_key_service_dual_write.py — 8/8 pass. - Existing 4 unrelated test_agents_api_keys_use_case.py docker-fixture errors predate this commit (verified via `git stash`). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent e72df68 commit 1fd2cf9

6 files changed

Lines changed: 152 additions & 30 deletions

File tree

agentex/src/config/environment_variables.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ class EnvVarKeys(str, Enum):
4444
REDIS_STREAM_TTL_SECONDS = "REDIS_STREAM_TTL_SECONDS"
4545
IMAGE_PULL_SECRET_NAME = "IMAGE_PULL_SECRET_NAME"
4646
AGENTEX_AUTH_URL = "AGENTEX_AUTH_URL"
47+
EGP_API_BACKEND_URL = "EGP_API_BACKEND_URL"
4748
ALLOWED_ORIGINS = "ALLOWED_ORIGINS"
4849
DD_AGENT_HOST = "DD_AGENT_HOST"
4950
DD_STATSD_PORT = "DD_STATSD_PORT"
@@ -100,6 +101,7 @@ class EnvironmentVariables(BaseModel):
100101
)
101102
IMAGE_PULL_SECRET_NAME: str | None = None
102103
AGENTEX_AUTH_URL: str | None = None
104+
EGP_API_BACKEND_URL: str | None = None
103105
ALLOWED_ORIGINS: str | None = None
104106
HTTPX_MAX_CONNECTIONS: int = 200 # Max total connections allowed
105107
HTTPX_MAX_KEEPALIVE_CONNECTIONS: int = 100 # Max connections to keep alive
@@ -166,6 +168,7 @@ def refresh(cls, force_refresh: bool = False) -> EnvironmentVariables | None:
166168
),
167169
IMAGE_PULL_SECRET_NAME=os.environ.get(EnvVarKeys.IMAGE_PULL_SECRET_NAME),
168170
AGENTEX_AUTH_URL=os.environ.get(EnvVarKeys.AGENTEX_AUTH_URL),
171+
EGP_API_BACKEND_URL=os.environ.get(EnvVarKeys.EGP_API_BACKEND_URL),
169172
ALLOWED_ORIGINS=os.environ.get(EnvVarKeys.ALLOWED_ORIGINS, "*"),
170173
DD_AGENT_HOST=os.environ.get(EnvVarKeys.DD_AGENT_HOST),
171174
DD_STATSD_PORT=os.environ.get(EnvVarKeys.DD_STATSD_PORT),

agentex/src/domain/use_cases/agent_api_keys_use_case.py

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -101,8 +101,10 @@ async def create(
101101
api_key_id = orm_id()
102102
zedtoken: str | None = None
103103

104-
if self.feature_flags.is_enabled(
105-
FeatureFlagName.FGAC_AGENT_API_KEYS_DUAL_WRITE, account_id
104+
if await self.feature_flags.is_enabled(
105+
FeatureFlagName.FGAC_AGENT_API_KEYS_DUAL_WRITE,
106+
principal_context=principal_context,
107+
account_id=account_id,
106108
):
107109
zedtoken = await self._register_api_key_in_spark_authz(
108110
api_key_id=api_key_id,
@@ -198,8 +200,10 @@ async def _deregister_api_key_from_spark_authz(
198200
for the caller's account. Failures are logged but do not block the
199201
delete.
200202
"""
201-
if not self.feature_flags.is_enabled(
202-
FeatureFlagName.FGAC_AGENT_API_KEYS_DUAL_WRITE, account_id
203+
if not await self.feature_flags.is_enabled(
204+
FeatureFlagName.FGAC_AGENT_API_KEYS_DUAL_WRITE,
205+
principal_context=self.authorization_service.principal_context,
206+
account_id=account_id,
203207
):
204208
return
205209
try:

agentex/src/utils/feature_flags.py

Lines changed: 125 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,30 @@
1-
import os
1+
"""Per-account feature flag provider.
2+
3+
Queries egp-api-backend's ``GET /feature-flag/{id}`` endpoint with the
4+
caller's identity headers. The endpoint evaluates the flag against the
5+
account_id resolved from those headers and returns a ``FeatureFlag``
6+
payload whose ``value`` field is a typed flag value (bool for ``boolean``
7+
flags, etc.).
8+
9+
Falls back to disabled (``False``) on any error so a transient
10+
egp-api-backend outage doesn't break the dual-write path — the flag-off
11+
behavior is the safe legacy path.
12+
"""
13+
14+
from __future__ import annotations
15+
216
from enum import StrEnum
3-
from typing import Annotated
17+
from typing import Annotated, Any
418

519
from fastapi import Depends
620

21+
from src.config.dependencies import DEnvironmentVariable
22+
from src.config.environment_variables import EnvVarKeys
23+
from src.utils.cached_httpx_client import get_async_client
24+
from src.utils.logging import make_logger
25+
26+
logger = make_logger(__name__)
27+
728

829
class FeatureFlagName(StrEnum):
930
FGAC_TASKS = "fgac-tasks"
@@ -12,19 +33,112 @@ class FeatureFlagName(StrEnum):
1233

1334

1435
class FeatureFlagProvider:
15-
"""Per-account feature flag provider.
36+
"""Per-account feature flag provider backed by egp-api-backend.
1637
17-
v1: env-var allowlist (per-account, comma-separated). The env var name is
18-
derived from the flag name, e.g. ``FGAC_AGENT_API_KEYS_DUAL_WRITE_ACCOUNTS``.
19-
A follow-up will swap this for LaunchDarkly with an account_id context.
38+
Calls ``GET {EGP_API_BACKEND_URL}/feature-flag/{name}`` with the
39+
caller's identity headers. The endpoint evaluates the flag against the
40+
caller's account and returns a ``FeatureFlag`` with a ``value`` field.
41+
For boolean flags this method coerces the value to ``bool``.
42+
43+
Returns ``False`` (flag off) when:
44+
- ``EGP_API_BACKEND_URL`` is not configured;
45+
- the caller's principal has no usable identity headers;
46+
- egp-api-backend returns a non-2xx response;
47+
- any network or parsing error occurs.
48+
49+
Fail-closed-to-disabled is intentional: the legacy code path is the
50+
safe default if FGAC dual-write is unreachable.
2051
"""
2152

22-
def is_enabled(self, name: FeatureFlagName, account_id: str | None) -> bool:
23-
if not account_id:
53+
def __init__(
54+
self,
55+
egp_api_backend_url: DEnvironmentVariable(EnvVarKeys.EGP_API_BACKEND_URL),
56+
):
57+
self.egp_api_backend_url = egp_api_backend_url
58+
59+
async def is_enabled(
60+
self,
61+
name: FeatureFlagName,
62+
*,
63+
principal_context: Any,
64+
account_id: str | None,
65+
) -> bool:
66+
if not self.egp_api_backend_url:
2467
return False
25-
env_key = f"{name.value.upper().replace('-', '_')}_ACCOUNTS"
26-
allowed = os.environ.get(env_key, "")
27-
return account_id in {a.strip() for a in allowed.split(",") if a.strip()}
68+
69+
headers = self._principal_headers(principal_context, account_id)
70+
if not headers:
71+
return False
72+
73+
url = f"{self.egp_api_backend_url.rstrip('/')}/feature-flag/{name.value}"
74+
try:
75+
client = get_async_client()
76+
response = await client.get(url, headers=headers)
77+
except Exception as exc:
78+
logger.warning(
79+
"Feature flag fetch failed; treating as disabled",
80+
extra={
81+
"flag": name.value,
82+
"account_id": account_id,
83+
"error_type": type(exc).__name__,
84+
},
85+
)
86+
return False
87+
88+
if response.status_code != 200:
89+
logger.warning(
90+
"Feature flag non-2xx response; treating as disabled",
91+
extra={
92+
"flag": name.value,
93+
"account_id": account_id,
94+
"status_code": response.status_code,
95+
},
96+
)
97+
return False
98+
99+
try:
100+
payload = response.json()
101+
value = payload.get("value")
102+
except Exception:
103+
logger.warning(
104+
"Feature flag response not JSON-parseable; treating as disabled",
105+
extra={"flag": name.value, "account_id": account_id},
106+
)
107+
return False
108+
109+
return bool(value)
110+
111+
@staticmethod
112+
def _principal_headers(
113+
principal_context: Any, account_id: str | None
114+
) -> dict[str, str]:
115+
"""Build identity headers from the caller's principal_context so
116+
egp-api-backend's ``REQUIRE_IDENTITY_AND_OPTIONAL_ACCOUNT`` policy
117+
admits the request.
118+
119+
Returns ``{}`` when no usable identity is present — the caller
120+
should treat that as flag-off (the legacy path is safe).
121+
"""
122+
if principal_context is None:
123+
return {}
124+
125+
api_key = getattr(principal_context, "api_key", None)
126+
user_id = getattr(principal_context, "user_id", None)
127+
service_account_id = getattr(principal_context, "service_account_id", None)
128+
129+
if not api_key and not user_id and not service_account_id:
130+
return {}
131+
132+
headers: dict[str, str] = {}
133+
if api_key:
134+
headers["x-api-key"] = api_key
135+
if user_id:
136+
headers["x-user-id"] = user_id
137+
if service_account_id:
138+
headers["x-service-account-id"] = service_account_id
139+
if account_id:
140+
headers["x-selected-account-id"] = account_id
141+
return headers
28142

29143

30144
DFeatureFlagProvider = Annotated[FeatureFlagProvider, Depends(FeatureFlagProvider)]

agentex/tests/integration/fixtures/integration_client.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -409,7 +409,7 @@ def create_agent_api_keys_use_case():
409409
agent_repository=isolated_repositories["agent_repository"],
410410
client=isolated_api_key_http_client, # Use mock client for forwarding requests
411411
authorization_service=noop_authorization_service,
412-
feature_flags=FeatureFlagProvider(),
412+
feature_flags=FeatureFlagProvider(egp_api_backend_url=None),
413413
)
414414

415415
def create_deployment_history_use_case():

agentex/tests/integration/services/test_agent_api_key_service_dual_write.py

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@
3737
from src.domain.entities.agent_api_keys import AgentAPIKeyEntity, AgentAPIKeyType
3838
from src.domain.entities.agents import ACPType, AgentEntity, AgentStatus
3939
from src.domain.use_cases.agent_api_keys_use_case import AgentAPIKeysUseCase
40-
from src.utils.feature_flags import FeatureFlagProvider
4140
from src.utils.ids import orm_id
4241

4342

@@ -62,16 +61,14 @@ def _agent() -> AgentEntity:
6261

6362
def _build_use_case(
6463
*,
65-
flag_accounts: str,
64+
flag_on: bool,
6665
principal: SimpleNamespace | None,
6766
register_resource: AsyncMock | None = None,
6867
deregister_resource: AsyncMock | None = None,
6968
agent: AgentEntity | None = None,
7069
create_raises: Exception | None = None,
7170
monkeypatch: pytest.MonkeyPatch,
7271
) -> tuple[AgentAPIKeysUseCase, Mock, AsyncMock, AsyncMock]:
73-
monkeypatch.setenv("FGAC_AGENT_API_KEYS_DUAL_WRITE_ACCOUNTS", flag_accounts)
74-
7572
sample_agent = agent or _agent()
7673

7774
agent_repository = Mock()
@@ -103,7 +100,11 @@ def _build_use_case(
103100
return_value=None
104101
)
105102

106-
feature_flags = FeatureFlagProvider()
103+
# FeatureFlagProvider normally calls egp-api-backend over HTTP. Mock it
104+
# so tests are hermetic; behaviour under test is the use case's response
105+
# to the flag value, not the provider's transport.
106+
feature_flags = Mock()
107+
feature_flags.is_enabled = AsyncMock(return_value=flag_on)
107108

108109
# Patch env var lookup inside UseCase __init__ so we don't depend on real
109110
# env configuration to instantiate.
@@ -133,7 +134,7 @@ async def test_create_api_key_skips_grant_when_flag_off(
133134
) -> None:
134135
agent = _agent()
135136
use_case, repo, register, _ = _build_use_case(
136-
flag_accounts="",
137+
flag_on=False,
137138
principal=_principal(user_id="user-A", account_id="acct-1"),
138139
agent=agent,
139140
monkeypatch=monkeypatch,
@@ -161,7 +162,7 @@ async def test_create_api_key_calls_grant_when_flag_on(
161162
) -> None:
162163
agent = _agent()
163164
use_case, repo, register, _ = _build_use_case(
164-
flag_accounts="acct-1",
165+
flag_on=True,
165166
principal=_principal(user_id="user-A", account_id="acct-1"),
166167
agent=agent,
167168
monkeypatch=monkeypatch,
@@ -197,7 +198,7 @@ async def test_delete_api_key_calls_revoke_when_flag_on(
197198
monkeypatch: pytest.MonkeyPatch,
198199
) -> None:
199200
use_case, repo, _, deregister = _build_use_case(
200-
flag_accounts="acct-1",
201+
flag_on=True,
201202
principal=_principal(user_id="user-A", account_id="acct-1"),
202203
monkeypatch=monkeypatch,
203204
)
@@ -218,7 +219,7 @@ async def test_delete_api_key_skips_revoke_when_flag_off(
218219
monkeypatch: pytest.MonkeyPatch,
219220
) -> None:
220221
use_case, repo, _, deregister = _build_use_case(
221-
flag_accounts="",
222+
flag_on=False,
222223
principal=_principal(user_id="user-A", account_id="acct-1"),
223224
monkeypatch=monkeypatch,
224225
)
@@ -237,7 +238,7 @@ async def test_create_api_key_grant_failure_prevents_db_row(
237238
register_resource = AsyncMock(side_effect=RuntimeError("spark unavailable"))
238239
agent = _agent()
239240
use_case, repo, _, _ = _build_use_case(
240-
flag_accounts="acct-1",
241+
flag_on=True,
241242
principal=_principal(user_id="user-A", account_id="acct-1"),
242243
register_resource=register_resource,
243244
agent=agent,
@@ -263,7 +264,7 @@ async def test_delete_api_key_revoke_failure_does_not_block_delete(
263264
) -> None:
264265
deregister = AsyncMock(side_effect=RuntimeError("spark unavailable"))
265266
use_case, repo, _, deregister_ref = _build_use_case(
266-
flag_accounts="acct-1",
267+
flag_on=True,
267268
principal=_principal(user_id="user-A", account_id="acct-1"),
268269
deregister_resource=deregister,
269270
monkeypatch=monkeypatch,
@@ -285,7 +286,7 @@ async def test_create_api_key_skips_grant_when_no_creator_resolvable(
285286
the dual-write is a no-op (logged) and the row still lands without a tuple."""
286287
agent = _agent()
287288
use_case, repo, register, _ = _build_use_case(
288-
flag_accounts="acct-1",
289+
flag_on=True,
289290
principal=_principal(user_id=None, account_id="acct-1"),
290291
agent=agent,
291292
monkeypatch=monkeypatch,
@@ -313,7 +314,7 @@ async def test_delete_by_agent_id_and_key_name_revokes_existing(
313314
agent = _agent()
314315
existing_id = orm_id()
315316
use_case, repo, _, deregister = _build_use_case(
316-
flag_accounts="acct-1",
317+
flag_on=True,
317318
principal=_principal(user_id="user-A", account_id="acct-1"),
318319
agent=agent,
319320
monkeypatch=monkeypatch,

agentex/tests/unit/use_cases/test_agents_api_keys_use_case.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ def agent_api_keys_use_case(
4545
agent_repository=agent_repository,
4646
client=mock_http_client,
4747
authorization_service=authorization_service,
48-
feature_flags=FeatureFlagProvider(),
48+
feature_flags=FeatureFlagProvider(egp_api_backend_url=None),
4949
)
5050

5151

0 commit comments

Comments
 (0)