feat(stack): surface apiKeys/sis/reval gateway-route enable flags - #1376
feat(stack): surface apiKeys/sis/reval gateway-route enable flags#1376sparve-nv wants to merge 4 commits into
Conversation
The self-managed stack passes the nats/ess/nvcfApi.grpc/grpcWorker route enable flags through from ingress.gatewayApi.routes.* in global.yaml.gotmpl, but not apiKeys/sis/reval -- those emitted only routeAnnotations, so an operator could disable them only by patching global.yaml.gotmpl. Add dig passthroughs for the three enable flags, each defaulting to the chart default (true), so existing installs render byte-identical and operators can disable a route from environments/<env>.yaml (e.g. keep api-keys off the public edge in a split/multi-cluster deployment). No chart change -- the flags already exist. Fixes NVIDIA#1374 Signed-off-by: sparve <sparve@nvidia.com>
|
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 (3)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 WalkthroughWalkthroughThe self-managed stack omits unset ChangesGateway route flags
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change exposes existing gateway-route enable flags while preserving current defaults, so existing deployments remain unchanged; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Out of Scope Changes checkExplanation The gateway-route changes are in scope, but base.yaml also changes NATS server tags and invocation subjects from local to regional values. These changes are not covered by issue Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 1 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…oggles Assert the apiKeys/sis/reval route enable flags thread from environments/<env>.yaml through global.yaml.gotmpl into the gateway-routes chart values: unset renders enabled: true (chart default preserved); set false reaches the chart as false. Wired into the offline `make test` target next to gateway-routes-local-chart.sh. Signed-off-by: sparve <sparve@nvidia.com>
| # the route exactly as before; set it to false to drop the route (for example, | ||
| # to keep api-keys off the public edge in a split/multi-cluster deployment). | ||
| apiKeys: | ||
| enabled: {{ dig "ingress" "gatewayApi" "routes" "apiKeys" "enabled" true .Values }} |
There was a problem hiding this comment.
Can we keep this default owned by the gateway-routes chart instead of re-declaring true here and in base.yaml? The stack adapter should only forward enabled when the operator explicitly supplies an override.
The broader concern is that copying a downstream default into an integration layer creates two sources of truth. For example, if a future gateway-routes release defaults this route off to address a security or compatibility concern, the self-managed stack would silently force it back on. Please omit or comment out the base values, conditionally emit enabled, and adjust the default test to assert that the stack passes no override. The chart tests can remain responsible for the chart default.
There was a problem hiding this comment.
Thanks for suggestion.
Addressed in latest commit (d577db5)
Changes:
- Dropped the true default from both global.yaml.gotmpl and base.yaml. The stack now forwards enabled only when an operator sets it (hasKey guard); otherwise it passes nothing and the gateway-routes chart owns the default.
- Updated the test so the default case asserts no override is passed. Verified with helmfile write-values: unset → chart default stands (existing installs unchanged); enabled: false → forwarded.
…e default Address review feedback: forward the apiKeys/sis/reval route enable flag only when an environment file sets it, instead of re-declaring the chart's true default in the stack. Copying the downstream default into the integration layer created two sources of truth -- if a future gateway-routes release defaulted a route off, the self-managed stack would have silently forced it back on. - global.yaml.gotmpl: emit enabled conditionally (hasKey), no hardcoded true - environments/base.yaml: comment out enabled; document the chart owns it - tests/gateway-routes-enable-wiring.sh: default case now asserts the stack passes no override (enabled absent); override case still asserts false Unset behavior is unchanged: the chart default (true) applies, so existing installs render identically. Signed-off-by: sparve <sparve@nvidia.com>
Summary
Surface the
apiKeys,sis, andrevalgateway-route enable flags in the self-managed stack so they can be toggled fromenvironments/<env>.yaml, instead of only by patchingglobal.yaml.gotmpl. Each flag defaults totrue(the chart default), so existing deployments are unchanged.Additional Details
global.yaml.gotmpl'snvcfGatewayRoutes.routesblock already passes the enable flags through fromingress.gatewayApi.routes.*fornats,ess,nvcfApi.grpc,nvctApi.grpc, andgrpcWorker, but forapiKeysit emitted onlyrouteAnnotationsand did not emitsis/revalat all. Theingressrelease reads onlyglobal.yaml.gotmpl, soingress.gatewayApi.routes.{apiKeys,sis,reval}.enabledset in an environment file never reached the release — the only way to disable those routes was a carried patch toglobal.yaml.gotmpl.The
nvcf-gateway-routeschart already gates all three vianvcfGatewayRoutes.routes.{apiKeys,sis,reval}.enabled(defaulttrue); this change only adds the missing stack passthrough. No chart change.deploy/stacks/self-managed/global.yaml.gotmpl— addenableddigpassthroughs (defaulttrue) forapiKeys/sis/reval, alongside their existingrouteAnnotations.deploy/stacks/self-managed/environments/base.yaml— document the three flags (enabled: true) underingress.gatewayApi.routes.Example — keep api-keys off the public edge and drop the unused sis/reval routes:
For the Reviewer
dig-defaults totrue, matching the chart default, so an unset flag renders identically.api.accountBootstrap.enabled(Allow disabling the NVCF API account-bootstrap post-install hook in the nvcf-api chart #894 / feat(nvcf-api): add api.accountBootstrap.enabled flag to gate account-bootstrap hook #895): add the knob, default to current behavior, let operators opt out.For QA
Automated test (
make test). Addeddeploy/stacks/self-managed/tests/gateway-routes-enable-wiring.sh, wired into the offlinemake testtarget next togateway-routes-local-chart.sh. It follows the existing value-wiring pattern (pdb-value-wiring.sh): it renders theingressrelease values withhelmfile write-valuesand asserts, viayq, thatnvcfGatewayRoutes.routes.{apiKeys,sis,reval}.enabledistrueby default andfalsewhen the environment file setsingress.gatewayApi.routes.{apiKeys,sis,reval}.enabled: false. As a teeth-check, reverting onlyglobal.yaml.gotmpltomain(keeping thebase.yamldocs) makes the test fail (expected enabled=true), confirming it guards the passthrough and thatbase.yamlalone is inert without it.Manual render. Also rendered the
ingressrelease withhelmfile template -l name=ingress(helmfile v1.7.4) against the localnvcf-gateway-routeschart, using a minimal env file that supplies the operator-required gateway names.api-keys,sis, andrevalHTTPRoutes all render, alongsidenvcf-api,nvct-api,invocation-service,llm-api-gateway, and thegrpcTCPRoute. Adiffof this render against the same render onorigin/main(before the change) is empty — byte-identical, confirming no behavior change when the flags are unset.ingress.gatewayApi.routes.{apiKeys,sis,reval}.enabled=false): theapi-keys,sis, andrevalHTTPRoutes are absent; every other route still renders.Issues
Fixes #1374
Checklist
Summary by CodeRabbit
New Features
Tests