LCORE-754: Add TLS profiles #869
Conversation
WalkthroughAdds TLS security profile support for outgoing Llama Stack connections: new TLSSecurityProfile config, TLS utility helpers and constants, TLS-aware httpx.AsyncClient construction in the client loader, documentation, and accompanying unit/integration tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Config as Configuration
participant Holder as AsyncLlamaStackClientHolder
participant SSL as ssl.SSLContext
participant HTTPX as httpx.AsyncClient
participant LS as AsyncLlamaStackClient
Config->>Holder: load(llama_stack_config)
Holder->>Holder: _construct_httpx_client(tls_security_profile)
alt TLS profile present
Holder->>SSL: create_default_context(cafile=caCertPath?)
Holder->>SSL: set_minimum_version(minTLSVersion)
Holder->>SSL: set_ciphers(cipher_string)
SSL-->>Holder: configured SSLContext
Holder->>HTTPX: AsyncClient(verify=SSLContext, ...)
HTTPX-->>Holder: http_client
Holder->>LS: __init__(http_client=http_client)
else no TLS profile
Holder-->>LS: __init__(http_client=None)
LS->>HTTPX: use default AsyncClient internally
end
LS-->>Holder: initialized client
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/tls-security-profile.md (1)
41-46: Consider adding a security warning for legacy TLS versions.The documentation mentions
OldTypeprofile supports TLS 1.0, which is deprecated and has known vulnerabilities. Consider adding a note warning users thatOldTypeshould only be used for legacy system compatibility and carries security risks.tests/unit/test_client.py (2)
3-4: Consider using pytest-mock instead of unittest.mock.The coding guidelines specify using
pytest-mockwithAsyncMockobjects for mocking in tests. The current implementation usesunittest.mock.patchandMagicMock. Consider refactoring to usemockerfixture from pytest-mock for consistency.Example refactor for
test_ssl_context_minimum_version_set:-from unittest.mock import patch, MagicMock +from unittest.mock import MagicMockdef test_ssl_context_minimum_version_set(self, mocker) -> None: """Test that SSL context minimum version is set correctly.""" mock_context = MagicMock() mocker.patch("client.ssl.create_default_context", return_value=mock_context) holder = AsyncLlamaStackClientHolder() profile = TLSSecurityProfile( profile_type="ModernType", min_tls_version="VersionTLS13", ) holder._construct_httpx_client(profile) assert mock_context.minimum_version == ssl.TLSVersion.TLSv1_3Based on coding guidelines.
79-145: Consider adding tests for additional TLS code paths.The test class covers main scenarios but is missing coverage for:
skip_tls_verification=Truepath (returns client withverify=False)ca_cert_pathloading viacontext.load_verify_locations()- SSL error handling when
set_ciphers()fails
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
docs/tls-security-profile.md(1 hunks)src/client.py(3 hunks)src/constants.py(1 hunks)src/models/config.py(3 hunks)src/utils/tls.py(1 hunks)tests/configuration/lightspeed-stack-tls.yaml(1 hunks)tests/integration/test_tls_configuration.py(1 hunks)tests/unit/models/config/test_dump_configuration.py(3 hunks)tests/unit/models/config/test_tls_security_profile.py(1 hunks)tests/unit/test_client.py(2 hunks)tests/unit/utils/test_tls.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/constants.pysrc/models/config.pysrc/utils/tls.pysrc/client.py
src/**/constants.py
📄 CodeRabbit inference engine (CLAUDE.md)
Define shared constants in central
constants.pyfile with descriptive comments
Files:
src/constants.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests; do not use unittest framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/unit/utils/test_tls.pytests/unit/models/config/test_tls_security_profile.pytests/unit/models/config/test_dump_configuration.pytests/unit/test_client.pytests/integration/test_tls_configuration.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/unit/utils/test_tls.pytests/unit/models/config/test_tls_security_profile.pytests/unit/models/config/test_dump_configuration.pytests/unit/test_client.pytests/integration/test_tls_configuration.py
src/models/config.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/config.py: All configuration must use Pydantic models extendingConfigurationBasewithextra="forbid"to reject unknown fields
Use type hintsOptional[FilePath],PositiveInt,SecretStrfor Pydantic configuration models
Files:
src/models/config.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Use@field_validatorand@model_validatorfor custom validation in Pydantic models
Pydantic configuration classes must extendConfigurationBase; data models must extendBaseModel
Files:
src/models/config.py
src/**/{client,app/endpoints/**}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Handle
APIConnectionErrorfrom Llama Stack in integration code
Files:
src/client.py
🧠 Learnings (3)
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/models/config.py : All configuration must use Pydantic models extending `ConfigurationBase` with `extra="forbid"` to reject unknown fields
Applied to files:
src/models/config.py
📚 Learning: 2025-09-02T11:09:40.404Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
Learning: In the lightspeed-stack e2e tests, noop authentication tests use the default lightspeed-stack.yaml configuration, while noop-with-token tests use the Authorized tag to trigger a config swap to the specialized noop-with-token configuration file.
Applied to files:
tests/configuration/lightspeed-stack-tls.yaml
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to tests/**/*.py : Use `pytest-mock` with AsyncMock objects for mocking in tests
Applied to files:
tests/unit/test_client.py
🧬 Code graph analysis (6)
tests/unit/utils/test_tls.py (1)
src/utils/tls.py (7)
TLSProfiles(15-21)TLSProtocolVersion(24-34)ssl_tls_version(98-110)min_tls_version(113-125)ciphers_from_list(128-132)ciphers_for_tls_profile(135-138)ciphers_as_string(141-148)
src/models/config.py (1)
src/utils/tls.py (3)
min_tls_version(113-125)TLSProfiles(15-21)TLSProtocolVersion(24-34)
tests/unit/models/config/test_tls_security_profile.py (2)
src/models/config.py (3)
config(366-372)TLSSecurityProfile(79-168)LlamaStackConfiguration(414-462)src/utils/tls.py (1)
min_tls_version(113-125)
src/client.py (2)
src/models/config.py (3)
config(366-372)LlamaStackConfiguration(414-462)TLSSecurityProfile(79-168)src/utils/tls.py (4)
TLSProfiles(15-21)ciphers_as_string(141-148)min_tls_version(113-125)ssl_tls_version(98-110)
tests/unit/test_client.py (3)
src/client.py (2)
_construct_httpx_client(25-93)load(95-126)src/models/config.py (3)
config(366-372)LlamaStackConfiguration(414-462)TLSSecurityProfile(79-168)src/utils/tls.py (1)
min_tls_version(113-125)
tests/integration/test_tls_configuration.py (4)
src/configuration.py (4)
configuration(73-77)AppConfig(39-181)load_configuration(56-62)llama_stack_configuration(87-91)src/client.py (2)
AsyncLlamaStackClientHolder(20-134)load(95-126)src/utils/tls.py (1)
min_tls_version(113-125)src/models/config.py (2)
config(366-372)LlamaStackConfiguration(414-462)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🔇 Additional comments (16)
tests/unit/models/config/test_dump_configuration.py (1)
138-139: LGTM!The serialization test correctly expects
tls_security_profile: Nonewhen no TLS profile is configured, consistent with the new field added toLlamaStackConfiguration.tests/configuration/lightspeed-stack-tls.yaml (1)
1-21: LGTM!The test configuration file correctly demonstrates a
ModernTypeTLS profile withVersionTLS13and valid TLS 1.3 cipher suites, providing good coverage for TLS integration testing.tests/unit/test_client.py (1)
148-166: LGTM with optional enhancement.The test correctly validates TLS profile integration with
LlamaStackConfiguration. Consider additionally verifying that the constructedhttp_clientwas passed to the underlyingAsyncLlamaStackClientby mocking the constructor and asserting thehttp_clientparameter.tests/unit/models/config/test_tls_security_profile.py (4)
1-6: LGTM!The test module is well-structured with clear imports and follows pytest conventions as required by the coding guidelines.
8-41: LGTM!The initialization tests comprehensively cover default values, single-field initialization, YAML alias handling, and full configuration scenarios.
44-106: LGTM!Thorough validation tests covering all profile types, TLS versions, and cipher validation behavior including the distinction between
Customprofiles (any ciphers allowed) and predefined profiles (validated against supported list).
121-164: LGTM!The integration tests properly verify that
TLSSecurityProfileis correctly nested withinLlamaStackConfigurationand that all fields are propagated correctly, including custom ciphers.src/client.py (3)
59-65: Verify: skip_tls_verification ignores other TLS settings.When
skip_tls_verification=True, the method returns early withverify=False, ignoringmin_tls_versionandcipherssettings. This is likely intentional for testing scenarios, but confirm this behavior is expected—users might expect other TLS settings to still apply even when verification is skipped.
112-126: LGTM!The TLS client integration is clean—the
http_clientis constructed only for remote mode and correctly passed toAsyncLlamaStackClient. The client construction happens lazily here without actual connections, so error handling would occur at call sites.
3-14: LGTM!Imports are well-organized and follow the absolute import pattern required by coding guidelines.
src/constants.py (1)
154-156: Verify constant format consistency with TLS utilities.The
DEFAULT_SSL_VERSIONuses format"TLSv1_2", but the TLS utilities insrc/utils/tls.pymay use different enum values like"VersionTLS12". Confirm whether these constants align with how they're consumed by the TLS utilities, or clarify if the string format is the intended interface.tests/unit/utils/test_tls.py (1)
1-225: LGTM! Comprehensive unit test coverage for TLS utilities.The test file is well-structured with clear test organization by class. Tests cover all TLS utility functions including enum validation, protocol version mappings, cipher handling, and edge cases like None values and empty lists. Type annotations and docstrings follow the project conventions.
src/models/config.py (2)
79-169: LGTM! TLSSecurityProfile configuration model is well-designed.The Pydantic model correctly extends ConfigurationBase with
extra="forbid", includes comprehensive docstrings, and uses appropriate validators. The validation logic properly checks profile types, TLS versions, and cipher compatibility. Field aliases enable both Python and YAML-friendly naming conventions.
421-425: LGTM! TLS security profile integration into LlamaStackConfiguration.The tls_security_profile field is properly typed as Optional and includes clear documentation. This enables TLS configuration for outgoing connections to Llama Stack.
tests/integration/test_tls_configuration.py (1)
18-54: LGTM! Configuration loading tests are well-structured.The tests properly validate that TLS configuration loads correctly from YAML and checks expected default values. The assertions verify profile type, minimum TLS version, ciphers, and default fields like ca_cert_path and skip_tls_verification.
src/utils/tls.py (1)
1-149: LGTM! Well-designed TLS utilities module.The module provides a clean abstraction for TLS configuration with:
- Clear enum definitions for profiles and protocol versions
- Comprehensive cipher suite mappings for each security profile (OldType, IntermediateType, ModernType)
- Helper functions with proper type annotations and None handling
- Appropriate logging for invalid TLS version inputs
- Reference to OpenShift TLS security profiles for context
The implementation correctly separates TLS 1.3 ciphers from earlier versions and provides sensible defaults for each profile type. All functions include complete docstrings and follow the project's coding conventions.
| """Integration tests for TLS security profile configuration.""" | ||
|
|
||
| import ssl | ||
| from unittest.mock import patch, MagicMock |
There was a problem hiding this comment.
Use pytest-mock instead of unittest.mock for test mocking.
The tests import and use unittest.mock (patch, MagicMock), but the coding guidelines require using pytest-mock with AsyncMock objects for mocking in tests. As per coding guidelines for tests/**/*.py: "Use pytest-mock with AsyncMock objects for mocking in tests".
Apply this pattern throughout the file:
-from unittest.mock import patch, MagicMock
+from pytest_mock import MockerFixture
+from unittest.mock import AsyncMockThen update the test functions to use the mocker fixture:
@pytest.mark.asyncio
async def test_client_construction_with_tls_profile(
tls_configuration_filename: str,
+ mocker: MockerFixture,
) -> None:
"""Test that AsyncLlamaStackClientHolder constructs client with TLS settings."""
cfg = AppConfig()
cfg.load_configuration(tls_configuration_filename)
holder = AsyncLlamaStackClientHolder()
- # Mock httpx.AsyncClient to capture the SSL context
- with patch("client.httpx.AsyncClient") as mock_async_client:
- with patch("client.AsyncLlamaStackClient") as mock_llama_client:
- mock_async_client.return_value = MagicMock()
- mock_llama_client.return_value = MagicMock()
+ mock_async_client = mocker.patch("client.httpx.AsyncClient")
+ mock_llama_client = mocker.patch("client.AsyncLlamaStackClient")
+ mock_async_client.return_value = AsyncMock()
+ mock_llama_client.return_value = AsyncMock()
- await holder.load(cfg.llama_stack_configuration)
+ await holder.load(cfg.llama_stack_configuration)
- # Verify httpx.AsyncClient was called with an SSL context
- mock_async_client.assert_called_once()
+ # Verify httpx.AsyncClient was called with an SSL context
+ mock_async_client.assert_called_once()Apply similar changes to test_client_construction_without_tls_profile.
Also applies to: 67-70, 105-107
🤖 Prompt for AI Agents
In tests/integration/test_tls_configuration.py around line 4 (and also affecting
blocks at lines 67-70 and 105-107), replace direct imports from unittest.mock
with usage of the pytest-mock fixture: remove "from unittest.mock import patch,
MagicMock" and stop calling patch/MagicMock directly; instead update tests that
patch or create mocks to accept the "mocker" fixture in their signature and use
mocker.patch(...) for patching and mocker.AsyncMock(...) for async callables (or
mocker.MagicMock(...) for synchronous mocks) so mocks follow the AsyncMock
guideline; update the two named tests (including
test_client_construction_without_tls_profile) to include the mocker parameter
and replace any patch(...) context managers or MagicMock instantiations
accordingly.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| "TLS verification is disabled. This is insecure and should " | ||
| "only be used for testing purposes." | ||
| ) | ||
| return httpx.AsyncClient(verify=False) |
Check failure
Code scanning / Bandit
Call to httpx with verify=False disabling SSL certificate checks, security issue. Error
| "TLS verification is disabled. This is insecure and should " | ||
| "only be used for testing purposes." | ||
| ) | ||
| return httpx.AsyncClient(verify=False) |
Check warning
Code scanning / Bandit
Call to httpx without timeout Warning
| ) | ||
|
|
||
| logger.info("Creating httpx.AsyncClient with TLS security profile") | ||
| return httpx.AsyncClient(verify=context) |
Check warning
Code scanning / Bandit
Call to httpx without timeout Warning
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/client.py (1)
59-66:verify=Falsebranch remains a real TLS bypass and keeps Bandit failingThe
skip_tls_verificationflag intentionally disables certificate verification and returnshttpx.AsyncClient(verify=False). Even though it’s documented and logged as “testing only”, this is still a full TLS bypass and is what Bandit/GitHub Advanced Security are flagging.If you want to keep this capability, I’d recommend at least one of:
- Guarding it behind a stronger condition (e.g., explicit “debug/testing” environment flag), or
- Adding a targeted Bandit suppression (
# nosec B501) with a brief justification so the intent is explicit.Functionally the code is correct; this is about tightening security guarantees and satisfying security scanning.
-->
🧹 Nitpick comments (10)
tests/unit/models/config/test_tls_security_profile.py (2)
132-165: Tighten typing aroundtls_security_profileto satisfy pylint and improve clarityPylint’s
no-memberwarnings onprofile_type,min_tls_version, andcipherscome from accessing attributes onconfig.tls_security_profilewithout a prior type check.A small refactor clarifies intent and should silence the warnings:
def test_llama_stack_config_with_tls_profile(self) -> None: @@ - config = LlamaStackConfiguration( + config = LlamaStackConfiguration( url="https://llama-stack:8321", use_as_library_client=False, tls_security_profile=tls_profile, ) - assert config.tls_security_profile is not None - assert config.tls_security_profile.profile_type == "ModernType" - assert config.tls_security_profile.min_tls_version == "VersionTLS13" + assert config.tls_security_profile is not None + assert isinstance(config.tls_security_profile, TLSSecurityProfile) + assert config.tls_security_profile.profile_type == "ModernType" + assert config.tls_security_profile.min_tls_version == "VersionTLS13" @@ - config = LlamaStackConfiguration( + config = LlamaStackConfiguration( url="https://llama-stack:8321", use_as_library_client=False, tls_security_profile=tls_profile, ) - assert config.tls_security_profile is not None - assert config.tls_security_profile.ciphers == ciphers + assert config.tls_security_profile is not None + assert isinstance(config.tls_security_profile, TLSSecurityProfile) + assert config.tls_security_profile.ciphers == ciphers-->
1-165: Fix formatting to satisfy Black and trailing-newline lintCI reports that Black would reformat this file and that there’s a trailing newline issue. Please run Black on the file (and ensure a single newline at EOF):
black tests/unit/models/config/test_tls_security_profile.pyThis should also normalize blank-line spacing.
-->tests/unit/test_client.py (3)
3-5: Prefer pytest-mock + AsyncMock overunittest.mockin testsPer the repo testing guidelines, mocks in
tests/**/*.pyshould go throughpytest-mockwithAsyncMockwhere applicable. Here you’re usingunittest.mock.patchandMagicMockdirectly.Consider refactoring to the
mockerfixture, e.g.:def test_ssl_context_minimum_version_set(mocker) -> None: mock_context = mocker.MagicMock() mock_create_context = mocker.patch("client.ssl.create_default_context", return_value=mock_context) ...This keeps mocking style consistent across the suite and makes async mocking easier elsewhere.
-->Also applies to: 116-146
79-146: Acceptable use of private helper in tests; optionally silence pylintThe tests in
TestConstructHttpxClientcall the private_construct_httpx_clienthelper, triggeringprotected-accesswarnings. Given there’s no public surface that exposes this behavior, testing the helper directly is reasonable.If you want a clean lint run, you can either:
- Add a module-level disable for
protected-accessin this test file, or- Add a
# pylint: disable=protected-accesscomment on specific lines.Functionally the tests look correct.
-->
1-166: Run Black to address formatting issues reported by CIBlack reports this file would be reformatted. Please run:
black tests/unit/test_client.pyto normalize spacing and imports and clear the Black check.
-->src/client.py (2)
1-126: Run Black on this module to satisfy CIBlack reports that this file would be reformatted. Please run:
black src/client.pyto normalize layout, including any spacing around imports and blank lines.
-->
92-93: Consider adding explicit timeouts tohttpx.AsyncClientfor clarityWhile httpx enforces a default 5-second timeout, explicitly specifying the
timeoutparameter improves code clarity and makes timeout expectations visible. This is particularly useful if your application requires different timeout values.For example:
- return httpx.AsyncClient(verify=False) + return httpx.AsyncClient(verify=False, timeout=10.0) @@ - return httpx.AsyncClient(verify=context) + return httpx.AsyncClient(verify=context, timeout=10.0)Alternatively, extract the timeout value to a module-level constant to avoid magic numbers.
tests/unit/utils/test_tls.py (1)
1-225: Fix Black/trailing-newline issuesCI indicates Black would reformat this file and there’s a trailing-newlines warning. Please run:
black tests/unit/utils/test_tls.pyto normalize formatting and ensure a single newline at EOF.
-->src/models/config.py (1)
124-129: Wrap longskip_tls_verificationdescription line to satisfy linterPylint reports a line-too-long warning here. You can split the description string across lines without changing semantics:
- skip_tls_verification: bool = Field( - False, - alias="skipTLSVerification", - title="Skip TLS verification", - description="Skip TLS certificate verification (for testing only, not recommended for production)", - ) + skip_tls_verification: bool = Field( + False, + alias="skipTLSVerification", + title="Skip TLS verification", + description=( + "Skip TLS certificate verification (for testing only, not recommended " + "for production)" + ), + )-->
src/utils/tls.py (1)
1-149: Run Black and fix trailing-newlines warningCI shows Black would reformat this file and there’s a trailing-newlines warning at the end. Please run:
black src/utils/tls.pyto normalize formatting and ensure exactly one newline at EOF.
-->
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
docs/tls-security-profile.md(1 hunks)src/client.py(3 hunks)src/constants.py(1 hunks)src/models/config.py(3 hunks)src/utils/tls.py(1 hunks)tests/configuration/lightspeed-stack-tls.yaml(1 hunks)tests/integration/test_tls_configuration.py(1 hunks)tests/unit/models/config/test_dump_configuration.py(3 hunks)tests/unit/models/config/test_tls_security_profile.py(1 hunks)tests/unit/test_client.py(2 hunks)tests/unit/utils/test_tls.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/tls-security-profile.md
🚧 Files skipped from review as they are similar to previous changes (4)
- src/constants.py
- tests/configuration/lightspeed-stack-tls.yaml
- tests/integration/test_tls_configuration.py
- tests/unit/models/config/test_dump_configuration.py
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/client.pysrc/utils/tls.pysrc/models/config.py
src/**/{client,app/endpoints/**}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Handle
APIConnectionErrorfrom Llama Stack in integration code
Files:
src/client.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests; do not use unittest framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/unit/models/config/test_tls_security_profile.pytests/unit/test_client.pytests/unit/utils/test_tls.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/unit/models/config/test_tls_security_profile.pytests/unit/test_client.pytests/unit/utils/test_tls.py
src/models/config.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/config.py: All configuration must use Pydantic models extendingConfigurationBasewithextra="forbid"to reject unknown fields
Use type hintsOptional[FilePath],PositiveInt,SecretStrfor Pydantic configuration models
Files:
src/models/config.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Use@field_validatorand@model_validatorfor custom validation in Pydantic models
Pydantic configuration classes must extendConfigurationBase; data models must extendBaseModel
Files:
src/models/config.py
🧠 Learnings (2)
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to tests/**/*.py : Use `pytest-mock` with AsyncMock objects for mocking in tests
Applied to files:
tests/unit/test_client.py
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/models/config.py : All configuration must use Pydantic models extending `ConfigurationBase` with `extra="forbid"` to reject unknown fields
Applied to files:
src/models/config.py
🧬 Code graph analysis (5)
src/client.py (2)
src/models/config.py (3)
config(371-377)LlamaStackConfiguration(487-578)TLSSecurityProfile(80-169)src/utils/tls.py (3)
TLSProfiles(15-21)min_tls_version(113-125)ssl_tls_version(98-110)
tests/unit/models/config/test_tls_security_profile.py (2)
src/models/config.py (3)
config(371-377)TLSSecurityProfile(80-169)LlamaStackConfiguration(487-578)src/utils/tls.py (1)
min_tls_version(113-125)
tests/unit/test_client.py (2)
src/models/config.py (3)
config(371-377)LlamaStackConfiguration(487-578)TLSSecurityProfile(80-169)src/utils/tls.py (1)
min_tls_version(113-125)
tests/unit/utils/test_tls.py (1)
src/utils/tls.py (7)
TLSProfiles(15-21)TLSProtocolVersion(24-34)ssl_tls_version(98-110)min_tls_version(113-125)ciphers_from_list(128-132)ciphers_for_tls_profile(135-138)ciphers_as_string(141-148)
src/models/config.py (1)
src/utils/tls.py (3)
min_tls_version(113-125)TLSProfiles(15-21)TLSProtocolVersion(24-34)
🪛 GitHub Actions: Black
src/client.py
[error] 1-1: Black formatting check would reformat this file. Run 'black --write' to fix formatting.
tests/unit/models/config/test_tls_security_profile.py
[error] 1-1: Black formatting check would reformat this file. Run 'black --write' to fix formatting.
tests/unit/test_client.py
[error] 1-1: Black formatting check would reformat this file. Run 'black --write' to fix formatting.
src/utils/tls.py
[error] 1-1: Black formatting check would reformat this file. Run 'black --write' to fix formatting.
tests/unit/utils/test_tls.py
[error] 1-1: Black formatting check would reformat this file. Run 'black --write' to fix formatting.
🪛 GitHub Actions: Python linter
tests/unit/models/config/test_tls_security_profile.py
[warning] 165-165: Trailing newlines (trailing-newlines)
[warning] 109-109: Too few public methods (too-few-public-methods)
[warning] 144-144: E1101: Instance of 'FieldInfo' has no 'profile_type' member (no-member)
[warning] 145-145: E1101: Instance of 'FieldInfo' has no 'min_tls_version' member (no-member)
[warning] 164-164: E1101: Instance of 'FieldInfo' has no 'ciphers' member (no-member)
tests/unit/test_client.py
[warning] 85-85: Access to a protected member _construct_httpx_client of a client class (protected-access)
[warning] 92-92: Access to a protected member _construct_httpx_client of a client class (protected-access)
[warning] 102-102: Access to a protected member _construct_httpx_client of a client class (protected-access)
[warning] 113-113: Access to a protected member _construct_httpx_client of a client class (protected-access)
[warning] 127-127: Access to a protected member _construct_httpx_client of a client class (protected-access)
[warning] 143-143: Access to a protected member _construct_httpx_client of a client class (protected-access)
src/utils/tls.py
[warning] 149-149: Trailing newlines (trailing-newlines)
tests/unit/utils/test_tls.py
[warning] 225-225: Trailing newlines (trailing-newlines)
src/models/config.py
[warning] 128-128: Line too long (107/100) (line-too-long)
🪛 GitHub Check: Bandit
src/client.py
[failure] 65-65:
Call to httpx with verify=False disabling SSL certificate checks, security issue.
[warning] 65-65:
Call to httpx without timeout
[warning] 93-93:
Call to httpx without timeout
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🔇 Additional comments (7)
src/client.py (1)
25-94: Overall TLS profile handling and SSLContext construction look correctThe use of
tls.TLSProfiles,tls.min_tls_version,tls.ssl_tls_version, andtls.ciphers_as_stringcorrectly maps the configuration model onto anssl.SSLContext. Handling of missing profile / profile_type by returningNoneand falling back to the default httpx client is clear and matches the new tests.Aside from the
verify=False/timeout points already noted, the structure and logging here look solid.
-->tests/unit/utils/test_tls.py (1)
20-225: TLS utilities test coverage looks solidThese tests exercise all the key enum values, mappings, and helper behaviors (including edge cases like invalid versions and empty cipher lists) and align with the implementation in
src/utils/tls.py. No functional issues from the test logic side.-->
src/models/config.py (2)
80-169: TLS security profile model and validation align with TLS utilities
TLSSecurityProfilecorrectly:
- Forbids extra fields via
ConfigDict(extra="forbid", populate_by_name=True).- Validates
profile_typeagainsttls.TLSProfiles.- Validates
min_tls_versionagainsttls.TLSProtocolVersion.- Ensures non‑Custom profiles only accept ciphers contained in
tls.TLS_CIPHERS[profile].This is consistent with the new TLS utilities and the added unit tests.
-->
527-531: LlamaStackConfiguration TLS profile wiring looks correctThe new
tls_security_profile: Optional[TLSSecurityProfile]field cleanly attaches TLS configuration toLlamaStackConfigurationand is used by the client to construct a TLS-aware httpx client. The existingcheck_llama_stack_modelvalidator remains focused on URL vs library-mode semantics, so no additional validation is needed here.-->
src/utils/tls.py (1)
15-148: TLS profiles, mappings, and helpers are well-structured and match the testsThe StrEnums,
MIN_TLS_VERSIONS,TLS_CIPHERS, and the helper functions (ssl_tls_version,min_tls_version,ciphers_from_list,ciphers_for_tls_profile,ciphers_as_string) are cohesive and behave as expected (including graceful fallback on invalid versions). This provides a clear, centralized TLS configuration surface for the rest of the code.-->
tests/unit/models/config/test_tls_security_profile.py (1)
112-118: Changepytest.raises(ValueError)topytest.raises(ValidationError)for extra field validationWith
extra="forbid"on aBaseModel, Pydantic raisesValidationErrorwhen an unknown field is provided. Update the test expectation:+from pydantic import ValidationError + - def test_extra_fields_forbidden(self) -> None: - """Test that extra fields are rejected.""" - with pytest.raises(ValueError): + def test_extra_fields_forbidden(self) -> None: + """Test that extra fields are rejected.""" + with pytest.raises(ValidationError): TLSSecurityProfile( profile_type="ModernType", unknown_field="value", )tests/unit/test_client.py (1)
148-166: Verify test isolation and mocking setup
test_get_async_llama_stack_remote_client_with_tlsappears to instantiate a real client with a configuration file path. Confirm thatAsyncLlamaStackClientHolderand its dependencies are properly mocked in this unit test, or consider moving this to the integration test suite if it requires an actual Llama Stack service. Ensure all tests in this file follow pytest framework (not unittest) and use pytest-mock with AsyncMock for external dependencies per project guidelines.
1 participant
Description
This PR implements configurable TLS security profiles for outgoing connections to the Llama Stack provider, allowing users to enforce specific cipher lists and minimum TLS versions.
Changes
New Files
src/utils/tls.pytests/unit/utils/test_tls.pytests/unit/models/config/test_tls_security_profile.pytests/integration/test_tls_configuration.pytests/configuration/lightspeed-stack-tls.yamldocs/tls-security-profile.mdModified Files
src/models/config.pyTLSSecurityProfileclass andtls_security_profilefield toLlamaStackConfigurationsrc/client.py_construct_httpx_client()method to create httpx client with TLS configurationsrc/constants.pytests/unit/test_client.pytests/unit/models/config/test_dump_configuration.pytls_security_profilefieldType of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Step 1: Generate Self-Signed Certificates
Step 2: Configure Llama Stack with TLS
Create or update your Llama Stack configuration (
run.yaml):Step 3: Configure Lightspeed Stack
Create a configuration file (
lstack.yaml):Step 4: Start Services
Terminal 1 - Start Llama Stack:
Terminal 2 - Start Lightspeed Stack:
Step 5: Verify TLS Connection
Check the logs for TLS configuration being applied:
If the application starts successfully, TLS is working correctly.
Step 6: Test with a Query
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.