Skip to content

fix(helm): validate gateway listener ports - #3179

Open
guanchzhou wants to merge 3 commits into
NVIDIA:mainfrom
guanchzhou:helm-gateway-port-validation/am
Open

guanchzhou wants to merge 3 commits into
NVIDIA:mainfrom
guanchzhou:helm-gateway-port-validation/am

Conversation

@guanchzhou

@guanchzhou guanchzhou commented Sep 4, 2026 •

Copy link
Copy Markdown

Summary

Reject invalid gateway listener port values during Helm rendering, before Kubernetes admission or gateway startup.

Related Issue

No issue required: this is localized chart validation for configurations the gateway already cannot run.

Changes

  • Reject booleans, fractional numbers, and other malformed port values before integer conversion
  • Require the primary and probe-backed health listener ports to be valid TCP ports
  • Allow metrics port 0 as disabled and validate enabled metrics ports
  • Reject collisions between primary, health, and metrics listeners
  • Add focused Helm unit tests for malformed values, invalid ranges, boundaries, and collisions

Testing

  • mise run pre-commit passes
  • Unit tests added/updated
  • E2E tests added/updated (not applicable)
  • helm unittest deploy/helm/openshell (133 tests)
  • helm unittest deploy/helm/openshell-workspace (4 tests)
  • Helm lint passes for defaults and all CI values variants
  • Direct deployment render rejects malformed listener ports

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (not applicable)

@copy-pr-bot

copy-pr-bot Bot commented Sep 4, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions

github-actions Bot commented Sep 4, 2026 •

Copy link
Copy Markdown

All contributors have signed the DCO ✍️ ✅
Posted by the DCO Assistant Lite bot.

@guanchzhou

Copy link
Copy Markdown
Author

I have read the DCO document and I hereby sign the DCO.

@guanchzhou

Copy link
Copy Markdown
Author

recheck

@github-actions

Copy link
Copy Markdown

This pull request has had no activity for 14 days and is now marked stale. It may be closed in 7 days if there is no further activity.

@github-actions github-actions Bot added the state:stale Inactive item at risk of automatic closure. label Sep 19, 2026
@guanchzhou

Copy link
Copy Markdown
Author

not stale

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

service.metricsPort: false still slips through this validation. Helm's default treats false as empty, so it becomes 0 before the type check. Could this distinguish a missing value from an explicit boolean and add a false case?

@guanchzhou

Copy link
Copy Markdown
Author

@sylvesterkaczmarek good catch. Helm's default treats false as empty, so the previous fallback turned service.metricsPort: false into 0 before the integer check.

Fixed in d8cd988. The "0" fallback now applies only when the key is absent; an explicit value is stringified as-is, so false stays false and fails the integer validation. Added a service.metricsPort: false unit case. Helm tests and lint pass on this head.

@guanchzhou
guanchzhou force-pushed the helm-gateway-port-validation/am branch from d8cd988 to 93d0317 Compare September 19, 2026 15:17

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rechecked 93d0317. metricsPort now distinguishes an absent key from an explicit value before string conversion, so false remains "false" and fails the integer check instead of being defaulted to 0. The focused Helm regression covers the exact case I raised. My concern is resolved.

@github-actions github-actions Bot removed the state:stale Inactive item at risk of automatic closure. label Sep 20, 2026
@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown

This pull request has had no activity for 14 days and is now marked stale. It may be closed in 7 days if there is no further activity.

@github-actions github-actions Bot added the state:stale Inactive item at risk of automatic closure. label Oct 4, 2026
@guanchzhou

guanchzhou commented Oct 4, 2026 via email

Copy link
Copy Markdown
Author

@github-actions github-actions Bot removed the state:stale Inactive item at risk of automatic closure. label Oct 5, 2026
Reject disabled required listeners, out-of-range values, and port collisions
before Kubernetes accepts an unusable gateway workload.

Signed-off-by: Andrey Maltsev <maltsev.andrey@gmail.com>
Validate raw listener port values before integer conversion so booleans and fractional numbers cannot bypass range and collision checks.

Signed-off-by: Andrey Maltsev <maltsev.andrey@gmail.com>
Preserve explicitly configured metrics port values during validation so boolean false cannot be converted into the disabled numeric value.

Signed-off-by: Andrey Maltsev <maltsev.andrey@gmail.com>
@guanchzhou
guanchzhou force-pushed the helm-gateway-port-validation/am branch from 93d0317 to 62866fc Compare October 9, 2026 09:11

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants