feat(ingress): expose helm_release_name_override on legacy nginx_gateway_fabric (#550)#552
Merged
Merged
Conversation
…way_fabric (#550) * feat(ingress): expose helm_release_name_override on legacy nginx_gateway_fabric Add a `helm_release_name_override` advanced spec field to the legacy AWS, Azure, and GCP nginx_gateway_fabric modules so users can pin a stable Helm release name. The utility module reads this field directly from var.instance.spec; no main.tf passthrough needed. Empty/unset preserves the existing truncated-name behavior, so existing deployments see no state diff. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(ingress): validate helm_release_name_override input in form Add JSON-schema pattern (DNS-1123 label) and maxLength: 34 on the helm_release_name_override field across legacy AWS/Azure/GCP modules. Includes x-ui-error-message and placeholder so the UI surfaces a clear error before submit. Pairs with the TF-side validation in facets-utility-modules#36. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(ingress): expose proxy timeout overrides on legacy fabric flavors Add proxy_{connect,read,send}_timeout (default 60s) at the instance level and per-rule nginx_timeouts overrides on all three nginx_gateway_fabric_legacy_* flavors (aws, azure, gcp). Lower body_size default from 150m to 1m to match the utility module change. Required for workloads migrating off legacy nginx_ingress_controller, which exposed these as per-ingress annotations. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(ingress/tools_migration): add convert_nginx_ingress script + MIGRATION.md Tooling for converting a legacy `nginx_ingress_controller` blueprint JSON into the production `nginx_gateway_fabric_legacy_{aws,gcp,azure}/1.0` shape. Built around the saas-cp/tools rollout. Key behaviors: - `--cloud` flag picks the wrapper flavor; output always emits version "1.0". - Drops the entire `advanced` block (old nginx-ingress Helm overrides don't port cleanly to NGF's chart) with a warning listing what was lost. - Always injects legacy-parity defaults the new module does not default to: `body_size: 150m`, `proxy_{connect,read,send}_timeout: 300s`. - Hardcodes `helm_release_name_override: "tools-facets"` for this rollout. - For rules whose service name contains "grafana", auto-emits `request_header_modifier.remove.Authorization` so Grafana's anonymous mode isn't shadowed by the basic-auth header from the gateway. - Drops `nginx.ingress.kubernetes.io/backend-protocol: HTTPS` annotations from any rule that carries them and prints a loud per-rule warning — Gateway API has no `proxy_ssl_verify off` equivalent, so the operator must either expose plain HTTP on the backend or set up a BackendTLSPolicy. MIGRATION.md walks through the three options. Lives at modules/ingress/tools_migration/ rather than inside any flavor's versioned directory so it's not packaged into the IaC modules. * docs(ingress/tools_migration): expand k8s-dashboard-new Kong-fix instructions Calls out the specific helm-values change (`kong.proxy.http.enabled: true`) that flips the bundled Kong proxy from TLS-only to TLS+HTTP, plus the corresponding port flip in the converted blueprint. This is the concrete fix the saas-cp/tools rollout actually used; the previous wording was generic. * fix(ingress): align legacy nginx_gateway_fabric wrappers with utility module - Add helm_release_name_override to top-level x-ui-order (was defined but unordered, so hidden/misplaced in the form) — addresses review blocker. - Drop per-rule timeouts and nginx_timeouts schema fields: HTTPRoute timeouts are ignored by NGF (#2164) and nginx_timeouts had no render path. Gateway-wide proxy_*_timeout covers the supported case. - body_size default 1m -> 150m to match utility default, avoid silent 413. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(ingress): remove tools_migration tooling from wrapper PR Out of scope for the wrapper modules per review (sanmesh #4). convert_nginx_ingress.py + MIGRATION.md moved to a standalone location; will land separately if needed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(ingress): re-add per-rule nginx_timeouts to legacy wrappers Per-rule proxy_{connect,read,send}_timeout, now backed by a real SnippetsFilter rendering path in the utility module (capillary parity). Dead Gateway-API rules[].timeouts stays removed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(ingress): remove dead disable_endpoint_validation field Declared in the legacy wrapper schemas but consumed nowhere (0 refs in wrapper or utility main.tf) — a no-op UI toggle. Removed from aws/azure/gcp. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ingress): accept unit-less seconds for proxy timeouts Relax pattern ^\d+[smh]$ -> ^\d+[smh]?$ on instance + per-rule proxy timeout fields. nginx treats a bare number as seconds (e.g. '120' = 120s), so the unit requirement rejected valid input. Still blocks non-numeric garbage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Shashwat Pathak <shashwat.pathak@facets.cloud> (cherry picked from commit 366c39b)
sanmesh-kakade
requested review from
anshulsao,
ishaankalra,
pramodh-ayyappan,
rr0hit,
unni-facets and
vishnukv-facets
as code owners
June 16, 2026 05:29
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThree cloud-provider facet schemas (AWS, Azure, GCP) for the ChangesNGINX Gateway Fabric Legacy Ingress Schema Updates
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 1
🤖 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 `@modules/ingress/nginx_gateway_fabric_legacy_aws/1.0/facets.yaml`:
- Around line 906-913: Remove the `disable_endpoint_validation` field reference
from the README documentation files to align with the schema changes where this
field has been removed from all three provider implementations. In
modules/ingress/nginx_gateway_fabric_legacy_aws/1.0/README.md at line 287,
remove the mention of `disable_endpoint_validation`. In
modules/ingress/nginx_gateway_fabric_legacy_azure/1.0/README.md at line 152,
remove the mention of `disable_endpoint_validation`. In
modules/ingress/nginx_gateway_fabric_legacy_gcp/1.0/README.md at line 152,
remove the mention of `disable_endpoint_validation`. Ensure all three README
files are updated to eliminate the documentation/schema mismatch.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 799734db-c378-4d7d-8cd0-18b4bd9f72ed
📒 Files selected for processing (3)
modules/ingress/nginx_gateway_fabric_legacy_aws/1.0/facets.yamlmodules/ingress/nginx_gateway_fabric_legacy_azure/1.0/facets.yamlmodules/ingress/nginx_gateway_fabric_legacy_gcp/1.0/facets.yaml
Comment on lines
+906
to
+913
| - helm_release_name_override | ||
| - domains | ||
| - force_ssl_redirection | ||
| - basic_auth | ||
| - body_size | ||
| - disable_endpoint_validation | ||
| - proxy_connect_timeout | ||
| - proxy_read_timeout | ||
| - proxy_send_timeout |
Contributor
There was a problem hiding this comment.
Update README files to reflect removal of disable_endpoint_validation across all providers.
The disable_endpoint_validation field has been removed from all three provider schemas, but the corresponding README documentation still references it. This creates a documentation/schema mismatch that will confuse users.
modules/ingress/nginx_gateway_fabric_legacy_aws/1.0/facets.yaml#L906-L913: Removedisable_endpoint_validationfrommodules/ingress/nginx_gateway_fabric_legacy_aws/1.0/README.md(line 287).modules/ingress/nginx_gateway_fabric_legacy_azure/1.0/facets.yaml#L903-L910: Removedisable_endpoint_validationfrommodules/ingress/nginx_gateway_fabric_legacy_azure/1.0/README.md(line 152).modules/ingress/nginx_gateway_fabric_legacy_gcp/1.0/facets.yaml#L903-L910: Removedisable_endpoint_validationfrommodules/ingress/nginx_gateway_fabric_legacy_gcp/1.0/README.md(line 152).
📍 Affects 3 files
modules/ingress/nginx_gateway_fabric_legacy_aws/1.0/facets.yaml#L906-L913(this comment)modules/ingress/nginx_gateway_fabric_legacy_azure/1.0/facets.yaml#L903-L910modules/ingress/nginx_gateway_fabric_legacy_gcp/1.0/facets.yaml#L903-L910
🤖 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 `@modules/ingress/nginx_gateway_fabric_legacy_aws/1.0/facets.yaml` around lines
906 - 913, Remove the `disable_endpoint_validation` field reference from the
README documentation files to align with the schema changes where this field has
been removed from all three provider implementations. In
modules/ingress/nginx_gateway_fabric_legacy_aws/1.0/README.md at line 287,
remove the mention of `disable_endpoint_validation`. In
modules/ingress/nginx_gateway_fabric_legacy_azure/1.0/README.md at line 152,
remove the mention of `disable_endpoint_validation`. In
modules/ingress/nginx_gateway_fabric_legacy_gcp/1.0/README.md at line 152,
remove the mention of `disable_endpoint_validation`. Ensure all three README
files are updated to eliminate the documentation/schema mismatch.
…_fabric wrappers
Form default + placeholder for instance-level proxy_{connect,read,send}_timeout 60s -> 300s across the AWS/Azure/GCP wrappers, matching the utility-module default (facets-utility-modules#36) and restoring parity with the legacy nginx_ingress_controller. The form default is what gets persisted into spec for new instances, so this is what makes 300s the effective default.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
unni-facets
approved these changes
Jun 16, 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
2 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.
Add a
helm_release_name_overrideadvanced spec field to the legacy AWS,Azure, and GCP nginx_gateway_fabric modules so users can pin a stable
Helm release name. The utility module reads this field directly from
var.instance.spec; no main.tf passthrough needed.
Empty/unset preserves the existing truncated-name behavior, so existing
deployments see no state diff.
Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
Add JSON-schema pattern (DNS-1123 label) and maxLength: 34 on the
helm_release_name_override field across legacy AWS/Azure/GCP modules.
Includes x-ui-error-message and placeholder so the UI surfaces a clear
error before submit. Pairs with the TF-side validation in
facets-utility-modules#36.
Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
Add proxy_{connect,read,send}timeout (default 60s) at the instance level and
per-rule nginx_timeouts overrides on all three nginx_gateway_fabric_legacy*
flavors (aws, azure, gcp). Lower body_size default from 150m to 1m to match
the utility module change.
Required for workloads migrating off legacy nginx_ingress_controller, which
exposed these as per-ingress annotations.
Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
Tooling for converting a legacy
nginx_ingress_controllerblueprint JSON intothe production
nginx_gateway_fabric_legacy_{aws,gcp,azure}/1.0shape. Builtaround the saas-cp/tools rollout.
Key behaviors:
--cloudflag picks the wrapper flavor; output always emits version "1.0".advancedblock (old nginx-ingress Helm overrides don'tport cleanly to NGF's chart) with a warning listing what was lost.
body_size: 150m,proxy_{connect,read,send}_timeout: 300s.helm_release_name_override: "tools-facets"for this rollout.request_header_modifier.remove.Authorizationso Grafana's anonymous modeisn't shadowed by the basic-auth header from the gateway.
nginx.ingress.kubernetes.io/backend-protocol: HTTPSannotations fromany rule that carries them and prints a loud per-rule warning — Gateway API
has no
proxy_ssl_verify offequivalent, so the operator must either exposeplain HTTP on the backend or set up a BackendTLSPolicy. MIGRATION.md walks
through the three options.
Lives at modules/ingress/tools_migration/ rather than inside any flavor's
versioned directory so it's not packaged into the IaC modules.
Calls out the specific helm-values change (
kong.proxy.http.enabled: true)that flips the bundled Kong proxy from TLS-only to TLS+HTTP, plus the
corresponding port flip in the converted blueprint. This is the concrete
fix the saas-cp/tools rollout actually used; the previous wording was
generic.
unordered, so hidden/misplaced in the form) — addresses review blocker.
timeouts are ignored by NGF (#2164) and nginx_timeouts had no render path.
Gateway-wide proxy_*_timeout covers the supported case.
Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com
Out of scope for the wrapper modules per review (sanmesh #4). convert_nginx_ingress.py
Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com
Per-rule proxy_{connect,read,send}_timeout, now backed by a real SnippetsFilter
rendering path in the utility module (capillary parity). Dead Gateway-API
rules[].timeouts stays removed.
Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com
Declared in the legacy wrapper schemas but consumed nowhere (0 refs in wrapper or
utility main.tf) — a no-op UI toggle. Removed from aws/azure/gcp.
Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com
Relax pattern ^\d+[smh]$ -> ^\d+[smh]?$ on instance + per-rule proxy timeout
fields. nginx treats a bare number as seconds (e.g. '120' = 120s), so the unit
requirement rejected valid input. Still blocks non-numeric garbage.
Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com
Co-authored-by: Claude Opus 4.7 (1M context) noreply@anthropic.com
Co-authored-by: Shashwat Pathak shashwat.pathak@facets.cloud
(cherry picked from commit 366c39b)
Description
Related issues
Type of change
Checklist
developbranchTesting
Reviewer instructions
Summary by CodeRabbit
Release Notes
New Features
Removed
Notes