annotate_client did not annotate error:* transactions #256
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cc71dd7 introduced an admin-level BUG requirement that a non-empty message pipeline means an existing HttpRequest object. That requirement was incorrect because when the pipeline receives the HttpStream object (returned by ConnStateData::parseOneRequest()), the corresponding HttpRequest is not created yet (an may be not created at all, depending on further checks). Now, instead of demanding HttpRequest presence in annotate_client, we annotate requests if/when they get created (and imported into ClientHttpRequest) by copying the existing connection annotation. That copying already worked for successfully parsed and fake HttpRequests, but was missing for error-uri requests (such as "error:invalid-request"). Also fill these 'error:*' HttpRequests with connection-level information. For example, access_log takes this information from the request, and fails to match some ACLs, such as 'src'.
| clientConnectionManager = aMgr; | ||
|
|
||
| if (!clientConnectionManager.valid()) | ||
| return; | ||
|
|
||
| // TODO: We fill request notes here until we find a way to verify whether | ||
| // no ACL checking is performed before ClientHttpRequest::doCallouts(). |
There was a problem hiding this comment.
I copied this TODO from buildHttpRequest(), for now. I'm not sure whether it is relevant now, there are some ACL checking before ClientHttpRequest::doCallouts().
| if (!request) { | ||
| debugs(33, 5, "Invalid URL: " << http->uri); | ||
| // setReplyToError() requires log_uri | ||
| http->setLogUriToRawUri(http->uri, parser_->method()); | ||
|
|
||
| const char * requestErrorBytes = inBuf.c_str(); | ||
| if (!tunnelOnError(ERR_INVALID_URL)) { | ||
| setReplyError(context, request, ERR_INVALID_URL, Http::scBadRequest, requestErrorBytes); |
There was a problem hiding this comment.
The request passed to all setReplyError() was either nil or it got deleted once buildHttpRequest() returned.
| @@ -773,11 +773,19 @@ UpdateRequestNotes(ConnStateData *csd, HttpRequest &request, NotePairs const &he | |||
| void | |||
| HttpRequest::manager(const CbcPointer<ConnStateData> &aMgr, const AccessLogEntryPointer &al) | |||
| { | |||
| if (clientConnectionManager == aMgr) | |||
There was a problem hiding this comment.
It looks like that HttpRequest::manager() should not allow overwrite with the same aMgr. We can even require it with an 'Assure'.
On splice() path, we always create fake request (with SNI, if needed) On tunnelOnError() path, the request must be created beforehand (e.g., one of the "error:*" requests).
tunnelStart() needs an HttpRequest object initalized with the correct destination port/host values (the fake 'error:*" is not enough). For example, ConnStateData::borrowPinnedConnection() checks the port before providing pinned connection.
We can create "error:*" requests even earlier than it was done in 3b70fb3 - just after the parsing code detects an error and aborts parsing.
This annotation is calculated via Format::assemble() and gets an empty (dash) value if ALE::request is missing. Previous branch commits guaranteed earlier HttpRequest creation, and now ALE::request got the correct value.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
cc71dd7 introduced an admin-level BUG requirement that a non-empty
message pipeline means an existing HttpRequest object. That requirement
was incorrect because when the pipeline receives the HttpStream object
(returned by ConnStateData::parseOneRequest()), the corresponding
HttpRequest is not created yet (an may be not created at all, depending
on further checks). Now, instead of demanding HttpRequest
presence in annotate_client, we annotate requests if/when they get
created (and imported into ClientHttpRequest) by copying the existing
connection annotation. That copying already worked for successfully
parsed and fake HttpRequests, but was missing for error-uri requests
(such as "error:invalid-request").
Also create an HttpRequest object for 'error:' and other
tunnelOnError()-related cases as soon as possible because some
directives require it. For example, if HttpRequest is missing,
access_log fails to match some ACLs (such as 'src') and the
'error:' transaction is not logged. Another problem caused by
missing request was that %>handshake annotations were empty
(logged as a "-") because it Format::assemble() needs an existing
ALE::request to fill this format code.
Also, on the tunnelOnError() path, do not abandon the existing
HttpRequest (and its ClientHttpRequest) in favor of the new one created
by buildFakeRequest(). Before this fix, the tunneled transaction was
represented by two 'CONNECT' access.log entries, instead of a single
one without any fake method.