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

Mark Michelson mmichels at redhat.com
Tue May 18 01:08:10 UTC 2021


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.

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.

> +        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?

>   
>           case OPT_PEER_CA_CERT:
>               stream_ssl_set_peer_ca_cert_file(optarg);
> 



More information about the dev mailing list