diff --git a/tests/e2e/coverage_registry/mgmt.yaml b/tests/e2e/coverage_registry/mgmt.yaml index 9890902fa5e..85fbd0acd91 100644 --- a/tests/e2e/coverage_registry/mgmt.yaml +++ b/tests/e2e/coverage_registry/mgmt.yaml @@ -72,7 +72,6 @@ - {id: mgmt.callback.list.happy_path, module: mgmt, tier: P2, surface: api, assertions: [happy_path], source: "callback_management_endpoints.py", rationale: "Callback config (smoke)"} - {id: mgmt.cost_tracking.estimate.happy_path, module: mgmt, tier: P2, surface: api, assertions: [happy_path], source: "cost_tracking_settings.py", rationale: "Cost estimate (smoke)"} - {id: mgmt.router_settings.update.happy_path, module: mgmt, tier: P2, surface: api, assertions: [happy_path], source: "router_settings_endpoints.py", rationale: "Router config (smoke)"} -- {id: mgmt.config.allowed_ip.changed_key_only, module: mgmt, tier: P2, surface: api, assertions: [persists], source: "proxy_setting_endpoints.py:496", rationale: "An allowed-IP change leaves unrelated file settings out of the DB row"} - {id: mgmt.jwt_key_mapping.new.happy_path, module: mgmt, tier: P2, surface: api, assertions: [happy_path], source: "jwt_key_mapping_endpoints.py", rationale: "JWT->key mapping (smoke)"} - {id: mgmt.compliance.gdpr.happy_path, module: mgmt, tier: P2, surface: api, assertions: [happy_path], source: "compliance_endpoints.py", rationale: "GDPR ops (smoke)"} - {id: mgmt.tool_management.list.happy_path, module: mgmt, tier: P2, surface: api, assertions: [happy_path], source: "tool_management_endpoints.py", rationale: "Tool inventory (smoke)"} diff --git a/tests/e2e/gateway/stage_mirror_ci_config.yml b/tests/e2e/gateway/stage_mirror_ci_config.yml index 1b6ae93f461..8c8e64443cb 100644 --- a/tests/e2e/gateway/stage_mirror_ci_config.yml +++ b/tests/e2e/gateway/stage_mirror_ci_config.yml @@ -1,5 +1,4 @@ general_settings: - max_parallel_requests: 100 proxy_batch_write_at: 5 enable_jwt_auth: true litellm_jwtauth: diff --git a/tests/e2e/management/test_config_misc_endpoints_e2e.py b/tests/e2e/management/test_config_misc_endpoints_e2e.py index 20e98e993d4..a3be0a64e7f 100644 --- a/tests/e2e/management/test_config_misc_endpoints_e2e.py +++ b/tests/e2e/management/test_config_misc_endpoints_e2e.py @@ -7,13 +7,18 @@ so a read-back reflects the change. Router settings, which mutate global proxy state, are exercised with a benign, self-restoring change so a shared proxy is left as it was found. -Cache settings and the Vault config override are deliberately not covered here. -Both routes reconfigure the whole proxy: /cache/settings persists what it receives -into a row that outranks the YAML cache_params and is re-applied on a timer, and -/config_overrides/hashicorp_vault swaps the process-wide secret manager. Neither can -be exercised safely against the shared proxy the suites run on, so they need an -isolated proxy before a test lands. Do not add a read-then-write-back test for -either one. +Cache settings, the Vault config override and the allowed-IP routes are deliberately +not covered here. All three reconfigure the whole proxy: /cache/settings persists what +it receives into a row that outranks the YAML cache_params and is re-applied on a timer, +/config_overrides/hashicorp_vault swaps the process-wide secret manager, and +/add/allowed_ip mutates the live general_settings["allowed_ips"] that +auth_utils._check_valid_ip reads, so the first call locks every other client out of the +shared proxy. The allowlist is an exact string match with no CIDR support, and no route +reports the caller's address as the proxy sees it, so a test cannot allowlist itself +first; /delete/allowed_ip sits behind the same auth dependency, so the cleanup is locked +out too and the proxy stays poisoned for the rest of the build. None of the three can be +exercised safely against the shared proxy the suites run on, so they need an isolated +proxy before a test lands. Do not add a read-then-write-back test for any of them. """ from __future__ import annotations @@ -21,10 +26,9 @@ from __future__ import annotations import math import time from collections.abc import Callable -from typing import Final import pytest -from pydantic import BaseModel, JsonValue, RootModel +from pydantic import BaseModel from e2e_config import unique_marker from e2e_http import NoBody, Success, unwrap, unwrap_status @@ -188,7 +192,7 @@ class JwtKeyMappingResponse(BaseModel): class RouterSettingsPatch(BaseModel): - num_retries: int + retry_after: int class ConfigUpdateBody(BaseModel): @@ -199,39 +203,8 @@ class ConfigUpdateResponse(BaseModel): message: str -class AllowedIpBody(BaseModel): - ip: str - - -class ConfigFieldInfoParams(BaseModel): - field_name: str - - -class ConfigFieldInfoResponse(BaseModel): - field_name: str - field_value: JsonValue - source: str - editable: bool - - -class ConfigListParams(BaseModel): - config_type: str - - -class ConfigListEntry(BaseModel): - field_name: str - field_value: JsonValue - stored_in_db: bool | None - source: str - editable: bool - - -class ConfigListResponse(RootModel[list[ConfigListEntry]]): - pass - - class RouterCurrentValues(BaseModel): - num_retries: int | None = None + retry_after: int | None = None class RouterSettingsResponse(BaseModel): @@ -493,17 +466,25 @@ class TestRouterSettings: ) -> None: """/config/update is the only write path for router_settings (there is no dedicated router-settings write route). The change is restored on teardown so - the shared proxy keeps its original retry policy.""" - original = self._read_num_retries(client) - assert original is not None, "GET /router/settings did not report num_retries; cannot prove a change" - resources.defer(lambda: self._write_num_retries(client, original)) + the shared proxy keeps its original retry policy. - target = original + 5 + retry_after is the subject because it satisfies all three constraints at once: + no lane's config file declares it, so the database owns it and the write is not + refused as config-owned; it is in RUNTIME_UPDATABLE_ROUTER_SETTINGS, so + /config/update accepts it; and it is in ROUTER_SETTINGS_FIELDS backed by an + always-set Router attribute, so GET /router/settings reports it for the + read-back. Bumping it by one second is the smallest change that proves the + round-trip without slowing a concurrent test that hits a retry.""" + original = self._read_retry_after(client) + assert original is not None, "GET /router/settings did not report retry_after; cannot prove a change" + resources.defer(lambda: self._write_retry_after(client, original)) + + target = original + 1 response = unwrap( client.proxy.transport.post( "/config/update", headers=client.proxy.transport.master, - json=ConfigUpdateBody(router_settings=RouterSettingsPatch(num_retries=target)), + json=ConfigUpdateBody(router_settings=RouterSettingsPatch(retry_after=target)), response_type=ConfigUpdateResponse, ) ) @@ -513,20 +494,20 @@ class TestRouterSettings: _ = _poll( client, - lambda: True if self._read_num_retries(client) == target else None, - f"GET /router/settings never reported num_retries {target} after /config/update", + lambda: True if self._read_retry_after(client) == target else None, + f"GET /router/settings never reported retry_after {target} after /config/update", ) - self._write_num_retries(client, original) + self._write_retry_after(client, original) restored = _poll( client, - lambda: original if self._read_num_retries(client) == original else None, - f"GET /router/settings never returned to the original num_retries {original} after the restore", + lambda: original if self._read_retry_after(client) == original else None, + f"GET /router/settings never returned to the original retry_after {original} after the restore", ) - assert restored == original, f"router num_retries left at {restored}, expected the original {original}" + assert restored == original, f"router retry_after left at {restored}, expected the original {original}" @staticmethod - def _read_num_retries(client: ManagementClient) -> int | None: + def _read_retry_after(client: ManagementClient) -> int | None: return unwrap( client.proxy.transport.get( "/router/settings", @@ -534,72 +515,20 @@ class TestRouterSettings: params=NoBody(), response_type=RouterSettingsResponse, ) - ).current_values.num_retries + ).current_values.retry_after @staticmethod - def _write_num_retries(client: ManagementClient, value: int) -> None: + def _write_retry_after(client: ManagementClient, value: int) -> None: _ = unwrap( client.proxy.transport.post( "/config/update", headers=client.proxy.transport.master, - json=ConfigUpdateBody(router_settings=RouterSettingsPatch(num_retries=value)), + json=ConfigUpdateBody(router_settings=RouterSettingsPatch(retry_after=value)), response_type=ConfigUpdateResponse, ) ) -class TestConfigPersistence: - @pytest.mark.covers("mgmt.config.allowed_ip.changed_key_only") - def test_add_allowed_ip_does_not_store_unrelated_config_value( - self, client: ManagementClient, resources: ResourceManager - ) -> None: - allowed_ip: Final = "127.0.0.1" - added: Final = unwrap( - client.proxy.transport.post( - "/add/allowed_ip", - headers=client.proxy.transport.master, - json=AllowedIpBody(ip=allowed_ip), - response_type=ConfigUpdateResponse, - ) - ) - resources.defer( - lambda: unwrap( - client.proxy.transport.post( - "/delete/allowed_ip", - headers=client.proxy.transport.master, - json=AllowedIpBody(ip=allowed_ip), - response_type=ConfigUpdateResponse, - ) - ) - ) - assert added.message == f"IP {allowed_ip} address added successfully" - - listed: Final = unwrap( - client.proxy.transport.get( - "/config/list", - headers=client.proxy.transport.master, - params=ConfigListParams(config_type="general_settings"), - response_type=ConfigListResponse, - ) - ) - unrelated: Final = next(entry for entry in listed.root if entry.field_name == "max_parallel_requests") - assert unrelated.stored_in_db is not True - assert unrelated.source == "config" - assert unrelated.editable is False - - field_info: Final = unwrap( - client.proxy.transport.get( - "/config/field/info", - headers=client.proxy.transport.master, - params=ConfigFieldInfoParams(field_name="max_parallel_requests"), - response_type=ConfigFieldInfoResponse, - ) - ) - assert field_info.source == "config" - assert field_info.editable is False - assert field_info.field_value == unrelated.field_value - - class TestMcpServerSubmission: @pytest.mark.covers("mgmt.mcp_server.register.happy_path") def test_register_submits_pending_server(self, client: ManagementClient, resources: ResourceManager) -> None: diff --git a/tests/test_litellm/proxy/ui_crud_endpoints/test_proxy_setting_endpoints.py b/tests/test_litellm/proxy/ui_crud_endpoints/test_proxy_setting_endpoints.py index 58201bd14ce..b01544e54e3 100644 --- a/tests/test_litellm/proxy/ui_crud_endpoints/test_proxy_setting_endpoints.py +++ b/tests/test_litellm/proxy/ui_crud_endpoints/test_proxy_setting_endpoints.py @@ -2604,6 +2604,69 @@ def test_add_allowed_ip_writes_audit_log(mock_proxy_config, monkeypatch): app.dependency_overrides.pop(user_api_key_auth, None) +def test_add_allowed_ip_hands_save_config_only_the_changed_general_setting(monkeypatch): + """An allowed-IP write must not drag the config file's own general_settings into + the database row. This covers the route end of that contract: what /add/allowed_ip + hands save_config differs from the loaded config in allowed_ips and nothing else. + save_config's end -- that the row it writes holds only those changed keys -- is + covered by test_ProxyConfig_save_config_merges_changed_keys_without_copying_file_settings. + + This lives here rather than in the e2e suite because /add/allowed_ip mutates the + live general_settings["allowed_ips"] that auth_utils._check_valid_ip reads, so on a + shared proxy the first call locks every later request out, cleanup included. + """ + from copy import deepcopy + from unittest.mock import AsyncMock, MagicMock + + import litellm.proxy.proxy_server as proxy_server_module + from litellm.proxy._types import UserAPIKeyAuth + from litellm.proxy.auth.user_api_key_auth import user_api_key_auth + from litellm.proxy.config_resolvers.changed_section_keys import changed_section_keys + from litellm.proxy.config_resolvers.settings_store import SettingsStore + + file_settings = {"max_parallel_requests": 100, "proxy_config_reload_interval_seconds": 7} + store = SettingsStore("general_settings") + store.load_yaml(file_settings) + saved = [] + + fake_prisma = MagicMock() + fake_prisma.db.litellm_auditlog.create = AsyncMock() + + async def _get_config(): + return {"general_settings": deepcopy(file_settings)} + + async def _save_config(new_config=None): + saved.append(new_config) + return new_config + + monkeypatch.setattr(proxy_server_module, "prisma_client", fake_prisma) + monkeypatch.setattr(proxy_server_module, "store_model_in_db", True) + monkeypatch.setattr(proxy_server_module, "premium_user", True) + monkeypatch.setattr(proxy_server_module, "general_settings", store) + monkeypatch.setattr(proxy_server_module.proxy_config, "get_config", _get_config) + monkeypatch.setattr(proxy_server_module.proxy_config, "save_config", _save_config) + + async def _admin_auth(): + return UserAPIKeyAuth( + user_id="config-admin", + api_key="hashed-admin-key", + user_role=LitellmUserRoles.PROXY_ADMIN, + ) + + app.dependency_overrides[user_api_key_auth] = _admin_auth + try: + resp = client.post("/add/allowed_ip", json={"ip": "203.0.113.77"}) + assert resp.status_code == 200, resp.text + + assert len(saved) == 1, f"expected exactly one save_config call, got {len(saved)}" + changed, removed = changed_section_keys(file_settings, saved[0]["general_settings"]) + assert dict(changed) == {"allowed_ips": ["203.0.113.77"]} + assert removed == frozenset() + assert store["allowed_ips"] == ["203.0.113.77"] + finally: + app.dependency_overrides.pop(user_api_key_auth, None) + + def test_delete_allowed_ip_writes_deleted_audit_log(monkeypatch): """Removing an allowed IP must be audited as a deletion, symmetric with the add path."""