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.

Sign up for GitHub

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

annotate_client and annotate_transaction ACLs must always match #1820

Closed
wants to merge 5 commits into from
Closed

annotate_client and annotate_transaction ACLs must always match #1820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0247e16
Do not require an HTTP request in annotate_client
eduard-bagdasaryan May 19, 2024
4e0d40f
Made AnnotateClient and AnnotateTransaction "always matching"
eduard-bagdasaryan May 21, 2024
7e5f1cd
Polished and simplified
eduard-bagdasaryan May 21, 2024
8f62f40
fixup: Polished debugs() text and comment
rousskov May 21, 2024
59b44f9
fixup: Disclosed BUG effects
rousskov May 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 21 additions & 7 deletions src/acl/AnnotateClient.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,29 @@ int
Acl::AnnotateClientCheck::match(ACLChecklist * const ch)
{
const auto checklist = Filled(ch);
auto annotated = false;
const auto tdata = dynamic_cast<ACLAnnotationData*>(data.get());
assert(tdata);
const auto conn = checklist->conn();

if (const auto conn = checklist->conn()) {
const auto tdata = dynamic_cast<ACLAnnotationData*>(data.get());
assert(tdata);
if (conn) {
tdata->annotate(conn->notes(), &delimiters.value, checklist->al);
if (const auto request = checklist->request)
tdata->annotate(request->notes(), &delimiters.value, checklist->al);
return 1;
annotated = true;
}
return 0;

if (const auto &request = checklist->request) {
tdata->annotate(request->notes(), &delimiters.value, checklist->al);
annotated = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

} else if (conn && !conn->pipeline.empty()) {
debugs(28, DBG_IMPORTANT, "ERROR: Squid BUG: " << name << " ACL is used in context with " <<
"an unexpectedly nil ACLFilledChecklist::request. Did not annotate the current transaction.");
}

if (!annotated) {
debugs(28, DBG_IMPORTANT, "WARNING: " << name << " ACL is used in context without " <<
"active client-to-Squid connection and current transaction information. Did not annotate.");
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

return 1; // this is an "always matching" ACL
}

1 change: 0 additions & 1 deletion src/acl/AnnotateClient.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ class AnnotateClientCheck: public Acl::AnnotationCheck
public:
/* Acl::Node API */
int match(ACLChecklist *) override;
bool requiresRequest() const override { return true; }
};

} // namespace Acl
Expand Down
6 changes: 4 additions & 2 deletions src/acl/AnnotateTransaction.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,10 @@ Acl::AnnotateTransactionCheck::match(ACLChecklist * const ch)
const auto tdata = dynamic_cast<ACLAnnotationData*>(data.get());
assert(tdata);
tdata->annotate(request->notes(), &delimiters.value, checklist->al);
return 1;
} else {
debugs(28, DBG_IMPORTANT, "WARNING: " << name << " ACL is used in context without " <<
"current transaction information. Did not annotate.");
}
return 0;
return 1; // this is an "always matching" ACL
}

1 change: 0 additions & 1 deletion src/acl/AnnotateTransaction.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ class AnnotateTransactionCheck: public Acl::AnnotationCheck
public:
/* Acl::Node API */
int match(ACLChecklist *) override;
bool requiresRequest() const override { return true; }
};

} // namespace Acl
Expand Down
Loading