fix(platform): gate tls volume on tls.enabled instead of kas/all mode#171
fix(platform): gate tls volume on tls.enabled instead of kas/all mode#171jp-ayyappan merged 4 commits intomainfrom
Conversation
…l mode The tls volume was nested inside the kas/all mode block with a misplaced empty if tls.enabled check that had no effect. This caused the tls volume to only render when mode contained kas or all, while the tls volumeMount was correctly gated on tls.enabled alone — producing an invalid pod spec when using mode: core with tls enabled. Fix: separate kas-private-keys and tls into independent blocks, each gated on their own condition. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical templating bug in the Helm chart's Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a bug where the tls volume in the Helm chart was incorrectly tied to the kas or all mode. The change moves the tls volume definition to be conditional only on .Values.server.tls.enabled, which aligns its behavior with the corresponding volumeMount and fixes deployment failures in core mode with TLS enabled. The implementation is correct and improves the template's logic and readability.
elizabethhealy
left a comment
There was a problem hiding this comment.
actually could we add a unit tests to ensure we the tls volume and mount are created when we expect https://github.com/opentdf/charts/blob/main/tests/chart_platform_template_test.go
auth.enabled was hardcoded to true then .Values.server.auth was rendered after it, creating a duplicate YAML key when users set server.auth.enabled: false — causing strict yaml parsers to reject the config file entirely. Use .Values.server.auth.enabled with a default of true, and render the rest of the auth block using omit to exclude the enabled key. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@elizabethhealy Unit tests added: #172 cc: @jp-ayyappan |
…ing (#172) ## Summary - Add unit test: `mode: core` with `tls.enabled: true` — tls volume and mount should be present - Add unit test: `mode: core` with `tls.enabled: false` — tls volume and mount should be absent - Add unit test: `mode: core,kas` with `tls.enabled: true` — both tls and kas-private-keys volumes and mounts should be present Closes FEDCD-469 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…ume-mode-condition
|
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 (3)
📝 WalkthroughWalkthroughThis PR adjusts YAML template indentation for volume configuration in Helm templates, updates server authentication configuration rendering logic with proper default handling, and adds three new tests validating TLS volume and mount behavior in deployment manifests. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@elizabethhealy, do the unit tests look sufficient or are there other changes you'd like to see? |
Summary
The
tlsvolume in_volume.tplwas nested inside theif kas/allmode block with a misplaced emptyif tls.enabledcheck that had no effect. This caused thetlsvolume to only render when mode containedkasorall, while thetlsvolumeMount was correctly gated ontls.enabledalone — producing an invalid pod spec when usingmode: corewith TLS enabled:Before:
After:
Test plan
mode: coreandserver.tls.enabled: true— pod should start successfully withtlsvolume mountedmode: coreandserver.tls.enabled: false— notlsvolume or mountmode: core,kasandserver.tls.enabled: true— bothkas-private-keysandtlsvolumes rendered🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests