chore(rest-api): Add ingress based TLS support for REST API#3351
chore(rest-api): Add ingress based TLS support for REST API#3351thossain-nv wants to merge 1 commit into
Conversation
thossain-nv
requested review from
a team,
polarweasel and
shayan1995
as code owners
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}
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
| # ingress-nginx cluster-scoped resources are normally removed by helm uninstall, | ||
| # but remove stragglers so clean.sh can recover from partial installs. | ||
| echo "Removing ingress-nginx cluster-scoped resources..." | ||
| kubectl delete ingressclass nginx --ignore-not-found 2>/dev/null || true | ||
| kubectl get clusterrole,clusterrolebinding -o name \ | ||
| | grep ingress-nginx \ | ||
| | xargs kubectl delete --ignore-not-found 2>/dev/null || true | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Scope teardown to resources owned by this installation.
The setup flow explicitly allows operators to skip ingress-nginx when a controller already exists, but cleanup deletes the global IngressClass nginx, every matching ingress-nginx ClusterRole/ClusterRoleBinding, and the entire ingress-nginx namespace. This can remove another team’s ingress routing, RBAC, and workloads. Require recorded ownership or verify Helm ownership metadata before deleting these resources.
As per path instructions, helm-prereqs/** cleanup must be idempotent and must not remove shared cluster resources.
Also applies to: 205-210
🤖 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-prereqs/clean.sh` around lines 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.
Source: Path instructions
| | jq -r '.items[] | select( | ||
| .spec.storageClassName == "local-path-persistent" and | ||
| (.spec.claimRef.namespace // "" | test("^(nico-system|cert-manager|vault|external-secrets|postgres|metallb-system|nico-rest|temporal)$")) | ||
| (.spec.claimRef.namespace // "" | test("^(nico-system|cert-manager|vault|external-secrets|postgres|ingress-nginx|metallb-system|nico-rest|temporal)$")) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not treat all ingress-nginx PVs as stack-owned.
This filter selects every local-path-persistent PV whose claim namespace is ingress-nginx. A pre-existing or shared namespace can contain unrelated retained volumes, so teardown may delete data outside this stack. Restrict deletion using ownership metadata or known claims, or exclude the namespace unless this script created and owns it.
As per path instructions, prerequisite teardown must avoid deleting resources outside its ownership boundary.
🤖 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-prereqs/clean.sh` at 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.
Source: Path instructions
| if [[ -z "${INGRESS_NGINX_NS:-}" ]]; then | ||
| INGRESS_NGINX_NS=$(kubectl get deployment ingress-nginx-controller -A \ | ||
| -o jsonpath='{.items[0].metadata.namespace}' 2>/dev/null || true) | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Do not select an arbitrary controller namespace.
If multiple namespaces contain ingress-nginx-controller, .items[0] can identify the wrong controller and produce misleading health results. Select the deployment using Helm/release labels or a known namespace, or explicitly report ambiguity when multiple matches exist.
As per path instructions, helm-prereqs/** health operations should provide reliable lifecycle status.
🤖 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-prereqs/health-check.sh` around lines 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.
Source: Path instructions
| # --------------------------------------------------------------------------- | ||
| # ingress-nginx — optional Ingress controller for clusters that do not already | ||
| # provide one. setup.sh installs it only when requested; the controller Service | ||
| # uses type LoadBalancer and receives an address from MetalLB. | ||
| # --------------------------------------------------------------------------- | ||
| - name: ingress-nginx | ||
| namespace: ingress-nginx | ||
| createNamespace: true | ||
| chart: ingress-nginx/ingress-nginx | ||
| version: "4.12.1" | ||
| wait: true | ||
| timeout: 600 | ||
| values: | ||
| - operators/values/ingress-nginx.yaml | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Prevent the optional release from broadening teardown scope.
setup.sh installs this release only when requested, but clean.sh runs helmfile destroy without a selector. A teardown can therefore target an existing ingress-nginx Helm release that this setup did not install. Track installation ownership or guard this release’s destruction explicitly.
As per path instructions, helm-prereqs/** lifecycle operations must be idempotent and must not remove resources outside their ownership boundary.
🤖 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-prereqs/helmfile.yaml` around lines 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.
Source: Path instructions
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
1 participant
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