From dd8d12dfb88e33a06149fa1d6f53b0ff1e07d761 Mon Sep 17 00:00:00 2001 From: Yucheng He Date: Mon, 5 Oct 2026 20:13:55 -0700 Subject: [PATCH] fix(proxy): build proxy-only failure logs from an allowlist and ignore request-time GCS credentials Backport of #26 to rc/1.105.0. Cherry-picked from 9c2877c4be (main). --- .../gcs_bucket/gcs_bucket_base.py | 49 ++--------- litellm/proxy/utils.py | 11 ++- .../gcs_bucket/test_gcs_bucket_base.py | 18 ++++ .../files/test_vertex_ai_files_handler.py | 84 ------------------- ..._utils_model_creation_and_error_logging.py | 23 +++++ 5 files changed, 58 insertions(+), 127 deletions(-) diff --git a/litellm/integrations/gcs_bucket/gcs_bucket_base.py b/litellm/integrations/gcs_bucket/gcs_bucket_base.py index 0856a420d6f..a7211e38fee 100644 --- a/litellm/integrations/gcs_bucket/gcs_bucket_base.py +++ b/litellm/integrations/gcs_bucket/gcs_bucket_base.py @@ -18,7 +18,7 @@ from litellm.llms.custom_httpx.http_handler import ( httpxSpecialProvider, ) from litellm.types.integrations.gcs_bucket import * -from litellm.types.utils import StandardCallbackDynamicParams, StandardLoggingPayload +from litellm.types.utils import StandardLoggingPayload if TYPE_CHECKING: from litellm.llms.vertex_ai.vertex_llm_base import VertexBase @@ -137,49 +137,16 @@ class GCSBucketBase(CustomBatchLogger): async def get_gcs_logging_config(self, kwargs: dict[str, Any] | None = {}) -> GCSLoggingConfig: """ - This function is used to get the GCS logging config for the GCS Bucket Logger. - It checks if the dynamic parameters are provided in the kwargs and uses them to get the GCS logging config. - If no dynamic parameters are provided, it uses the default values. + Returns the bucket and service account this logger was configured with. """ - if kwargs is None: - kwargs = {} - - standard_callback_dynamic_params: Final[StandardCallbackDynamicParams | None] = kwargs.get( - "standard_callback_dynamic_params", None - ) - - bucket_name: str - path_service_account: str | None - if standard_callback_dynamic_params is not None: - verbose_logger.debug("Using dynamic GCS logging") - verbose_logger.debug("standard_callback_dynamic_params: %s", standard_callback_dynamic_params) - - _bucket_name: str | None = standard_callback_dynamic_params.get("gcs_bucket_name", None) or self.BUCKET_NAME - _path_service_account: Final[str | None] = ( - standard_callback_dynamic_params.get("gcs_path_service_account", None) or self.path_service_account_json + if self.BUCKET_NAME is None: + raise ValueError( + "GCS_BUCKET_NAME is not set in the environment, but GCS Bucket is being used as a logging callback. Please set 'GCS_BUCKET_NAME' in the environment." ) - - if _bucket_name is None: - raise ValueError( - "GCS_BUCKET_NAME is not set in the environment, but GCS Bucket is being used as a logging callback. Please set 'GCS_BUCKET_NAME' in the environment." - ) - bucket_name = _bucket_name - path_service_account = _path_service_account - vertex_instance = await self.get_or_create_vertex_instance(credentials=path_service_account) - else: - # If no dynamic parameters, use the default instance - if self.BUCKET_NAME is None: - raise ValueError( - "GCS_BUCKET_NAME is not set in the environment, but GCS Bucket is being used as a logging callback. Please set 'GCS_BUCKET_NAME' in the environment." - ) - bucket_name = self.BUCKET_NAME - path_service_account = self.path_service_account_json - vertex_instance = await self.get_or_create_vertex_instance(credentials=path_service_account) - return GCSLoggingConfig( - bucket_name=bucket_name, - vertex_instance=vertex_instance, - path_service_account=path_service_account, + bucket_name=self.BUCKET_NAME, + vertex_instance=await self.get_or_create_vertex_instance(credentials=self.path_service_account_json), + path_service_account=self.path_service_account_json, ) async def get_or_create_vertex_instance(self, credentials: str | None) -> VertexBase: diff --git a/litellm/proxy/utils.py b/litellm/proxy/utils.py index 2b1259eb469..d1a09b0b065 100644 --- a/litellm/proxy/utils.py +++ b/litellm/proxy/utils.py @@ -1328,6 +1328,13 @@ def _call_type_for_route(route: str | None) -> str | None: _PROXY_ONLY_LLM_API_ERRORS: Final = (HTTPException, ProxyException, GuardrailRaisedException) _LOG_DB_METRICS_CALL_TYPES: Final = frozenset(("get_data", "insert_data", "update_data", "delete_data")) +_PROXY_ONLY_ERROR_LOGGED_FIELDS: Final = frozenset( + {"model", "messages", "input", "prompt", "user", "stream", "metadata", "litellm_metadata", "litellm_call_id"} +) + + +def _proxy_only_error_log_fields(request_data: Mapping[str, _T]) -> Mapping[str, _T]: + return MappingProxyType({k: v for k, v in request_data.items() if k in _PROXY_ONLY_ERROR_LOGGED_FIELDS}) _EXCEPTION_CHAIN_MAX_DEPTH: Final = 16 @@ -3714,7 +3721,7 @@ class ProxyLogging: original_function=route or "IGNORE_THIS", rules_obj=litellm.utils.Rules(), start_time=datetime.now(), - **request_data, + **_proxy_only_error_log_fields(request_data), ) request_data["litellm_logging_obj"] = litellm_logging_obj # rebind-ok: lifted then popped by the caller if "metadata" not in request_data: @@ -3727,7 +3734,7 @@ class ProxyLogging: _litellm_params: Final = {} litellm_param_keys: Final = LoggedLiteLLMParams.__annotations__.keys() - for k, v in request_data.items(): + for k, v in _proxy_only_error_log_fields(request_data).items(): if k in litellm_param_keys: _litellm_params[k] = v elif k not in ("model", "user", "litellm_logging_obj"): diff --git a/tests/unit/integrations/gcs_bucket/test_gcs_bucket_base.py b/tests/unit/integrations/gcs_bucket/test_gcs_bucket_base.py index 781b56cae14..d9d017c1893 100644 --- a/tests/unit/integrations/gcs_bucket/test_gcs_bucket_base.py +++ b/tests/unit/integrations/gcs_bucket/test_gcs_bucket_base.py @@ -91,6 +91,24 @@ class TestGCSBucketBase: custom_llm_provider="vertex_ai", ) + @pytest.mark.asyncio + async def test_gcs_logging_config_ignores_request_time_bucket_and_credentials(self, monkeypatch): + monkeypatch.setenv("GCS_PATH_SERVICE_ACCOUNT", "/admin/sa.json") + handler = GCSBucketBase(bucket_name="admin-bucket") + + with patch.object(handler, "get_or_create_vertex_instance", new_callable=AsyncMock) as mock_vertex: + config = await handler.get_gcs_logging_config( + { + "standard_callback_dynamic_params": { + "gcs_bucket_name": "caller-bucket", + "gcs_path_service_account": "/caller/sa.json", + } + } + ) + + assert (config["bucket_name"], config["path_service_account"]) == ("admin-bucket", "/admin/sa.json") + mock_vertex.assert_awaited_once_with(credentials="/admin/sa.json") + @pytest.mark.asyncio async def test_log_json_data_on_gcs_url_encodes_object_name(self): handler = GCSBucketBase(bucket_name="test-bucket") diff --git a/tests/unit/llms/vertex_ai/files/test_vertex_ai_files_handler.py b/tests/unit/llms/vertex_ai/files/test_vertex_ai_files_handler.py index 9b7cd127b83..39310836bf2 100644 --- a/tests/unit/llms/vertex_ai/files/test_vertex_ai_files_handler.py +++ b/tests/unit/llms/vertex_ai/files/test_vertex_ai_files_handler.py @@ -260,87 +260,3 @@ class TestVertexAIFilesHandler: ) assert service_account == '{"type": "service_account", "project_id": "p"}' - - @pytest.mark.asyncio - async def test_afile_content_honors_per_model_bucket_over_env(self, monkeypatch): - """ - Regression for #32640: a batch output written to a per-model gcs_bucket_name must be - readable even when the global GCS_BUCKET_NAME points at a different bucket. Before the - fix the read path resolved the bucket from env only and raised - "file_id bucket does not match the configured storage bucket". - """ - monkeypatch.setenv("GCS_BUCKET_NAME", "env-default-bucket") - monkeypatch.delenv("GCS_PATH_SERVICE_ACCOUNT", raising=False) - - file_id = "gs%3A%2F%2Fmy-model-bucket%2Flitellm-vertex-files%2Fuploads%2Fabc-batch-output.jsonl" - file_content_request = FileContentRequest(file_id=file_id, extra_headers=None, extra_body=None) - - with ( - patch.object(self.handler, "download_gcs_object", new_callable=AsyncMock) as mock_download, - patch.object( - self.handler, - "get_or_create_vertex_instance", - new_callable=AsyncMock, - return_value=object(), - ), - ): - mock_download.return_value = b"batch output" - - result = await self.handler.afile_content( - file_content_request=file_content_request, - vertex_credentials="/model/sa.json", - vertex_project="test-project", - vertex_location="us-central1", - timeout=60.0, - max_retries=0, - litellm_params={ - "gcs_bucket_name": "my-model-bucket", - "vertex_credentials": "/model/sa.json", - }, - ) - - assert isinstance(result, HttpxBinaryResponseContent) - assert result.response.content == b"batch output" - - dynamic_params = mock_download.call_args.kwargs["standard_callback_dynamic_params"] - assert dynamic_params["gcs_bucket_name"] == "my-model-bucket" - assert dynamic_params["gcs_path_service_account"] == "/model/sa.json" - assert mock_download.call_args.kwargs["object_name"] == "litellm-vertex-files/uploads/abc-batch-output.jsonl" - - @pytest.mark.asyncio - async def test_afile_content_reads_without_global_env_bucket(self, monkeypatch): - """ - Regression for #32640: with no global GCS_BUCKET_NAME set, a model-group-level - deployment (per-model gcs_bucket_name) must still be readable. Before the fix the read - path raised "GCS_BUCKET_NAME is not set in the environment". - """ - monkeypatch.delenv("GCS_BUCKET_NAME", raising=False) - monkeypatch.delenv("GCS_PATH_SERVICE_ACCOUNT", raising=False) - - file_id = "gs%3A%2F%2Fmy-model-bucket%2Flitellm-vertex-files%2Fuploads%2Fabc-batch-output.jsonl" - file_content_request = FileContentRequest(file_id=file_id, extra_headers=None, extra_body=None) - - with ( - patch.object(self.handler, "download_gcs_object", new_callable=AsyncMock) as mock_download, - patch.object( - self.handler, - "get_or_create_vertex_instance", - new_callable=AsyncMock, - return_value=object(), - ), - ): - mock_download.return_value = b"batch output" - - result = await self.handler.afile_content( - file_content_request=file_content_request, - vertex_credentials="/model/sa.json", - vertex_project="test-project", - vertex_location="us-central1", - timeout=60.0, - max_retries=0, - litellm_params={"gcs_bucket_name": "my-model-bucket"}, - ) - - assert isinstance(result, HttpxBinaryResponseContent) - dynamic_params = mock_download.call_args.kwargs["standard_callback_dynamic_params"] - assert dynamic_params["gcs_bucket_name"] == "my-model-bucket" diff --git a/tests/unit/proxy/test_proxy_utils_model_creation_and_error_logging.py b/tests/unit/proxy/test_proxy_utils_model_creation_and_error_logging.py index d577ef354f0..29f99bb76da 100644 --- a/tests/unit/proxy/test_proxy_utils_model_creation_and_error_logging.py +++ b/tests/unit/proxy/test_proxy_utils_model_creation_and_error_logging.py @@ -163,6 +163,29 @@ async def test_proxy_only_error_log_keeps_litellm_metadata_in_litellm_params(): assert "litellm_metadata" not in captured["optional_params"] +@pytest.mark.asyncio +async def test_proxy_only_error_log_drops_caller_logging_fields_and_keeps_key_details(): + request_data: Final[dict[str, object]] = { + "model": "gpt-4o", + "messages": [{"role": "user", "content": "hi"}], + "litellm_metadata": {"tags": ["t"]}, + "langsmith_base_url": "https://caller.invalid", + "standard_callback_dynamic_params": {"gcs_path_service_account": "/caller/sa.json"}, + } + + await ProxyLogging(user_api_key_cache=DualCache())._handle_logging_proxy_only_error( + request_data=request_data, + user_api_key_dict=UserAPIKeyAuth(api_key="sk-rejected", team_id="team-a", request_route="/v1/chat/completions"), + route="/v1/chat/completions", + original_exception=Exception("bad key"), + ) + + details: Final = request_data["litellm_logging_obj"].model_call_details + assert "caller" not in str(details["standard_callback_dynamic_params"]) + assert "langsmith_base_url" not in details + assert details["litellm_params"]["metadata"]["user_api_key_team_id"] == "team-a" + + @pytest.mark.asyncio async def test_proxy_only_error_log_keeps_the_request_litellm_call_id(monkeypatch: pytest.MonkeyPatch): """LIT-7836: a route that already stamped the caller's litellm_call_id must