fix(http-logger): explicitly read the req body in rewrite phase

[cj2a7t]

Description

Explicitly read the req body:
When the pre plugins returns, the following collection request will be empty
Such as the pre jwt plugin in rewrite phase:
return 401, {message = "Missing JWT token in request"}
@see log-util line 179 or line 183
local body = req_get_body_data() or local body_file = ngx.req.get_body_file()
body or body_file is empty

如果使用了前置jwt-plugin或者其他自定义的鉴权插件，使用如下的退出
return 401, {message = "Missing JWT token in request"}
会导致在log阶段调用 log-util 179行 或者 183行读取请求体为空
local body = req_get_body_data() or local body_file = ngx.req.get_body_file()

Fixes #7890

Checklist

• I have explained the need for this PR and the problem it solves
• I have explained the changes or the new features added to this PR
• I have added tests corresponding to this change
• I have updated the documentation to reflect this change
• I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

[spacewander]

Personally, I don't like this change. It will force Nginx to read unused body, even from an untrusted client.

Maybe we can add it as a case "request body is missing" in the doc: https://github.com/apache/apisix/blob/master/docs/en/latest/plugins/http-logger.md#attributes

[cj2a7t]

@spacewander
I see.
However, in most of our scenarios, the authentication plugin will go ahead. In the rewrite phase, the logger's rewrite is post-existing and does not exist untrusted client.
Whether to turn it on as a schema configuration(default false)，give the user the read_body option to enable configuration.

[spacewander]

If the authentication plugin goes ahead and Nginx reads the body, it should be available in the logger, unless the body is too big or proxy_request_buffering is disabled.

It is not a good idea to force reading an unused body just because the logger needs it.

[cj2a7t]

hi , @spacewander oh yes, I see what you mean.
This is not the case that body is too big or proxy_request_buffering is disabled.
You can try the combination of http-logger plugin and jwt-auth plugin.

In this case, I want to get the request body from the http-logger(jwt-auth plugin goes ahead return). Is there any other way?

[tzssangglass]

However, in most of our scenarios, the authentication plugin will go ahead.

If you always need to read the body in your scenario, you can try the lua_need_request_body directive, but it is not recommended.
read more: https://moonbingbing.gitbooks.io/openresty-best-practices/content/openresty/get_req_body.html

[cj2a7t]

@tzssangglass Is it more unfriendly to control through lua_need_request_body command

[cj2a7t]

From the perspective of the logger plugin only, include_req_body = true.
However, the request body cannot be obtained, which is ambiguous for users.

[tzssangglass]

From the perspective of the logger plugin only, include_req_body = true. However, the request body cannot be obtained, which is ambiguous for users.
This is from your point of view. Would this problem still occur without jwt-auth plugin?

I was thinking maybe we could try ngx.req.read_body() when get body is nil, now this fix is not reasonable.

[cj2a7t]

Copy that. I'll try. Is there any better plan @tzssangglass

[tzssangglass]

Copy that. I'll try. Is there any better plan @tzssangglass

No more idea for now, let's hear from others.

[spacewander]

I don't think it needs to be fixed.

The log code should avoid changing the behavior of the normal code.

When event A happens, we can log event A down. But we can't say because we want to log event A, so event A should happen.

[spacewander]

From the perspective of the logger plugin only, include_req_body = true. However, the request body cannot be obtained, which is ambiguous for users.
This is from your point of view. Would this problem still occur without jwt-auth plugin?

I was thinking maybe we could try ngx.req.read_body() when get body is nil, now this fix is not reasonable.

There are many situations that get body is nil, for example:

1. Nginx doesn't read the body (this case)
2. the body is too large so Nginx stores it in a temp file
3. the request doesn't have a body
4. Nginx is configured to pass the request body streamingly (which will be broken if we always force Nginx to read the body just because someone wants to log the body)
5. Nginx is configured to always buffer the request body in a temp file

Not every situation needs to be "fixed"

[github-actions]

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@apisix.apache.org list. Thank you for your contributions.

[github-actions]

This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.