feat: ngf http options via snippet filters - #429
Conversation
|
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:
WalkthroughNGINX Gateway configuration now uses per-route ChangesSnippetsFilter route extensions
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HTTPRouteReconciliation
participant GatewayProvider
participant nginxGateway
participant KubernetesAPI
HTTPRouteReconciliation->>GatewayProvider: BuildRouteExtensions(...)
GatewayProvider->>nginxGateway: BuildRouteExtensions(...)
nginxGateway-->>GatewayProvider: SnippetsFilter resource
HTTPRouteReconciliation->>KubernetesAPI: Create or update SnippetsFilter
HTTPRouteReconciliation->>KubernetesAPI: Publish HTTPRoute with ExtensionRef
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cluster/kube/gateway/httproute.go`:
- Around line 113-114: Update the route reconciliation flow around
applyRouteExtensions to apply SnippetsFilter before publishing its ExtensionRef:
for existing routes, pass existing.GetUID() and apply the extension before
updating the HTTPRoute; for new routes, create the route without the filter,
apply the extension, then add the filter reference only after success.
In `@cluster/kube/gateway/nginx_gateway.go`:
- Around line 189-198: Update the NextCases rendering in the raw
location-context SnippetsFilter to validate each value against a closed set of
permitted proxy_next_upstream tokens before appending it to cases. Continue
normalizing valid HTTP status codes to http_ prefixed tokens, but omit or reject
all other values, including semicolons, newlines, and unknown tokens, so only
safe values reach strings.Join and line.
🪄 Autofix
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: Pro
Run ID: dd48570b-d0ae-42c8-99b3-344b7da2cb22
📒 Files selected for processing (6)
_docs/kustomize/akash-operator-hostname/cluster-roles.yaml_docs/kustomize/nginx-gateway-fabric/kustomization.yamlcluster/kube/gateway/gateway.gocluster/kube/gateway/httproute.gocluster/kube/gateway/nginx_gateway.gocluster/kube/gateway/nginx_gateway_test.go
393b3b3 to
3a4f8dd
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
3a4f8dd to
4265e8f
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
cluster/kube/gateway/nginx_gateway.go (2)
147-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winProcess-global viper state leaks into the builders and into their tests.
proxyBufferSizereads a process-global viper key from inside otherwise pure builder functions, so both the builders and the tests that assert "no snippet" depend on ambient global state and on test order.
cluster/kube/gateway/nginx_gateway.go#L147-L152: readproviderflags.FlagProxyBufferSizeonce inNewNginxGateway, store it onnginxGateway, and pass it tohttpOptionsSnippetfromBuildHTTPRouteSpecandBuildRouteExtensions.cluster/kube/gateway/nginx_gateway_test.go#L81-L84: construct the provider with an explicit empty buffer size instead of relying on viper being unset, and apply the same change inTestBuildHTTPRouteSpecExtensionRef.🤖 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 `@cluster/kube/gateway/nginx_gateway.go` around lines 147 - 152, Remove process-global viper access from the builders: have NewNginxGateway read providerflags.FlagProxyBufferSize once, store it on nginxGateway, and pass that value to httpOptionsSnippet from BuildHTTPRouteSpec and BuildRouteExtensions. In cluster/kube/gateway/nginx_gateway_test.go lines 81-84 and TestBuildHTTPRouteSpecExtensionRef, construct the provider with an explicit empty buffer size.
67-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompute the snippet once and derive both the filter reference and the object from it.
BuildHTTPRouteSpecandBuildRouteExtensionsboth callhttpOptionsSnippet(directive, proxyBufferSize())and both branch on the result being empty. The two decisions must agree. If they diverge later, the route publishes anExtensionRefwith no matchingSnippetsFilter, and NGF setsResolvedRefs=Falseand serves HTTP 500 for that rule. Consider computing the snippet once per reconcile and passing it to both paths, or deriving the filter reference from the extension list thatBuildRouteExtensionsreturns.Also applies to: 121-125
🤖 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 `@cluster/kube/gateway/nginx_gateway.go` around lines 67 - 79, Compute the httpOptionsSnippet result once in the shared route-building flow and reuse it for both BuildHTTPRouteSpec and BuildRouteExtensions. Ensure the filter reference is created exactly when the corresponding snippet object is included, so both paths make the same empty/non-empty decision and never publish an unmatched ExtensionRef.cluster/kube/gateway/httproute.go (2)
186-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the extension GVR from the provider instead of pluralizing the kind.
strings.ToLower(gvk.Kind) + "s"works forSnippetsFilter, but it is not a valid pluralization rule for Kubernetes resources in general. A future extension kind such asPolicy,Ingress, orObservabilityPolicymaps to a wrong resource name and the write fails at runtime. LetGatewayProviderreturn theschema.GroupVersionResourcenext to each object, or use aRESTMapper.🤖 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 `@cluster/kube/gateway/httproute.go` around lines 186 - 191, The GVR construction in the extension handling path must not pluralize gvk.Kind manually. Update the GatewayProvider integration to obtain the correct schema.GroupVersionResource for each extension object, or resolve it through a RESTMapper, and use that value when performing the write while preserving the existing group and version context.
86-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDelete a stale extension when the route no longer needs one.
BuildRouteExtensionsreturns nil when the directive carries nohttp_options. On a reconcile that removes all options,extsis empty, the updated route drops itsExtensionRef, and the previously createdSnippetsFilterstays in the namespace until the route is deleted. The leftover object is inert, but it hides the current configuration state. Consider deleting extension objects that the route no longer references.Also applies to: 137-139
🤖 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 `@cluster/kube/gateway/httproute.go` at line 86, Update the reconciliation logic around BuildRouteExtensions and the route update handling to delete previously created SnippetsFilter extension objects that are no longer referenced when exts is nil or empty. Ensure cleanup occurs when http_options are removed, while preserving existing creation and reference behavior for routes that still require extensions.cluster/kube/gateway/httproute_test.go (1)
111-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the new-route failure path.
This test pre-creates the
HTTPRoute, sorouteExistsis true and the bare-create branch never runs. The riskier path is a brand-new route:CreateOrUpdateHTTPRoutecreates the route without filters, then the extension apply fails. Add a case without a pre-existing route that asserts the persisted route carries noExtensionRefand that the observer sees the create outcome.🤖 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 `@cluster/kube/gateway/httproute_test.go` around lines 111 - 137, The existing TestCreateOrUpdateHTTPRouteDoesNotDangleOnExtensionFailure covers only the update path because it pre-creates the HTTPRoute. Add a separate test for a brand-new route that invokes CreateOrUpdateHTTPRoute with no existing route, forces SnippetsFilter creation to fail, and verifies the persisted route has no ExtensionRef while NoopHTTPRouteObserver or the appropriate observer records the route-create outcome.
🤖 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 `@cluster/kube/gateway/httproute.go`:
- Around line 127-139: Update the applyRouteExtensions failure path to notify
httpRouteMetricsObserver via observer.OnCreate only when the bare HTTPRoute was
created in this operation, using createdBare to guard the callback. Preserve the
existing successful-create callback flow and avoid adding an unconditional
notification that would double-count routes after the final update.
In `@cluster/kube/gateway/nginx_gateway_test.go`:
- Around line 81-84: Make the no-snippet assertions deterministic by explicitly
setting providerflags.FlagProxyBufferSize to an empty value through viper before
exercising BuildRouteExtensions and the corresponding
TestBuildHTTPRouteSpecExtensionRef case. Add the required viper and
providerflags imports, while preserving the existing assertions and test
behavior.
---
Nitpick comments:
In `@cluster/kube/gateway/httproute_test.go`:
- Around line 111-137: The existing
TestCreateOrUpdateHTTPRouteDoesNotDangleOnExtensionFailure covers only the
update path because it pre-creates the HTTPRoute. Add a separate test for a
brand-new route that invokes CreateOrUpdateHTTPRoute with no existing route,
forces SnippetsFilter creation to fail, and verifies the persisted route has no
ExtensionRef while NoopHTTPRouteObserver or the appropriate observer records the
route-create outcome.
In `@cluster/kube/gateway/httproute.go`:
- Around line 186-191: The GVR construction in the extension handling path must
not pluralize gvk.Kind manually. Update the GatewayProvider integration to
obtain the correct schema.GroupVersionResource for each extension object, or
resolve it through a RESTMapper, and use that value when performing the write
while preserving the existing group and version context.
- Line 86: Update the reconciliation logic around BuildRouteExtensions and the
route update handling to delete previously created SnippetsFilter extension
objects that are no longer referenced when exts is nil or empty. Ensure cleanup
occurs when http_options are removed, while preserving existing creation and
reference behavior for routes that still require extensions.
In `@cluster/kube/gateway/nginx_gateway.go`:
- Around line 147-152: Remove process-global viper access from the builders:
have NewNginxGateway read providerflags.FlagProxyBufferSize once, store it on
nginxGateway, and pass that value to httpOptionsSnippet from BuildHTTPRouteSpec
and BuildRouteExtensions. In cluster/kube/gateway/nginx_gateway_test.go lines
81-84 and TestBuildHTTPRouteSpecExtensionRef, construct the provider with an
explicit empty buffer size.
- Around line 67-79: Compute the httpOptionsSnippet result once in the shared
route-building flow and reuse it for both BuildHTTPRouteSpec and
BuildRouteExtensions. Ensure the filter reference is created exactly when the
corresponding snippet object is included, so both paths make the same
empty/non-empty decision and never publish an unmatched ExtensionRef.
🪄 Autofix
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: Pro
Run ID: f961c3c1-4561-49d8-b890-2777a27c61b5
📒 Files selected for processing (8)
_docs/kustomize/akash-operator-hostname/cluster-roles.yaml_docs/kustomize/nginx-gateway-fabric/kustomization.yamlcluster/kube/client_gateway_test.gocluster/kube/gateway/gateway.gocluster/kube/gateway/httproute.gocluster/kube/gateway/httproute_test.gocluster/kube/gateway/nginx_gateway.gocluster/kube/gateway/nginx_gateway_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cluster/kube/gateway/gateway.go
- _docs/kustomize/akash-operator-hostname/cluster-roles.yaml
- _docs/kustomize/nginx-gateway-fabric/kustomization.yaml
4265e8f to
221cc8c
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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 `@cluster/kube/client_gateway_test.go`:
- Around line 50-53: Extend the ExtensionRef assertions in the gateway test to
verify its Group field matches the expected SnippetsFilter API group configured
by BuildHTTPRouteSpec, alongside the existing Kind and Name checks.
🪄 Autofix
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: Pro
Run ID: 2440e943-a8e7-4ae7-8791-74096047fed1
📒 Files selected for processing (8)
_docs/kustomize/akash-operator-hostname/cluster-roles.yaml_docs/kustomize/nginx-gateway-fabric/kustomization.yamlcluster/kube/client_gateway_test.gocluster/kube/gateway/gateway.gocluster/kube/gateway/httproute.gocluster/kube/gateway/httproute_test.gocluster/kube/gateway/nginx_gateway.gocluster/kube/gateway/nginx_gateway_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- cluster/kube/gateway/gateway.go
- _docs/kustomize/akash-operator-hostname/cluster-roles.yaml
- cluster/kube/gateway/httproute.go
- cluster/kube/gateway/nginx_gateway_test.go
- _docs/kustomize/nginx-gateway-fabric/kustomization.yaml
- cluster/kube/gateway/nginx_gateway.go
- cluster/kube/gateway/httproute_test.go
cloud-j-luna
changed the title
221cc8c to
60cf273
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
60cf273 to
230d236
Compare
chalabi2
left a comment
chalabi2
left a comment
There was a problem hiding this comment.
One additional blocking proof gap: the green E2E check does not run TestGatewayAPISuite. The integration target explicitly uses -run TestIntegrationTestSuite, and the GitHub Actions log shows that exact command. The new Gateway API test therefore compiles but never executes.
Please add a Gateway/NGF CI job that runs TestGatewayAPISuite and verifies SnippetsFilter Accepted=True, HTTPRoute ResolvedRefs=True, Gateway/data-plane programming, and at least one real HTTP option such as the body-size limit.
…eady Create the placeholder from an empty HTTPRouteSpec (no parentRefs, no routable rules) instead of stripping only the filters. If the SnippetsFilter apply or the acceptance wait fails, the route is never attached to the Gateway, so it cannot serve traffic without its http_options. Adds a new-route regression test asserting the placeholder has no parentRefs and no rules on that failure path.
The value is inserted verbatim into a per-route SnippetsFilter, and NGF marks a structurally valid filter Accepted=True without parsing its nginx contents, so a bad value such as "16kb" would only surface later at nginx -t and block the data plane. Reject anything that is not empty or a valid positive nginx size (16k, 512k, 1m) at startup for both the provider run and hostname operator commands.
2 participants
Providers to create
SnippetFilterfor options on Gateway providersTest evidence