fix: honor ModelConfig.spec.tls in the embedding path#1994
Open
davidkarlsen wants to merge 2 commits into
Open
fix: honor ModelConfig.spec.tls in the embedding path#1994davidkarlsen wants to merge 2 commits into
davidkarlsen wants to merge 2 commits into
Conversation
The chat/LLM path threads spec.tls (disableVerify / caCertSecretRef / disableSystemCAs) into a custom httpx client, but the embedding path ignored TLS config entirely: EmbeddingConfig carried no TLS fields and _embed_openai built the OpenAI client with no http_client override. As a result, embeddings (session-memory auto-save) fail behind a TLS-inspecting proxy even when the embedding ModelConfig sets spec.tls.disableVerify: true. This is a full-chain fix: the TLS settings only reach the Python agent via the Go-serialized config, so a Python-only change would be a no-op. Go: - Add TLSInsecureSkipVerify / TLSCACertPath / TLSDisableSystemCAs to EmbeddingConfig (+ UnmarshalJSON parsing). - ModelToEmbeddingConfig copies the TLS pointers from each model variant's BaseModel. (translateEmbeddingConfig already runs deriveTLSFields via translateModel, so the source model carries them.) Python: - Add tls_disable_verify / tls_ca_cert_path / tls_disable_system_cas to EmbeddingConfig (accepting the Go wire name tls_insecure_skip_verify via AliasChoices). - _embed_openai builds http_client=httpx.AsyncClient(verify=create_ssl_context(...)) for both AsyncOpenAI and AsyncAzureOpenAI, only when a TLS field is set so the default path is unchanged. Adds Go and Python unit tests covering propagation and client wiring. Fixes kagent-dev#1992 Signed-off-by: David J. M. Karlsen <david@davidkarlsen.com>
davidkarlsen
requested review from
EItanya,
iplay88keys,
jmhbh,
peterj,
supreme-gg-gg and
yuval-k
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds end-to-end TLS/SSL configuration support for embedding requests so the Python embedding client honors the same TLS settings as the chat/LLM path (incl. Go → Python config propagation).
Changes:
- Add TLS fields to EmbeddingConfig in both Go and Python (with Python accepting
tls_insecure_skip_verifyvia alias). - Thread a TLS-aware
httpx.AsyncClient(verify=...)into OpenAI / Azure OpenAI embedding SDK clients. - Add Go and Python unit tests validating TLS field propagation and client construction behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| python/packages/kagent-adk/tests/unittests/models/test_embedding_tls.py | Adds unit tests asserting TLS config results in the expected httpx/OpenAI client wiring. |
| python/packages/kagent-adk/src/kagent/adk/types.py | Extends EmbeddingConfig with TLS fields and an alias for tls_insecure_skip_verify. |
| python/packages/kagent-adk/src/kagent/adk/models/_embedding.py | Builds and injects a TLS-configured httpx.AsyncClient into OpenAI/Azure OpenAI embedding clients. |
| go/api/adk/types.go | Extends Go EmbeddingConfig w/ TLS fields; propagates TLS from BaseModel in ModelToEmbeddingConfig. |
| go/api/adk/types_test.go | Adds tests for TLS JSON unmarshalling + TLS propagation into EmbeddingConfig. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
170
to
+192
| @@ -153,11 +179,17 @@ async def _embed_openai(self, texts: List[str]) -> List[List[float]]: | |||
| api_base = self.config.base_url or os.environ.get("AZURE_OPENAI_ENDPOINT") | |||
| if not api_base: | |||
| raise ValueError("Azure OpenAI endpoint must be set via base_url or AZURE_OPENAI_ENDPOINT env var") | |||
| client = AsyncAzureOpenAI(api_version=api_version, azure_endpoint=api_base) | |||
| azure_kwargs: dict[str, Any] = {"api_version": api_version, "azure_endpoint": api_base} | |||
| if http_client is not None: | |||
| azure_kwargs["http_client"] = http_client | |||
| client = AsyncAzureOpenAI(**azure_kwargs) | |||
| else: | |||
| from openai import AsyncOpenAI | |||
|
|
|||
| client = AsyncOpenAI(base_url=self.config.base_url or None) | |||
| openai_kwargs: dict[str, Any] = {"base_url": self.config.base_url or None} | |||
| if http_client is not None: | |||
| openai_kwargs["http_client"] = http_client | |||
| client = AsyncOpenAI(**openai_kwargs) | |||
Comment on lines
+125
to
+143
| with mock.patch("kagent.adk.models._embedding.create_ssl_context") as mock_create_ssl: | ||
| with mock.patch("kagent.adk.models._embedding.httpx.AsyncClient") as mock_httpx: | ||
| with mock.patch("openai.AsyncAzureOpenAI") as mock_azure: | ||
| mock_create_ssl.return_value = False | ||
| mock_client = mock.MagicMock() | ||
| mock_httpx.return_value = mock_client | ||
| mock_azure.return_value.embeddings.create = mock.AsyncMock( | ||
| return_value=mock.MagicMock(data=[]) | ||
| ) | ||
|
|
||
| await embedding._embed_openai(["hello"]) | ||
|
|
||
| mock_create_ssl.assert_called_once_with( | ||
| disable_verify=True, | ||
| ca_cert_path=None, | ||
| disable_system_cas=False, | ||
| ) | ||
| azure_kwargs = mock_azure.call_args[1] | ||
| assert azure_kwargs["http_client"] is mock_client |
Comment on lines
+422
to
+426
| copyTLS := func(b BaseModel) { | ||
| e.TLSInsecureSkipVerify = b.TLSInsecureSkipVerify | ||
| e.TLSCACertPath = b.TLSCACertPath | ||
| e.TLSDisableSystemCAs = b.TLSDisableSystemCAs | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The chat/LLM path threads
ModelConfig.spec.tls(disableVerify/caCertSecretRef/disableSystemCAs) into a custom httpx client, but the embedding path ignores TLS config entirely:EmbeddingConfig(both Go and Python) carried no TLS fields._embed_openaiconstructed the OpenAI/AzureOpenAI client with nohttp_clientoverride.So embeddings (used for session-memory auto-save) fail behind a TLS-inspecting corporate proxy even when the embedding
ModelConfigsetsspec.tls.disableVerify: true— the setting has nowhere to go. Observed as:Fixes #1992.Why a full-chain (Go + Python) fix
The TLS settings reach the Python agent only via the Go-serialized agent config. A Python-only change would be a no-op because the Go controller never populates the fields. Confirmed by tracing
translateEmbeddingConfig->ModelToEmbeddingConfig.Changes
Go
TLSInsecureSkipVerify/TLSCACertPath/TLSDisableSystemCAstoEmbeddingConfig(+UnmarshalJSON).ModelToEmbeddingConfigcopies the TLS pointers from each model variant'sBaseModel.translateEmbeddingConfigalready runsderiveTLSFieldsviatranslateModel, so the source model already carries them.Python
tls_disable_verify/tls_ca_cert_path/tls_disable_system_castoEmbeddingConfig, accepting the Go wire nametls_insecure_skip_verifyviaAliasChoices._embed_openaibuildshttp_client=httpx.AsyncClient(verify=create_ssl_context(...))for bothAsyncOpenAIandAsyncAzureOpenAI— reusing the existingcreate_ssl_contexthelper. The custom client is built only when a TLS field is set, so the default path is unchanged.Tests
ModelToEmbeddingConfigpropagates TLS across all model variants;UnmarshalJSONparses the TLS fields; no-TLS leaves them nil._embed_openaiwirescreate_ssl_context+http_clientfor disable-verify / custom-CA / Azure; no-TLS preserves the default client.go test ./api/adk/... ./core/internal/controller/translator/agent/...-> ok.pytest .../test_embedding_tls.py-> 4 passed. (Pre-existingtest_tls_e2e.pyfailures are unrelated — they need a cert fixture and fail identically onmain.)