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.

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

Supply ALE with HttpReply before checking http_reply_access #398

Conversation

eduard-bagdasaryan
Copy link
Contributor

@eduard-bagdasaryan eduard-bagdasaryan commented May 2, 2019

Before this fix, Squid warned "ALE missing HttpReply object",
because ALE::reply was initialized too late. This problem affected
both HTTP and FTP code paths.

To avoid these problems, ALE::reply is initialized early with the
available (received) response.

Also:

  • Fixed logging control (1xx) responses in case these responses
    are final. Before this fix, '-' was logged.

  • All reply headers ('<h') were logged with a single CR separator,
    instead of CRLF.

  • Supply ALE for send_hit ACL. cb36505 already covered many (but not
    all) similar cases.

Before this fix, Squid warned that "ALE missing HttpReply object",
because ALE::reply was initialized too late. This problem affected
both HTTP and FTP code paths.
@eduard-bagdasaryan eduard-bagdasaryan changed the title Supply ALE with HttpReply before checking http_reply_access WIP: Supply ALE with HttpReply before checking http_reply_access May 2, 2019
Copy link
Contributor

@yadij yadij left a comment

Choose a reason for hiding this comment

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

Can you please explain in the commit description why/how removing several "al->reply = reply" does not add this log occurrence for other access checks when the client-facing code is not involved.

How certain are you that every code path where replies are read from servers calls client-side cloneReply() before using access controls? or sets the checklist->al->reply member explicitly? eg RESPMOD adaptation access or reply body size limits on internal fetches may not involve client-side code at all.

src/AccessLogEntry.cc Outdated Show resolved Hide resolved
@eduard-bagdasaryan
Copy link
Contributor Author

How certain are you that every code path where replies are read from servers calls client-side cloneReply() before using access controls?

No, there is no such guarantee in this solution, focused on fixing http_reply_access. I assume that http_reply_access is the first which is checked when passing the adapted reply to the client side, so I think that this solution should cover all access checks performed after adaptation completion, i.e., after getting an adapted reply (which ALE::reply holds). A full solution, including early checks such as adaptation_access, as you noted, would require setting ALE::reply even earlier, storing there a virgin reply.

With this change, it is now impossible to directly assign public
ALE::reply field. Another approach is to make this field private, but it
would require many out-of-scope changes.
... since FwdState::al may be nil sometimes and it is difficult to
predict whether these situations are possible in the two affected
places.
@squid-anubis squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Jun 18, 2019
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

The existing reply access check for non 1xx replies works for all client
replies (HTTP and FTP). Consequently, assigning ALE::reply just before
this check should work for all client replies

Is it possible to set ALE::reply when we actually get the reply, rather than rely (AFAICT) on a side effect of doing an access control directive check? I may be missing something, but the current PR code looks fragile -- there is no clearly guaranteed/intentional ALE::reply setting.

Please see inlined change requests for other, more context-specific concerns.

@@ -1656,6 +1656,8 @@ clientReplyContext::cloneReply()

/* do header conversions */
buildReplyHeader();

http->al->setReply(reply);
Copy link
Contributor

Choose a reason for hiding this comment

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

This setReply() argument points to a to-be-sent response rather than the (adapted) received response, right? Is that what we want in ALE? Your new ALE::reply member documentation seems to imply that we want the received (and possibly adapted) response instead. Please check and adjust this code or the new data member description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It points to a 'to-be-sent' response. I fixed the documentation with this terminology.

HTTPMSGLOCK(al.reply);
} else
al.reply = NULL;
al.setReply(adapted_reply_);
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, we set ICAP ALE::reply way after the corresponding ICAP transaction is over. AFAICT, this is conceptually wrong because that same ALE may be used a lot earlier for various ACL-driven directives. We should be populating ALE fields as we learn ALE information instead. Do you agree?

If yes, then I do not think this PR should fix this problem (because a lot of other fields are already set in this too-late method, and we should probably fix all or most of them at once), but please add an XXX comment to mark this problem. For example:

Suggested change
al.setReply(adapted_reply_);
// XXX: This reply (and other ALE members!) may have been needed earlier.
al.setReply(adapted_reply_);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not mind adding this comment. However I see the only ACL check in Icap::Launcher::canRepeat() (apart from the one in icapLogLog()). It looks like ALE is unavailable in the context of that method (and Lancher in unfamiliar with ALE). So for the time being, earlier ALE initialization would provide little benefit. Some new ACL to be added in future may require this fix, of course.

Copy link
Contributor Author

@eduard-bagdasaryan eduard-bagdasaryan left a comment

Choose a reason for hiding this comment

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

Done.

HTTPMSGLOCK(al.reply);
} else
al.reply = NULL;
al.setReply(adapted_reply_);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not mind adding this comment. However I see the only ACL check in Icap::Launcher::canRepeat() (apart from the one in icapLogLog()). It looks like ALE is unavailable in the context of that method (and Lancher in unfamiliar with ALE). So for the time being, earlier ALE initialization would provide little benefit. Some new ACL to be added in future may require this fix, of course.

@@ -1656,6 +1656,8 @@ clientReplyContext::cloneReply()

/* do header conversions */
buildReplyHeader();

http->al->setReply(reply);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It points to a 'to-be-sent' response. I fixed the documentation with this terminology.

src/AccessLogEntry.cc Outdated Show resolved Hide resolved
@eduard-bagdasaryan eduard-bagdasaryan changed the title WIP: Supply ALE with HttpReply before checking http_reply_access Supply ALE with HttpReply before checking http_reply_access Jul 3, 2019
@squid-anubis squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Jul 3, 2019
@yadij yadij added the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Jul 15, 2019
@yadij
Copy link
Contributor

yadij commented Jul 15, 2019

Looks like this now just needs @rousskov review re-check on the ICAP comments.

@rousskov
Copy link
Contributor

Looks like this now just needs @rousskov review re-check on the ICAP comments.

My biggest concern was:

Is it possible to set ALE::reply when we actually get the reply, rather than rely (AFAICT) on a side effect of doing an access control directive check? I may be missing something, but the current PR code looks fragile -- there is no clearly guaranteed/intentional ALE::reply setting.

@eduard-bagdasaryan, was that concern addressed by the recent branch changes? Or is it an invalid concern?

@squid-anubis squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Jul 17, 2019
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

I think we are getting close here. Thank you for moving this PR forward.

src/client_side.cc Outdated Show resolved Hide resolved
src/client_side.cc Outdated Show resolved Hide resolved
src/cf.data.pre Outdated Show resolved Hide resolved
@@ -1656,6 +1656,8 @@ clientReplyContext::cloneReply()

/* do header conversions */
buildReplyHeader();

http->al->reply = reply;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this initialization higher, to give all ACLs access to sent reply, including reply_header_add ACLs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

src/AccessLogEntry.h Outdated Show resolved Hide resolved
src/cf.data.pre Outdated Show resolved Hide resolved
src/cf.data.pre Outdated

[http::]mt MIME content type
[http::]mt MIME content type of the last sent reply.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we be more specific here? What is a MIME content type in this context? The contents of the Content-Type response header? Something more complex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The contents of the Content-Type response header?

No, it is the MIME type (i.e., type/subtype part) of the Content-Type header value. Probably simply 'MIME type' should be better than 'MIME content type'.

* Fixed comments and documentation according to the wiki
  'response terminology'.

* Initialize http->al->reply just after clone(), in order to supply
  it to early ACLs checks called during building the final response
  header (e.g., reply_header_add ACLs).
Copy link
Contributor Author

@eduard-bagdasaryan eduard-bagdasaryan left a comment

Choose a reason for hiding this comment

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

Done at 824da98.

src/AccessLogEntry.h Outdated Show resolved Hide resolved
src/cf.data.pre Outdated Show resolved Hide resolved
src/cf.data.pre Outdated Show resolved Hide resolved
src/cf.data.pre Outdated

[http::]mt MIME content type
[http::]mt MIME content type of the last sent reply.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The contents of the Content-Type response header?

No, it is the MIME type (i.e., type/subtype part) of the Content-Type header value. Probably simply 'MIME type' should be better than 'MIME content type'.

src/client_side.cc Outdated Show resolved Hide resolved
@@ -1656,6 +1656,8 @@ clientReplyContext::cloneReply()

/* do header conversions */
buildReplyHeader();

http->al->reply = reply;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

After be6c958 ALE was supplied with 'sent' HttpReply, and many (but not
all) ACL checks could use it. Earlier directives, such as
adaptation_access and store_miss still lacked it. To resolve this, we
need ALE::reply to point to the reply available at the moment, starting
with the virgin reply.

Also fixed ALE::reply for all 1xx (control) messages. Though be6c958
adjusted it for control responses coming from the origin, it missed
self-generated control responses (force_request_body_continuation
directive).
@rousskov
Copy link
Contributor

rousskov commented Aug 8, 2019

As an alternative, we could just refer to Squid wiki

Let's merge the best of the two ideas into one:

HttpReplyPointer reply; ///< sent reply (SeeSquidWiki)

SeeSquidWiki already has 9 hits on Google right now, but I think that is acceptable. If we want zero hits, we can use more cryptic See_Sqd_Wiki (the shorter SeeSqdWiki version has 2 million hits in an unquoted search).

(assuming that it would be trivial to do the remaining search within the wiki)

It depends on whether the reader knows that she should search for something like "squid terminology". It would be very difficult to find the definitions while searching for common words like "sent" or "received" on the wiki. We semi-address this problem by merging the two ideas as summarized above -- searching for SeeSquidWiki should work very well on wiki. We can (and probably should) even make a wiki page with that name/URL.

To facilitate this search, we would need creating a hierarchical structure of term pages.

A hierarchy will not help IMO -- "nobody" reads Table Of Contents and similar indexes on wiki pages, and there are too many very different concepts that our wiki has to cover anyway (the top-level index will be long). And once the reader finds the terminology page, it would be easy to find the term she needs there (and a ToC/hierarchy can help at that level if our list of terms grows long).

@rousskov
Copy link
Contributor

rousskov commented Aug 8, 2019

@eduard-bagdasaryan, if you do not see any objections here soon, please proceed with refactoring PR changes to use the SeeSquidWiki label for well-defined terms.

@eduard-bagdasaryan
Copy link
Contributor Author

proceed with refactoring PR changes to use the SeeSquidWiki label

Done at 5f8055f.

@@ -427,7 +427,7 @@ ClientHttpRequest::logRequest()
if (request) {
SBuf matched;
for (auto h: Config.notes) {
if (h->match(request, al->reply, NULL, matched)) {
if (h->match(request, al->reply.getRaw(), NULL, matched)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nullptr on touched lines please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done at 3318fe2.

@@ -177,7 +179,8 @@ class AccessLogEntry: public RefCountable
SBuf lastAclData; ///< string for external_acl_type %DATA format code

HierarchyLogEntry hier;
HttpReply *reply = nullptr;
HttpReplyPointer reply; ///< sent reply (SeeSquidWiki)
Copy link
Contributor

@yadij yadij Aug 13, 2019

Choose a reason for hiding this comment

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

All this fussing over the terminology seems a bit unnecessary to me. Some of it appears to be derived from the fact that this single member is not really suitable for top-level ALE placement relative to the logformat needs and the 'h' and 'ha' codes specifically have not yet been updated to use it.

We really have three types of response being juggled;

  1. the "reply from server or peer" which really should be at ALE::HttpDetails::serverReply
  2. the "adapted reply" - partially processed, maybe adapted from (1) but not yet sent as (3).
  • this one maybe could be top-level, but in a member named better than just "reply". or for now stored as (3).
  1. the "reply sent to the client" which really should be at ALE::HttpDetails::clientReply

The "<h" is (1), "<ha" is (2) / (3).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "<h" is (1), "<ha" is (2) / (3).

To clarify: we don't have '<ha'.
'<h' logs sent replies (as it logged before the PR changes).

Copy link
Contributor

Choose a reason for hiding this comment

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

That was part of my point. Much of this mind-bending to re-define the common terms you guys seem to have been doing is working around that. After it gets added there is likely very little remainder to worry about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "<h" is (1), "<ha" is (2) / (3).

Ideally, the reply terminology and log format codes should correlate with their request counterparts. However, '>ha' stands for 'received request after adaptation/redirection' and usually differs from the request, whereas you suggest that '<ha' should keep the sent reply( if available). So, in order to simplify/unify this task, we would need to have three logformat codes for request (virgin/adapted/sent) and three for reply. Currently we have only two for request(virgin/adapted) and one for reply (sent). Do you think we should add the remaining(missing) logformat codes and (if yes) do it in this PR?

Copy link
Contributor

@rousskov rousskov Aug 13, 2019

Choose a reason for hiding this comment

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

Amos: After it gets added there is likely very little remainder to worry about.

Regardless of how many %codes we add or have, we still need to explain what those codes mean. At least some of those %codes (and fields/methods) will require lengthy, non-trivial explanations because their computation method is naturally complex. For example, many admins want to log the "latest reply available", which is a complex notion regardless of how many (other) %codes we have/add.

So yes, there will be quite a bit to "worry about" even after adding more %codes. And, ideally, deciding what %codes to add and thinking about how to describe them should be done at the same time, not in sequence!

Your ad-hoc attempt to suggest what should be done instead is a good illustration of the natural problem complexity -- the suggestion immediately spawned a discussion full of controversial ideas and misunderstandings (all using inadequate terminology). If we want to revise message %codes, a comprehensive solution should be carefully thought out outside a largely unrelated PR.

Eduard: Do you think we should add the remaining(missing) logformat codes and (if yes) do it in this PR?

The answer to this question is simple: We should remove documentation changes from this PR if their presence triggers "add more %codes" change requests. In this case, improving documentation simply becomes too much side work. We should cut our losses and just focus on fixing the bug, leaving the existing documentation as useless as it is right now.

@yadij, if you do not like the idea of adding SeeSquidWiki references, we will remove those controversial documentation improvements from this PR. Is that your preference?

Documentation and %codes issues can be revisited in the future, of course.


The only viable alternative to so called "redefining of common terms" is to invent uncommon ones.

  • If we continue to restrict this PR from adding new %codes, then I do not see any good uncommon terms that we should use instead. Referring to wiki where the current status quo can be documented and where the terminology can be developed/perfected is an imperfect but viable solution here IMO. However, we do not have to change documentation at all if there is no consensus regarding this SeeSquidWiki idea.
  • If we add new %codes, then, for some sets of new %codes, those %codes themselves can be used as "uncommon terms". However, doing this right is too much optional work. We will not do it in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Amos: After it gets added there is likely very little remainder to worry about.

Regardless of how many %codes we add or have, we still need to explain what those codes mean. At least some of those %codes (and fields/methods) will require lengthy, non-trivial explanations because their computation method is naturally complex. For example, many admins want to log the "latest reply available", which is a complex notion regardless of how many (other) %codes we have/add.

Years of experience with the request-path format codes belies those assertions. Request handling has just as much processing applied and the descriptions are still mostly one-liners which are easily read and understood without having to resort to diagrams and pages teaching admin definitions. The exceptions have been where you chose to add new un-common terms.

The major relevant difference between the two right now is that the response code paths are lacking a lot of the cleanup which happened in the request pathways already. Which is what my initial comment was saying, there are things here which are already planned out to change. When it happens that change can be accompanied by a better description. It is a waste of time re-doing the description now when things are still a mess ...

So yes, there will be quite a bit to "worry about" even after adding more %codes. And, ideally, deciding what %codes to add and thinking about how to describe them should be done at the same time, not in sequence!

By that reason ideally the existing %code should not have the apparently arbitrary description change we see going on here.

The %codes I mentioned in my comment (and other missing %codes too) have already been selected when the existing terms were designed. The label reservations can be found in ByteCode.h as commented out entries symmetrical with request handling codes (though the Token.cc "<ha" part seems to be missing now):
``

/* response header details pre-adaptation */
LFT_REPLY_HEADER,
LFT_REPLY_HEADER_ELEM,
LFT_REPLY_ALL_HEADERS,

/* response header details post-adaptation */
/* LFT_ADAPTED_REPLY_HEADER, */
/* LFT_ADAPTED_REPLY_HEADER_ELEM, */
/* LFT_ADAPTED_REPLY_ALL_HEADERS, */

``

All that remains is the code cleanup in the reply handling to provide the pointers to ALE differently, and implementation of the %code's using the resulting ALE members. Sadly the incremental changes leading to that end kept getting stalled in review over not being important enough in small chunks.

Your ad-hoc attempt to suggest what should be done instead

Is not ad-hoc at all. It is the result of a) the previous round of discussions the then-core team had on this topic, and b) years of study and months of drawn out discussions about the design for ALE and MasterTransaction contents. The docs terms are well established and have been used by all the logformat additions up until this point. Now I see the two of you without any other consultation beginning a complete re-write to naming things in a way that looks quite at odds with the previous long-term plans.

If we want to revise message %codes, a comprehensive solution should be carefully thought out outside a largely unrelated PR.

Agreed.

Eduard: Do you think we should add the remaining(missing) logformat codes and (if yes) do it in this PR?

The answer to this question is simple: We should remove documentation changes from this PR if their presence triggers "add more %codes" change requests. In this case, improving documentation simply becomes too much side work. We should cut our losses and just focus on fixing the bug, leaving the existing documentation as useless as it is right now.

@yadij, if you do not like the idea of adding SeeSquidWiki references, we will remove those controversial documentation improvements from this PR. Is that your preference?

It is, yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we will remove those controversial documentation improvements

Done at 7bd1b13.

Copy link
Contributor

Choose a reason for hiding this comment

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

Years of experience with the request-path format codes belies those assertions.

That is your opinion, not a fact. My years of experience have been different. Glad we may have found a way to move forward despite these differences in opinion. Are you OK with the current documentation changes in this PR? If not, we should remove them (which is what I meant earlier but @eduard-bagdasaryan has removed just the SeeSquidWiki references).

Copy link
Contributor

@yadij yadij left a comment

Choose a reason for hiding this comment

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

I would like this PR to restrict itself to the changes needed to get reply member set as necessary. The documentation which is being controversial is not necessary for that and we can debate it for a separate PR.

@@ -177,7 +179,8 @@ class AccessLogEntry: public RefCountable
SBuf lastAclData; ///< string for external_acl_type %DATA format code

HierarchyLogEntry hier;
HttpReply *reply = nullptr;
HttpReplyPointer reply; ///< sent reply
Copy link
Contributor

Choose a reason for hiding this comment

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

This reply may never be sent. eg in the case of a "http_reply_access deny " line matching. Calling it "sent" is just wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done at 064bf97.

src/cf.data.pre Outdated
@@ -4696,14 +4696,14 @@ DOC_START

RESPONSE

[http::]<Hs HTTP status code received from the next hop
[http::]>Hs HTTP status code sent to the client
[http::]<Hs HTTP status code of the final virgin reply
Copy link
Contributor

Choose a reason for hiding this comment

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

Still doing controversial documentation changes unrelated to the bug this PR is purporting to be fixing.

These codes also print details pf replies that have not been "sent" anywhere - sometimes the printed reply details will never get sent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done at 064bf97.

Copy link
Contributor Author

@eduard-bagdasaryan eduard-bagdasaryan left a comment

Choose a reason for hiding this comment

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

Done at 064bf97.

@@ -177,7 +179,8 @@ class AccessLogEntry: public RefCountable
SBuf lastAclData; ///< string for external_acl_type %DATA format code

HierarchyLogEntry hier;
HttpReply *reply = nullptr;
HttpReplyPointer reply; ///< sent reply
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done at 064bf97.

src/cf.data.pre Outdated
@@ -4696,14 +4696,14 @@ DOC_START

RESPONSE

[http::]<Hs HTTP status code received from the next hop
[http::]>Hs HTTP status code sent to the client
[http::]<Hs HTTP status code of the final virgin reply
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done at 064bf97.

@yadij yadij added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Aug 22, 2019
@yadij
Copy link
Contributor

yadij commented Aug 22, 2019

Thank you.

Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

One old and three new small polishing requests.

src/AccessLogEntry.cc Outdated Show resolved Hide resolved
src/AccessLogEntry.h Outdated Show resolved Hide resolved
src/client_side.cc Outdated Show resolved Hide resolved
src/client_side_request.cc Outdated Show resolved Hide resolved
Copy link
Contributor Author

@eduard-bagdasaryan eduard-bagdasaryan left a comment

Choose a reason for hiding this comment

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

Done at 8cd9b08.

src/AccessLogEntry.cc Outdated Show resolved Hide resolved
src/AccessLogEntry.h Outdated Show resolved Hide resolved
src/client_side.cc Outdated Show resolved Hide resolved
src/client_side_request.cc Outdated Show resolved Hide resolved
@squid-anubis squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Sep 5, 2019
@rousskov rousskov dismissed their stale review September 9, 2019 16:29

Thank you for addressing my change requests.

@rousskov rousskov added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels and removed S-waiting-for-author author action is expected (and usually required) labels Sep 9, 2019
squid-anubis pushed a commit that referenced this pull request Sep 10, 2019
Before this fix, Squid warned "ALE missing HttpReply object",
because ALE::reply was initialized too late. This problem affected
both HTTP and FTP code paths.

To avoid these problems, ALE::reply is initialized early with the
available (received) response.

Also:

* Fixed logging control (1xx) responses in case these responses
  are final. Before this fix, '-' was logged.

* All reply headers ('<h') were logged with a single CR separator,
  instead of CRLF.

* Supply ALE for send_hit ACL. cb36505 already covered many (but not
  all) similar cases.
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Sep 10, 2019
@squid-anubis squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-merged https://github.com/measurement-factory/anubis#pull-request-labels
Projects
None yet
4 participants