RFC: Support connection-linked helper notes

[yadij]

Allow admin to configure which helper kv-pair notes
to share across transactions sharing a client TCP
connection.

Previously, this functionality was limited to notes named clt_conn_tag.

[rousskov]

I support adding this feature, in principle.

Are all annotations not named by the new configuration option guaranteed to not be treated like clt_conn_tag? Or will we make that decision later, possibly on a helper-by-helper basis? In other words, are we drawing a hard line here or just adding one more way to say "clt_conn_tag", with the rest of it still being undecided/uncertain at this point? For example, are these changes compatible, in principle, with helper-driven decision where helpers send clt_conn_*=value annotations and expect those annotations to annotate the client connection without any squid.conf changes?

Will the new configuration option apply to all key=value fields returned by the helper or only to the custom fields that Squid does not already handle specially (e.g., will some of the annotations listed in Notes::ReservedKeys() be banned from being listed as connection-annotating)?

It would also be nice to document which same-connection transactions are going to get the annotations set by another transaction. I might say something about getting request headers (or equivalent) parsed being the moment when the previously stored connection annotations would be copied to the "new" transaction, but I have not checked those details.

We should also document that once associated with a client connection, connection annotations survive Squid reconfiguration, even if the new configuration no longer names them.

Finally, we should mention that annotation names are case sensitive.

All those documentation request should ultimately be addressed by cf.data.pre changes, of course, but we can abuse PR description to finalize those decisions first and then move that text to cf.data.pre, with proper formatting.

PR description: Previously this was limited to exactly one note called clt_conn_tag which was erased and replaced each time a new value was received from any helper.

I have rephrased that paragraph to avoid an implication that the new functionality does not "erase and replace" like clt_conn_tag did. This PR preserves that old functionality. New option documentation should be explicit about that functionality, of course.

P.S. This RFC has a lot of code noise that makes it difficult for me to see the key changes we are supposed to review at this stage. AFAICT, most of those non-key changes can and should be avoided (in this PR). I tried to filter out that noise during this initial review, but I could easily miss something important.

[rousskov]

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.

[yadij]

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.

[rousskov]

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(); }

[rousskov]

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).

[yadij]

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. ]

[rousskov]

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.

[rousskov]

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

[yadij]

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

[yadij]

I support adding this feature, in principle.

Are all annotations not named by the new configuration option guaranteed to not be treated like clt_conn_tag?

The answer is "yes" for code touched by this PR.
Of course, no guarantee regarding code outside what this PR adds.

In other words, are we drawing a hard line here or just adding one more way to say "clt_conn_tag", with the rest of it still being undecided/uncertain at this point?

Yes, "just another way". Although a super-set way with more flexibility on key names copied.

For example, are these changes compatible, in principle, with helper-driven decision where helpers send clt_conn_*=value annotations and expect those annotations to annotate the client connection without any squid.conf changes?

Compatible in principle. We can string-compare the key name prefix with "clt_conn_" instead of the full "clt_conn_tag" currently coded to implement your proposed naming schema.
I have not seen an actual distinct need for that feature though, so not coded it explicitly here.

( Overall I see design issues with that naming plan, but that is off-topic here. )

Will the new configuration option apply to all key=value fields returned by the helper,

As can be seen in the code only keys which are being added to the transaction HttpRequest are considered. Same restrictions and ban lists apply with no changes.

It would also be nice to document ...

Yes, nothing is documented yet, this being an RFC and draft.

P.S. This RFC has a lot of code noise that makes it difficult for me to see the key changes we are supposed to review at this stage. AFAICT, most of those non-key changes can and should be avoided (in this PR). I tried to filter out that noise during this initial review, but I could easily miss something important.

Sorry, most of it is for build and testability.

The core logic change is in 3c16bed. Specifically in UpdateRequestNotes() the rest of that patch is syntax fluff to pass the pre-parsed config setting in there.

The other commit is contextual necessity for squid.conf handling ... connection-notes="CVS,list,of,key,names" .... The exact lines-of-change vary depending on PRs I submitted earlier to change that parser. Those other PRs merging first would make this PR a lot smaller diff.

[ankor2023]

@yadij
Hello, Amos,

Thank you very much for this PR. I believe it will provide administrators with the means that will help them greatly.

Will the new configuration option apply to all key=value fields returned by the helper,

As can be seen in the code only keys which are being added to the transaction HttpRequest are considered. Same restrictions and ban lists apply with no changes.

    csd->notes()->remove(note->name());
    csd->notes()->add(note->name(), note->value());

If the helper outputs group=g1 group=g2, does the last attribute value override the previous ones? I assume group=g2 will be the one used. Correct me if I'm wrong.

[yadij]

@ankor2023: you are correct only group="group2" will be used due to the replace nature. Which is why the modification you are adding to combine the groups into a list format is useful, or even a prerequisite.

[ankor2023]

@ankor2023: you are correct only group="group2" will be used due to the replace nature. Which is why the modification you are adding to combine the groups into a list format is useful, or even a prerequisite.

Amos, what do you think about implementing a filter method in the NotePairs class as part of this PR (as I mentioned in PR#2395)? This might allow us to add all attribute values and make the current PR more universal.
Also, if the filter method supports the '*' wildcard, we could incorporate Alex's idea into your PR by adding clt_conn_* to the list.

Or is that a bad idea?