feat: Add nginx gateway fabric legacy ingress module #508
Conversation
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
…lector mismatch - Remove app.kubernetes.io/name and app.kubernetes.io/instance from common_labels - Add helm_release_name local for dynamic label matching - Update ServiceMonitor selector to use helm_release_name
sanmesh-kakade
requested review from
anshulsao,
ishaankalra,
pramodh-ayyappan,
rr0hit,
unni-facets and
vishnukv-facets
as code owners
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Gateway API CRD intent and output facet files, a Kubernetes module that installs Gateway API CRDs via a Job (with ServiceAccount, ClusterRole, ClusterRoleBinding), and a legacy NGINX Gateway Fabric ingress module (Helm deployment, bootstrap TLS/cert-manager flows, Gateway API resource generation, DNS records, and many outputs). Changes
Sequence Diagram(s)sequenceDiagram
participant Config as Configuration
participant K8s as Kubernetes API
participant Job as Installer Job
participant CRD as CRD Source
Config->>K8s: Create ServiceAccount, ClusterRole, ClusterRoleBinding
Config->>K8s: Create Job manifest (installer)
K8s->>Job: Schedule installer pod
Job->>Job: Resolve channel/version -> install_url
Job->>CRD: Download manifests from install_url
Job->>K8s: kubectl apply --server-side CRDs
K8s->>K8s: Register CustomResourceDefinitions
K8s->>Config: Return Job metadata (name/namespace)
sequenceDiagram
participant User as Configuration
participant Helm as Helm Controller
participant K8s as Kubernetes API
participant CertMgr as cert-manager
participant LB as Cloud Load Balancer
participant DNS as Route53
User->>K8s: Create bootstrap TLS secrets (self-signed)
User->>Helm: Deploy nginx_gateway_fabric chart (helm_release)
Helm->>K8s: Create Deployments, Services, Gateway resources
K8s->>LB: Provision/load-balance gateway Service
LB->>K8s: Provide hostname/IP
User->>CertMgr: Request Certificate (HTTP-01/DNS-01)
CertMgr->>K8s: Issue and store TLS secrets
User->>K8s: Create HTTPRoute/GRPCRoute and ReferenceGrants
K8s->>K8s: Configure Gateway API resources (routes, grants)
User->>DNS: Create base/wildcard DNS records
DNS->>User: DNS records available
K8s->>User: Expose outputs (domains, LB, tls_secret)
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 5
🤖 Fix all issues with AI agents
In `@modules/gateway_api_crd/k8s/1.0/main.tf`:
- Around line 94-101: The kubectl container currently uses a non-deterministic
image tag ("bitnami/kubectl:latest"); update the container block for the kubectl
container (container { name = "kubectl" }) to pin the image to a specific,
tested version (e.g., replace "bitnami/kubectl:latest" with a fixed tag like
"bitnami/kubectl:<X.Y.Z>") so builds are reproducible and secure; pick a stable
tag you’ve validated, update the image attribute in main.tf, and optionally add
a brief comment noting the chosen version and rationale.
In `@modules/ingress/nginx_gateway_fabric_legacy/1.0/facets.yaml`:
- Around line 615-621: The backend_request field has a mismatched placeholder
and default (default: 300s vs x-ui-placeholder: '30s'); update the
x-ui-placeholder to reflect the actual default or change the default to match
the placeholder. Locate the backend_request property in facets.yaml and either
set x-ui-placeholder: '300s' to match default: 300s or change default to a
realistic value like 30s if that was intended, ensuring the pattern ^\d+[smh]$
still validates the value.
In `@modules/ingress/nginx_gateway_fabric_legacy/1.0/main.tf`:
- Around line 871-893: The current config block sets
rewriteClientIP.trustedAddresses to CIDR "0.0.0.0/0" (inside the conditional
where local.cloud_provider == "AWS"), which is overly permissive; remove or
replace the trustedAddresses CIDR entry so the rewriteClientIP configuration
does not accept all IPs (e.g., remove the trustedAddresses array or leave it
empty) and instead enforce access via network-layer controls by restricting the
target security group to only allow inbound from the NLB security group; update
the conditional around rewriteClientIP in the config merge (the rewriteClientIP
object) to no longer include the 0.0.0.0/0 trustedAddresses entry.
In `@modules/ingress/nginx_gateway_fabric_legacy/1.0/README.md`:
- Around line 602-613: The README incorrectly states that the `domains` output
is a Map while the implementation in outputs.tf defines output "domains" by
concatenating arrays (using concat) and thus returns a list; update the README
entry for `domains` to say "List of all configured domains" (or the exact list
type used) so it matches the outputs.tf implementation, referencing the output
named "domains" and the use of concat in outputs.tf to locate the code to
reconcile.
- Around line 441-472: The example places "domains" at the top level but the
code reads lookup(var.instance.spec, "domains", {}), so move the entire
"domains" object inside "spec" in the README JSON example; update the sample so
"spec" contains "domains", "private", "disable_base_domain",
"force_ssl_redirection", and "rules" (preserving the existing domain entries and
rule structure) so the documented schema matches the implementation.
🧹 Nitpick comments (3)
modules/ingress/nginx_gateway_fabric_legacy/1.0/main.tf (1)
15-15: Edge case in domain length calculation.The condition
length(var.environment.unique_name) + length(var.instance_name) + length(var.cc_metadata.tenant_base_domain) >= 60doesn't account for the separator characters (-and.) in the actual domain string"${var.instance_name}-${var.environment.unique_name}". Consider including the separator lengths for accurate boundary checking.💡 Suggested fix
- instance_env_name = length(var.environment.unique_name) + length(var.instance_name) + length(var.cc_metadata.tenant_base_domain) >= 60 ? substr(md5("${var.instance_name}-${var.environment.unique_name}"), 0, 20) : "${var.instance_name}-${var.environment.unique_name}" + instance_env_name = length(var.environment.unique_name) + length(var.instance_name) + length(var.cc_metadata.tenant_base_domain) + 2 >= 60 ? substr(md5("${var.instance_name}-${var.environment.unique_name}"), 0, 20) : "${var.instance_name}-${var.environment.unique_name}"modules/gateway_api_crd/k8s/1.0/main.tf (1)
98-101: Consider supply chain security for remote YAML application.Applying manifests directly from a remote URL at runtime (
kubectl apply -f ${local.install_url}) introduces supply chain risk. If the upstream release is compromised or the URL is tampered with, malicious CRDs could be installed.Consider:
- Verifying the release checksum/signature before applying
- Bundling the CRD manifests within the module for audited deployments
modules/gateway_api_crd/k8s/1.0/variables.tf (1)
20-23: Consider removing or documenting the default instance_name.The default value
"test_instance"appears to be a placeholder. In production, if this default is inadvertently used, it could lead to naming collisions or confusion. Consider either removing the default to make it required, or documenting when this default is appropriate.
sanmesh-kakade
force-pushed
the
nginx-gateway-fabric-legacy-to-master
branch
from
a37fd35 to
f1d10ff
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@modules/ingress/nginx_gateway_fabric_legacy/1.0/main.tf`:
- Around line 1138-1169: The Route53 records fail when the LB hostname/IP is not
yet available because local.lb_record_value can be empty; update both
aws_route53_record resources (aws_route53_record.cluster-base-domain and
aws_route53_record.cluster-base-domain-wildcard) to only create when
local.lb_record_value is non-empty by adding that check to the count condition
(e.g., require local.tenant_provider == "aws" && !lookup(var.instance.spec,
"disable_base_domain", false) && length(local.lb_record_value) > 0). Ensure the
depends_on stays the same so data.kubernetes_service.gateway_lb is waited on,
and keep lifecycle.prevent_destroy unchanged.
- Around line 484-503: The ServiceMonitor selector in servicemonitor_resources
is using local.helm_release_name for both labels, but the chart's name label
should be the literal chart name; update the matchLabels in the ServiceMonitor
spec (the block under servicemonitor_resources ->
"servicemonitor-${local.name}") so that "app.kubernetes.io/name" is set to the
literal "nginx-gateway-fabric" and keep "app.kubernetes.io/instance" as
local.helm_release_name, ensuring the ServiceMonitor selects the actual
chart-managed Service.
- Around line 518-533: The ReferenceGrant generation in referencegrant_resources
only authorizes HTTPRoute in spec.from; update the spec.from entries for the
ReferenceGrant (map key "referencegrant-${ns}", metadata.name
"${local.name}-allow-routes") to include an additional entry with group
"gateway.networking.k8s.io" and kind "GRPCRoute" (same namespace
var.environment.namespace) so GRPCRoute can reference cross-namespace Services
similar to HTTPRoute; ensure the new entry mirrors the existing HTTPRoute from
block and is added for every namespace in local.cross_namespace_backends.
…tput type (#509) - Move domains configuration inside spec in documentation examples - Fix output description: domains returns a list, not a map Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
- Pin kubectl image to 1.31.4 instead of latest - Fix ServiceMonitor selector to use literal "nginx-gateway-fabric" for app.kubernetes.io/name - Add GRPCRoute to ReferenceGrant for cross-namespace support - Fix backend_request placeholder to match default (300s) - YAML formatting fixes in facets.yaml Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@modules/ingress/nginx_gateway_fabric_legacy/1.0/facets.yaml`:
- Around line 54-61: The domain schema's regex (property "domain" in
facets.yaml) currently limits TLDs to {2,6}; update the pattern to allow modern
gTLDs by changing the final TLD quantifier to {2,63} (or remove the upper bound)
so labels like ".technology" or other long TLDs are accepted; adjust the pattern
on the "domain" field (the line with pattern: ^...\\.[A-Za-z]{2,6}$) and ensure
the x-ui-error-message still matches the new allowance.
In `@modules/ingress/nginx_gateway_fabric_legacy/1.0/main.tf`:
- Around line 179-200: The Access-Control-Allow-Origin value currently builds a
comma-separated list (in cors_headers under local.rulesFiltered) which is
invalid; replace the join(...) expression for "Access-Control-Allow-Origin" so
it does not emit a CSV: emit "*" only when allow_origins is empty AND
allow_credentials is false, otherwise emit a single origin token you can
evaluate at request-time (e.g. the nginx variable "$http_origin") or the first
configured origin if you prefer server-side selection; update the expression
that references lookup(lookup(v, "cors", {}), "allow_origins", {}) and
lookup(lookup(v, "cors", {}), "allow_credentials", false) to implement this
conditional output instead of join.
- Around line 481-501: The helm release name is computed without the
"-nginx-fabric" suffix causing selector and service lookups to miss the actual
release; update the computation of local.helm_release_name to include the suffix
(e.g., compute a new local like local.helm_release_name_with_suffix or replace
local.helm_release_name with "${local.name}-nginx-fabric") and then use that
value consistently in the ServiceMonitor selector (servicemonitor_resources ->
spec.selector.matchLabels["app.kubernetes.io/instance"]) and in any
kubernetes_service lookups or other places that assume the release name
(references around servicemonitor_resources and the kubernetes_service usage
mentioned); ensure the same symbol is reused everywhere to align with the
chart’s .Release.Name.
🧹 Nitpick comments (1)
modules/ingress/nginx_gateway_fabric_legacy/1.0/facets.yaml (1)
291-316: Enforcepathfor non‑gRPC routes to avoid silent drops.
The description sayspathis required for HTTP routes, but the schema doesn’t enforce it; missing paths are filtered out later in the module. If the facets schema supports conditional validation, consider enforcing it here.🛠️ Possible schema guard (if supported)
type: object properties: disable: type: boolean @@ grpc_config: type: object title: gRPC Configuration description: Configure gRPC routing x-ui-toggle: true properties: enabled: type: boolean title: Enable gRPC description: Enable gRPC routing for this service default: false + allOf: + - if: + properties: + grpc_config: + properties: + enabled: + const: true + then: {} + else: + required: + - path
Comment on lines
+54
to
+61
| domain: | ||
| type: string | ||
| title: Domain | ||
| description: Host name of the domain | ||
| pattern: ^[A-Za-z0-9]([A-Za-z0-9-]{0,61}[A-Za-z0-9])?(\.[A-Za-z0-9]([A-Za-z0-9-]{0,61}[A-Za-z0-9])?)*\.[A-Za-z]{2,6}$ | ||
| x-ui-unique: true | ||
| x-ui-placeholder: "Domain to map ingress. Eg: example.com, sub.example.com, my-domain.co.uk" | ||
| x-ui-error-message: "Value doesn't match the format. Eg: example.com, my-domain.co.uk" |
Contributor
There was a problem hiding this comment.
Domain regex rejects valid modern TLDs.
The {2,6} TLD limit will block legitimate domains (e.g., longer gTLDs). Consider widening to {2,63} (max label length) or removing the upper bound.
🔧 Suggested update
- pattern: ^[A-Za-z0-9]([A-Za-z0-9-]{0,61}[A-Za-z0-9])?(\.[A-Za-z0-9]([A-Za-z0-9-]{0,61}[A-Za-z0-9])?)*\.[A-Za-z]{2,6}$
+ pattern: ^[A-Za-z0-9]([A-Za-z0-9-]{0,61}[A-Za-z0-9])?(\.[A-Za-z0-9]([A-Za-z0-9-]{0,61}[A-Za-z0-9])?)*\.[A-Za-z]{2,63}$📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| domain: | |
| type: string | |
| title: Domain | |
| description: Host name of the domain | |
| pattern: ^[A-Za-z0-9]([A-Za-z0-9-]{0,61}[A-Za-z0-9])?(\.[A-Za-z0-9]([A-Za-z0-9-]{0,61}[A-Za-z0-9])?)*\.[A-Za-z]{2,6}$ | |
| x-ui-unique: true | |
| x-ui-placeholder: "Domain to map ingress. Eg: example.com, sub.example.com, my-domain.co.uk" | |
| x-ui-error-message: "Value doesn't match the format. Eg: example.com, my-domain.co.uk" | |
| domain: | |
| type: string | |
| title: Domain | |
| description: Host name of the domain | |
| pattern: ^[A-Za-z0-9]([A-Za-z0-9-]{0,61}[A-Za-z0-9])?(\.[A-Za-z0-9]([A-Za-z0-9-]{0,61}[A-Za-z0-9])?)*\.[A-Za-z]{2,63}$ | |
| x-ui-unique: true | |
| x-ui-placeholder: "Domain to map ingress. Eg: example.com, sub.example.com, my-domain.co.uk" | |
| x-ui-error-message: "Value doesn't match the format. Eg: example.com, my-domain.co.uk" |
🤖 Prompt for AI Agents
In `@modules/ingress/nginx_gateway_fabric_legacy/1.0/facets.yaml` around lines 54
- 61, The domain schema's regex (property "domain" in facets.yaml) currently
limits TLDs to {2,6}; update the pattern to allow modern gTLDs by changing the
final TLD quantifier to {2,63} (or remove the upper bound) so labels like
".technology" or other long TLDs are accepted; adjust the pattern on the
"domain" field (the line with pattern: ^...\\.[A-Za-z]{2,6}$) and ensure the
x-ui-error-message still matches the new allowance.
- Add prometheus_details optional input - Use PodMonitor instead of ServiceMonitor (services don't expose metrics port) - PodMonitor scrapes both control plane and data plane pods - Use helm_release_id from prometheus for the release label Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@modules/ingress/nginx_gateway_fabric_legacy/1.0/facets.yaml`:
- Around line 297-304: The schema for the "path" field currently allows an empty
string because the pattern permits an entirely absent value; update the "path"
property (the YAML block named path) to reject empties by either adding
"minLength: 1" or tightening the regex (e.g., require at least "/" as the value)
and update the x-ui-error-message accordingly so the validator enforces that a
non-empty path (at least "/") is supplied.
- Around line 639-659: The allow_origins schema currently models a map
(patternProperties) which permits multiple entries; change it to enforce a
single origin (or `*`) by either replacing the object/map with a single string
property (e.g., type: string, title: Allowed Origin) or keep the object but add
a maxProperties: 1 constraint and update the UI/placeholder accordingly; update
references such as spec.rules.{{this}}.cors.enabled and the Origin property
description to indicate only one origin or `*` is allowed so the generated
Access-Control-Allow-Origin header remains valid.
Comment on lines
+297
to
+304
| path: | ||
| type: string | ||
| title: Path | ||
| description: Path of the application (required for HTTP routes) | ||
| pattern: ^(/[^/]+)*(/)?$ | ||
| x-ui-placeholder: Enter path (e.g., / or /api) | ||
| x-ui-error-message: "Value doesn't match pattern, eg: / or /api" | ||
| x-ui-visible-if: |
Contributor
There was a problem hiding this comment.
Reject empty paths in validation.
The current regex allows an empty string, which can slip through when users clear the field. Add a minimum length (or adjust the pattern) to ensure at least / is provided.
🩹 Proposed fix
path:
type: string
title: Path
description: Path of the application (required for HTTP routes)
pattern: ^(/[^/]+)*(/)?$
+ minLength: 1
x-ui-placeholder: Enter path (e.g., / or /api)
x-ui-error-message: "Value doesn't match pattern, eg: / or /api"🤖 Prompt for AI Agents
In `@modules/ingress/nginx_gateway_fabric_legacy/1.0/facets.yaml` around lines 297
- 304, The schema for the "path" field currently allows an empty string because
the pattern permits an entirely absent value; update the "path" property (the
YAML block named path) to reject empties by either adding "minLength: 1" or
tightening the regex (e.g., require at least "/" as the value) and update the
x-ui-error-message accordingly so the validator enforces that a non-empty path
(at least "/") is supplied.
Comment on lines
+639
to
+659
| allow_origins: | ||
| type: object | ||
| title: Allowed Origins | ||
| description: List of allowed origins (* for all) | ||
| x-ui-visible-if: | ||
| field: spec.rules.{{this}}.cors.enabled | ||
| values: | ||
| - true | ||
| patternProperties: | ||
| ^[a-zA-Z0-9_.-]*$: | ||
| type: object | ||
| title: Origin | ||
| properties: | ||
| origin: | ||
| type: string | ||
| title: Origin URL | ||
| description: Allowed origin URL | ||
| x-ui-placeholder: https://example.com | ||
| required: | ||
| - origin | ||
| allow_methods: |
Contributor
There was a problem hiding this comment.
Restrict CORS allow-origins to a single entry or *.
allow_origins is modeled as a map, which encourages multiple origins; if these are joined into Access-Control-Allow-Origin, the header becomes invalid. Consider enforcing a single entry (or switching to a single string).
🛠️ Proposed fix (enforce a single origin entry)
allow_origins:
type: object
title: Allowed Origins
- description: List of allowed origins (* for all)
+ description: Single allowed origin or "*" for all
+ maxProperties: 1
x-ui-visible-if:
field: spec.rules.{{this}}.cors.enabled
values:
- trueBased on learnings, the Access-Control-Allow-Origin header in this module is designed to accept a single origin or *, not a comma-separated list.
🤖 Prompt for AI Agents
In `@modules/ingress/nginx_gateway_fabric_legacy/1.0/facets.yaml` around lines 639
- 659, The allow_origins schema currently models a map (patternProperties) which
permits multiple entries; change it to enforce a single origin (or `*`) by
either replacing the object/map with a single string property (e.g., type:
string, title: Allowed Origin) or keep the object but add a maxProperties: 1
constraint and update the UI/placeholder accordingly; update references such as
spec.rules.{{this}}.cors.enabled and the Origin property description to indicate
only one origin or `*` is allowed so the generated Access-Control-Allow-Origin
header remains valid.
unni-facets
approved these changes
Feb 9, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Summary
Changes
gateway_api_crdintent and moduleingress/nginx_gateway_fabric_legacy/1.0module🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation