feat: forward x headers#30
Conversation
m-misiura
force-pushed
the
forward-headers
branch
2 times, most recently
from
94dc3d5 to
992ddc2
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| def test_no_headers_returns_none(): | ||
| api_request_headers_var.set(None) | ||
| assert get_extra_headers_from_request() is None |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| assert result == { | ||
| "Authorization": "Bearer llm-key", |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| }, | ||
| } | ||
| result = _redact_invocation_params(params) | ||
| assert result["extra_headers"]["Authorization"] == "Bearer [REDACTED]" |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #30 +/- ##
==========================================
Coverage ? 76.91%
==========================================
Files ? 200
Lines ? 20469
Branches ? 0
==========================================
Hits ? 15743
Misses ? 4726
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
m-misiura
force-pushed
the
forward-headers
branch
from
b27b536 to
d3dfc77
Compare
| if api_key: | ||
| kwargs["api_key"] = api_key | ||
| kwargs["openai_api_key"] = api_key | ||
| elif "api_key" not in kwargs: |
There was a problem hiding this comment.
I think we need to deal with the case where the header forwarding fails or no auth header is provided. IIUC, in these cases the LLM call will just be made with api_key="runtime-provided"
There was a problem hiding this comment.
Thanks. Previously If a model had no API key configured (no api_key_env_var, no api_key in params), LangChain would raise an error at init time and the server would error out.
In this PR, models without a static key get a runtime-provided placeholder so LangChain initialises successfully. Real auth should arrive via forwarded Authorization headers at call time. If no auth header is present, the provider would return 401
For models with a static key: forward_auth=False, so we only forward custom X-* headers (e.g. x-maas-subscription), not Authorization. Providers ignore unknown headers, so this should be harmless
Let me know what you think about this logic
| # Generate multiple responses with temperature 1. | ||
| # Bind the config parameters to the LLM for this call | ||
| llm_with_config = llm.bind(temperature=1.0, n=num_responses) | ||
| bind_kwargs = {"temperature": 1.0, "n": num_responses} |
There was a problem hiding this comment.
Will there be other actions that call .bind() ? If so, it would be nice to have a helper function rather than duplicating this header forwarding
There was a problem hiding this comment.
Sure, added a helper function
| if not request_headers: | ||
| return None | ||
|
|
||
| _INFRA_PREFIXES = ("x-forwarded", "x-real-", "x-request-id", "x-remote-") |
There was a problem hiding this comment.
This gets recreated every time we run get_extra_headers_from_request - minor nit but can we make this a global variable ?
m-misiura
force-pushed
the
forward-headers
branch
from
225c5d4 to
ec99315
Compare
m-misiura
force-pushed
the
forward-headers
branch
from
ec99315 to
673387f
Compare
|
|
||
| def test_forward_auth_false_skips_auth(): | ||
| _set_headers({"authorization": "Bearer key", "x-authorization": "Bearer key2"}) | ||
| assert get_extra_headers_from_request(forward_auth=False) is None |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| """Authorization header (K8s/proxy auth) must never reach the LLM.""" | ||
| _set_headers({"authorization": "Bearer k8s-token"}) | ||
| result = get_extra_headers_from_request(forward_auth=True) | ||
| assert result is None |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| def test_x_authorization_forwarded_without_authorization(): | ||
| _set_headers({"x-authorization": "Bearer llm-key"}) | ||
| result = get_extra_headers_from_request(forward_auth=True) | ||
| assert result == {"Authorization": "Bearer llm-key"} |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
m-misiura
force-pushed
the
forward-headers
branch
from
c6e72e8 to
639a9c3
Compare
m-misiura
merged commit 15e9757
into
trustyai-explainability:develop
5 participants
Description
Implemented:
X-*headers from incoming requests are automatically forwarded to outgoing LLM calls (excluding infrastructure headers from proxies)Image to test: quay.io/rh-ee-mmisiura/nemo-guardrails:forward-headers_v6
This PR is intended to address the following JIRA