Hi Alexander,
finally back to this! Overall it's a great and very clean series, I really
want to thank you for this high quality work!
On Wed, Aug 16, 2023 at 04:24:21PM +0000, Stephan, Alexander wrote:
> I was not able to use a check function for authority and unique_id without
> modifying sample.c or allowing an argument. As far as I can tell, the check
> function is only executed for sample fetches that have not specified 0 for
> the arg function. I think it's okay this way since, semantically, those
> function do not handle arguments, so doing the argument setup internally
> makes sense IMO.
Yeah it initially gave me a bit of head scratching when reading this part
but I understood what you did and find that it pretty much makes sense
after all.
> If there is way to adjust this without any hacks, I will of course apply it.
It's not necessarily a hack given that what you've done consists in passing
a pre-parsed argument internally. It's not much different from the few cases
where we emulate older functions or actions using new mechanisms, building
some config elements on the fly just to parse them.
> For now, I have appended them to this message to not cause to much spam on
> the list while also keeping the previous, related discussion.
> If I should send them individually, please tell me and I will do so.
I tend to *prefer* to have them separately for long series that take a lot
of time to review and cannot be done at once. But this series could be
reviewed at once, most patches remain small enough so that's no big deal.
> Restructuring, especially in regard to pooling lead to quite a bit of
> changes, but the overall logic still applies and should hopefully be
> relatively straight forward.
> Actually, the code even got simpler in many places! :)
I noticed as well. I think I understood it all, which is a good point.
> BTW, I also added some TLV type constants in the 6th patch, feel free to
> merge it or ignore it. I think it helps with readability and is not really
> risky.
I'm fine with this, however I find that the doc is not very clear about
what is permitted:
fc_pp_tlv(<tlv>) : string
Returns the TLV value for the given TLV ID or type constant sent by the
client in the PROXY protocol header, if any. TLV constants correspond
to their type suffix as specified in the PPv2 spec, e.g., AUTHORITY.
It's not clear to me as a user that this <tlv> argument supports a
numeric value, nor what is the allowed range (I don't even know
personally). And given that you added a number of special keywords,
it's important that those recognized by the code are also mentioned
here. I would suggest something around:
... the given TLV ID which must either be a numeric value between
XXX and YYY included, or one of the supported following symbolic
names that correspond to the TLV constant suffixes in the PPv2 spec:
AUTHORITY:x, ...
I would appreciate it if you can just reword this part with the ranges,
names and values according to what you know about it, I'll happily
apply it directly into your last patch.
> If there should be a nit that you quickly want to change, feel free to. I am
> not upset about it at all.
Unless you're having any objection, I'm going to flip type and len below:
struct conn_tlv_list {
struct list list;
unsigned char type;
unsigned short len; // 65535 should be more than enough!
char value[0];
} __attribute__((packed));
These result in <len> being unaligned, which is less clean on some archs,
and would also make the compiler complain if a pointer to len is ever
passed to a function taking a short*.
Finally, I noticed a behaviour change and I don't know if it's intentional
or not, but I think it needs to be discussed and possibly also documented.
Prior to your patchset, the authority and uniqueid were unique (I don't
know which of the first or last was kept, and I haven't checked but it
could even be possible that in case of duplicate it results in a memory
leak). But at least a single one was used. Now since you're setting
NOT_LAST on the sample, multiple such samples can be checked, e.g. in an
ACL that comes back and retrieves the next TLV of the same type when looking
for something. While I find this good, I do not think it is a good idea to
support that for important stuff like AUTHORITY or UNIQUEID: if an upstream
equipment can be fooled into sending more than one of each, the behavior
can be particularly confusing and may even be abused. I think this could
be addressed like this:
/* fetch the authority TLV from a PROXY protocol header */
int smp_fetch_fc_pp_authority(const struct arg *args, struct sample *smp,
const char *kw, void *private)
{
struct arg tlv_arg;
int ret;
set_tlv_arg(PP2_TYPE_AUTHORITY, &tlv_arg);
ret = smp_fetch_fc_pp_tlv(&tlv_arg, smp, kw, private);
smp->flags &= ~SMP_F_NOT_LAST; // return only the first authority
return ret;
}
In this case it could be clarified in the doc that the sample fetch
functions for authority/uniqueid only return the first of each, and that
fc_pp_tlv() iterates over all occurrences of the requested ID. This would
then place a clear separation between trying to extract THE authority, or
checking ALL TLVs of type AUTHORITY.
What do you think ?
If you're OK with this, I'd appreciate it if you could send a few informal
incremental patches that I'd squash into yours. I had to resolve some small
context conflicts by hand so I'd rather keep the branch as-is than take a
new series ;-) Or alternatively I'm pushing it as "20230828-pp2-tlv-1" if
you prefer to rework it, just do as you prefer, I can adapt.
Thanks!
Willy
