From eca59aa90b9ea6b696040857bbc3b495e717ca84 Mon Sep 17 00:00:00 2001 From: mateo-berri <277851410+mateo-berri@users.noreply.github.com> Date: Sun, 6 Sep 2026 03:35:07 -0700 Subject: [PATCH] fix(proxy): make an ambiguous config include loud, not silent An include entry that matches both a file next to the config that declares it and one next to the root config now warns naming both, so a config that resolves to a different file than it used to says so instead of quietly serving other models. Also from reviewing that change: - an empty root object in a bucket fails the boot again instead of coming up empty - a YAML syntax error in a bucket object logs its own line naming the object - an include already loaded is skipped before it is read rather than after - reading a config out of GCS builds the plain bucket client, so it needs no enterprise license and starts no flush loop that nothing ever cancels --- litellm/proxy/common_utils/config_includes.py | 59 ++++++++----- .../proxy/common_utils/load_config_utils.py | 46 ++++++---- litellm/proxy/proxy_server.py | 11 +-- .../common_utils/test_load_config_utils.py | 83 +++++++++++++++++++ .../proxy/proxy_server/test_proxy_config.py | 24 ++++++ 5 files changed, 183 insertions(+), 40 deletions(-) diff --git a/litellm/proxy/common_utils/config_includes.py b/litellm/proxy/common_utils/config_includes.py index 23e6c615a09..c1bb5ae952f 100644 --- a/litellm/proxy/common_utils/config_includes.py +++ b/litellm/proxy/common_utils/config_includes.py @@ -13,28 +13,41 @@ def resolve_include_file_path(include_file: str, declared_in: str, root_config_p Resolve one `include` entry to the file it names, next to the config that declares it. A config written before nested entries resolved this way can name a file sitting next to the root - config instead, so that file is still read, with a warning naming where it was found. + config instead, so that file is still read, with a warning naming where it was found. When both + files exist the one next to the declaring config wins and the other is named in a warning. """ declared_relative: Final = os.path.abspath(os.path.join(os.path.dirname(declared_in), include_file)) - if os.path.exists(declared_relative): - return declared_relative - root_relative: Final = os.path.abspath(os.path.join(os.path.dirname(root_config_path), include_file)) if root_relative == declared_relative or not os.path.exists(root_relative): return declared_relative + if not os.path.exists(declared_relative): + verbose_proxy_logger.warning( + "Config include '%s' declared in %s was not found next to it, so %s was read instead. " + "Move the included file next to the config that declares it.", + include_file, + declared_in, + root_relative, + ) + return root_relative + verbose_proxy_logger.warning( - "Config include '%s' declared in %s was not found next to it, so %s was read instead. " - "Move the included file next to the config that declares it.", + "Config include '%s' declared in %s matches two files. %s sits next to that config and was read, " + "so %s was skipped. Rename one of the two to say which one you meant.", include_file, declared_in, + declared_relative, root_relative, ) - return root_relative + return declared_relative -class ConfigLoader(Protocol): - def __call__(self, include_entry: str, declared_in: str, /) -> Awaitable[tuple[str, Mapping[str, object]]]: ... +class IncludeResolver(Protocol): + def __call__(self, include_entry: str, declared_in: str, /) -> str: ... + + +class ConfigReader(Protocol): + def __call__(self, location: str, /) -> Awaitable[Mapping[str, object]]: ... def _merged_value(base_value: object, included_value: object) -> object: @@ -80,32 +93,40 @@ async def _resolve( config: Mapping[str, object], pending: tuple[tuple[str, str], ...], loaded: frozenset[str], - load: ConfigLoader, + resolve: IncludeResolver, + read: ConfigReader, ) -> Mapping[str, object]: if not pending: return _without_include(config) entry, declared_in = pending[0] - location, included = await load(entry, declared_in) + location: Final = resolve(entry, declared_in) if location in loaded: - return await _resolve(config, pending[1:], loaded, load) + return await _resolve(config, pending[1:], loaded, resolve, read) + included: Final = await read(location) return await _resolve( _merged(config, _without_include(included)), (*pending[1:], *_pending_from(included, location)), loaded | frozenset((location,)), - load, + resolve, + read, ) -async def resolve_includes(config: Mapping[str, object], location: str, load: ConfigLoader) -> dict[str, object]: +async def resolve_includes( + config: Mapping[str, object], + location: str, + resolve: IncludeResolver, + read: ConfigReader, +) -> dict[str, object]: """ Merge every config named by the `include` directive into the config that declares it. - List values are extended and every other value is overridden, each entry is resolved relative to - the config that declares it, a config already pulled in is not merged a second time, and `load` - decides where an entry is read from, so the same merge applies to configs on disk and to configs - hosted in a bucket. + List values are extended and every other value is overridden, `resolve` turns each entry into the + location it names relative to the config that declares it, a config already pulled in is neither + read nor merged a second time, and `read` decides where a location is read from, so the same merge + applies to configs on disk and to configs hosted in a bucket. """ - merged: Final = await _resolve(config, _pending_from(config, location), frozenset((location,)), load) + merged: Final = await _resolve(config, _pending_from(config, location), frozenset((location,)), resolve, read) return dict(merged) # mutable-ok: the proxy mutates the config it loads diff --git a/litellm/proxy/common_utils/load_config_utils.py b/litellm/proxy/common_utils/load_config_utils.py index d62286a50f6..4a082eb307b 100644 --- a/litellm/proxy/common_utils/load_config_utils.py +++ b/litellm/proxy/common_utils/load_config_utils.py @@ -29,8 +29,12 @@ class SyncBucketObjectReader(Protocol): def __call__(self, object_key: str, /) -> object | None: ... -def _parsed_config(file_contents: str) -> object: - parsed: Final = yaml.safe_load(file_contents) +def _parsed_config(object_key: str, file_contents: str) -> object | None: + try: + parsed: Final = yaml.safe_load(file_contents) + except yaml.YAMLError as e: + verbose_proxy_logger.error("Config object %s is not valid YAML: %s", object_key, e) + return None return MappingProxyType({}) if parsed is None else parsed @@ -64,11 +68,13 @@ def s3_object_reader(bucket_name: str) -> SyncBucketObjectReader: try: verbose_proxy_logger.debug("Retrieving %s from S3 bucket: %s", object_key, bucket_name) response: Final = s3_client.get_object(Bucket=bucket_name, Key=object_key) - return _parsed_config(response["Body"].read().decode("utf-8")) + file_contents: Final = response["Body"].read().decode("utf-8") except Exception as e: # noqa: BLE001 # any boto3 error must read as a missing object verbose_proxy_logger.error("Error retrieving %s from S3 bucket %s: %s", object_key, bucket_name, e) return None + return _parsed_config(object_key, file_contents) + return read @@ -77,10 +83,16 @@ def get_file_contents_from_s3(bucket_name: str, object_key: str) -> object | Non def gcs_config_bucket(bucket_name: str) -> "GCSBucketBase | None": - try: - from litellm.integrations.gcs_bucket.gcs_bucket import GCSBucketLogger + """ + Build a plain GCS client for reading config objects. - return GCSBucketLogger(bucket_name=bucket_name) + Reading a config out of a bucket is not GCS logging, so it neither needs the enterprise license + that gate covers nor the batching task the logger starts and never stops. + """ + try: + from litellm.integrations.gcs_bucket.gcs_bucket_base import GCSBucketBase + + return GCSBucketBase(bucket_name=bucket_name) except Exception as e: # noqa: BLE001 # an unbuildable client must read as an unreadable bucket verbose_proxy_logger.error("Error creating the GCS client for bucket %s: %s", bucket_name, e) return None @@ -98,12 +110,14 @@ async def get_config_file_contents_from_gcs( file_contents: Final = await bucket.download_gcs_object(object_key) if file_contents is None: raise Exception(f"File contents are None for {object_key}") - return _parsed_config(file_contents.decode("utf-8")) + decoded: Final = file_contents.decode("utf-8") except Exception as e: verbose_proxy_logger.error("Error retrieving %s from GCS bucket %s: %s", object_key, bucket_name, e) return None + return _parsed_config(object_key, decoded) + def resolve_include_object_key(config_object_key: str, include_entry: str) -> str: """ @@ -123,17 +137,19 @@ async def resolve_bucket_includes( object_key: str, fetch: BucketObjectFetcher, ) -> dict[str, object]: - async def load(include_entry: str, declared_in: str) -> tuple[str, Mapping[str, object]]: - include_key: Final = resolve_include_object_key(declared_in, include_entry) + async def read(include_key: str) -> Mapping[str, object]: included: Final = await fetch(include_key) if included is None: raise FileNotFoundError( f"Included config could not be read from bucket: {include_key}. " "The underlying bucket error is logged above." ) - return include_key, included + return included - return await resolve_includes(config=config, location=object_key, load=load) + def resolve(include_entry: str, declared_in: str) -> str: + return resolve_include_object_key(declared_in, include_entry) + + return await resolve_includes(config=config, location=object_key, resolve=resolve, read=read) async def bucket_object_reader(bucket_type: str | None, bucket_name: str) -> BucketObjectReader: @@ -176,7 +192,7 @@ async def get_config_from_bucket( raise ValueError(f"Config object in bucket is not a YAML mapping: {key}") from e config: Final = await fetch(object_key) - if config is None: + if not config: return None return await resolve_bucket_includes(config=config, object_key=object_key, fetch=fetch) @@ -256,11 +272,9 @@ async def download_python_file_from_gcs( bool: True if successful, False otherwise """ try: - from litellm.integrations.gcs_bucket.gcs_bucket import GCSBucketLogger + from litellm.integrations.gcs_bucket.gcs_bucket_base import GCSBucketBase - gcs_bucket: Final = GCSBucketLogger( - bucket_name=bucket_name, - ) + gcs_bucket: Final = GCSBucketBase(bucket_name=bucket_name) file_contents = await gcs_bucket.download_gcs_object(object_key) if file_contents is None: raise Exception(f"File contents are None for {object_key}") diff --git a/litellm/proxy/proxy_server.py b/litellm/proxy/proxy_server.py index 7fada09febf..ae9cd71a956 100644 --- a/litellm/proxy/proxy_server.py +++ b/litellm/proxy/proxy_server.py @@ -4559,17 +4559,18 @@ class ProxyConfig: included_config_adapter: Final = TypeAdapter(dict[str, object]) - async def load_included(include_file: str, declared_in: str) -> tuple[str, Mapping[str, object]]: - file_path: Final = resolve_include_file_path(include_file, declared_in, config_file_path) + def resolve(include_file: str, declared_in: str) -> str: + return resolve_include_file_path(include_file, declared_in, config_file_path) + + async def read_included(file_path: str) -> Mapping[str, object]: if not os.path.exists(file_path): raise FileNotFoundError(f"Included file not found: {file_path}") try: - included: Final = included_config_adapter.validate_python(self._load_yaml_file(file_path)) + return included_config_adapter.validate_python(self._load_yaml_file(file_path)) except ValidationError as e: raise ValueError(f"Included config file is not a YAML mapping: {file_path}") from e - return file_path, included - return await resolve_includes(config=config, location=config_file_path, load=load_included) + return await resolve_includes(config=config, location=config_file_path, resolve=resolve, read=read_included) async def save_config(self, new_config: dict, include_env_vars: bool = False): global prisma_client, general_settings, user_config_file_path, store_model_in_db diff --git a/tests/test_litellm/proxy/common_utils/test_load_config_utils.py b/tests/test_litellm/proxy/common_utils/test_load_config_utils.py index 75aa5a3f0f8..1042dbe9653 100644 --- a/tests/test_litellm/proxy/common_utils/test_load_config_utils.py +++ b/tests/test_litellm/proxy/common_utils/test_load_config_utils.py @@ -1,4 +1,5 @@ import asyncio +import logging import re import threading from unittest.mock import MagicMock, mock_open, patch @@ -7,6 +8,7 @@ import pytest import yaml from litellm.proxy.common_utils.load_config_utils import ( + gcs_config_bucket, get_config_from_bucket, get_file_contents_from_s3, resolve_bucket_includes, @@ -391,3 +393,84 @@ class TestBucketConfigIncludes: ) is None ) + + @pytest.mark.asyncio + async def test_an_object_pulled_in_twice_is_read_once(self): + objects = { + "configs/a.yaml": {"include": ["shared.yaml"]}, + "configs/b.yaml": {"include": ["./shared.yaml"]}, + "configs/shared.yaml": {"model_list": [{"model_name": "shared"}]}, + } + requested = [] + + async def fetch(object_key): + requested.append(object_key) + return objects.get(object_key) + + await resolve_bucket_includes( + config={"include": ["a.yaml", "b.yaml"]}, + object_key="configs/config.yaml", + fetch=fetch, + ) + + assert requested == ["configs/a.yaml", "configs/b.yaml", "configs/shared.yaml"] + + @pytest.mark.asyncio + async def test_an_empty_root_object_does_not_boot_an_empty_proxy(self, monkeypatch): + class FakeGCSBucket: + async def download_gcs_object(self, object_key): + return b"" + + monkeypatch.setattr( + "litellm.proxy.common_utils.load_config_utils.gcs_config_bucket", + lambda bucket_name: FakeGCSBucket(), + ) + + config = await get_config_from_bucket( + bucket_type="gcs", bucket_name="litellm-configs", object_key="lit6982/config.yaml" + ) + + assert config is None + + @pytest.mark.asyncio + async def test_an_object_that_is_not_valid_yaml_is_reported_as_a_yaml_error(self, monkeypatch, caplog): + class FakeGCSBucket: + async def download_gcs_object(self, object_key): + return b"model_list: [\n" + + monkeypatch.setattr( + "litellm.proxy.common_utils.load_config_utils.gcs_config_bucket", + lambda bucket_name: FakeGCSBucket(), + ) + + with caplog.at_level(logging.ERROR, logger="LiteLLM Proxy"): + config = await get_config_from_bucket( + bucket_type="gcs", bucket_name="litellm-configs", object_key="lit6982/config.yaml" + ) + + assert config is None + assert [ + record + for record in caplog.records + if "not valid YAML" in record.getMessage() and "lit6982/config.yaml" in record.getMessage() + ] + + +class TestGCSConfigBucketClient: + @pytest.mark.asyncio + async def test_reading_a_config_from_gcs_does_not_need_an_enterprise_license(self, monkeypatch): + monkeypatch.setattr("litellm.proxy.proxy_server.premium_user", False) + + bucket = gcs_config_bucket("litellm-configs") + + assert bucket is not None + assert bucket.BUCKET_NAME == "litellm-configs" + + @pytest.mark.asyncio + async def test_reading_a_config_from_gcs_starts_no_background_task(self, monkeypatch): + monkeypatch.setattr("litellm.proxy.proxy_server.premium_user", True) + running_before = asyncio.all_tasks() + + gcs_config_bucket("litellm-configs") + + assert asyncio.all_tasks() - running_before == set() diff --git a/tests/test_litellm/proxy/proxy_server/test_proxy_config.py b/tests/test_litellm/proxy/proxy_server/test_proxy_config.py index e90f101a555..fe64bcc2c10 100644 --- a/tests/test_litellm/proxy/proxy_server/test_proxy_config.py +++ b/tests/test_litellm/proxy/proxy_server/test_proxy_config.py @@ -10,6 +10,7 @@ from __future__ import annotations import asyncio import json +import logging import os import re from types import SimpleNamespace @@ -772,6 +773,29 @@ async def test_ProxyConfig__process_includes_still_reads_a_nested_include_left_b assert result == {"model_list": [{"model_name": "first"}, {"model_name": "second"}]} +@pytest.mark.asyncio +async def test_ProxyConfig__process_includes_names_both_files_when_a_nested_include_matches_two(tmp_path, caplog): + (tmp_path / "shared").mkdir() + (tmp_path / "shared" / "models.yaml").write_text( + "include:\n - more_models.yaml\nmodel_list:\n - model_name: first\n" + ) + (tmp_path / "shared" / "more_models.yaml").write_text("model_list:\n - model_name: next-to-the-declaring-file\n") + (tmp_path / "more_models.yaml").write_text("model_list:\n - model_name: next-to-the-root-config\n") + + with caplog.at_level(logging.WARNING, logger="LiteLLM Proxy"): + result = await ProxyConfig()._process_includes( + {"include": ["shared/models.yaml"]}, config_file_path=str(tmp_path / "config.yaml") + ) + + assert result == {"model_list": [{"model_name": "first"}, {"model_name": "next-to-the-declaring-file"}]} + assert [ + record + for record in caplog.records + if str(tmp_path / "shared" / "more_models.yaml") in record.getMessage() + and str(tmp_path / "more_models.yaml") in record.getMessage() + ] + + @pytest.mark.asyncio async def test_ProxyConfig__process_includes_merges_a_shared_file_once(tmp_path): (tmp_path / "shared.yaml").write_text("model_list:\n - model_name: shared\n")