Hi Willy, Sorry for the late reply. I know it has been some time since our last discussion. I finally have a chance to follow up on this after working on some other tasks and the Thanksgiving break.
What I'm suspecting is that if users find it useful, it will not be long before some ask for a way to designate prefixes (e.g. select x-companyname-*, or exclude accept-* etc). The way you did it will make it easy to extend it for this, for example, by appending '*' to a header name, so that's fine. Hmmm well, '*' is actually permitted in header field names, so maybe another solution could be that we consider that we'd use prefixes by default and terminante names with ':' to designate full names. E.g. req.hdrs(accept:) would match only "accept" while req.hdrs(accept) would also match accept-encoding, accept-range etc. It's just up to us to decide (and you first since you're the first one to need this, so the ability to extend this and/or to factor arguments may be relevant to your use case. Yeah, I think that would be a possible ask / need from the client. Currently, we don't have the immediate need for this but I agree at some point this feature might come in handy even for us as well. I am thinking maybe it can be left as a separate follow-up patch after this one? Let me know if you feel strongly that we should include this in the current patch. Similarly I'm seeing that having the '!' in the first argument does add some special cases everywhere down the inner loop. Maybe just having a new name such as "req.hdrs_exc()" to enumerate exclusion would simplify everything, and possibly make it easier even for APIs which will feed these arguments, so as to remove the special case of the first argument. I don't know, but feel free to explore such possibilities, it's important to think about how configs will be used and updated. Very often the amount of work needed to make the core easy to use is less than the work needed externally to adapt to it ;-) This makes sense. After discussing within the team, we think it might be easier to just add separate new fetchers like "req.hdrs_inc", "req.hdrs_exc", "res.hdrs_inc", "res.hdrs_exc" to be clearer what each fetcher is doing. This way, we can also make sure to not impact the existing flow for "req.hdrs" and "res.hdrs", wondering what are your thoughts on this? Thanks, Ruei-Bang ________________________________ From: Willy Tarreau <w...@1wt.eu> Sent: Thursday, November 9, 2023 9:22 PM To: Ruei-Bang Chen <ruec...@linkedin.com> Cc: haproxy@formilux.org <haproxy@formilux.org> Subject: Re: [PATCH] MEDIUM: sample: Modify fetchers for req.hdrs and res.hdrs to selectively include / exclude headers Hi Ruei-Bang, On Fri, Nov 10, 2023 at 01:33:01AM +0000, Ruei-Bang Chen wrote: > Hi team, > > Based on the feedback from > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.mail-archive.com%2Fhaproxy%40formilux.org%2Fmsg44153.html&data=05%7C01%7Cruechen%40linkedin.com%7Ca24985ccb6534318c6ba08dbe1ad10e5%7C72f988bf86f141af91ab2d7cd011db47%7C0%7C0%7C638351905684956832%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=htOFRDEBc0GIbCF6EbcFmFZK0A6U%2FM8HGhP6OIbsi2c%3D&reserved=0<https://www.mail-archive.com/haproxy@formilux.org/msg44153.html>, > I have > attached a patch for modifying fetchers for req.hdrs and res.hdrs to > selectively include / exclude headers. Great! > I have the following 3 questions while I was doing this change: > > 1. Should I place the helper function skip_hdr_for_fetch within > http_fetch.c or there is a better place for this? I think it's fine like this. > 2. Should I do this to smp_fetch_hdrs_bin as well? I can update the code, > comments, documentation if that is needed. Indeed, that would be an excellent idea, in order to maintain consistency between the two. > 3.> Not sure if this is the best style for modifying the fetch to support > 10 arguments as it does not align with other fetchers due to the huge > number of characters required. I had a look at it, and it's fine to me. What I'm suspecting is that if users find it useful, it will not be long before some ask for a way to designate prefixes (e.g. select x-companyname-*, or exclude accept-* etc). The way you did it will make it easy to extend it for this, for example, by appending '*' to a header name, so that's fine. Hmmm well, '*' is actually permitted in header field names, so maybe another solution could be that we consider that we'd use prefixes by default and terminante names with ':' to designate full names. E.g. req.hdrs(accept:) would match only "accept" while req.hdrs(accept) would also match accept-encoding, accept-range etc. It's just up to us to decide (and you first since you're the first one to need this, so the ability to extend this and/or to factor arguments may be relevant to your use case. If you want to slightly optimize the loop in smp_fetch_hdrs(), you should call skip_hdr_for_fetch() only when you have at least one argument, so that by default when called with no argument, it skips it. I don't know what the most common use case will be, to be honest, so just take this as a hint, maybe we don't care that much about all headers given that the overall processing around it will be expensive anyway. > Please let me know what are your thoughts on this. I think it's quite nice like this and you patch looks clean. Please just consider the points above in case that gives you some ideas. Similarly I'm seeing that having the '!' in the first argument does add some special cases everywhere down the inner loop. Maybe just having a new name such as "req.hdrs_exc()" to enumerate exclusion would simplify everything, and possibly make it easier even for APIs which will feed these arguments, so as to remove the special case of the first argument. I don't know, but feel free to explore such possibilities, it's important to think about how configs will be used and updated. Very often the amount of work needed to make the core easy to use is less than the work needed externally to adapt to it ;-) Maybe other participants here have any idea on this matter ? Thanks! Willy
