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
