traffic_dump: print request body data and client address filtering#6259
traffic_dump: print request body data and client address filtering#6259bneradt wants to merge 2 commits intoapache:masterfrom
Conversation
4f0a1ba to
6a343ed
Compare
|
Are the two commits separable? Or should they be squashed before merging? Looking more closely at the individual commits they do look independent, and should not be squashed. |
8ca15ae to
3d2b1a0
Compare
| struct TxnData { | ||
| std::string request_body; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Looks like perhaps another plugin that could be simplified by using https://github.com/ywkaras/trafficserver/blob/c53b525f27f10b7cea21c794e2351d6ce4be6e5f/plugins/xdebug/Cleanup.h#L127
There was a problem hiding this comment.
( an example of use of Cleanup.h is here: https://github.com/ywkaras/trafficserver/blob/c53b525f27f10b7cea21c794e2351d6ce4be6e5f/plugins/xdebug/xdebug.cc#L62 )
3d2b1a0 to
5ca36ea
Compare
|
[approve ci autest] |
925f3f4 to
64d1da4
Compare
6951e8d to
b44ce2e
Compare
| // client/proxy-request/response headers | ||
| TSMBuffer buffer = nullptr; | ||
| TSMLoc hdr_loc; | ||
| if (TS_SUCCESS == TSHttpTxnClientReqGet(txnp, &buffer, &hdr_loc)) { |
There was a problem hiding this comment.
Independent of these changes, but if we don't fetch the client request data until TXN_CLOSE, will some of the client data (like the URL) be changed from the original? Have you seen that in your dumps so far?
There was a problem hiding this comment.
Yes, good observation. I did face issues with exactly that (the request target, in particular). That got addressed here:
This branch will need to be rebased to get that fix.
| REBASE(CACHE_LOOKUP_COMPLETE), // TS_HTTP_CACHE_LOOKUP_COMPLETE_HOOK | ||
| REBASE(PRE_REMAP), // TS_HTTP_PRE_REMAP_HOOK | ||
| REBASE(POST_REMAP), // TS_HTTP_POST_REMAP_HOOK | ||
| REBASE(REQUEST_BUFFER_READ_COMPLETE), // TS_HTTP_REQUEST_BUFFER_READ_COMPLETE_HOOK |
There was a problem hiding this comment.
I wasn't familiar with this hook, so I looked it up in the docs, and it isn't there. Would be a great thing to add to the docs. Perhaps in a separate PR.
shinrich
left a comment
There was a problem hiding this comment.
My comments are probably outside the scope of this PR. The restructuring looks good to me. Will need to rebase.
|
This doesn't apply cleanly on the 9.0.x branch either, can you fix the conflicts above please, and also if there are other PRs need for 9.0.x before this, let me know. |
|
@bneradt Can you rebase so we can merge this change? |
|
Can we move this out to 9.1.x then ? |
|
Yes, that would be fine.
Thank you Leif.
Brian
…On Mon, Jul 13, 2020 at 4:29 PM Leif Hedstrom ***@***.***> wrote:
Can we move this out to 9.1.x then ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6259 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACMJMYZUPG3R2L37YKYDZ23R3N4DVANCNFSM4J2F52SQ>
.
--
"Come to Me, all who are weary and heavy-laden, and I will
give you rest. Take My yoke upon you and learn from Me, for
I am gentle and humble in heart, and you will find rest for
your souls. For My yoke is easy and My burden is light."
~ Matthew 11:28-30
|
1cd6401 to
f2ae2e8
Compare
f2ae2e8 to
134667b
Compare
|
What did I do? Why try and block me? I lost half of my carrier. |
The value for TS_HTTP_REQUEST_BUFFER_READ_COMPLETE_HOOK was out of sync with the corresponding event, TS_EVENT_HTTP_REQUEST_BUFFER_READ_COMPLETE. This caused the handler for TS_HTTP_REQUEST_BUFFER_READ_COMPLETE_HOOK to never be invoked with the TS_EVENT_HTTP_REQUEST_BUFFER_READ_COMPLETE event. This patch fixes this problem and adds comments explaining the otherwise implicit but very important requirement that these values correspond as they do. The naming of the event and hook were off too. A hook with name TS_HTTP_X_HOOK should have an event of name TS_EVENT_HTTP_X, but these got out of sync somehow for request buffer read complete. This adjusts the event name appropriately.
This adds the -b option to traffic dump to dump client request body data. It also adds the -4 and -6 options so that the client can filter what is dumped based upon a client IP address.
134667b to
b34a8ab
Compare
|
[approve ci clang-analyzer] |
|
[approve ci docs] |
Yes, let's not squash them. I separated out the HOOK/EVENT fix into this PR: #8066 Once that is merged I'll create another PR for the Traffic Dump work. In the meantime I'll close this PR. |
This adds the -b option to traffic dump to dump client request body
data. It also adds the -4 and -6 options so that the client can filter
what is dumped based upon a client IP address.
Note that this has a dependency upon the outstanding bneradt:fix_request_buffer_read_complete_hook branch from this PR:
#6243
The latter is the first commit in this diff and can be ignored in this PR. The second commit is the commit to be reviewed in this PR.