Re: [PATCH v2 0/4] add set server ssl command
Hi William, On Tue, Oct 13, 2020 at 04:10:35PM +0200, William Dauchy wrote: > Hi William, Willy, > > On Wed, Oct 7, 2020 at 5:02 PM William Dauchy wrote: > > adding some more thoughts we discussed internally with Pierre: > > - we started to use `show servers state`, as it was the only way to > > get the current config for the parameters we wanted to change with the > > existing runtime API. > > - even if it was not necessarily designed for that purpose, we need a > > way to get the current config. > > - we understood `show servers state` was more likely designed for > > internal usage, especially for `load-server-state-from-file`, so that > > my patch set has some implications. > > > > That being said, would it more acceptable to add a new API which which > > reflects all the `set` commands such as: > > - show server [/] > > would display all the config of a given server or all of them > > - show server / agent > > - show server / state > > - show server / weight > > etc > > and for my first proposition: > > - show server ssl [/] > > - set server ssl [/] on/off > > > > for our immediate usage around ssl we are also thinking about ca_file, > > crt, crt-file. > > > > This also would be a good answer to simplify how it is managed through > > the current dataplane API > > https://www.haproxy.com/documentation/dataplaneapi/latest/#operation/createServer > > > > What are your thoughts about it? > > any thoughts about this proposition? We've discussed about some of these points last week with William. I think at some point it would be useful to have a discussion on these. I'm personally in favor of being able to configure more things dynamically, I also know that server-state files tend to be abused to store configuration parameters instead of just pure server states, but overall based on what is needed, we could possibly rethink the whole thing, to have for example: - servers fed from a dedicated, easily parsable/updatable file - ssl optionally pre-configured to allow easy on/off at run time - better detection of changed parameters across reloads (currently the server-state file doesn't hold the config values, so that there are quite a number of ambigous cases on reload). And clearly I'd rather go back to the white board based on a clearly defined set of requirements and use cases than add more hacks on top of something that was not initially designed for this. For sure some parts are still extensible (e.g. I don't see anything preventing from preloading SSL contexts into servers at boot time), but that still leaves us with server-state, templates etc and maybe we can do better, simpler, with less impact for everyone. We should probably have a live discussion between at least the 3 of us, maybe a few more if that helps, and define how to go into that direction. What do you think ? Thanks, Willy
Re: [PATCH v2 0/4] add set server ssl command
Hi William, Willy, On Wed, Oct 7, 2020 at 5:02 PM William Dauchy wrote: > adding some more thoughts we discussed internally with Pierre: > - we started to use `show servers state`, as it was the only way to > get the current config for the parameters we wanted to change with the > existing runtime API. > - even if it was not necessarily designed for that purpose, we need a > way to get the current config. > - we understood `show servers state` was more likely designed for > internal usage, especially for `load-server-state-from-file`, so that > my patch set has some implications. > > That being said, would it more acceptable to add a new API which which > reflects all the `set` commands such as: > - show server [/] > would display all the config of a given server or all of them > - show server / agent > - show server / state > - show server / weight > etc > and for my first proposition: > - show server ssl [/] > - set server ssl [/] on/off > > for our immediate usage around ssl we are also thinking about ca_file, > crt, crt-file. > > This also would be a good answer to simplify how it is managed through > the current dataplane API > https://www.haproxy.com/documentation/dataplaneapi/latest/#operation/createServer > > What are your thoughts about it? any thoughts about this proposition? Thanks, -- William
Re: [PATCH v2 0/4] add set server ssl command
Hello, adding some more thoughts we discussed internally with Pierre: - we started to use `show servers state`, as it was the only way to get the current config for the parameters we wanted to change with the existing runtime API. - even if it was not necessarily designed for that purpose, we need a way to get the current config. - we understood `show servers state` was more likely designed for internal usage, especially for `load-server-state-from-file`, so that my patch set has some implications. That being said, would it more acceptable to add a new API which which reflects all the `set` commands such as: - show server [/] would display all the config of a given server or all of them - show server / agent - show server / state - show server / weight etc and for my first proposition: - show server ssl [/] - set server ssl [/] on/off for our immediate usage around ssl we are also thinking about ca_file, crt, crt-file. This also would be a good answer to simplify how it is managed through the current dataplane API https://www.haproxy.com/documentation/dataplaneapi/latest/#operation/createServer What are your thoughts about it? -- William
Re: [PATCH v2 0/4] add set server ssl command
Hello William, Thank you for your answer. On Tue, Oct 6, 2020 at 7:17 PM William Lallemand wrote: > The problem with activating SSL on-the-fly is that SSL is not only an > on/off option but there are a lot of parameters that can be configured, > and that won't fit the server state file. I fear it will complicate a > lot of things in the future in this form. My plan was to iterate on this and add other possible parameters to be updatable on the fly. But now that you raised the "server state file", I remember it is used for the "load-server-state-from-file". So it is starting to be tricky if we add other parameters. I indeed overlooked the problem around `server state` as we use it externally in our control plane: if we detect a diff, we either try to make the change through the API, or through a reload in the worst case scenario. I overlooked the origin use case for "load-server-state-from-file" and we have built a lot of things on top of `show servers state`. > Maybe you could have pre-configured but disabled servers with SSL in your > configuration and enable them progressively with the CLI instead ? this is not an option for us as it would over-complexify our control plane. That being said, I now completely understand this patchset cannot be accepted as is unless we would agree on a list of parameters to be added to `show servers state`? Maybe a good opportunity to start a discussion and find alternative ways? -- William
Re: [PATCH v2 0/4] add set server ssl command
On Sun, Oct 04, 2020 at 08:13:11PM +0200, William Dauchy wrote: > Hello, > > This patchset is an attempt to add a new command for configure ssl on > server at runtime: > > - the first patch adds the possibility to observe the change on a `show > servers state`. > - the two next ones are only here to prepare the last one to add the > command. I added them separatly to facilitate the review. > `ssl_sock_prepare_srv_ctx` protection is not mandatory but I found it > safer while writing my patch. > - the last one is adding the new command. I'm not 100% sure of the > consequences of`prepare_srv` and `destroy_srv` but from what I read > and tested, it seems ok. > That's an interesting idea but I'm kind of confused about this. The problem with activating SSL on-the-fly is that SSL is not only an on/off option but there are a lot of parameters that can be configured, and that won't fit the server state file. I fear it will complicate a lot of things in the future in this form. Maybe you could have pre-configured but disabled servers with SSL in your configuration and enable them progressively with the CLI instead ? Willy has maybe a better suggestion about this. -- William Lallemand
[PATCH v2 0/4] add set server ssl command
Hello, This patchset is an attempt to add a new command for configure ssl on server at runtime: - the first patch adds the possibility to observe the change on a `show servers state`. - the two next ones are only here to prepare the last one to add the command. I added them separatly to facilitate the review. `ssl_sock_prepare_srv_ctx` protection is not mandatory but I found it safer while writing my patch. - the last one is adding the new command. I'm not 100% sure of the consequences of`prepare_srv` and `destroy_srv` but from what I read and tested, it seems ok. --- changed in v2: - patch1/4: reorder parameters to match format string - patch3/4: reorder includes, error introduced while splitting my patch. --- William Dauchy (4): MINOR: cli/proxy: add `srv_use_ssl` to `show servers state` MINOR: ssl: protect ssl_sock_prepare_srv_ctx from double ctx allocation MINOR: ssl: create common ssl_ctx init MINOR: cli/ssl: configure ssl on server at runtime doc/management.txt | 4 +++ include/haproxy/server-t.h | 3 ++- include/haproxy/server.h | 2 ++ src/cfgparse-ssl.c | 32 +++--- src/proxy.c| 5 ++-- src/server.c | 55 +- src/ssl_sock.c | 16 --- 7 files changed, 81 insertions(+), 36 deletions(-) -- 2.28.0
