feat(helm): support admin client CA bundles - #4423
Conversation
Signed-off-by: Larry Chen <lachen@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Summary by CodeRabbit
WalkthroughThe Helm chart adds ChangesAdmin client certificate configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Trivy (0.72.0)Trivy execution failed: 2026-07-31T03:18:19Z FATAL Fatal error run error: fs scan error: scan error: scan failed: failed analysis: post analysis error: post analysis error: kubernetes scan error: fs filter error: fs filter error: walk error open smartylint.json: no such file or directory: open smartylint.json: no such file or directory Comment |
Signed-off-by: Larry Chen <lachen@nvidia.com>
|
/ok to test |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
helm/charts/nico-api/values.yaml (1)
239-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIdentify the chart defaults in the operator comments.
State that
additionalIssuerCns: []andadminRootCertPem: ""are the chart defaults. This makes the disabled default behavior clear before an operator overrides either value.As per coding guidelines, “identify chart values as defaults when templates allow overrides.”
🤖 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/values.yaml` around lines 239 - 270, Update the operator comments for auth.additionalIssuerCns and siteConfig.adminRootCertPem to explicitly identify [] and "" as the chart defaults, respectively, while preserving the existing guidance for overriding either value.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@helm/charts/nico-api/values.yaml`:
- Around line 239-270: Update the operator comments for auth.additionalIssuerCns
and siteConfig.adminRootCertPem to explicitly identify [] and "" as the chart
defaults, respectively, while preserving the existing guidance for overriding
either value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4e84782b-4e87-4714-8ac5-60aefdcddc77
📒 Files selected for processing (5)
helm/PREREQUISITES.mdhelm/charts/nico-api/templates/_helpers.tplhelm/charts/nico-api/templates/configmap.yamlhelm/charts/nico-api/tests/admin_root_cert_test.yamlhelm/charts/nico-api/values.yaml
shayan1995
left a comment
shayan1995
left a comment
There was a problem hiding this comment.
Backward-compatible, well-validated, and the trust model caveats are all correctly documented. The existing site ConfigMap mount already covered both paths — this just gives operators a supported way to populate the file. Good to go.
4 participants
The nico-api runtime supports
auth.adminRootCafilePath, and the chart alreadymounts the site ConfigMap at the corresponding site directory. The chart did
not, however, provide a supported way to materialize an external admin-client
CA bundle into that ConfigMap. Deployments using an existing user-certificate
PKI therefore needed an out-of-band workaround before admin client
certificates could be trusted.
This change:
nico-api.siteConfig.adminRootCertPemvalue;admin_root_cert_pemin the existing site ConfigMap;CERTIFICATEPEM blocksand rejects private keys, metadata/comments, malformed or trailing content,
disabled site config, and paths outside the existing site mounts;
the CN-only
auth.additionalIssuerCnsmapping and the requirement to use aglobally unique dedicated admin-client intermediate; and
supported mount paths, defaults, invalid inputs, CRLF input, and missing
final newlines.
The value defaults to empty, and the default rendered manifests remain
byte-for-byte unchanged. The existing ConfigMap mount is reused, so this does
not change the Deployment, volumes, or in-cluster service certificate flow.
Related issues
None.
Type of Change
Breaking Changes
Testing
Validated with:
helm unittest --strict -f 'tests/admin_root_cert_test.yaml' helm/charts/nico-api(14/14 tests pass)
helm lint helm/charts/nico-apihelm lint helmhelm template nico helm --namespace nico-system--set-fileand parsing the resultingConfigMap entry with OpenSSL
value types, disabled site config, and unsupported file paths
Additional Notes
Adding a CA expands the TLS client trust store. A client certificate accepted
through that store is represented as
TrustedCertificateand passes thechart's default Casbin
forge/*rule. With internal RBAC enabled (the default),admin CLI methods still require the certificate to map to
ExternalUser.additionalIssuerCnsis a CN-only classifier across the combined TLS truststore, not a cryptographic binding to this bundle, so deployments should use a
globally unique dedicated issuing-intermediate CN and keep
bypass_rbacdisabled in production.
The Helm validation intentionally checks only the PEM envelope. Operators
remain responsible for validating X.509 CA semantics, chain placement, and
issuance policy before deployment.
A full pre-existing
helm unittest helm/charts/nico-apirun also reports sevenunrelated failures in
certificate_test.yaml: those expectations omit thealready-default
carbide-api.forgeDNS SAN. This PR does not changecertificate generation, and its focused suite passes.