Hi William ! Sorry for the delay. We have rediscussed this issue this morning and here is my answer on your patch.
It is definitely legitimate to want to be able to use reverse HTTP without SSL on the server line. However, the way that haproxy currently uses idle connection is that at least the SNI parameter alone must be set to match the name parameter of the corresponding attach-srv rule. If this is not the case, the connection will never be reused. But in fact, when rereading haproxy code this morning, we found that it is valid to have SNI parameter set even if SSL is not active, and this will produce the desired effect. We are definitely okay with merging an adjusted version of your patch for the moment, see my remarks below. However, using SNI on a server line without SSL is something tricky. Thus we plan to add a new keyword to replace it when SSL is not used to have the same effect. When this will be done, you should update your configuration to use it. On Fri, Apr 12, 2024 at 02:29:30PM +0100, William Manley wrote: > An attach-srv config line usually looks like this: > tcp-request session attach-srv be/srv name ssl_c_s_dn(CN) > The name is a key that is used when looking up connections in the > connection pool. Without this patch you'd get an error if you passed > anything other than "ssl_c_s_dn(CN)" as the name expression. Now you can > pass arbitrary expressions and it will just warn you if you aren't > producing a configuration that is RFC compliant. > I'm doing this as I want to use `fc_pp_unique_id` as the name. I'm not 100% okay with your description here. The current code condition does not check that "ssl_c_s_dn(CN)" is used as expression, but rather that if name is defined for an attach-srv rule, the targetted server must have both SSL and SNI activated. I think this paragraph should be reworded. > --- > src/tcp_act.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > diff --git a/src/tcp_act.c b/src/tcp_act.c > index a88fab4af..4d2a56c67 100644 > --- a/src/tcp_act.c > +++ b/src/tcp_act.c > @@ -522,8 +522,7 @@ static int tcp_check_attach_srv(struct act_rule *rule, > struct proxy *px, char ** > > if ((rule->arg.attach_srv.name && (!srv->use_ssl || !srv->sni_expr)) || > (!rule->arg.attach_srv.name && srv->use_ssl && srv->sni_expr)) { > - memprintf(err, "attach-srv rule: connection will never be used; > either specify name argument in conjunction with defined SSL SNI on targeted > server or none of these"); > - return 0; > + ha_warning("attach-srv rule: connection may never be used; > usually name argument is defined SSL SNI on targeted server or none of > these"); > } > > rule->arg.attach_srv.srv = srv; > -- > 2.34.1 > I think an alternative patch may be more desirable here. We could update the condition with the following statement without removing the fatal error : > if ((rule->arg.attach_srv.name && !srv->sni_expr) || > (!rule->arg.attach_srv.name && srv->sni_expr)) { This reflects the current mandatory usage of reverse-http : if name is used on attach-srv, sni keyword must be specified on the server line. Let me know your thoughts. If you're okay, can you adjust your patch please ? If not, do not hesitate to tell me if there is something you disagreeing with. Thanks, -- Amaury Denoyelle