Skip to content

Conversation

andrew-s
Copy link
Contributor

@andrew-s andrew-s commented Sep 21, 2025

Proposed changes

This fixes #7332 which when using virtual server/routes with a match condition then the max body size of the upstream is ultimately lost as nginx doesn't inherit from the sub locations, this PR adds it to the internal redirect location (the parent).

I couldn't see another way for this to work other than creating an issue against nginx, but there's no guarantees that would be accepted as I would imagine instant parameter inheritance in a version might break existing systems that are expected to work in the current way.

Some edge cases to this implementation;

  • any upstream with a client max body size then applies to all matched routes on that path
  • if a path with matched routes has multiple upstreams all with different max client body sizes, only the first would be used (perhaps we log a warning if this happens?)

I've added the initial unit tests, I haven't added the template test (I guess this is still worthwhile to prevent this breaking/removal in the future) - I wanted to gauge if it's also worth while adding to the suite tests? (which the helm template looks to be broke in the main branch currently)

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@andrew-s andrew-s requested a review from a team as a code owner September 21, 2025 00:33
Copy link
Contributor

github-actions bot commented Sep 21, 2025

✅ All required contributors have signed the F5 CLA for this PR. Thank you!
Posted by the CLA Assistant Lite bot.

@github-actions github-actions bot added bug An issue reporting a potential bug go Pull requests that update Go code labels Sep 21, 2025
@andrew-s
Copy link
Contributor Author

I have hereby read the F5 CLA and agree to its terms

@AlexFenlon
Copy link
Contributor

Hi @andrew-s,

Thank you for the pull request!

We will take a look at this as soon as we can!

@andrew-s
Copy link
Contributor Author

No problem @AlexFenlon

I'll await your initial response but I have been thinking, since this location is a rewrite I would assume (correctly or incorrectly) that all upstream parameters would be lost, I haven't tested this but it would be odd we only lose client_max_body_size.

So my thoughts are, would it be best to add Parameters to the InternalRedirectLocation of a new type InternalRedirectLocationParameters - we still face the issue of having only one of the upstream parameters/attributes take precedent over the others.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue reporting a potential bug go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: client-max-body-size on (upstream) VirtualServer is never considered
2 participants