Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ClientSettingsPolicy does not work on HTTPRoutes with matching conditions #2079

Closed
kate-osborn opened this issue Jun 3, 2024 · 5 comments
Closed
Assignees
Labels
bug Something isn't working refined Requirements are refined and the issue is ready to be implemented. size/large Estimated to be completed within two weeks
Milestone

Comments

@kate-osborn
Copy link
Contributor

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 (118488e) 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"
  1. 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 118488e

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.

@kate-osborn kate-osborn added the bug Something isn't working label Jun 3, 2024
@sjberman
Copy link
Collaborator

sjberman commented Jun 3, 2024

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).

@mpstefan mpstefan added this to the v1.3.0 milestone Jun 3, 2024
@sjberman sjberman assigned sjberman and kate-osborn and unassigned sjberman Jun 3, 2024
@sjberman sjberman modified the milestones: v1.3.0, v1.4.0 Jun 6, 2024
@mpstefan mpstefan changed the title Policies do not work in named locations Policies do not work on redirected locations Jun 10, 2024
@mpstefan mpstefan changed the title Policies do not work on redirected locations ClientSettings policies do not work on http routes with matching conditions Jun 10, 2024
@mpstefan mpstefan changed the title ClientSettings policies do not work on http routes with matching conditions ClientSettingsPolicy does not work on HTTPRoutes with matching conditions Jun 10, 2024
@mpstefan mpstefan added refined Requirements are refined and the issue is ready to be implemented. size/large Estimated to be completed within two weeks labels Jun 10, 2024
@kate-osborn
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

sjberman commented Jul 2, 2024

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
Copy link
Contributor Author

Closing in favor of: #2308

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working refined Requirements are refined and the issue is ready to be implemented. size/large Estimated to be completed within two weeks
Projects
None yet
Development

No branches or pull requests

3 participants