fix(helm/nico-api): strip stale spiffe_trust_domain from site TOML - #4736
Open
shayan1995 wants to merge 2 commits into
Open
fix(helm/nico-api): strip stale spiffe_trust_domain from site TOML#4736shayan1995 wants to merge 2 commits into
shayan1995 wants to merge 2 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
Summary by CodeRabbit
WalkthroughThe nico-api chart removes ChangesSite configuration trust-domain handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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
Comment on lines
+48
to
+49
| {{- $raw := .Values.siteConfig.nicoApiSiteConfig | default "" }} | ||
| {{- $siteConfig := regexReplaceAll `(?m)^\s*spiffe_trust_domain\s*=\s*"[^"]*"\s*$` $raw "" }} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect all checked-in TOML and embedded YAML examples for assignment forms.
rg -n -C 2 'spiffe_trust_domain[[:space:]]*=' \
--glob '*.toml' --glob '*.yaml' --glob '*.yml' .Repository: NVIDIA/infra-controller
Length of output: 6258
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- configmap template ---'
nl -ba helm/charts/nico-api/templates/configmap.yaml | sed -n '35,65p'
printf '%s\n' '--- chart tests ---'
nl -ba helm/charts/nico-api/tests/auth_test.yaml | sed -n '1,90p'
printf '%s\n' '--- related values and configuration docs ---'
rg -n -C 4 'nicoApiSiteConfig|siteConfig|trustDomain|spiffe_trust_domain' \
helm/charts/nico-api helm/examples README.md 2>/dev/null || true
printf '%s\n' '--- all template references ---'
rg -n -C 3 'regexReplaceAll|carbide-api-site-config.toml|nico-api-site-config.toml' \
helm/charts/nico-apiRepository: NVIDIA/infra-controller
Length of output: 228
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- configmap template ---'
sed -n '35,65p' helm/charts/nico-api/templates/configmap.yaml | awk '{printf "%6d %s\n", NR+34, $0}'
printf '%s\n' '--- chart tests ---'
sed -n '1,90p' helm/charts/nico-api/tests/auth_test.yaml | awk '{printf "%6d %s\n", NR, $0}'
printf '%s\n' '--- related values and configuration docs ---'
rg -n -C 4 'nicoApiSiteConfig|siteConfig|trustDomain|spiffe_trust_domain' \
helm/charts/nico-api helm/examples README.md 2>/dev/null || true
printf '%s\n' '--- all template references ---'
rg -n -C 3 'regexReplaceAll|carbide-api-site-config.toml|nico-api-site-config.toml' \
helm/charts/nico-apiRepository: NVIDIA/infra-controller
Length of output: 44381
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- parser and configuration consumers ---'
rg -n -C 4 'spiffe_trust_domain|site-config-path|site_config|site config|toml::|toml =' \
--glob '*.rs' --glob 'Cargo.toml' --glob '*.lock' .
printf '%s\n' '--- regex runtime availability ---'
command -v go || true
command -v python3 || true
printf '%s\n' '--- regex behavior for supported TOML forms ---'
python3 - <<'PY'
import re
pattern = re.compile(r'(?m)^\s*spiffe_trust_domain\s*=\s*"[^"]*"\s*$')
cases = {
"double quoted": 'spiffe_trust_domain = "forge.local"\n',
"literal string": "spiffe_trust_domain = 'forge.local'\n",
"inline comment": 'spiffe_trust_domain = "forge.local" # migrated\n',
"extra spaces": ' spiffe_trust_domain = "forge.local" \n',
"escaped quote": r'spiffe_trust_domain = "forge\.local"\n',
"crlf": 'spiffe_trust_domain = "forge.local"\r\n',
"same line after section": '[auth.trust]\nspiffe_trust_domain = "forge.local"\nsome_other_key = "preserved"\n',
}
for name, value in cases.items():
result = pattern.sub("", value)
print(f"{name}: {'removed' if 'spiffe_trust_domain' not in result else 'REMAINS'}")
print(repr(result))
PYRepository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- API trust configuration references ---'
rg -l 'spiffe_trust_domain' crates | while IFS= read -r file; do
printf '%s\n' "--- $file"
rg -n -C 3 'spiffe_trust_domain' "$file"
done
printf '%s\n' '--- TOML dependency declaration ---'
sed -n '228,242p' Cargo.toml
printf '%s\n' '--- regex runtime availability ---'
command -v go || true
command -v python3 || true
printf '%s\n' '--- regex behavior for supported TOML forms ---'
python3 - <<'PY'
import re
pattern = re.compile(r'(?m)^\s*spiffe_trust_domain\s*=\s*"[^"]*"\s*$')
cases = {
"double quoted": 'spiffe_trust_domain = "forge.local"\n',
"literal string": "spiffe_trust_domain = 'forge.local'\n",
"inline comment": 'spiffe_trust_domain = "forge.local" # migrated\n',
"extra spaces": ' spiffe_trust_domain = "forge.local" \n',
"escaped quote": r'spiffe_trust_domain = "forge\.local"\n',
"crlf": 'spiffe_trust_domain = "forge.local"\r\n',
"same line after section": '[auth.trust]\nspiffe_trust_domain = "forge.local"\nsome_other_key = "preserved"\n',
}
for name, value in cases.items():
result = pattern.sub("", value)
print(f"{name}: {'removed' if 'spiffe_trust_domain' not in result else 'REMAINS'}")
print(repr(result))
PYRepository: NVIDIA/infra-controller
Length of output: 7095
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- API configuration merge path ---'
sed -n '1,270p' crates/api/src/resources.rs
printf '%s\n' '--- site configuration loading and merge references ---'
rg -n -C 5 \
'site_config|site-config|site_config_path|site-config-path|from_str|merge|auth\.trust|TrustConfig' \
crates/api crates/api-core crates/authn crates/bmc-proxy crates/bin 2>/dev/null || true
printf '%s\n' '--- API configuration entry points ---'
rg -n -C 4 \
'struct Config|impl Config|fn .*config|Config::|read_to_string|from_str' \
crates/api/src crates/api-core/src crates/authn/src 2>/dev/null | head -n 300Repository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- API merge function ---'
sed -n '190,240p' crates/api/src/resources.rs
printf '%s\n' '--- API source references only ---'
rg -n -C 5 \
'resources::|resources\.|auth\.trust|spiffe_trust_domain|site_config|site-config|site_config_path|site-config-path' \
crates/api/src crates/authn/src crates/bmc-proxy/src \
-g '*.rs' | head -n 400Repository: NVIDIA/infra-controller
Length of output: 14887
Remove spiffe_trust_domain from every accepted site TOML form.
The site configuration overrides the global configuration, and [auth.trust].spiffe_trust_domain populates TrustConfig. This regex does not remove valid literal strings or assignments with inline comments. A stale value can therefore override global.spiffe.trustDomain and reproduce the 403 failure. Use TOML-aware sanitization or reject any site configuration that contains this key. Add tests for each accepted form.
🤖 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/templates/configmap.yaml` around lines 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.
… 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
Comment on lines
+87
to
+102
| - it: also strips single-quoted spiffe_trust_domain (TOML literal string form) | ||
| documentIndex: 1 | ||
| set: | ||
| siteConfig: | ||
| enabled: true | ||
| nicoApiSiteConfig: | | ||
| [auth.trust] | ||
| spiffe_trust_domain = 'forge.local' | ||
| other_key = "kept" | ||
| asserts: | ||
| - notMatchRegex: | ||
| path: data["nico-api-site-config.toml"] | ||
| pattern: 'spiffe_trust_domain' | ||
| - matchRegex: | ||
| path: data["nico-api-site-config.toml"] | ||
| pattern: 'other_key = "kept"' |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target test ---'
sed -n '1,150p' helm/charts/nico-api/tests/auth_test.yaml
printf '%s\n' '--- configmap template ---'
sed -n '1,110p' helm/charts/nico-api/templates/configmap.yaml
printf '%s\n' '--- related references ---'
rg -n -C 3 'spiffe_trust_domain|trustDomain|nico-api-site-config' helm/charts/nico-apiRepository: NVIDIA/infra-controller
Length of output: 24443
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import re
from pathlib import Path
template = Path("helm/charts/nico-api/templates/configmap.yaml").read_text()
for i, line in enumerate(template.splitlines(), 1):
if "spiffe_trust_domain" in line:
print(f"template line {i}: {line}")
# Extract the regex literal only if it is visible in the template source.
# Probe the behavior described by the review comment using the line-anchored
# pattern shown by the template.
pattern = r"(?m)^\s*spiffe_trust_domain\s*=\s*(?:\"[^\"]*\"|'[^']*')\s*$"
cases = [
"spiffe_trust_domain = 'forge.local'",
"spiffe_trust_domain = 'forge.local' # migrated",
'spiffe_trust_domain = "forge.local" # migrated',
"other_key = \"kept\"",
]
compiled = re.compile(pattern)
for case in cases:
print(f"{case!r} -> {bool(compiled.search(case))}")
PYRepository: NVIDIA/infra-controller
Length of output: 653
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import re
from pathlib import Path
template = Path("helm/charts/nico-api/templates/configmap.yaml").read_text()
for i, line in enumerate(template.splitlines(), 1):
if "spiffe_trust_domain" in line:
print(f"template line {i}: {line}")
pattern = r"(?m)^\s*spiffe_trust_domain\s*=\s*(?:\"[^\"]*\"|'[^']*')\s*$"
compiled = re.compile(pattern)
for case in [
"spiffe_trust_domain = 'forge.local'",
"spiffe_trust_domain = 'forge.local' # migrated",
'spiffe_trust_domain = "forge.local" # migrated',
'other_key = "kept"',
]:
print(f"{case!r} -> {bool(compiled.search(case))}")
PYRepository: NVIDIA/infra-controller
Length of output: 653
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- all trust-domain references ---'
rg -n -C 4 'spiffe_trust_domain|site-config-path|site_config|site config|nico-api-site-config' . \
-g '!vendor/**' -g '!node_modules/**' -g '!dist/**' -g '!build/**'
printf '%s\n' '--- candidate source files ---'
git ls-files | rg '(^|/)(src|crates|cmd|internal|config|.*config.*)\.(rs|go|toml)$|Cargo\.toml$|go\.mod$'Repository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import re
from pathlib import Path
template = Path("helm/charts/nico-api/templates/configmap.yaml").read_text()
match = re.search(r"regexReplaceAll `([^`]+)` \$raw", template)
assert match, "sanitizer pattern not found"
pattern = re.compile(match.group(1))
raw = """[auth.trust]
spiffe_trust_domain = 'forge.local' # migrated
other_key = "kept"
"""
site = pattern.sub("", raw)
print("--- remaining site configuration ---")
print(site, end="")
print("--- stale key remains ---")
print("spiffe_trust_domain" in site)
PYRepository: NVIDIA/infra-controller
Length of output: 308
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact identifiers in tracked source/config files ---'
git grep -n -E 'spiffe_trust_domain|site-config-path|site_config_path|nicoApiSiteConfig|site-config' -- \
'*.rs' '*.go' '*.toml' '*.yaml' '*.yml' '*.md' \
':!**/node_modules/**' ':!**/vendor/**' ':!**/generated/**' \
| head -n 300
printf '%s\n' '--- chart documentation and startup arguments ---'
git grep -n -C 5 -E 'site TOML|site config|site-config-path|trustDomain|trust domain' -- \
'helm/charts/nico-api/**' '*.md' \
| head -n 300Repository: NVIDIA/infra-controller
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
for path in Path("helm/charts/nico-api").rglob("*"):
if path.is_file() and path.suffix in {".toml", ".yaml", ".yml", ".md"}:
text = path.read_text(errors="replace")
hits = [
(i, line)
for i, line in enumerate(text.splitlines(), 1)
if "spiffe_trust_domain" in line
or "site-config-path" in line
or "site config" in line.lower()
or "trustDomain" in line
]
if hits:
print(f"--- {path} ---")
for i, line in hits[:80]:
print(f"{i}: {line}")
PYRepository: NVIDIA/infra-controller
Length of output: 2428
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- configuration load implementation ---'
sed -n '1,145p' crates/api-core/src/cfg/load.rs
printf '%s\n' '--- configuration load tests and trust config definitions ---'
rg -n -C 8 'merged_carbide_config_figment|site_config_path|NormalizeLegacyDpuPolicy|spiffe_trust_domain|TrustConfig' \
crates/api-core crates/authn crates/api \
-g '*.rs' -g '*.md' | head -n 500
printf '%s\n' '--- overlay documentation ---'
sed -n '96,112p' deploy/README.mdRepository: NVIDIA/infra-controller
Length of output: 31973
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
path = Path("crates/api-core/src/cfg/load.rs")
lines = path.read_text().splitlines()
for start, end in [(80, 112)]:
for number in range(start, min(end, len(lines)) + 1):
print(f"{number}: {lines[number-1]}")
PYRepository: NVIDIA/infra-controller
Length of output: 1336
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
path = Path("crates/api-core/src/cfg/load.rs")
lines = path.read_text().splitlines()
for number in range(80, min(112, len(lines)) + 1):
print(f"{number}: {lines[number-1]}")
PYRepository: NVIDIA/infra-controller
Length of output: 1336
Cover valid TOML inline comments.
The sanitizer does not remove spiffe_trust_domain = 'forge.local' # migrated. The site configuration is merged after the built-in configuration, so this stale value can override global.spiffe.trustDomain and cause authorization failures. Add this regression case and allow an optional inline comment in the sanitizer.
🤖 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 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.
Source: MCP tools
Comment on lines
+104
to
+118
| - it: round-trip — built-in config still uses global.spiffe.trustDomain after site TOML is stripped | ||
| documentIndex: 0 | ||
| set: | ||
| global: | ||
| spiffe: | ||
| trustDomain: forge.local | ||
| siteConfig: | ||
| enabled: true | ||
| nicoApiSiteConfig: | | ||
| [auth.trust] | ||
| spiffe_trust_domain = "forge.local" | ||
| asserts: | ||
| - matchRegex: | ||
| path: data["nico-api-config.toml"] | ||
| pattern: 'spiffe_trust_domain = "forge\.local"' |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use different site and global trust domains.
Both values are forge.local. The test asserts only documentIndex: 0, so it does not inspect the sanitized site configuration. It can pass even when the site assignment is not removed because the built-in configuration renders forge.local directly from global.spiffe.trustDomain.
Set the global value to nico.local, keep the site value as forge.local, assert nico.local in the built-in configuration, and add a separate documentIndex: 1 assertion that the site file does not contain spiffe_trust_domain.
As per coding guidelines, the test must use evidence-backed values and explicit verification of the cross-layer contract.
🤖 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 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.
Source: Coding guidelines
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
1 participant
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