On Thu, Jan 06, 2022 at 04:57:15PM +0100, William Dauchy wrote: > While giving a fresh try to `set server ssl` (which I wrote), I realised > the behavior is a bit inconsistent. Indeed when using this command over > a server with ssl enabled for the data path but also for the health > check path we have: > > - data and health check done using tls > - emit `set server be_foo/srv0 ssl off` > - data path and health check path becomes plain text > - emit `set server be_foo/srv0 ssl on` > - data path becomes tls and health check path remains plain text > > while I thought the end result would be: > - data path and health check path comes back in tls > > In the current code we indeed erase all connections while deactivating, > but restore only the data path while activating. I made this mistake in > the past because I was testing with a case where the health check plain > text by default.
It's important to always keep in mind that checks are not necessarily related to the production traffic, and that configuring one part should not have any impact on the other one. By default a server running in SSL will not be checked using SSL unless "check-ssl" is set. You could for example have a server forwarding to multiple ports (say 80 and 443) and decide to check only one of them, or even check another one. As such, I think your patch is correct as it only affects what the user attempts to modify. I suspect that the reason for your initial choice was that it was not yet possible by then to enable SSL checks manually, it would be worth rechecking, because if that's the case, maybe we should not backport it to 2.4 and only document a behavior change between 2.4 and 2.5. If you could have a double-check at the history behind this, that would be nice so that we know how far to backport it. By the way, maybe your proposed alternative would be acceptable for older versions which do not allow to enable SSL health checks on the CLI. Thanks, Willy
