On 2017/6/30 01:00, Willy Tarreau wrote:
> Hi Patrick, sorry for the delay :-/
>
> On Mon, Jun 19, 2017 at 01:54:36PM -0400, Patrick Hemmer wrote:
>> Well my argument for keeping the name starting with `ssl_fc_session_` is
>> that there is also `ssl_fc_session_id`. These 2 fetches pull their
>> attribute from the same "session" structure. They are also closely
>> related as using `ssl_fc_session_key` almost requires the
>> `ssl_fc_session_id` value (you could technically get by without it, but
>> it'll make using the key rather difficult unless you have some other way
>> of correlating a key with a specific SSL session).
> OK, that totally makes sense then.
>
>>>> static int
>>>> +smp_fetch_ssl_fc_session_key(const struct arg *args, struct sample *smp,
>>>> const char *kw, void *private)
>>>> +{
>>>> +#if OPENSSL_VERSION_NUMBER >= 0x10100000L || defined(OPENSSL_IS_BORINGSSL)
>>> Here I'd put the ifdef around the whole function, like we have for
>>> ALPN for example, so that there's no risk it could be used in a non-working
>>> config.
>> My objection to this is that most of the other sample fetches don't do
>> this, and so it makes the user experience inconsistent. For example the
>> `ssl_fc_session_id` fetch, which `ssl_fc_session_key` is strongly
>> related to, behaves the same way.
> But this has already been causing problems, because people build with their
> lib, then configure their logs, and suddenly realize that the field is empty
> and report the bug here. The issue I'm having is that there's no notification
> that this will not work. Using #ifdef ensures that what is not supported will
> report an error. Then the user looks at the keyword in the doc and reads
> "requires openssl 1.1 or above" and understands why there's this problem.
I can include an additional patch to change the existing SSL fetches to
the desired behavior. Would that be acceptable?
>
>> The ALPN/NPN fetches are the only ones
>> that make using the fetch a config error if the underlying support is
>> missing.
> For SSL indeed, but if you look at other places like TCP, bind keywords
> or server keywords, it's exactly the opposite. There are other places
> which are cleaner, it's only the arg resolver which has the ifdef so
> that instead of not parsing, it's parsed and reported as "not supported"
> or "requires version blah". I just can't find such an example but I'm
> pretty sure we do have some.
>
> Cheers,
> Willy
