[ovs-dev] [PATCH ovn 1/5] ovn-controller: Support ssl cert rotation when command line options are used.

Han Zhou hzhou at ovn.org
Thu May 20 01:21:54 UTC 2021


On Mon, May 17, 2021 at 7:05 PM Han Zhou <hzhou at ovn.org> wrote:
>
>
>
> On Mon, May 17, 2021 at 6:08 PM Mark Michelson <mmichels at 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 <hzhou at 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


More information about the dev mailing list