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.

Sign up for GitHub

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

Enable request buffering when request body tracing #3440

Open
wants to merge 1 commit into
base: 2.0.x
Choose a base branch
from
Open

Enable request buffering when request body tracing #3440

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .../main/java/org/springframework/cloud/netflix/zuul/filters/pre/Servlet30WrapperFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@

import javax.servlet.http.HttpServletRequest;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.cloud.netflix.zuul.filters.ZuulProperties;
import org.springframework.cloud.netflix.zuul.util.RequestUtils;
import org.springframework.util.Assert;
import org.springframework.util.ReflectionUtils;
Expand All @@ -38,6 +40,8 @@
*/
public class Servlet30WrapperFilter extends ZuulFilter {

@Autowired
private ZuulProperties zuulProperties;
private Field requestField = null;

public Servlet30WrapperFilter() {
Expand Down Expand Up @@ -80,6 +84,10 @@ else if (RequestUtils.isDispatcherServletRequest()) {
// If it's going through the dispatcher we need to buffer the body
ctx.setRequest(new Servlet30RequestWrapper(request));
}
else if (zuulProperties.isTraceRequestBody()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is exactly what I described.

turn off trace request body if zuul.use-filter=true.

This enables buffering if isTraceRequestBody() which isn't the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

turn off trace request body if zuul.use-filter=true.

What you suggest seems like it'll be really awkward to program and unexpected for users:
if zuul.use-filter=true, ignore zuul.traceRequestBody and treat it as if it were always false. This implementation would confuse users who expect zuul.traceRequestBody to do something, and it's weird for traceRequestBody to be a variable that's sometimes ignored. IMHO, making zuul.traceRequestBody always work as expected is a better solution for maintainability and users alike, which is the approach I took in this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also unexpectedly introduces buffering because trace request body is true by default. I won't apply this PR as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide any guidance on what I can do?
Things I can think of include making some other change that you suggest or rebasing against another branch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add another case here

spring-cloud-netflix/spring-cloud-netflix-zuul/src/main/java/org/springframework/cloud/netflix/zuul/filters/ProxyRequestHelper.java

Line 266 in eff47e0

if (!this.traceRequestBody || ctx.isChunkedRequestBody()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for my density... what case are suggesting be added?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if zuul.use-filter=true: return false

// If requesty body tracing we need to buffer the body
ctx.setRequest(new Servlet30RequestWrapper(request));
}
return null;
}

Expand Down