| Message ID | 20170824181321.25177-1-mmichels@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Russell Bryant |
| Headers | show |
On Thu, Aug 24, 2017 at 2:13 PM, Mark Michelson <mmichels@redhat.com> wrote: > OVN is lenient with the types of logical switch ports. Maybe too > lenient. This patch attempts to solve this problem on two fronts: > > 1) In ovn-nbctl, if you attempt to set the port type to an unknown > type, the command will not end up setting the type. > 2) In northd, when copying the port type from the northbound database to > the corresponding port-binding in the southbound database, a warning > will be issued if the port is of an unknown type. > > Signed-off-by: Mark Michelson <mmichels@redhat.com> > --- > ovn/lib/ovn-util.c | 30 +++++++++++++++++++ > ovn/lib/ovn-util.h | 2 ++ > ovn/northd/ovn-northd.c | 4 +++ > ovn/utilities/ovn-nbctl.c | 7 ++++- > tests/ovn-nbctl.at | 73 +++++++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 115 insertions(+), 1 deletion(-) > Did you look into whether it was possible to use an enum in the db schema? In particular, what would happen on an upgrade of an existing database? and what happens if the existing db has an invalid type in it already? I just want to make sure we clearly rule that out first. > diff --git a/ovn/lib/ovn-util.c b/ovn/lib/ovn-util.c > index 037d0798a..883ce4ccb 100644 > --- a/ovn/lib/ovn-util.c > +++ b/ovn/lib/ovn-util.c > @@ -299,3 +299,33 @@ default_sb_db(void) > } > return def; > } > + > +/* l3gateway, chassisredirect, and patch > + * are not in this list since they are > + * only set in the SB DB by northd > + */ > +const char *OVN_NB_LSP_TYPES[] = { I would add "static" here. > + "router", > + "localnet", > + "localport", > + "l2gateway", > + "vtep", > +}; > + > +bool > +ovn_is_known_nb_lsp_type(const char *type) > +{ > + int i; > + > + if (!type || !type[0]) { > + return true; > + } > + > + for (i = 0; i < ARRAY_SIZE(OVN_NB_LSP_TYPES); ++i) { > + if (!strcmp(OVN_NB_LSP_TYPES[i], type)) { > + return true; > + } > + } > + > + return false; > +} > diff --git a/ovn/lib/ovn-util.h b/ovn/lib/ovn-util.h > index b3d2125a3..9b456426d 100644 > --- a/ovn/lib/ovn-util.h > +++ b/ovn/lib/ovn-util.h > @@ -67,4 +67,6 @@ char *alloc_nat_zone_key(const struct uuid *key, const char *type); > const char *default_nb_db(void); > const char *default_sb_db(void); > > +bool ovn_is_known_nb_lsp_type(const char *type); > + > #endif > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > index 49e4ac338..177df5a1e 100644 > --- a/ovn/northd/ovn-northd.c > +++ b/ovn/northd/ovn-northd.c > @@ -1962,6 +1962,10 @@ ovn_port_update_sbrec(struct northd_context *ctx, > } > sbrec_port_binding_set_options(op->sb, &options); > smap_destroy(&options); > + if (!ovn_is_known_nb_lsp_type(op->nbsp->type)) { > + VLOG_WARN("Unknown port type '%s' set on logical switch '%s'. " > + "Using anyway.", op->nbsp->type, op->nbsp->name); > + } > sbrec_port_binding_set_type(op->sb, op->nbsp->type); > } else { > const char *chassis = NULL; I tend to think we should ignore the port as if it didn't exist in this case since it's an invalid configuration.
On Thu, Aug 24, 2017 at 2:42 PM Russell Bryant <russell@ovn.org> wrote: > On Thu, Aug 24, 2017 at 2:13 PM, Mark Michelson <mmichels@redhat.com> > wrote: > > OVN is lenient with the types of logical switch ports. Maybe too > > lenient. This patch attempts to solve this problem on two fronts: > > > > 1) In ovn-nbctl, if you attempt to set the port type to an unknown > > type, the command will not end up setting the type. > > 2) In northd, when copying the port type from the northbound database to > > the corresponding port-binding in the southbound database, a warning > > will be issued if the port is of an unknown type. > > > > Signed-off-by: Mark Michelson <mmichels@redhat.com> > > --- > > ovn/lib/ovn-util.c | 30 +++++++++++++++++++ > > ovn/lib/ovn-util.h | 2 ++ > > ovn/northd/ovn-northd.c | 4 +++ > > ovn/utilities/ovn-nbctl.c | 7 ++++- > > tests/ovn-nbctl.at | 73 > +++++++++++++++++++++++++++++++++++++++++++++++ > > 5 files changed, 115 insertions(+), 1 deletion(-) > > > > Did you look into whether it was possible to use an enum in the db > schema? In particular, what would happen on an upgrade of an existing > database? and what happens if the existing db has an invalid type in > it already? > > I just want to make sure we clearly rule that out first. > I just did a couple of experiments. I updated the DB schema to change the logical switch port type to be an enum instead of a string. I created an environment where I had clean values in the DB. I performed an upgrade and the northbound DB stayed intact and seemed fine. I then created an environment where there were logical switch ports with dirty values in the northbound DB. When I ran the upgrade this time, the entire northbound DB was empty afterwards. It wasn't just the offending switch ports that got removed; it was literally everything. My opinion on this is that the schema change is too risky. If it only removed offending switch ports, it might not be so bad. But a single switch port with a bad type results in the entire DB being empty. What do you think? Mark Michelson
diff --git a/ovn/lib/ovn-util.c b/ovn/lib/ovn-util.c index 037d0798a..883ce4ccb 100644 --- a/ovn/lib/ovn-util.c +++ b/ovn/lib/ovn-util.c @@ -299,3 +299,33 @@ default_sb_db(void) } return def; } + +/* l3gateway, chassisredirect, and patch + * are not in this list since they are + * only set in the SB DB by northd + */ +const char *OVN_NB_LSP_TYPES[] = { + "router", + "localnet", + "localport", + "l2gateway", + "vtep", +}; + +bool +ovn_is_known_nb_lsp_type(const char *type) +{ + int i; + + if (!type || !type[0]) { + return true; + } + + for (i = 0; i < ARRAY_SIZE(OVN_NB_LSP_TYPES); ++i) { + if (!strcmp(OVN_NB_LSP_TYPES[i], type)) { + return true; + } + } + + return false; +} diff --git a/ovn/lib/ovn-util.h b/ovn/lib/ovn-util.h index b3d2125a3..9b456426d 100644 --- a/ovn/lib/ovn-util.h +++ b/ovn/lib/ovn-util.h @@ -67,4 +67,6 @@ char *alloc_nat_zone_key(const struct uuid *key, const char *type); const char *default_nb_db(void); const char *default_sb_db(void); +bool ovn_is_known_nb_lsp_type(const char *type); + #endif diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c index 49e4ac338..177df5a1e 100644 --- a/ovn/northd/ovn-northd.c +++ b/ovn/northd/ovn-northd.c @@ -1962,6 +1962,10 @@ ovn_port_update_sbrec(struct northd_context *ctx, } sbrec_port_binding_set_options(op->sb, &options); smap_destroy(&options); + if (!ovn_is_known_nb_lsp_type(op->nbsp->type)) { + VLOG_WARN("Unknown port type '%s' set on logical switch '%s'. " + "Using anyway.", op->nbsp->type, op->nbsp->name); + } sbrec_port_binding_set_type(op->sb, op->nbsp->type); } else { const char *chassis = NULL; diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c index 46ede4e50..d00bd6ec9 100644 --- a/ovn/utilities/ovn-nbctl.c +++ b/ovn/utilities/ovn-nbctl.c @@ -1173,7 +1173,12 @@ nbctl_lsp_set_type(struct ctl_context *ctx) const struct nbrec_logical_switch_port *lsp; lsp = lsp_by_name_or_uuid(ctx, id, true); - nbrec_logical_switch_port_set_type(lsp, type); + if (ovn_is_known_nb_lsp_type(type)) { + nbrec_logical_switch_port_set_type(lsp, type); + } else { + ctl_fatal("Logical switch port type '%s' is unrecognized. " + "Not setting type.", type); + } } static void diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at index 354b8df96..e1c4b4473 100644 --- a/tests/ovn-nbctl.at +++ b/tests/ovn-nbctl.at @@ -951,3 +951,76 @@ IPv6 Routes OVN_NBCTL_TEST_STOP AT_CLEANUP + +dnl --------------------------------------------------------------------- + +AT_SETUP([ovn-nbctl - lsp types]) +OVN_NBCTL_TEST_START + +AT_CHECK([ovn-nbctl ls-add ls0]) +AT_CHECK([ovn-nbctl lsp-add ls0 lp0]) + +dnl switchport type defaults to empty +AT_CHECK([ovn-nbctl lsp-get-type lp0], [0], [dnl + +]) + +dnl The following are the valid entries for +dnl switchport type +AT_CHECK([ovn-nbctl lsp-set-type lp0 l2gateway]) +AT_CHECK([ovn-nbctl lsp-get-type lp0], [0], [dnl +l2gateway +]) + +AT_CHECK([ovn-nbctl lsp-set-type lp0 router]) +AT_CHECK([ovn-nbctl lsp-get-type lp0], [0], [dnl +router +]) + +AT_CHECK([ovn-nbctl lsp-set-type lp0 localnet]) +AT_CHECK([ovn-nbctl lsp-get-type lp0], [0], [dnl +localnet +]) + +AT_CHECK([ovn-nbctl lsp-set-type lp0 localport]) +AT_CHECK([ovn-nbctl lsp-get-type lp0], [0], [dnl +localport +]) + +AT_CHECK([ovn-nbctl lsp-set-type lp0 vtep]) +AT_CHECK([ovn-nbctl lsp-get-type lp0], [0], [dnl +vtep +]) + +dnl All of these are valid southbound port types but +dnl should be rejected for northbound logical switch +dnl ports. +AT_CHECK([ovn-nbctl lsp-set-type lp0 l3gateway], [1], [], [dnl +ovn-nbctl: Logical switch port type 'l3gateway' is unrecognized. Not setting type. +]) +AT_CHECK([ovn-nbctl lsp-set-type lp0 patch], [1], [], [dnl +ovn-nbctl: Logical switch port type 'patch' is unrecognized. Not setting type. +]) +AT_CHECK([ovn-nbctl lsp-set-type lp0 chassisredirect], [1], [], [dnl +ovn-nbctl: Logical switch port type 'chassisredirect' is unrecognized. Not setting type. +]) + +dnl switch port type should still be "vtep" since previous +dnl commands failed. +AT_CHECK([ovn-nbctl lsp-get-type lp0], [0], [dnl +vtep +]) + +dnl Attempt a nonsense type +AT_CHECK([ovn-nbctl lsp-set-type lp0 eggs], [1], [], [dnl +ovn-nbctl: Logical switch port type 'eggs' is unrecognized. Not setting type. +]) + +dnl Empty string should work too +AT_CHECK([ovn-nbctl lsp-set-type lp0 ""]) +AT_CHECK([ovn-nbctl lsp-get-type lp0], [0], [dnl + +]) + +OVN_NBCTL_TEST_STOP +AT_CLEANUP
OVN is lenient with the types of logical switch ports. Maybe too lenient. This patch attempts to solve this problem on two fronts: 1) In ovn-nbctl, if you attempt to set the port type to an unknown type, the command will not end up setting the type. 2) In northd, when copying the port type from the northbound database to the corresponding port-binding in the southbound database, a warning will be issued if the port is of an unknown type. Signed-off-by: Mark Michelson <mmichels@redhat.com> --- ovn/lib/ovn-util.c | 30 +++++++++++++++++++ ovn/lib/ovn-util.h | 2 ++ ovn/northd/ovn-northd.c | 4 +++ ovn/utilities/ovn-nbctl.c | 7 ++++- tests/ovn-nbctl.at | 73 +++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 115 insertions(+), 1 deletion(-)