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

client-max-body-size is not always enforced when applied to a virtual server upstream #5859

Open
kate-osborn opened this issue Jun 25, 2024 · 2 comments
Labels
backlog Pull requests/issues that are backlog items refined Issues that are ready to be prioritized

Comments

@kate-osborn
Copy link
Contributor

Describe the bug
The client-max-body-size set on a virtual server upstream is not enforced when the following is true:
(1) the directive is set in an internal location block, and
(2) the value is greater than the default value of 1m for client_max_body_size.

To Reproduce
Steps to reproduce the behavior:

  1. Deploy the ingress controller
  2. Follow the Advanced Routing example
    but add client_max_body_size to the tea-post upstream. Set it to "2m".
apiVersion: k8s.nginx.org/v1
kind: VirtualServer
metadata:
  name: cafe
spec:
  host: cafe.example.com
  upstreams:
  - name: tea-post
    service: tea-post-svc
    port: 80
    client-max-body-size: "2m"
  - name: tea
    service: tea-svc
    port: 80
  - name: coffee-v1
    service: coffee-v1-svc
    port: 80
  - name: coffee-v2
    service: coffee-v2-svc
    port: 80
  routes:
  - path: /tea
    matches:
    - conditions:
      - variable: $request_method
        value: POST
      action:
        pass: tea-post
    action:
      pass: tea
  - path: /coffee
    matches:
    - conditions:
      - cookie: version
        value: v2
      action:
        pass: coffee-v2
    action:
      pass: coffee-v1
  1. Send a POST request to /tea with a payload > 1m
    Screenshot 2024-06-25 at 3 00 58 PM

Expected behavior
The client-max-body-size for the tea-post upstream should be enforced instead of the default client-max-body-size.

Your environment

  • Version of the Ingress Controller - NGINX Ingress Controller Version=3.5.2 Commit=90d40829ca0b92e4a8ebcc36415c220e72da7405
  • Version of Kubernetes: 1.30
  • Kubernetes platform (e.g. Mini-kube or GCP): minikube
  • Using NGINX or NGINX Plus: nginx

Additional context
This bug was initially found in NGINX Gateway Fabric and may apply to more than just the client_max_body_size directive.
The underlying issue is that nginx checks the body size of the request twice -- once in the external location block /tea and again in the internal location block that actually proxies the request to tea-post. If the request body exceeds the max body size in the external location block, a 413 is returned without nginx ever redirecting the request to the internal location block with the greater client_max_body_size. Even if client_max_body_size is not specified in the external location block, nginx will still check that the request body is under the default value of "1m". This is why this bug only applies when the internal client_max_body_size is greater than "1m".

Related: nginxinc/nginx-gateway-fabric#2079 and nginxinc/nginx-gateway-fabric#2105

Copy link

Hi @kate-osborn thanks for reporting!

Be sure to check out the docs and the Contributing Guidelines while you wait for a human to take a look at this 🙂

Cheers!

@jjngx jjngx added bug An issue reporting a potential bug backlog Pull requests/issues that are backlog items labels Jun 26, 2024
@jjngx jjngx modified the milestone: v3.6.1 Jun 26, 2024
@haywoodsh
Copy link
Contributor

This seems to be relevant to #5317

@danielnginx danielnginx added ready for refinement An issue that was triaged and it is ready to be refined refined Issues that are ready to be prioritized and removed bug An issue reporting a potential bug ready for refinement An issue that was triaged and it is ready to be refined labels Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Pull requests/issues that are backlog items refined Issues that are ready to be prioritized
Projects
Status: Todo ☑
Development

No branches or pull requests

4 participants