annotate_client and annotate_transaction ACLs must always match

[eduard-bagdasaryan]

WARNING: markAsTunneled ACL is used in context without an HTTP
request. Assuming mismatch.

Our annotate_client and annotate_transaction ACLs are documented as
"always matching", and some existing Squid configurations rely on that
invariant. Both ACLs did not match when they lacked access to the
current transaction information because their requiresRequest() method
returned true; annotate_client ACL also did not match after the
client-to-Squid connection was gone and when the transaction was not
associated with a client-to-Squid connection.

This change makes ACL code conform to documentation. Squid still warns
the admin if an ACL cannot annotate. Such warnings may indicate s Squid
bug or misconfiguration, but mismatching in those cases causes more harm
because it makes it impossible for the admin to rely on the primary
matching invariant. "One reliable invariant plus one unreliable side
effect" is the lesser evil than "two unreliable effects".

# the following must deny even if it cannot annotate
http_access deny markAsDenied

# the following might not log denied traffic (with a prior warning)
access_log syslog:daemon.err markedAsDenied

Also fixed annotate_client to annotate the current transaction even
after ConnStateData destruction. Such annotations may happen when, for
example, Squid continues a large download after the HTTP client is gone.

[rousskov]

The following comment analyses this tricky code and discusses potential out-of-scope improvements, but does not request any PR changes.

Our annotate_client ACL is supposed to annotate the current transaction (if any) and future transactions (if any) on the client-to-Squid connection (i.e. conn() or ConnStateData). When conn() is nil, this if statement annotates just the current transaction (i.e. checklist->request) but remains silent about absent connection info. Should we also warn the admin? To find the answer, we have to consider three mutually exclusive possibilities:

1: The current transaction is not associated with any client-to-Squid connection. One could argue that Squid is misconfigured -- the admin should have used annotate_transaction instead. On the other hand, we can also argue that it is bothersome for the admin to detect such independent transactions in squid.conf rules and the utility of doing so is fairly low; we can encourage admins to use annotate_client for both client-to-Squid connection-associated transactions and independent ones because there is no harm in doing so. I am not requesting any code changes related to this case.

2: The current transaction is associated with a client-to-Squid connection, information about that connection was (or would have been) supplied to Checklist, but that connection is gone, resulting in nil conn(). The current PR code clearly treats this case correctly, annotating the current transaction (there are no future ones). There is nothing to warn the admin about -- connections disappear all the time, and their disappearance (on its own) does not invalidate squid.conf rules.

3: The current transaction is associated with a client-to-Squid connection, but there is a Squid bug -- we forgot to set ACLFilledChecklist::conn_. The current PR code annotates the current transaction but not future ones (because it lacks access to ConnStateData). I wonder whether it is possible for Squid to automatically detect this bug, like we already do for similar "missing checklist info" bugs here and in Acl::Node::matches()? One solution candidate is examining checklist->request->masterXaction->initiator and complaining if it is set to initClient while ACLFilledChecklist::conn_ (and not just conn()!) is nil. I am worried that ACLFilledChecklist::conn_ may be nil even if code correctly attempted to supply connection information (but the connection was gone by that time). Moreover, even if the answer is "yes, it is possible to detect", then such BUG detection code belongs to Acl::Node::matches() -- it is not really specific to this ACL type and this PR. Thus, I am not requesting any changes here -- it is just a possible (and questionable) out-of-scope TODO.

[rousskov]

Acl::Node::matches() has similar "used in context without X" warnings, but there is an important difference that explains why this particular warning is here rather than in Acl::Node::matches(): Unlike Acl::Node::matches() warning cases, these problems do not and must not lead to ACL mismatches. This warning is not about our inability to calculate AnnotateClientCheck::match() return value -- that value is always 1. The warnings in Acl::Node::matches() say "Assuming mismatch". This warning says "Did not annotate" (and this ACL then "matches" by returning 1).

This comment annotates this PR. It does not request any PR changes.