On Mon, May 17, 2021 at 6:08 PM Mark Michelson <[email protected]> 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 <[email protected]> > > --- > > 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. > > + stream_ssl_set_key_and_cert(ssl_private_key_file, > > + ssl_certificate_file); > > + stream_ssl_set_ca_cert_file(ssl_ca_cert_file, false); > > } > > } > > > > @@ -3320,7 +3330,19 @@ parse_options(int argc, char *argv[]) > > > > VLOG_OPTION_HANDLERS > > OVN_DAEMON_OPTION_HANDLERS > > - STREAM_SSL_OPTION_HANDLERS > > + > > + case 'p': > > + ssl_private_key_file = optarg; > > + break; > > + > > + case 'c': > > + ssl_certificate_file = optarg; > > + break; > > + > > + case 'C': > > + ssl_ca_cert_file = optarg; > > + break; > > + > > Is there any downside to not calling the stream_ssl_set_*() functions at > this point and waiting until the main loop to do it? > There shouldn't be any difference because in ovn-controller and other daemons the first attempt to connect to SB is after calling update_ssl_config() in main loop. For nbctl daemon mode it is different, so the update_ssl_config() is called directly in the apply_options_direct(). Thanks, Han > > > > case OPT_PEER_CA_CERT: > > stream_ssl_set_peer_ca_cert_file(optarg); > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
