On Tue, Jan 11, 2022 at 06:55:10PM +0100, Christopher Faulet 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.
> 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
> $ 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.
> > 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.
> Agree. But, if possible, a warning may be added in the documentation to warn
> about implicit changes.
> 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);
> 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;
> }
> #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.
> * 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...
> --
> Christopher Faulet
>
To add more context on this discussion, I looked at dynamic servers.
Currently, the behavior is identical with the configuration as
init_srv_check() is used in the "add server" CLI handler. This is really
problematic as "no-check-ssl" is not available for dynamic servers, so
if I understand correctly it's not possible to add a dynamic SSL server
with checks on the same port without SSL on checks.
Christopher's patch for init_srv_check() is probably a good idea, but if
not taken it should be probably at least used for servers with the
SRV_F_DYNAMIC flag.
--
Amaury Denoyelle
