ROSAENG-130 - feat: add database SSL mode configuration to helm chart#99
ROSAENG-130 - feat: add database SSL mode configuration to helm chart#99cdoan1 wants to merge 1 commit intoopenshift-hyperfleet:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @cdoan1. Thanks for your PR. I'm waiting for a openshift-hyperfleet member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe Helm chart adds a new values entry Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Add configurable database.sslMode to values.yaml with default "require" and pass --db-sslmode flag to both db-migrate initContainer and main serve container for secure database connections in production. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
cdoan1
force-pushed
the
ROSAENG-130-sslmode
branch
from
5bcc2ab to
6d0eab1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/templates/deployment.yaml`:
- Line 52: The startup/migrate command passes the wrong flag name
`--db-sslmode`; update the command invocation(s) for the hyperfleet-api migrate
runner to use the registered flag `--db-ssl-mode` instead (the flag is defined
in pkg/config/flags.go), i.e., replace occurrences of `--db-sslmode` with
`--db-ssl-mode` in the command arrays so the CLI recognizes the option and
migrations/startup won't fail.
- Around line 74-76: The template uses the wrong values path for the
bind-address flags: update the three arguments that reference .Values.server.*
to read from .Values.config.* (e.g. change --api-server-bindaddress to use
.Values.config.server.bindAddress | default ":8000", --health-server-bindaddress
to use .Values.config.health.bindAddress | default ":8080", and
--metrics-server-bindaddress to use .Values.config.metrics.metricsBindAddress or
.Values.config.metrics.bindAddress | default ":9090" as appropriate) so the
flags pull from the values.yml structure that actually defines these defaults.
In `@charts/values.yaml`:
- Around line 218-222: The default database SSL mode currently set by the value
sslMode: "prefer" is insecure for production because it can fall back to
plaintext; change the default value for the configuration key named sslMode
under database to "require" (one of the allowed options: disable, allow, prefer,
require, verify-ca, verify-full) so connections must use TLS by default; update
any related documentation lines or comments near the sslMode key to reflect the
new secure default.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d280bcc9-68b0-47eb-809f-0134b6a18756
📒 Files selected for processing (2)
charts/templates/deployment.yamlcharts/values.yaml
charts/templates/deployment.yaml
Outdated
| image: "{{ .Values.image.registry }}/{{ .Values.image.repository }}:{{ .Values.image.tag }}" | ||
| imagePullPolicy: {{ .Values.image.pullPolicy }} | ||
| command: ["/app/hyperfleet-api", "migrate"] | ||
| command: ["/app/hyperfleet-api", "migrate", "--db-sslmode={{ .Values.database.sslMode | default "require" }}"] |
There was a problem hiding this comment.
Use the registered flag name --db-ssl-mode.
Line 52 and Line 77 pass --db-sslmode, but the CLI registers --db-ssl-mode (pkg/config/flags.go). This will fail with an unknown-flag error and block startup/migration.
Suggested change
- command: ["/app/hyperfleet-api", "migrate", "--db-sslmode={{ .Values.database.sslMode | default "require" }}"]
+ command: ["/app/hyperfleet-api", "migrate", "--db-ssl-mode={{ .Values.database.sslMode | default "require" }}"]
...
- - --db-sslmode={{ .Values.database.sslMode | default "require" }}
+ - --db-ssl-mode={{ .Values.database.sslMode | default "require" }}Also applies to: 77-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/templates/deployment.yaml` at line 52, The startup/migrate command
passes the wrong flag name `--db-sslmode`; update the command invocation(s) for
the hyperfleet-api migrate runner to use the registered flag `--db-ssl-mode`
instead (the flag is defined in pkg/config/flags.go), i.e., replace occurrences
of `--db-sslmode` with `--db-ssl-mode` in the command arrays so the CLI
recognizes the option and migrations/startup won't fail.
| # SSL mode for database connections | ||
| # Options: disable, allow, prefer, require, verify-ca, verify-full | ||
| # For production, use "require" or higher | ||
| sslMode: "prefer" | ||
|
|
There was a problem hiding this comment.
Set a secure default for database.sslMode.
Line 221 sets sslMode: "prefer", which can downgrade to non-TLS if the server allows it. For a production-safe baseline, this should default to "require".
Suggested change
- sslMode: "prefer"
+ sslMode: "require"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # SSL mode for database connections | |
| # Options: disable, allow, prefer, require, verify-ca, verify-full | |
| # For production, use "require" or higher | |
| sslMode: "prefer" | |
| # SSL mode for database connections | |
| # Options: disable, allow, prefer, require, verify-ca, verify-full | |
| # For production, use "require" or higher | |
| sslMode: "require" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/values.yaml` around lines 218 - 222, The default database SSL mode
currently set by the value sslMode: "prefer" is insecure for production because
it can fall back to plaintext; change the default value for the configuration
key named sslMode under database to "require" (one of the allowed options:
disable, allow, prefer, require, verify-ca, verify-full) so connections must use
TLS by default; update any related documentation lines or comments near the
sslMode key to reflect the new secure default.
ciaranRoche
left a comment
ciaranRoche
left a comment
There was a problem hiding this comment.
Do you require this, or can you use config.database.ssl.mode to override the ssl mode in the configmap?
2 participants
Add configurable database.sslMode to values.yaml with default "require" and pass --db-sslmode flag to both db-migrate initContainer and main serve container for secure database connections in production.
Summary
Regional team deploys with aws rds, so we need db sslmode
Test Plan
make test-allpassesmake lintpassesmake test-helm(if applicable)Summary by CodeRabbit