chore(rest-api): Add ingress based TLS support for REST API#3351
Open
thossain-nv wants to merge 3 commits into
Open
chore(rest-api): Add ingress based TLS support for REST API#3351thossain-nv wants to merge 3 commits into
thossain-nv wants to merge 3 commits into
Conversation
thossain-nv
requested review from
a team,
polarweasel and
shayan1995
as code owners
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughAdds optional ingress-nginx installation and lifecycle management, plus configurable nico-rest-api Ingress and cert-manager TLS resources with Helm tests and setup documentation. ChangesIngress TLS exposure
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant setup.sh
participant helmfile
participant MetalLB
participant ingress-nginx-controller
participant nico-rest-api-Service
Operator->>setup.sh: Enable ingress-nginx installation
setup.sh->>helmfile: Sync ingress-nginx release
helmfile->>ingress-nginx-controller: Create controller and LoadBalancer Service
ingress-nginx-controller->>MetalLB: Request LoadBalancer address
ingress-nginx-controller->>nico-rest-api-Service: Route configured Ingress traffic
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3351.docs.buildwithfern.com/infra-controller |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-07-10 01:48:51 UTC | Commit: b131a4b |
![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: 4
🤖 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-prereqs/clean.sh`:
- Line 245: Update the PV filtering logic in the cleanup script so it does not
automatically select every PV claimed in the ingress-nginx namespace; instead,
restrict deletion to explicitly stack-owned resources using ownership metadata
or known claim names, or remove ingress-nginx from the namespace allowlist
unless the script can verify it created and owns the namespace.
- Around line 157-164: Update the ingress-nginx cleanup logic in clean.sh to
delete resources only when this installation recorded ownership or the resources
have matching Helm ownership metadata; never unconditionally delete the global
nginx IngressClass, ingress-nginx ClusterRoles/ClusterRoleBindings, or
namespace. Preserve idempotency and safely skip shared or externally managed
resources, including the corresponding cleanup block around the namespace
deletion.
In `@helm-prereqs/health-check.sh`:
- Around line 109-112: Update the namespace discovery logic around
INGRESS_NGINX_NS to avoid selecting kubectl’s arbitrary .items[0]. Identify the
intended ingress-nginx-controller using Helm/release labels or a configured
namespace, and explicitly fail or report ambiguity when multiple matching
deployments remain; ensure the health-check lifecycle status is reliable.
In `@helm-prereqs/helmfile.yaml`:
- Around line 60-74: Ensure the optional ingress-nginx release cannot be
destroyed unless this setup explicitly installed it: update the helmfile and
related setup/cleanup logic around the ingress-nginx release so installation
ownership is tracked and clean.sh’s teardown is limited to owned releases, while
preserving idempotent lifecycle behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b618f86f-45cd-4c71-b3f9-1becb2b4b0cb
📒 Files selected for processing (13)
docs/getting-started/installation-options/reference-install.mddocs/getting-started/quick-start.mdhelm-prereqs/README.mdhelm-prereqs/clean.shhelm-prereqs/health-check.shhelm-prereqs/helmfile.yamlhelm-prereqs/operators/values/ingress-nginx.yamlhelm-prereqs/setup.shhelm-prereqs/values/nico-rest.yamlhelm/rest/nico-rest/charts/nico-rest-api/templates/ingress-certificate.yamlhelm/rest/nico-rest/charts/nico-rest-api/templates/ingress.yamlhelm/rest/nico-rest/charts/nico-rest-api/tests/ingress_test.yamlhelm/rest/nico-rest/charts/nico-rest-api/values.yaml
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
shayan1995
approved these changes
Jul 10, 2026
shayan1995
left a comment
shayan1995
left a comment
Contributor
There was a problem hiding this comment.
generally looks good - there is a small issue with the healthcheck script.
| -o jsonpath='{.items[0].metadata.namespace}' 2>/dev/null || printf 'metallb-system') | ||
| fi | ||
| if [[ -z "${INGRESS_NGINX_NS:-}" ]]; then | ||
| INGRESS_NGINX_NS=$(kubectl get deployment ingress-nginx-controller -A \ |
Contributor
There was a problem hiding this comment.
@thossain-nv This autodetect can never succeed I've tried kubectl get deployment <name> -A to confirm and it is rejected on client-side with "a resource cannot be retrieved by name across all namespaces", so INGRESS_NGINX_NS is always empty and the health check always reports "not installed" even when ingress-nginx is deployed.
Contributor
Author
There was a problem hiding this comment.
Thanks for detecting this issue @shayan1995, I'll add a fix.
thossain-nv
force-pushed
the
chore/rest-api-tls
branch
from
b131a4b to
14ace92
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
helm/rest/nico-rest/charts/nico-rest-api/templates/ingress.yaml (1)
24-38: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGuard against empty
ruleswheningress.hostsis empty.If
ingress.hostsis empty,rules:renders with no list items — Kubernetes'networking.k8s.io/v1Ingress schema requiresrules(when using host-based rules) to be a populated array, so this would fail server-side validation with an unhelpful error rather than a clear chart-level failure.🛡️ Proposed defensive guard
+ {{- if not .Values.ingress.hosts }} + {{- fail "ingress.hosts must contain at least one entry when ingress.enabled is true" }} + {{- end }} rules: {{- range .Values.ingress.hosts }}🤖 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/rest/nico-rest/charts/nico-rest-api/templates/ingress.yaml` around lines 24 - 38, Guard the Ingress rules block around the hosts iteration so an empty .Values.ingress.hosts cannot render rules: without entries. Update the template containing the range over .Values.ingress.hosts to either fail explicitly with a clear Helm error or omit the rules field when no hosts are configured, while preserving the existing host and path rendering for populated values.helm/rest/nico-rest/charts/nico-rest-api/templates/ingress-certificate.yaml (1)
30-37: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGuard against an empty
dnsNameslist.If
ingress.certificate.dnsNamesis unset andingress.hostsis empty, this block rendersdnsNames:with no items underneath — an invalid value for a required, non-empty list in the cert-managerCertificateCRD schema, causing the apply to fail.🛡️ Proposed defensive guard
dnsNames: {{- if .Values.ingress.certificate.dnsNames }} {{- toYaml .Values.ingress.certificate.dnsNames | nindent 4 }} - {{- else }} + {{- else if .Values.ingress.hosts }} {{- range .Values.ingress.hosts }} - {{ .host }} {{- end }} + {{- else }} + {{- fail "ingress.certificate.dnsNames or ingress.hosts must be set when ingress.certificate.enabled is true" }} {{- end }}🤖 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/rest/nico-rest/charts/nico-rest-api/templates/ingress-certificate.yaml` around lines 30 - 37, Guard the dnsNames rendering in the Certificate template so it is emitted only when either ingress.certificate.dnsNames or ingress.hosts contains entries; otherwise fail template rendering with a clear validation error (or provide a documented valid fallback), preventing an empty required list. Update the conditional around the dnsNames block and preserve the existing explicit-list and host-derived behavior.
🤖 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/rest/nico-rest/charts/nico-rest-api/templates/ingress-certificate.yaml`:
- Around line 30-37: Guard the dnsNames rendering in the Certificate template so
it is emitted only when either ingress.certificate.dnsNames or ingress.hosts
contains entries; otherwise fail template rendering with a clear validation
error (or provide a documented valid fallback), preventing an empty required
list. Update the conditional around the dnsNames block and preserve the existing
explicit-list and host-derived behavior.
In `@helm/rest/nico-rest/charts/nico-rest-api/templates/ingress.yaml`:
- Around line 24-38: Guard the Ingress rules block around the hosts iteration so
an empty .Values.ingress.hosts cannot render rules: without entries. Update the
template containing the range over .Values.ingress.hosts to either fail
explicitly with a clear Helm error or omit the rules field when no hosts are
configured, while preserving the existing host and path rendering for populated
values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c77c02b1-cbd7-4b43-a3fe-0700340e8bfa
📒 Files selected for processing (13)
docs/getting-started/installation-options/reference-install.mddocs/getting-started/quick-start.mdhelm-prereqs/README.mdhelm-prereqs/clean.shhelm-prereqs/health-check.shhelm-prereqs/helmfile.yamlhelm-prereqs/operators/values/ingress-nginx.yamlhelm-prereqs/setup.shhelm-prereqs/values/nico-rest.yamlhelm/rest/nico-rest/charts/nico-rest-api/templates/ingress-certificate.yamlhelm/rest/nico-rest/charts/nico-rest-api/templates/ingress.yamlhelm/rest/nico-rest/charts/nico-rest-api/tests/ingress_test.yamlhelm/rest/nico-rest/charts/nico-rest-api/values.yaml
✅ Files skipped from review due to trivial changes (5)
- helm-prereqs/operators/values/ingress-nginx.yaml
- helm-prereqs/values/nico-rest.yaml
- docs/getting-started/quick-start.md
- helm-prereqs/README.md
- docs/getting-started/installation-options/reference-install.md
🚧 Files skipped from review as they are similar to previous changes (6)
- helm/rest/nico-rest/charts/nico-rest-api/values.yaml
- helm-prereqs/health-check.sh
- helm/rest/nico-rest/charts/nico-rest-api/tests/ingress_test.yaml
- helm-prereqs/clean.sh
- helm-prereqs/setup.sh
- helm-prereqs/helmfile.yaml
Collaborator
|
AFAIK there's already an envoy-based ingress controller that was part of the site controller deployments. Is that still there in the helm prerequisites @shayan1995 ? Should we use that instead? |
Contributor
Author
|
@ajf Contour/Envoy is mentioned in the pre-requisite in docs/getting-started/prerequisites/software.md but Helm prereqs doesn't install either. Another place that references Envoy is dev/envoy.yaml Other pre-requisites e.g. MetalLB/cert-manager are installed by Helm. Are existing Helm installed NICo using something else? Installing the nginx ingress controller is optional but I can switch to contour/envoy instead and make it mandatory. |
hwadekar-nv
reviewed
Jul 10, 2026
| @@ -114,6 +123,7 @@ printf " %-26s %s\n" "postgres namespace:" "${POSTGRES_NS}" | |||
| printf " %-26s %s\n" "cert-manager ns:" "${CERT_MANAGER_NS}" | |||
| printf " %-26s %s\n" "external-secrets ns:" "${ESO_NS}" | |||
| printf " %-26s %s\n" "metallb ns:" "${METALLB_NS}" | |||
Contributor
There was a problem hiding this comment.
Do we need to METALLB_NS needed here?
hwadekar-nv
approved these changes
Jul 10, 2026
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
4 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.
Currently we don't have a way to configure REST API to support TLS. Following K8s best practices, this PR introduces:
Related issues
#2587
Type of Change
Breaking Changes
Testing