Hi William! On Mon, Jan 10, 2022 at 10:49:38PM +0100, William Dauchy wrote: > On Mon, Jan 10, 2022 at 7:51 AM Willy Tarreau <[email protected]> wrote: > > 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. > > note it is only true in your example if you use another port.
Hmmm I didn't remember about this :-( > > 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, > > sorry what do you mean by manually? I meant "on the CLI". I.e. you added support for enabling/disabling SSL while few options were still configurable on the CLI by then. > "check-ssl" has been available for a long time, so that's not the > reason behind it, but I guess you were referring to something else. I > suspect I did a dumb copy/paste from the new_server function and > probably never thought was possibly wrong as my previous production > never had any check using tls. Maybe but then I don't remember about all the detailed rules in place that indicate when check-ssl *has* to be used. I'll have to read the doc. At this point I'm starting to think that we should probably avoid as much as possible to use implicit settings for whatever is dynamic. Originally a lot of settings were implicit because we don't want to have huge config lines to enforce lots of obvious settings. On the CLI it's less of a problem and for example if "ssl" only deals with the traffic without manipulating the checks, I'm personally not shocked, because these are normally sent by bots and we don't care if they have to set a few more settings to avoid multiple implicit changes that may not always be desired. This is where the CLI (or any future API) might differ a bit from the config, and an agent writing some config might have to explicitly state certain things like "no-check-ssl" for example to stay safe and avoid such implicit dependencies. Note that such a brainstorming doesn't apply to your fix and should not hold it from being merged in any way, I'm just speaking in the general sense, as I don't feel comfortable with keep all these special cases in a newly introduced API, that are only there for historical reasons. > > 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. > > unless I missed something, for me the current behavior is broken as > you can't come back to a working state if you are using tls on both > traffic and health check path. That's my understanding as well. > The only working setup is when you are > using `no-check-ssl` in your default server. In that sense I believe > it should be backported to v2.4. If you're *certain* that we're not going to break existing 2.4 deployments that could rely on the current behavior, I'm fine with that. It's just that I absolutely hate behavior changes in stable versions because we tend to think we've seen all corner cases, and after a release someone reports an issue :-/ I can't think of a case which would benefit from the current bug, I'm just trying to be careful. Thanks! Willy
