HYPERFLEET-1039 - fix: disable JWT auth in default chart values#128
Conversation
rh-amarin
changed the title
WalkthroughThe pull request modifies a single configuration default in the Helm values file. The Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/values.yaml`:
- Line 52: Change the default for JWT so the chart is secure-by-default: set
config.server.jwt.enabled to true in values.yaml (instead of false) and keep any
JWT disabling only in test/CI-specific override files (e.g., values-e2e.yaml or
via --set) so real deployments do not default to unauthenticated operation.
🪄 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: Enterprise
Run ID: 1f2e5609-a76b-42bc-830c-5ed6acc85fef
📒 Files selected for processing (1)
charts/values.yaml
|
|
||
| jwt: | ||
| enabled: true | ||
| enabled: false |
There was a problem hiding this comment.
Keep JWT enabled by default; disable it only in test overrides.
On Line 52, setting config.server.jwt.enabled: false makes unauthenticated operation the chart default, which is a high-risk auth regression if this value is used in real environments. Keep secure-by-default (true) and apply JWT disablement only in CI/E2E override values (-f values-e2e.yaml or --set) for test runs.
Suggested fix
jwt:
- enabled: false
+ enabled: true
issuer_url: ""
audience: ""As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
📝 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.
| enabled: false | |
| jwt: | |
| enabled: true | |
| issuer_url: "" | |
| audience: "" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@charts/values.yaml` at line 52, Change the default for JWT so the chart is
secure-by-default: set config.server.jwt.enabled to true in values.yaml (instead
of false) and keep any JWT disabling only in test/CI-specific override files
(e.g., values-e2e.yaml or via --set) so real deployments do not default to
unauthenticated operation.
There was a problem hiding this comment.
Having JWT enabled should be the default option because of security reasons. We want to explicitly disable it in JWT in production.
But authz is not yet a closed topic and in this case the default option is making the e2e tests to fail
There was a problem hiding this comment.
@rh-amarin, understood — the PR description captures the reasoning well. The concern here is that once this merges, any real deployment using default values will silently have JWT off, which is easy to miss.
A middle-ground that avoids changing the secure default would be to keep jwt.enabled: true in values.yaml and supply an override file specifically for CI/E2E runs:
# charts/values-e2e.yaml
config:
server:
jwt:
enabled: falseThen CI/E2E pipelines pass -f charts/values-e2e.yaml (or --set config.server.jwt.enabled=false). This way:
- Default chart values stay secure-by-default.
- E2E tests are unblocked without touching production defaults.
- When authz lands, removing the override is the only change needed.
Would you like me to open a GitHub issue to track the authz work and flip jwt.enabled back to true in defaults once it's resolved?
There was a problem hiding this comment.
No, thank you. I will do that when we decide on jwt
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ciaranRoche 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 |
openshift-merge-bot
Bot
merged commit d2c0ca9
into
openshift-hyperfleet:main
2 participants
Summary
Having JWT enabled should be the default option because of security reasons. We want to explicitly disable it in JWT in production.
But authz is not yet a closed topic and in this case the default option is making the e2e tests to fail
Summary by CodeRabbit