fix(helm/nico-api): strip stale spiffe_trust_domain from site TOML - #4736
Open
shayan1995 wants to merge 3 commits into
Open
fix(helm/nico-api): strip stale spiffe_trust_domain from site TOML#4736shayan1995 wants to merge 3 commits into
shayan1995 wants to merge 3 commits into
Conversation
…VIDIA#3666) When an operator migrates from carbide to NICo 2.0, their site config TOML often retains spiffe_trust_domain = "forge.local" from the old deployment. Because the site TOML is a Figment overlay loaded after the built-in config, this stale value overrides the chart's global.spiffe.trustDomain and breaks every in-cluster service client with a bare HTTP 403 ("Request denied: [TrustedCertificate]" in nico-api logs). The built-in config (carbide-api-config.toml) already renders: spiffe_trust_domain = "{{ .Values.global.spiffe.trustDomain }}" under [auth.trust], so it always has the correct chart value. The fix: strip spiffe_trust_domain from the site TOML at helm render time using regexReplaceAll so the built-in config's value takes effect. Other keys in the [auth.trust] section are preserved. Operators who need a non-default trust domain should set global.spiffe.trustDomain in their helm values rather than embedding it in nicoApiSiteConfig. This is already documented in helm/README.md under "Upgrading from pre-2.0.0". Two new helm-unittest cases added: one verifies the stale value is stripped while preserving sibling keys, one verifies a site TOML without the key is rendered unchanged. Signed-off-by: Shayan Namaghi <snamaghi@nvidia.com>
2 tasks
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThe nico-api chart removes ChangesSite configuration trust-domain handling
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to This localized chart change removes stale site-level SPIFFE configuration while preserving other settings and includes focused tests; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
helm/charts/nico-api/tests/auth_test.yaml (1)
53-69: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover both generated site-config filenames.
The template writes sanitized content to both
carbide-api-site-config.tomlandnico-api-site-config.toml. The Deployment mounts the ConfigMap for both legacy and nico paths. These tests inspect onlydata["nico-api-site-config.toml"], so a regression in the legacy key could bypass coverage.Add the same removal and preservation assertions for
data["carbide-api-site-config.toml"]in both test cases.Suggested test additions
- matchRegex: path: data["nico-api-site-config.toml"] pattern: 'some_other_key = "preserved"' + - notMatchRegex: + path: data["carbide-api-site-config.toml"] + pattern: 'spiffe_trust_domain' + - matchRegex: + path: data["carbide-api-site-config.toml"] + pattern: 'some_other_key = "preserved"' - notMatchRegex: path: data["nico-api-site-config.toml"] pattern: 'spiffe_trust_domain' + - matchRegex: + path: data["carbide-api-site-config.toml"] + pattern: 'some_other_key = "value"' + - notMatchRegex: + path: data["carbide-api-site-config.toml"] + pattern: 'spiffe_trust_domain'Also applies to: 71-85
🤖 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 `@helm/charts/nico-api/tests/auth_test.yaml` around lines 53 - 69, Add matching assertions for data["carbide-api-site-config.toml"] in both site-config sanitization test cases, verifying spiffe_trust_domain is removed while some_other_key = "preserved" remains. Keep the existing nico-api-site-config.toml assertions unchanged so both generated filenames are covered.
🤖 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 `@helm/charts/nico-api/templates/configmap.yaml`:
- Around line 48-49: Update the siteConfig sanitization around $siteConfig and
regexReplaceAll so every accepted TOML representation of spiffe_trust_domain,
including literal strings and assignments with inline comments, is removed or
rejected before site configuration is applied. Ensure
[auth.trust].spiffe_trust_domain cannot override global.spiffe.trustDomain, and
add tests covering each supported key form.
---
Nitpick comments:
In `@helm/charts/nico-api/tests/auth_test.yaml`:
- Around line 53-69: Add matching assertions for
data["carbide-api-site-config.toml"] in both site-config sanitization test
cases, verifying spiffe_trust_domain is removed while some_other_key =
"preserved" remains. Keep the existing nico-api-site-config.toml assertions
unchanged so both generated filenames are covered.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 41d5d50d-7334-45f9-8713-392fc1f2463a
📒 Files selected for processing (2)
helm/charts/nico-api/templates/configmap.yamlhelm/charts/nico-api/tests/auth_test.yaml
… test - Extend the regex to also strip single-quoted spiffe_trust_domain values (TOML literal string form `spiffe_trust_domain = 'forge.local'`) in addition to the more common double-quoted form. - Add an explicit upgrade warning in the template comment so operators know to set global.spiffe.trustDomain before upgrading if their site TOML has the key. - Add two new helm-unittest cases: single-quoted value is stripped, and a round-trip test confirming the built-in config still renders the correct trust domain from global.spiffe.trustDomain after the site TOML is processed. Signed-off-by: Shayan Namaghi <snamaghi@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@helm/charts/nico-api/tests/auth_test.yaml`:
- Around line 104-118: Update the round-trip test using distinct trust domains:
set global.spiffe.trustDomain to nico.local while retaining forge.local in
siteConfig, assert nico.local in documentIndex 0, and add a documentIndex 1
assertion confirming the sanitized site configuration omits spiffe_trust_domain.
- Around line 87-102: Extend the sanitizer used for nicoApiSiteConfig to remove
spiffe_trust_domain assignments with either quoted value form followed by an
optional inline TOML comment, while preserving unrelated keys. Update the
auth_test.yaml regression coverage for a value such as 'forge.local' # migrated
and assert the stale key is absent while other_key remains.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cca38d59-2986-4787-9938-edda9552d5c3
📒 Files selected for processing (2)
helm/charts/nico-api/templates/configmap.yamlhelm/charts/nico-api/tests/auth_test.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- helm/charts/nico-api/templates/configmap.yaml
The regex used \s*$ which did not match lines with an inline TOML comment after the value, e.g. spiffe_trust_domain = "forge.local" # migrated. Changed the trailing anchor to [^\n]*$ to consume any trailing content on the same line. Added a test case for the inline-comment form. Replaced the round-trip test that used forge.local for both global.spiffe.trustDomain and the site TOML — identical values meant the test could pass even if stripping failed, since the built-in config independently renders the global value. New tests use nico.local for the global value and forge.local in the site TOML, with separate assertions on the built-in config (must show nico.local) and the site config (must not contain spiffe_trust_domain). Signed-off-by: Shayan Namaghi <snamaghi@nvidia.com>
jabdulvahid
approved these changes
Aug 13, 2026
Contributor
Author
|
/ok-to-test af4d10c |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3666.
When an operator migrates from carbide to NICo 2.0, their site config TOML often retains
spiffe_trust_domain = "forge.local"from the old deployment. Because the site TOML is a Figment overlay loaded after the built-in config, this stale value overridesglobal.spiffe.trustDomainand breaks every in-cluster service client with a bare HTTP 403 (Request denied: [TrustedCertificate]in nico-api logs).Root cause
The built-in
carbide-api-config.tomlalready rendersspiffe_trust_domainfromglobal.spiffe.trustDomainunder[auth.trust]. ButsiteConfig.nicoApiSiteConfigis passed verbatim into the ConfigMap, so a stale value in the operator's TOML takes precedence and silently overrides the chart's authoritative value.Fix
Strip
spiffe_trust_domain = "..."fromsiteConfig.nicoApiSiteConfigat helm render time usingregexReplaceAll. Both double-quoted ("forge.local") and single-quoted ('forge.local') TOML string forms are handled. All other keys in the[auth.trust]section are preserved. With the key absent from the site TOML, the built-in config'sglobal.spiffe.trustDomainvalue takes effect.Upgrade note
This is already documented in the upgrade guide (PR #4261). Operators who already set
global.spiffe.trustDomainin their values are unaffected.Related issues
Closes #3666
Type of Change
Breaking Changes
No breaking changes for correctly-configured deployments. Operators with
spiffe_trust_domainonly in their site TOML (not inglobal.spiffe.trustDomain) should follow the upgrade note above.Testing
helm unittest helm/charts/nico-api -f tests/auth_test.yaml— 6 tests pass:forge.localfromglobal.spiffe.trustDomainafter site TOML is stripped