enable ssl and CA in llama-stack #135
Conversation
WalkthroughUpdated the llama-stack submodule to a new commit. Added TLS parameters (ssl_mode and ca_cert_path) to PostgreSQL configurations within template.yaml for lightspeed-stack and llama-stack client ConfigMaps. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: maorfr The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
template.yaml (1)
317-318: TLS enabled: verify-full requires CA presence and hostname match; consider parameterizing and tightening the secret mountGood move adding TLS. A few gotchas to avoid surprises at runtime:
- verify-full requires the server cert’s CN/SAN to match the hostname. Ensure ASSISTED_CHAT_POSTGRES_HOST is the FQDN on the cert (not an IP).
- The CA file is mounted from a Secret, but the volume sets items.optional: true. If the key isn’t present, the pod will start but DB connections will fail due to verify-full + missing CA. Either make the item mandatory or gate ssl_mode/ca_cert_path behind params.
- For flexibility across envs, parameterize ssl_mode and ca_cert_path instead of hard-coding.
Suggested diffs within the changed lines (parameterize the values):
@@ - ssl_mode: "verify-full" - ca_cert_path: /etc/tls/ca-bundle.pem + ssl_mode: "${DB_SSL_MODE}" + ca_cert_path: "${DB_CA_CERT_PATH}" @@ - ssl_mode: "verify-full" - ca_cert_path: /etc/tls/ca-bundle.pem + ssl_mode: "${DB_SSL_MODE}" + ca_cert_path: "${DB_CA_CERT_PATH}" @@ - ssl_mode: "verify-full" - ca_cert_path: /etc/tls/ca-bundle.pem + ssl_mode: "${DB_SSL_MODE}" + ca_cert_path: "${DB_CA_CERT_PATH}"Add parameters (outside this hunk) to allow safe overrides per environment:
parameters: - name: DB_SSL_MODE value: "verify-full" description: "PostgreSQL SSL mode: disable|allow|prefer|require|verify-ca|verify-full" - name: DB_CA_CERT_PATH value: "/etc/tls/ca-bundle.pem" description: "Path to the PostgreSQL CA bundle inside the container"And, to avoid silent misconfiguration, consider making the CA item non-optional (outside this hunk):
volumes: - name: db-ca-cert secret: secretName: ${ASSISTED_CHAT_DB_SECRET_NAME} items: - key: db.ca_cert path: ca-bundle.pem optional: falseIf you need to keep the CA optional for certain envs, set DB_SSL_MODE to require (or disable) when the CA isn’t provided, and keep verify-full only where the CA is mounted and hostnames match.
Also applies to: 326-327, 361-362
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
llama-stack(1 hunks)template.yaml(2 hunks)
🔇 Additional comments (1)
llama-stack (1)
1-1: Verify TLS support in llama-stack and manifest alignment
We’ve confirmed intemplate.yamlthat:
ssl_mode: "verify-full"andca_cert_path: /etc/tls/ca-bundle.pemappear at lines 181/182, 317/318, 326/327, and 361/362.- The CA bundle is mounted under
/etc/tlswithca-bundle.pemat lines 485–486 and 547–550.- All PostgreSQL
host:entries use the${env.ASSISTED_CHAT_POSTGRES_HOST}variable, so ensure this resolves to your service’s DNS name (not an IP) and matches the certificate SAN.However, since submodules aren’t checked out in this sandbox, we couldn’t locate
ssl_modeorca_cert_pathin the localllama-stackcode. Please manually confirm that the bumped commit in thellama-stacksubmodule indeed adds support for these TLS fields in its configuration parsing (e.g. in the config schema or YAML loader).
|
/test eval-test |
1 similar comment
|
/test eval-test |
|
@maorfr: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| db: ${env.ASSISTED_CHAT_POSTGRES_NAME} | ||
| user: ${env.ASSISTED_CHAT_POSTGRES_USER} | ||
| password: ${env.ASSISTED_CHAT_POSTGRES_PASSWORD} | ||
| ssl_mode: "verify-full" |
There was a problem hiding this comment.
Will this work with the DB we use for local development?
What's in /etc/tls/ca-bundle.pem
There was a problem hiding this comment.
OK, I see https://github.com/rh-ecosystem-edge/assisted-chat/pull/133/files
Still unclear if this doesn't break make run
There was a problem hiding this comment.
doesn't make run use the pod template?
|
/lgtm |
openshift-merge-bot
bot
merged commit a52007f
into
rh-ecosystem-edge:main
2 participants
part of https://issues.redhat.com/browse/MGMT-21487
this PR enables secure connectivity between the service and the database for the llama-stack persistence layer.
we mount a DB CA certificate to a file on the pod and pass that file through the llama-stack configuration, as was implemented in llamastack/llama-stack#3182
related to #133
Summary by CodeRabbit