ClientSettingsPolicy does not work on HTTPRoutes with matching conditions

[kate-osborn]

Describe the bug If you apply a policy to a route that has http matches with a method, params, or headers defined, the policy has no affect.

To Reproduce Steps to reproduce the behavior:

1. Deploy edge (118488e9b624f78c58add4f064909d7adb8ad571) version of NGF
2. Apply the files from the advanced routing example.
3. Create the following policy:

   apiVersion: gateway.nginx.org/v1alpha1
   kind: ClientSettingsPolicy
   metadata:
   name: tea
   namespace: default
   spec:
   targetRef:
   group: gateway.networking.k8s.io
   kind: HTTPRoute
   name: tea
   body:
   maxSize: "50"

4. Send the following request to the tea application:

   curl --resolve cafe.example.com:$GW_PORT:$GW_IP http://cafe.example.com:$GW_PORT/tea -X POST --data "this payload is greater than fifty bytes but less than seventy five"

Expected behavior You should receive a 413 entity too large since the request body exceeds the limit

Your environment

• Version of the NGINX Gateway Fabric - edge 118488e9b624f78c58add4f064909d7adb8ad571

Additional context The policy is correctly included in the named location block for the tea routes, but the directives are ignored by nginx. This seems to be a limitation of named locations in nginx.

For example, this config with named locations does not enforce the client max body size:

 server {
        listen 80;

        location / {
          try_files /dev/null @named;
        }

        location @named {
          client_max_body_size 50;
          return 200 "ok two";
        }
    }

however, if you change the named location to an internal location:

   server {
        listen 80;

        location / {
          try_files /dev/null /not-named;
        }

        location /not-named {
          internal;
          client_max_body_size 50;
          return 200 "ok two";
        }
    }

it does work.

[sjberman]

I'll add that I discovered this using the ObservabilityPolicy. Traces were not being sampled on requests going through the named locations (where otel_trace on; was set).

[kate-osborn]

This problem is not limited to named locations, it is an issue with internal redirects.

To reproduce without named locations:

1. Apply files from advanced routing example
2. Create the following policies:

   apiVersion: gateway.nginx.org/v1alpha1
   kind: ClientSettingsPolicy
   metadata:
   name: gateway-client-settings
   spec:
   targetRef:
   group: gateway.networking.k8s.io
   kind: Gateway
   name: cafe
   body:
   maxSize: "25" # sizes without a unit are bytes.
   ---
   apiVersion: gateway.nginx.org/v1alpha1
   kind: ClientSettingsPolicy
   metadata:
   name: tea-client-settings
   spec:
   targetRef:
   group: gateway.networking.k8s.io
   kind: HTTPRoute
   name: tea
   body:
   maxSize: "50" # sizes without a unit are bytes.

Then send the following requests:

curl --resolve cafe.example.com:$GW_PORT:$GW_IP http://cafe.example.com:$GW_PORT/coffee -X POST --data "this payload is small"

Expect: 200 Got: 200

curl --resolve cafe.example.com:$GW_PORT:$GW_IP http://cafe.example.com:$GW_PORT/coffee -X POST --data "this payload is over twenty five bytes"

Expect: 413 Got: 413

 curl --resolve cafe.example.com:$GW_PORT:$GW_IP http://cafe.example.com:$GW_PORT/tea -X POST --data "this payload is over twenty five bytes"

Expect: 200 Got: 413 // This is the issue

curl --resolve cafe.example.com:$GW_PORT:$GW_IP http://cafe.example.com:$GW_PORT/tea -X POST --data "this payload is greater than fifty bytes and less than seventy five"

Expect: 413 Got: 413

[kate-osborn]

Update:

The proposed solution to this bug was to set the directives (client_max_body_size, keepalive_requests, etc) in the external location block to the max value of all the internal location blocks.

This solution works for client_max_body_size, but does not work for keepalive_requests.

Some alternatives discussed:

(1) Change ClientSettingsPolicy from an Inherited Policy Attachment to a Direct Policy Attachment. ClientSettingsPolicy could only be attached to a Gateway. Possible to allow attaching to a Gateway.Listener which would allow the user to more selectively apply client settings to individual server blocks.

Pros:

• doesn't violate gateway API spec rules
• easy to reason about and explain to user
• simpler (no inheritance)
• settings apply to client -> nginx, not nginx -> backend so it makes sense to restrict to server context
• minimal code changes

Cons:

• breaking change for our ClientSettingsPolicy API (not terrible b/c it's v1alpha1)
• will have to deprecate v1alpha1 version
• less flexibility. User can't apply client settings to an HTTPRoute.
• doesn't help us with future policies that we want to apply to HTTPRoutes.

(2) Add a restriction that a URI (like /coffee) can only be defined in one HTTPRoute. This will allow us to define the client settings in the external location blocks instead of the internal location blocks. Since the bug only applies to internal location blocks, this will eliminate the issue.

Pros:

• no changes to ClientSettingsPolicy API
• more flexibility - user can apply client settings to HTTPRoutes or Gateways
• helps us with future policies that we want to apply to HTTPRoutes

Cons:

• violates gateway API spec rules
• could create issues down the line with conformance tests
• probably requires a long-term solution to the initial problem so we can eventually remove this restriction.

[sjberman]

I do like option 2 because it reduces complexity, but I do worry about the Gateway API expectations around it.

Option 1 could work until we get feedback that Route support is desired for this policy.

[kate-osborn]

Closing in favor of: https://github.com/nginxinc/nginx-gateway-fabric/issues/2308