diff --git a/docs/credential-hot-reload.md b/docs/credential-hot-reload.md new file mode 100644 index 000000000..fb7b02e82 --- /dev/null +++ b/docs/credential-hot-reload.md @@ -0,0 +1,175 @@ +# RFE-9380: In-Process LLM Credential Hot-Reload + +## Problem + +When `credentialsSecretRef` is updated, the Lightspeed operator triggers a rolling +restart of `lightspeed-app-server`. For customers using short-lived LLM tokens +(e.g. 1-hour validity rotated by CronJob), this causes hourly pod restarts and +temporary capacity loss. + +- **RFE:** [RFE-9380](https://redhat.atlassian.net/browse/RFE-9380) +- **Customer environment:** OCP 4.21, pre-production +- **Current workaround:** replicas >= 3 with manual PDB (minAvailable: 2) + +## Solution + +The service re-reads credential files from disk on every LLM request (Prometheus +`credentials_file` pattern), so rotated secrets are picked up without a pod +restart. + +### Why this matters + +The app-server pod contains the FAISS vector indexes (~100MB) and the +SentenceTransformer embedding model (~400MB), all loaded into memory at startup. +A rolling restart means: + +| Step | Time | +|------|------| +| FAISS index reload | ~5-15s | +| Embedding model reload | ~3-5s | +| Readiness probe pass | ~2-5s | +| **Total cold start** | **~10-25s per pod** | + +With hourly rotation and 3 replicas, that is up to 24 restart cycles/day with +degraded capacity during each one. + +### Cost comparison + +| Factor | Hot-Reload | 2+ Replicas workaround | +|--------|-----------|------------------------| +| CPU per request | 1 file read (~0.05ms) | Zero extra | +| Memory | Zero extra | 2-3x pod memory | +| Compute cost | Negligible | 2-3x vCPU reserved | +| Availability during rotation | 100% | ~95-99% (brief capacity drop) | +| Ops complexity | None | PDB + replica tuning + alert tuning | + +## Design + +Since `load_llm()` already creates a new `LLMProvider` per request, we add a +`get_credentials()` method to `ProviderConfig` that re-reads the credential file +each time it is called. + +```text +Request → load_llm() → LLMProvider.__init__() + → provider_config.get_credentials() + → open(credentials_path) + read() + → return fresh value +``` + +### Why re-read on every request (vs caching or fsnotify) + +- **Kubernetes symlink safety** — kubelet uses atomic symlink swaps (`..data`); + `os.stat()` mtime can be unreliable after swaps. Re-reading avoids edge cases. +- **Proven pattern** — Prometheus `credentials_file` / `password_file` re-reads + on every scrape request. +- **No new dependencies** — no watchdog, fsnotify, or background threads. +- **Negligible overhead** — one `open()+read()` of a <100 byte file per LLM + request is trivial vs multi-second LLM call latency. +- **Thread-safe by default** — each request reads independently. + +### Ecosystem alignment + +| Project | Pattern | +|---------|---------| +| Prometheus | `credentials_file` re-read on every scrape | +| OpenShift library-go | `fileobserver` hash-based polling (Go operators) | +| controller-runtime | `CertWatcher` fsnotify + periodic re-read | +| kube-proxy | watches parent directory (PR #139204) | +| Stakater Reloader | triggers rolling restart (what we are moving away from) | + +## Files Changed + +### Core + +- `ols/utils/checks.py` — added `read_secret_from_path()` helper +- `ols/app/models/config.py` — added `_credentials_path` private attr and + `get_credentials()` method to `ProviderConfig` + +### Providers (7 files) + +All providers changed from `self.provider_config.credentials` to +`self.provider_config.get_credentials()`: + +- `ols/src/llms/providers/openai.py` +- `ols/src/llms/providers/azure_openai.py` +- `ols/src/llms/providers/watsonx.py` +- `ols/src/llms/providers/rhoai_vllm.py` +- `ols/src/llms/providers/rhelai_vllm.py` +- `ols/src/llms/providers/google_vertex.py` (both Gemini and Anthropic) +- `ols/src/llms/providers/bedrock.py` + +### Tests (9 new tests) + +- `tests/unit/utils/test_checks.py` — 4 tests for `read_secret_from_path()` +- `tests/unit/app/models/test_config.py` — 4 tests for `get_credentials()` +- `tests/unit/llms/providers/test_openai.py` — 1 end-to-end rotation test + +## Testing on a Cluster + +### Local with Podman + +```bash +# Build image with changes +OLS_API_IMAGE=localhost/ols:hot-reload make images + +# Prepare config +mkdir -p /tmp/ols-test/config +cp examples/olsconfig.yaml /tmp/ols-test/config/olsconfig.yaml +echo "your-api-key" > /tmp/ols-test/config/apikey.txt + +# Run +podman run -it --rm \ + -v /tmp/ols-test/config:/app-root/config:Z \ + -e OLS_CONFIG_FILE=/app-root/config/olsconfig.yaml \ + -p 8080:8080 localhost/ols:hot-reload + +# Query, then rotate, then query again +curl -X POST http://localhost:8080/v1/query \ + -H 'Content-Type: application/json' \ + -d '{"query": "what is a pod?"}' + +echo "new-rotated-key" > /tmp/ols-test/config/apikey.txt + +curl -X POST http://localhost:8080/v1/query \ + -H 'Content-Type: application/json' \ + -d '{"query": "what is a deployment?"}' +``` + +### On OpenShift + +```bash +# Build and push +OLS_API_IMAGE=quay.io//lightspeed-service-api:hot-reload make images +podman push quay.io//lightspeed-service-api:hot-reload + +# Patch existing deployment +oc set image deployment/lightspeed-app-server \ + lightspeed-app-server=quay.io//lightspeed-service-api:hot-reload \ + -n openshift-lightspeed + +# Verify no restarts after secret rotation +oc get pods -n openshift-lightspeed -w +``` + +### Validation checklist + +| Check | How to verify | +|-------|---------------| +| No pod restarts | `oc get pods` — restartCount stays 0 | +| Credential picked up | Queries succeed after rotation | +| No capacity loss | Service responds during rotation window | +| No restart logs | No container start messages in logs | + +## Multi-Repo Contribution + +This feature spans two repositories: + +| PR | Repo | Purpose | Status | +|----|------|---------|--------| +| Service PR (first) | `openshift/lightspeed-service` | Add `get_credentials()` hot-reload | Ready | +| Operator PR (second) | `openshift/lightspeed-operator` | Skip restart on credential-only secret changes | Future | + +The service PR is **backward compatible** — if the operator still restarts pods, +nothing breaks. The operator PR depends on the service change being merged first. + +Both PRs reference RFE-9380 and cross-link each other. diff --git a/ols/app/models/config.py b/ols/app/models/config.py index bc4a80a64..464bbb82c 100644 --- a/ols/app/models/config.py +++ b/ols/app/models/config.py @@ -395,6 +395,8 @@ class ProviderConfig(BaseModel): certificates_store: Optional[str] = None tls_security_profile: Optional[TLSSecurityProfile] = None + _credentials_path: Optional[str] = PrivateAttr(default=None) + def __init__( self, data: Optional[dict] = None, @@ -409,6 +411,7 @@ def __init__( self.set_provider_type(data) self.url = data.get("url", None) + self._credentials_path = data.get(constants.CREDENTIALS_PATH_SELECTOR) try: self.credentials = checks.read_secret( data, constants.CREDENTIALS_PATH_SELECTOR, constants.API_TOKEN_FILENAME @@ -627,6 +630,24 @@ def validate_yaml(self) -> None: "provider URL is invalid, only http:// and https:// URLs are supported" ) + def get_credentials(self) -> Optional[str]: + """Return current credentials, re-reading from disk when a path is configured. + + When ``credentials_path`` was provided in the original configuration, + the credential file is re-read on every call so that secrets rotated + by Kubernetes (kubelet atomic symlink swap) or external CronJobs are + picked up without a pod restart. Falls back to the cached value when + no path is available or the file cannot be read. + """ + if self._credentials_path is not None: + fresh = checks.read_secret_from_path( + self._credentials_path, constants.API_TOKEN_FILENAME + ) + if fresh is not None: + self.credentials = fresh + return fresh + return self.credentials + class LLMProviders(BaseModel): """LLM providers configuration.""" diff --git a/ols/src/llms/providers/azure_openai.py b/ols/src/llms/providers/azure_openai.py index 9fac95958..10e1da784 100644 --- a/ols/src/llms/providers/azure_openai.py +++ b/ols/src/llms/providers/azure_openai.py @@ -52,7 +52,7 @@ class AzureOpenAI(LLMProvider): def default_params(self) -> dict[str, Any]: """Construct and return structure with default LLM params.""" self.url = str(self.provider_config.url or self.url) - self.credentials = self.provider_config.credentials + self.credentials = self.provider_config.get_credentials() api_version = self.provider_config.api_version deployment_name = self.provider_config.deployment_name azure_config = self.provider_config.azure_config diff --git a/ols/src/llms/providers/bedrock.py b/ols/src/llms/providers/bedrock.py index 1430dfad9..0eca8ffd0 100644 --- a/ols/src/llms/providers/bedrock.py +++ b/ols/src/llms/providers/bedrock.py @@ -34,7 +34,7 @@ def default_params(self) -> dict[str, Any]: """Construct and return default LLM params.""" if self.provider_config.url is not None: self.url = str(self.provider_config.url).rstrip("/") - self.credentials = self.provider_config.credentials + self.credentials = self.provider_config.get_credentials() if not self.url: raise LLMConfigurationError( diff --git a/ols/src/llms/providers/google_vertex.py b/ols/src/llms/providers/google_vertex.py index 419bac47a..96f8427ef 100644 --- a/ols/src/llms/providers/google_vertex.py +++ b/ols/src/llms/providers/google_vertex.py @@ -11,6 +11,7 @@ from ols.src.llms.providers.provider import LLMProvider from ols.src.llms.providers.registry import register_llm_provider_as from ols.src.llms.providers.utils import load_vertex_credentials +from ols.utils.checks import InvalidConfigurationError if TYPE_CHECKING: from google.auth.credentials import Credentials as GoogleCredentials @@ -32,9 +33,12 @@ class GoogleVertex(LLMProvider): def default_params(self) -> dict[str, Any]: """Construct and return structure with default LLM params.""" self.project = self.provider_config.project_id - if self.provider_config.credentials is None: - raise ValueError("credentials are required for Google Vertex provider") - self.credentials = load_vertex_credentials(self.provider_config.credentials) + creds_value = self.provider_config.get_credentials() + if creds_value is None: + raise InvalidConfigurationError( + "credentials are required for Google Vertex provider" + ) + self.credentials = load_vertex_credentials(creds_value) if self.provider_config.google_vertex_config is not None: vertex_config = self.provider_config.google_vertex_config self.project = vertex_config.project @@ -70,11 +74,12 @@ class GoogleVertexAnthropic(LLMProvider): def default_params(self) -> dict[str, Any]: """Construct and return structure with default LLM params.""" self.project = self.provider_config.project_id - if self.provider_config.credentials is None: - raise ValueError( + creds_value = self.provider_config.get_credentials() + if creds_value is None: + raise InvalidConfigurationError( "credentials are required for Google Vertex Anthropic provider" ) - self.credentials = load_vertex_credentials(self.provider_config.credentials) + self.credentials = load_vertex_credentials(creds_value) if self.provider_config.google_vertex_anthropic_config is not None: vertex_config = self.provider_config.google_vertex_anthropic_config self.project = vertex_config.project diff --git a/ols/src/llms/providers/openai.py b/ols/src/llms/providers/openai.py index 4948c5ae9..9784ba2ed 100644 --- a/ols/src/llms/providers/openai.py +++ b/ols/src/llms/providers/openai.py @@ -25,7 +25,7 @@ class OpenAI(LLMProvider): def default_params(self) -> dict[str, Any]: """Construct and return structure with default LLM params.""" self.url = str(self.provider_config.url or self.url) - self.credentials = self.provider_config.credentials + self.credentials = self.provider_config.get_credentials() # provider-specific configuration has precendence over regular configuration if self.provider_config.openai_config is not None: openai_config = self.provider_config.openai_config diff --git a/ols/src/llms/providers/rhelai_vllm.py b/ols/src/llms/providers/rhelai_vllm.py index 319d61547..bf5ea22a4 100644 --- a/ols/src/llms/providers/rhelai_vllm.py +++ b/ols/src/llms/providers/rhelai_vllm.py @@ -25,7 +25,7 @@ class RHELAIVLLM(LLMProvider): def default_params(self) -> dict[str, Any]: """Construct and return structure with default LLM params.""" self.url = str(self.provider_config.url or self.url) - self.credentials = self.provider_config.credentials + self.credentials = self.provider_config.get_credentials() # provider-specific configuration has precendence over regular configuration if self.provider_config.rhelai_vllm_config is not None: rhelai_vllm_config = self.provider_config.rhelai_vllm_config diff --git a/ols/src/llms/providers/rhoai_vllm.py b/ols/src/llms/providers/rhoai_vllm.py index 3ef0987ae..e6d103d67 100644 --- a/ols/src/llms/providers/rhoai_vllm.py +++ b/ols/src/llms/providers/rhoai_vllm.py @@ -25,7 +25,7 @@ class RHOAIVLLM(LLMProvider): def default_params(self) -> dict[str, Any]: """Construct and return structure with default LLM params.""" self.url = str(self.provider_config.url or self.url) - self.credentials = self.provider_config.credentials + self.credentials = self.provider_config.get_credentials() # provider-specific configuration has precendence over regular configuration if self.provider_config.rhoai_vllm_config is not None: rhoai_vllm_config = self.provider_config.rhoai_vllm_config diff --git a/ols/src/llms/providers/watsonx.py b/ols/src/llms/providers/watsonx.py index 2d5b037bc..bcbc19876 100644 --- a/ols/src/llms/providers/watsonx.py +++ b/ols/src/llms/providers/watsonx.py @@ -42,7 +42,7 @@ def default_params(self) -> dict[str, Any]: def load(self) -> BaseChatModel: """Load LLM.""" self.url = str(self.provider_config.url or self.url) - self.credentials = self.provider_config.credentials + self.credentials = self.provider_config.get_credentials() self.project_id = self.provider_config.project_id # provider-specific configuration has precendence over regular configuration diff --git a/ols/utils/checks.py b/ols/utils/checks.py index 78c025a93..e4e1dadf8 100644 --- a/ols/utils/checks.py +++ b/ols/utils/checks.py @@ -68,6 +68,38 @@ def read_secret( return None +def read_secret_from_path( + path: str, + default_filename: str, +) -> Optional[str]: + """Re-read a secret from a file path, resolving directories. + + Unlike ``read_secret`` this function takes a concrete path string + (not a dict lookup) and is designed to be called on every LLM + request so that credential files rotated by Kubernetes (atomic + symlink swap) are picked up without a pod restart. + + Args: + path: File path or directory containing the secret. + default_filename: Filename to append when *path* is a directory. + + Returns: + The secret value with trailing whitespace stripped, or ``None`` + when the file cannot be read. + """ + filename = path + if os.path.isdir(path): + filename = os.path.join(path, default_filename) + + try: + with open(filename, encoding="utf-8") as f: + return f.read().rstrip() + except OSError: + logger = logging.getLogger(__name__) + logger.warning("Failed to re-read credential from %s", filename) + return None + + def dir_check(path: FilePath, desc: str) -> None: """Check that path is a readable directory.""" if not os.path.exists(path): diff --git a/tests/unit/app/models/test_config.py b/tests/unit/app/models/test_config.py index 46416290f..6d69c37ce 100644 --- a/tests/unit/app/models/test_config.py +++ b/tests/unit/app/models/test_config.py @@ -4,6 +4,7 @@ import errno import logging import os +from pathlib import Path from unittest import mock import pytest @@ -4352,3 +4353,96 @@ def test_skills_config_validation(): SkillsConfig(alpha=-0.1) with pytest.raises(ValidationError, match="less than or equal to 1"): SkillsConfig(alpha=1.1) + + +def test_provider_config_get_credentials_returns_cached_when_no_path() -> None: + """Test get_credentials() returns cached value when no path is configured.""" + provider_config = ProviderConfig() + provider_config.credentials = "cached-key" + assert provider_config.get_credentials() == "cached-key" + + +def test_provider_config_get_credentials_rereads_from_disk(tmp_path: Path) -> None: + """Test get_credentials() re-reads credential file on every call.""" + secret_file = tmp_path / "apitoken" + secret_file.write_text("original-key") + + provider_config = ProviderConfig( + { + "name": "test_provider", + "type": "openai", + "url": "http://test.url", + "credentials_path": str(secret_file), + "models": [ + { + "name": "test_model", + "url": "http://test.url/", + "credentials_path": str(secret_file), + } + ], + } + ) + + assert provider_config.get_credentials() == "original-key" + + secret_file.write_text("rotated-key") + assert provider_config.get_credentials() == "rotated-key" + + +def test_provider_config_get_credentials_falls_back_on_read_failure( + tmp_path: Path, +) -> None: + """Test get_credentials() falls back to cached value when file disappears.""" + secret_file = tmp_path / "apitoken" + secret_file.write_text("original-key") + + provider_config = ProviderConfig( + { + "name": "test_provider", + "type": "openai", + "url": "http://test.url", + "credentials_path": str(secret_file), + "models": [ + { + "name": "test_model", + "url": "http://test.url/", + "credentials_path": str(secret_file), + } + ], + } + ) + + assert provider_config.get_credentials() == "original-key" + + secret_file.write_text("rotated-key") + assert provider_config.get_credentials() == "rotated-key" + + secret_file.unlink() + assert provider_config.get_credentials() == "rotated-key" + + +def test_provider_config_get_credentials_with_directory(tmp_path: Path) -> None: + """Test get_credentials() when credentials_path is a directory.""" + secret_file = tmp_path / "apitoken" + secret_file.write_text("dir-key") + + provider_config = ProviderConfig( + { + "name": "test_provider", + "type": "openai", + "url": "http://test.url", + "credentials_path": str(tmp_path), + "models": [ + { + "name": "test_model", + "url": "http://test.url/", + "credentials_path": str(secret_file), + } + ], + } + ) + + assert provider_config.get_credentials() == "dir-key" + + secret_file.write_text("new-dir-key") + assert provider_config.get_credentials() == "new-dir-key" diff --git a/tests/unit/llms/providers/test_bedrock.py b/tests/unit/llms/providers/test_bedrock.py index 6b33b6b62..6d108c40b 100644 --- a/tests/unit/llms/providers/test_bedrock.py +++ b/tests/unit/llms/providers/test_bedrock.py @@ -492,3 +492,31 @@ def test_build_sigv4_auth_with_role( RoleArn="arn:aws:iam::123456789012:role/TestRole", RoleSessionName="ols-bedrock", ) + + +def test_bedrock_picks_up_rotated_credentials(tmp_path: Path) -> None: + """Test that Bedrock provider re-reads credentials on each default_params access.""" + secret_file = tmp_path / "apitoken" + secret_file.write_text("initial-key") + + config = ProviderConfig( + { + "name": "test_provider", + "type": "bedrock", + "url": "https://bedrock-mantle.us-east-1.api.aws", + "credentials_path": str(secret_file), + "models": [{"name": "anthropic.claude-opus-4-7"}], + } + ) + + bedrock_1 = Bedrock( + model="anthropic.claude-opus-4-7", params={}, provider_config=config + ) + assert bedrock_1.default_params["api_key"] == "initial-key" + + secret_file.write_text("rotated-key") + + bedrock_2 = Bedrock( + model="anthropic.claude-opus-4-7", params={}, provider_config=config + ) + assert bedrock_2.default_params["api_key"] == "rotated-key" diff --git a/tests/unit/llms/providers/test_openai.py b/tests/unit/llms/providers/test_openai.py index 88dbdd4df..ca3109766 100644 --- a/tests/unit/llms/providers/test_openai.py +++ b/tests/unit/llms/providers/test_openai.py @@ -1,6 +1,7 @@ """Unit tests for OpenAI provider.""" import os +from pathlib import Path import httpx import pytest @@ -270,3 +271,35 @@ def test_gpt5_and_o_series_models_parameter_exclusion( assert "model" in openai.default_params assert "max_completion_tokens" in openai.default_params assert openai.default_params["model"] == model_name + + +def test_openai_picks_up_rotated_credentials( + tmp_path: Path, fake_certifi_store: str +) -> None: + """Test that OpenAI provider re-reads credentials on each load.""" + secret_file = tmp_path / "apitoken" + secret_file.write_text("initial-key") + + config = ProviderConfig( + { + "name": "test_provider", + "type": "openai", + "url": "http://test.url", + "credentials_path": str(secret_file), + "models": [ + { + "name": "test_model", + "url": "http://test.url/", + "credentials_path": str(secret_file), + } + ], + } + ) + + openai_1 = OpenAI(model="test_model", provider_config=config) + assert openai_1.default_params["openai_api_key"] == "initial-key" + + secret_file.write_text("rotated-key") + + openai_2 = OpenAI(model="test_model", provider_config=config) + assert openai_2.default_params["openai_api_key"] == "rotated-key" diff --git a/tests/unit/utils/test_checks.py b/tests/unit/utils/test_checks.py index ee7d710e2..685069ace 100644 --- a/tests/unit/utils/test_checks.py +++ b/tests/unit/utils/test_checks.py @@ -3,7 +3,7 @@ from pathlib import Path from ols.constants import NOOP_WITH_TOKEN_AUTHENTICATION_MODULE -from ols.utils.checks import resolve_headers +from ols.utils.checks import read_secret_from_path, resolve_headers def test_resolve_headers_empty() -> None: @@ -152,3 +152,38 @@ def test_resolve_headers_kubernetes_with_no_auth_module(caplog) -> None: assert result == {} assert "kubernetes" in caplog.text.lower() assert "skipped" in caplog.text.lower() + + +def test_read_secret_from_path_file(tmp_path: Path) -> None: + """Test reading a secret from a file path.""" + secret_file = tmp_path / "apitoken" + secret_file.write_text("my-api-key\n") + + result = read_secret_from_path(str(secret_file), "apitoken") + assert result == "my-api-key" + + +def test_read_secret_from_path_directory(tmp_path: Path) -> None: + """Test reading a secret when path is a directory.""" + secret_file = tmp_path / "apitoken" + secret_file.write_text("dir-api-key") + + result = read_secret_from_path(str(tmp_path), "apitoken") + assert result == "dir-api-key" + + +def test_read_secret_from_path_nonexistent() -> None: + """Test reading from a nonexistent path returns None.""" + result = read_secret_from_path("/nonexistent/path", "apitoken") + assert result is None + + +def test_read_secret_from_path_picks_up_rotation(tmp_path: Path) -> None: + """Test that re-reading picks up rotated credential values.""" + secret_file = tmp_path / "apitoken" + secret_file.write_text("original-key") + + assert read_secret_from_path(str(secret_file), "apitoken") == "original-key" + + secret_file.write_text("rotated-key") + assert read_secret_from_path(str(secret_file), "apitoken") == "rotated-key"