Hello Christopher, Thanks for your research,
On Tue, Jan 11, 2022 at 6:55 PM Christopher Faulet <[email protected]> wrote: > Le 1/10/22 à 23:19, Willy Tarreau a écrit : > > 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. > > > > I agree with Willy on this point. Especially because, depending the order of > commands, the result can be different because of implicit changes and it is > pretty hard to predict how it will behave in all cases. yup totally agree with both of you. I can't really remember why I did that in the first place. > For instance, to fix William's bug about "set server ssl" command, in a way or > another, we must stop to dynamically update the health-check if its port or > its > address is explicitly specified. With this change, the result of following set > of commands will be different: > > $ set server BK/SRV ssl on > $ set server BK/SRV check-port XXX > > ==> SSL is enabled for the server and the health-check interesting one :)) > $ set server BK/SRV check-port XXX > $ set server BK/SRV ssl on > > ==> because the check's port was updated first, the SSL is only enabled for > the > server. > Agree. But, if possible, a warning may be added in the documentation to warn > about implicit changes. >From the discussion, I would be tempted to say the opposite, as I feel like keeping implicit things for this command is worse. > About the fix, I checked the code, and, at first glance, there is no reason to > change "srv->check.use_ssl" value when the health-check uses the server > settings. Thus, we may fix "set server ssl" command this way: > > diff --git a/src/check.c b/src/check.c > index f0ae81504..8cf8a1c5b 100644 > --- a/src/check.c > +++ b/src/check.c > @@ -1565,10 +1565,8 @@ int init_srv_check(struct server *srv) > * default, unless one is specified. > */ > if (!srv->check.port && !is_addr(&srv->check.addr)) { > - if (!srv->check.use_ssl && srv->use_ssl != -1) { > - srv->check.use_ssl = srv->use_ssl; > - srv->check.xprt = srv->xprt; > - } > + if (!srv->check.use_ssl && srv->use_ssl != -1) > + srv->check.xprt = srv->xprt; > else if (srv->check.use_ssl == 1) > srv->check.xprt = xprt_get(XPRT_SSL); > srv->check.send_proxy |= (srv->pp_opts); indeed this is simplified that way > diff --git a/src/server.c b/src/server.c > index 2061153bc..a18836a71 100644 > --- a/src/server.c > +++ b/src/server.c > @@ -2113,10 +2113,11 @@ void srv_set_ssl(struct server *s, int use_ssl) > return; > > s->use_ssl = use_ssl; > - if (s->use_ssl) > - s->xprt = xprt_get(XPRT_SSL); > - else > - s->xprt = s->check.xprt = s->agent.xprt = xprt_get(XPRT_RAW); > + s->xprt = (s->use_ssl == 1) ? xprt_get(XPRT_SSL) : xprt_get(XPRT_RAW); > + > + if ((s->check.state & CHK_ST_CONFIGURED) && !s->check.use_ssl && > + !s->check.port && !is_addr(&s->check.addr)) > + s->check.xprt = s->xprt; > } as mentioned above I am not sure I am aligned here; I would rather remove the side effect of changing s->check. > #endif /* USE_OPENSSL */ > > This may be done with the following 3 steps: > > * First, stop to change "srv->check.use_ssl" value > > * Then, stop to change the agent settings in srv_set_ssl() because there is > no ssl support for agent-check. good point, I did not know > * Finally, fix the bug identified by William, adding the according > documentation. > > Note I don't know if all this stuff works properly with the server-state > file... I am always scared to look at the server state functionality... -- William
