Skip to content

RFC: Support connection-linked helper notes #2399

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

Draft
yadij wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

RFC: Support connection-linked helper notes #2399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
197270f
Support connection-notes=CSV option on all helper interfaces
yadij Apr 2, 2026
3c16bed
Actually use configured connection-notes=CSV settings
yadij Apr 2, 2026
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
20 changes: 12 additions & 8 deletions src/HttpRequest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include "HttpRequest.h"
#include "log/Config.h"
#include "MemBuf.h"
#include "sbuf/List.h"
#include "sbuf/StringConvert.h"
#include "SquidConfig.h"
#include "Store.h"
Expand Down Expand Up @@ -754,16 +755,19 @@ HttpRequest::notes()
}

void
UpdateRequestNotes(ConnStateData *csd, HttpRequest &request, NotePairs const &helperNotes)
{
// Tag client connection if the helper responded with clt_conn_tag=tag.
const char *cltTag = "clt_conn_tag";
if (const char *connTag = helperNotes.findFirst(cltTag)) {
if (csd) {
csd->notes()->remove(cltTag);
csd->notes()->add(cltTag, connTag);
UpdateRequestNotes(ConnStateData *csd, HttpRequest &request, NotePairs const &helperNotes, const SBufList &connTags)
{
// Tag client connection if the helper responded with clt_conn_tag=tag
// OR, if the admin configured the annotation to be copied to the client connection
if (csd) {
for (const auto &note : helperNotes) {
if (IsMember(connTags, note->name())) {
csd->notes()->remove(note->name());
csd->notes()->add(note->name(), note->value());
}
}
}

request.notes()->replaceOrAdd(&helperNotes);
}

Expand Down
2 changes: 1 addition & 1 deletion src/HttpRequest.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ class ConnStateData;
/**
* Updates ConnStateData ids and HttpRequest notes from helpers received notes.
*/
void UpdateRequestNotes(ConnStateData *csd, HttpRequest &request, NotePairs const &notes);
void UpdateRequestNotes(ConnStateData *csd, HttpRequest &request, NotePairs const &notes , const SBufList &clientConnectionTags);

/// \returns listening/*_port address used by the client connection (or nil)
/// nil parameter(s) indicate missing caller information and are handled safely
Expand Down
6 changes: 6 additions & 0 deletions src/Notes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ class NotePairs: public RefCountable
};
typedef std::vector<Entry::Pointer> Entries; ///< The key/value pair entries
typedef std::vector<SBuf> Names;
typedef Entries::const_iterator const_iterator; ///< iterates over the notes list

NotePairs() {}
NotePairs &operator=(NotePairs const &) = delete;
Expand Down Expand Up @@ -265,6 +266,11 @@ class NotePairs: public RefCountable
/// pair to multiple single-token pairs; returns existing entries otherwise.
const Entries &expandListEntries(const CharacterSet *delimiters) const;

/// points to the first argument
const_iterator begin() const { return entries.cbegin(); }
/// points to the end of list
const_iterator end() const { return entries.cend(); }
Comment on lines +269 to +272
Copy link
Copy Markdown
Contributor

@rousskov rousskov Apr 2, 2026

Choose a reason for hiding this comment

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

Please use (less problematic) STL descriptions and avoid an otherwise unused type alias:

Suggested change
/// points to the first argument
const_iterator begin() const { return entries.cbegin(); }
/// points to the end of list
const_iterator end() const { return entries.cend(); }
/// an iterator to the first note
auto begin() const { return entries.cbegin(); }
/// an iterator past the last note
auto end() const { return entries.cend(); }


private:
Entries entries; ///< The key/value pair entries
};
Expand Down
3 changes: 2 additions & 1 deletion src/auth/UserRequest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,8 @@ authTryGetUser(Auth::UserRequest::Pointer auth_user_request, ConnStateData * con
// workaround by using anything already set in HttpRequest
// OR use new and rely on a later Sync copying these to AccessLogEntry

UpdateRequestNotes(conn, *request, res->user()->notes);
auto schemeCfg = Auth::SchemeConfig::Find(res->scheme()->type()); // TODO: refactor to avoid this
UpdateRequestNotes(conn, *request, res->user()->notes, schemeCfg->authenticateChildren.clientConnectionTags);
}

return res;
Expand Down
4 changes: 2 additions & 2 deletions src/client_side_request.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1019,7 +1019,7 @@ ClientRequestContext::clientRedirectDone(const Helper::Reply &reply)
if (http->al)
http->al->syncNotes(old_request);

UpdateRequestNotes(http->getConn(), *old_request, reply.notes);
UpdateRequestNotes(http->getConn(), *old_request, reply.notes, Config.redirectChildren.clientConnectionTags);

switch (reply.result) {
case Helper::TimedOut:
Expand Down Expand Up @@ -1135,7 +1135,7 @@ ClientRequestContext::clientStoreIdDone(const Helper::Reply &reply)
if (http->al)
http->al->syncNotes(old_request);

UpdateRequestNotes(http->getConn(), *old_request, reply.notes);
UpdateRequestNotes(http->getConn(), *old_request, reply.notes, Config.storeIdChildren.clientConnectionTags);

switch (reply.result) {
case Helper::Unknown:
Expand Down
2 changes: 1 addition & 1 deletion src/external_acl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,7 +583,7 @@ copyResultsFromEntry(const HttpRequest::Pointer &req, const ExternalACLEntryPoin
req->extacl_message = entry->message;

// attach the helper kv-pair to the transaction
UpdateRequestNotes(req->clientConnectionManager.get(), *req, entry->notes);
UpdateRequestNotes(req->clientConnectionManager.get(), *req, entry->notes, entry->def->children.clientConnectionTags);
}
}

Expand Down
90 changes: 75 additions & 15 deletions src/helper/ChildConfig.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,17 @@
*/

#include "squid.h"
#include "base/CharacterSet.h"
#include "base/PackableStream.h"
#include "cache_cf.h"
#include "ConfigParser.h"
#include "debug/Stream.h"
#include "globals.h"
#include "helper/ChildConfig.h"
#include "Parsing.h"
#include "sbuf/SBuf.h"
#include "parser/Tokenizer.h"
#include "sbuf/List.h"
#include "Store.h"

#include <cstring>

Expand Down Expand Up @@ -72,7 +76,7 @@ Helper::ChildConfig::needNew() const
void
Helper::ChildConfig::parseConfig()
{
char const *token = ConfigParser::NextToken();
char *token = ConfigParser::NextToken();

if (!token) {
self_destruct();
Expand All @@ -89,22 +93,23 @@ Helper::ChildConfig::parseConfig()
}

/* Parse extension options */
for (; (token = ConfigParser::NextToken()) ;) {
if (strncmp(token, "startup=", 8) == 0) {
n_startup = xatoui(token + 8);
} else if (strncmp(token, "idle=", 5) == 0) {
n_idle = xatoui(token + 5);
char *value;
while (ConfigParser::NextKvPair(token, value)) {
if (strncmp(token, "startup", 7) == 0) {
n_startup = xatoui(value);
} else if (strncmp(token, "idle", 4) == 0) {
n_idle = xatoui(value);
if (n_idle < 1) {
debugs(0, DBG_CRITICAL, "WARNING: OVERRIDE: Using idle=0 for helpers causes request failures. Overriding to use idle=1 instead.");
n_idle = 1;
}
} else if (strncmp(token, "concurrency=", 12) == 0) {
concurrency = xatoui(token + 12);
} else if (strncmp(token, "queue-size=", 11) == 0) {
queue_size = xatoui(token + 11);
} else if (strncmp(token, "concurrency", 11) == 0) {
concurrency = xatoui(value);
} else if (strncmp(token, "queue-size", 10) == 0) {
queue_size = xatoui(value);
defaultQueueSize = false;
} else if (strncmp(token, "on-persistent-overload=", 23) == 0) {
const SBuf action(token + 23);
} else if (strncmp(token, "on-persistent-overload", 22) == 0) {
const SBuf action(value);
if (action.cmp("ERR") == 0)
onPersistentOverload = actErr;
else if (action.cmp("die") == 0)
Expand All @@ -114,8 +119,10 @@ Helper::ChildConfig::parseConfig()
self_destruct();
return;
}
} else if (strncmp(token, "reservation-timeout=", 20) == 0)
reservationTimeout = xatoui(token + 20);
} else if (strncmp(token, "reservation-timeout", 19) == 0)
reservationTimeout = xatoui(value);
else if (strncmp(token, "connection-notes", 16) == 0)
parseNotesList(SBuf(value));
else {
debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), "ERROR: Undefined option: " << token << ".");
self_destruct();
Expand All @@ -139,3 +146,56 @@ Helper::ChildConfig::parseConfig()
queue_size = 2 * n_max;
}

/// parses comma-separated list of key names to be
/// treated like clt_conn_tag
void
Helper::ChildConfig::parseNotesList(const SBuf &buf)
{
::Parser::Tokenizer tok(buf);

static const CharacterSet delims("comma", ",");
SBuf item;
while (tok.token(item, delims)) {
static const SBuf wsp(" ");
item.trim(wsp);
if (!item.isEmpty())
clientConnectionTags.emplace_back(item);
Copy link
Copy Markdown
Contributor

@rousskov rousskov Apr 2, 2026

Choose a reason for hiding this comment

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

Please refuse configurations with duplicate names because they may be dangerous typos.

Copy link
Copy Markdown
Contributor Author

@yadij yadij Apr 3, 2026

Choose a reason for hiding this comment

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

Okay, though I do not see what danger there is. What are you thinking can happen?

}
}

void
Helper::ChildConfig::printConfig(StoreEntry *e, const char *directive)
{
PackableStream os(*e);
os << "\n" << directive << " " << n_max;

if (n_startup != 0)
os << " startup=" << n_startup;

if (n_idle != 0)
os << " idle=" << n_idle;

if (concurrency != 0)
os << " concurrency=" << concurrency;

if (!defaultQueueSize)
os << " queue-size=" << queue_size;

switch (onPersistentOverload) {
case actErr:
os << " on-persistent-overload=ERR";
break;
case actDie: // defaults not printed
break;
}

if (reservationTimeout != 64)
os << " reservation-timeout=" << reservationTimeout;

static const SBuf comma(",");
auto cnotes = JoinContainerToSBuf(clientConnectionTags.begin(), clientConnectionTags.end(), comma);
if (cnotes.cmp("clt_conn_tags") != 0)
os << " connection-notes=\"" << cnotes << '"';

os << "\n";
}
14 changes: 13 additions & 1 deletion src/helper/ChildConfig.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
#ifndef SQUID_SRC_HELPER_CHILDCONFIG_H
#define SQUID_SRC_HELPER_CHILDCONFIG_H

#include "sbuf/SBuf.h"

namespace Helper
{

Expand All @@ -34,6 +36,9 @@ class ChildConfig
int needNew() const;
void parseConfig();

/// produce squid.conf syntax for mgr:config report
void printConfig(StoreEntry *, const char *directive);

/**
* Update an existing set of details with new start/max/idle/concurrent limits.
* This is for parsing new child settings into an object incrementally then updating
Expand Down Expand Up @@ -107,13 +112,20 @@ class ChildConfig

/// older stateful helper server reservations may be forgotten
time_t reservationTimeout = 64; // reservation-timeout

/// List of kv-pair keys to set as annotations of the client TCP connection.
/// Default: clt_conn_tag
Comment on lines +116 to +117
Copy link
Copy Markdown
Contributor

@rousskov rousskov Apr 2, 2026

Choose a reason for hiding this comment

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

clt_conn_tag should always annotate TCP connection because that is what happens today, and it would be very easy for an admin to forget to list that annotation name explicitly when customizing their configuration. It is not just the default (to be used when no annotation names were configured); it has to be added to the container even if some other annotation names were configured.

Copy link
Copy Markdown
Contributor Author

@yadij yadij Apr 3, 2026

Choose a reason for hiding this comment

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

IMO clt_conn_tag was a repetition of the external ACL helper "tag=" mistake. A rushed design producing the same name, limitations, side-effects, and long-term support problems all over again.

SBufList clientConnectionTags = { SBuf("clt_conn_tag") };
Copy link
Copy Markdown
Contributor

@rousskov rousskov Apr 2, 2026

Choose a reason for hiding this comment

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

The need to apply annotations to client-to-Squid connections goes beyond helpers. For example, ICAP/eCAP adapters and "note" ACL also set transaction annotations and would benefit from a very similar feature. Please do not place that feature code inside ChildConfig. Let ChildConfig use code defined elsewhere (e.g., AnnotationNames or even Configuration::AnnotationNames).

Copy link
Copy Markdown
Contributor Author

@yadij yadij Apr 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

You propose a much different project. Any code which is shared can be moved if/when other use cases are implemented.

The scope of this PR is implementing a helper API configuration option. Other components of Squid will have their own logic requirements. We should not over-engineer this.

[EDIT: specifically the core logic of this PR is the code of HttpRequest::UpdateRequestNotes() - which is not currently used by any of the components you mention. They need a refactoring to use that function before this PR change is even applicable to them. ]

Copy link
Copy Markdown
Contributor

@rousskov rousskov Apr 2, 2026

Choose a reason for hiding this comment

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

SBufList optimizes insertions and deletions while preserving order and supporting duplicates. For AnnotationNames, we need to optimize search time and ban duplicates. Please use an std::unordered_set container instead of SBufList.


private:
void parseNotesList(const SBuf &);
};

} // namespace Helper

/* Legacy parser interface */
#define parse_HelperChildConfig(c) (c)->parseConfig()
#define dump_HelperChildConfig(e,n,c) storeAppendPrintf((e), "\n%s %d startup=%d idle=%d concurrency=%d\n", (n), (c).n_max, (c).n_startup, (c).n_idle, (c).concurrency)
#define dump_HelperChildConfig(e,n,c) (c).printConfig((e),(n))
#define free_HelperChildConfig(dummy) // NO.

#endif /* SQUID_SRC_HELPER_CHILDCONFIG_H */
Expand Down
Loading