On Mon, May 17, 2021 at 7:05 PM Han Zhou <hz...@ovn.org> wrote: > > > > On Mon, May 17, 2021 at 6:08 PM Mark Michelson <mmich...@redhat.com> wrote: > > > > Hi Han, > > > > My comments below apply equally to the other patches in this series > > since they are generally similar. > > > > I think each patch could use a simple accompanying test case. The test > > could ensure that updates to the files are picked up by the OVN > > component. It could also potentially ensure that nothing awful happens > > if, say, you delete one or more of the files. > > > Thanks Mark for the review. > Ok, I was manually testing by playing around with the expiration time of the certs. Let me see if I can work out a way that is feasible for a test case. > > > See below for more: > > > > On 5/13/21 6:46 PM, Han Zhou wrote: > > > When SSL configurations are set in Open_vSwitch SSL table, > > > ovn-controller handles file update properly by re-applying the settings > > > in the main loop. However, it is also valid to set the options in > > > command line of ovn-controller without using the SSL table. In this > > > case, the options are set onetime only and it never reapplies when the > > > file content changes. This patch fixes this by allowing reapplying the > > > command line options in the main loop, if they are set. SSL table > > > settings still takes precedence if both exist. > > > > > > Signed-off-by: Han Zhou <hz...@ovn.org> > > > --- > > > controller/ovn-controller.c | 24 +++++++++++++++++++++++- > > > 1 file changed, 23 insertions(+), 1 deletion(-) > > > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > > index 67c51a86f..5a755276b 100644 > > > --- a/controller/ovn-controller.c > > > +++ b/controller/ovn-controller.c > > > @@ -97,6 +97,11 @@ static unixctl_cb_func debug_delay_nb_cfg_report; > > > static char *parse_options(int argc, char *argv[]); > > > OVS_NO_RETURN static void usage(void); > > > > > > +/* SSL options */ > > > +static const char *ssl_private_key_file; > > > +static const char *ssl_certificate_file; > > > +static const char *ssl_ca_cert_file; > > > + > > > /* By default don't set an upper bound for the lflow cache. */ > > > #define DEFAULT_LFLOW_CACHE_MAX_ENTRIES UINT32_MAX > > > #define DEFAULT_LFLOW_CACHE_MAX_MEM_KB (UINT64_MAX / 1024) > > > @@ -441,6 +446,11 @@ update_ssl_config(const struct ovsrec_ssl_table *ssl_table) > > > if (ssl) { > > > stream_ssl_set_key_and_cert(ssl->private_key, ssl->certificate); > > > stream_ssl_set_ca_cert_file(ssl->ca_cert, ssl->bootstrap_ca_cert); > > > + } else if (ssl_private_key_file && ssl_certificate_file && > > > + ssl_ca_cert_file) { > > > > Why must all three of these be non-NULL to call the stream_ssl functions > > below? Since the command line options used to result in individual calls > > to stream_ssl functions while parsing options, this represents a > > potential behavior change. For instance, if a user had for some reason > > only specified the -C option when starting ovn-controller, we would have > > called stream_ssl_set_ca_cert_file(). But now if they only specify -C, > > then we will not call stream_ssl_set_ca_cert_file() since they did not > > also set the -c and -p options. > > > IIUC, the three files must be supplied for the SSL to work, so I think it makes no sense to proceed if any of them is NULL. > However, you are right that this is a behavior change. In the old implementation, there will be error logs complaining the missing file, but now it would be complaining about all the three files. Since the stream_ssl_xxx() interfaces already checks NULL, I will skip the NULL check here and call the functions with whatever is available in v2. > Hi Mark, I think it is still necessary to check NULL pointers, so in v2 I just change the if condition to check key+cert and cacert separately before calling the respective ssl_set_xxx functions. Please take a look at v2: https://patchwork.ozlabs.org/project/ovn/list/?series=244813
Thanks, Han _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev