To make ovn-controller recompute incrementally, we need accurate dependencies for each function that reads or writes a table. It's currently difficult to be sure about these dependencies, and certainly difficult to maintain them over time, because there's no way to actually enforce them.
This commit experiments with an approach that allows for fairly fine-grained access control within ovn-controller to tables and columns. It's based on generating a new version of the IDL data structures for each case where we want different access control. All of these data structures have the same format, but the columns that a given piece of code is not supposed to touch are renamed to discourage programmers from using them, e.g. they're given names suffixed with "__accessdenied". (This means that there is no runtime overhead to the access control since it only requires a cast to convert between the regular and restricted versions.) In addition, when a columns is supposed to be read-only, functions for modifying the column are not supplied. This commit only tries out this experiment for a single file within ovn-controller, the BFD implementation (mostly because that's alphabetically first, no other real reason). It would require a little more work to apply it everywhere, but it's probably not a huge deal. Comments? CC: Han Zhou <[email protected]> Signed-off-by: Ben Pfaff <[email protected]> --- ovn/controller/automake.mk | 5 + ovn/controller/bfd-vswitch-idl.def | 21 ++++ ovn/controller/bfd.c | 20 ++-- ovn/controller/bfd.h | 8 +- ovn/controller/ovn-controller.c | 13 ++- ovsdb/ovsdb-idlc.in | 223 ++++++++++++++++++++++++++++++++++++- 6 files changed, 268 insertions(+), 22 deletions(-) create mode 100644 ovn/controller/bfd-vswitch-idl.def diff --git a/ovn/controller/automake.mk b/ovn/controller/automake.mk index cd5505ca6b7c..78c275b219f5 100644 --- a/ovn/controller/automake.mk +++ b/ovn/controller/automake.mk @@ -28,3 +28,8 @@ ovn_controller_ovn_controller_LDADD = ovn/lib/libovn.la lib/libopenvswitch.la man_MANS += ovn/controller/ovn-controller.8 EXTRA_DIST += ovn/controller/ovn-controller.8.xml CLEANFILES += ovn/controller/ovn-controller.8 + +$(ovn_controller_ovn_controller_SOURCES:.c=.$(OBJEXT)): \ + ovn/controller/bfd-vswitch-idl.h +ovn/controller/bfd-vswitch-idl.h: lib/vswitch-idl.ovsidl ovn/controller/bfd-vswitch-idl.def ovsdb/ovsdb-idlc.in + $(AM_V_GEN)$(OVSDB_IDLC) c-idl-subset lib/vswitch-idl.ovsidl $(srcdir)/ovn/controller/bfd-vswitch-idl.def > [email protected] && mv [email protected] $@ diff --git a/ovn/controller/bfd-vswitch-idl.def b/ovn/controller/bfd-vswitch-idl.def new file mode 100644 index 000000000000..da60a4a40529 --- /dev/null +++ b/ovn/controller/bfd-vswitch-idl.def @@ -0,0 +1,21 @@ +# -*- python -*- + +{ + "prefix": "bfd_ovsrec_", + "tables": { + "Interface": { + "name": RO, + "bfd": RW, + "bfd_status": RO, + "options": RO}, + "Port": { + "name": RO, + "interfaces": RO, + "external_ids": RO + }, + "Bridge": { + "name": RO, + "ports": RO + } + } +} diff --git a/ovn/controller/bfd.c b/ovn/controller/bfd.c index 051781f38ba8..50a092205a28 100644 --- a/ovn/controller/bfd.c +++ b/ovn/controller/bfd.c @@ -40,7 +40,7 @@ bfd_register_ovs_idl(struct ovsdb_idl *ovs_idl) static void -interface_set_bfd(const struct ovsrec_interface *iface, bool bfd_setting) +interface_set_bfd(const struct bfd_ovsrec_interface *iface, bool bfd_setting) { const char *new_setting = bfd_setting ? "true":"false"; const char *current_setting = smap_get(&iface->bfd, "enable"); @@ -50,14 +50,14 @@ interface_set_bfd(const struct ovsrec_interface *iface, bool bfd_setting) return; } const struct smap bfd = SMAP_CONST1(&bfd, "enable", new_setting); - ovsrec_interface_verify_bfd(iface); - ovsrec_interface_set_bfd(iface, &bfd); + bfd_ovsrec_interface_verify_bfd(iface); + bfd_ovsrec_interface_set_bfd(iface, &bfd); VLOG_INFO("%s BFD on interface %s", bfd_setting ? "Enabled" : "Disabled", iface->name); } void -bfd_calculate_active_tunnels(const struct ovsrec_bridge *br_int, +bfd_calculate_active_tunnels(const struct bfd_ovsrec_bridge *br_int, struct sset *active_tunnels) { int i; @@ -68,7 +68,7 @@ bfd_calculate_active_tunnels(const struct ovsrec_bridge *br_int, } for (i = 0; i < br_int->n_ports; i++) { - const struct ovsrec_port *port_rec = br_int->ports[i]; + const struct bfd_ovsrec_port *port_rec = br_int->ports[i]; if (!strcmp(port_rec->name, br_int->name)) { continue; @@ -76,7 +76,7 @@ bfd_calculate_active_tunnels(const struct ovsrec_bridge *br_int, int j; for (j = 0; j < port_rec->n_interfaces; j++) { - const struct ovsrec_interface *iface_rec; + const struct bfd_ovsrec_interface *iface_rec; iface_rec = port_rec->interfaces[j]; /* Check if this is a tunnel interface. */ @@ -264,8 +264,8 @@ bfd_calculate_chassis( void bfd_run(struct ovsdb_idl_index *sbrec_chassis_by_name, struct ovsdb_idl_index *sbrec_port_binding_by_datapath, - const struct ovsrec_interface_table *interface_table, - const struct ovsrec_bridge *br_int, + const struct bfd_ovsrec_interface_table *interface_table, + const struct bfd_ovsrec_bridge *br_int, const struct sbrec_chassis *chassis_rec, const struct hmap *local_datapaths) { @@ -293,8 +293,8 @@ bfd_run(struct ovsdb_idl_index *sbrec_chassis_by_name, } /* Enable or disable bfd */ - const struct ovsrec_interface *iface; - OVSREC_INTERFACE_TABLE_FOR_EACH (iface, interface_table) { + const struct bfd_ovsrec_interface *iface; + BFD_OVSREC_INTERFACE_TABLE_FOR_EACH (iface, interface_table) { if (sset_contains(&tunnels, iface->name)) { interface_set_bfd( iface, sset_contains(&bfd_ifaces, iface->name)); diff --git a/ovn/controller/bfd.h b/ovn/controller/bfd.h index 7ea345a3e642..25382cf85c2e 100644 --- a/ovn/controller/bfd.h +++ b/ovn/controller/bfd.h @@ -16,6 +16,8 @@ #ifndef OVN_BFD_H #define OVN_BFD_H 1 +#include "ovn/controller/bfd-vswitch-idl.h" + struct hmap; struct ovsdb_idl; struct ovsdb_idl_index; @@ -27,11 +29,11 @@ struct sset; void bfd_register_ovs_idl(struct ovsdb_idl *); void bfd_run(struct ovsdb_idl_index *sbrec_chassis_by_name, struct ovsdb_idl_index *sbrec_port_binding_by_datapath, - const struct ovsrec_interface_table *interface_table, - const struct ovsrec_bridge *br_int, + const struct bfd_ovsrec_interface_table *interface_table, + const struct bfd_ovsrec_bridge *br_int, const struct sbrec_chassis *chassis_rec, const struct hmap *local_datapaths); -void bfd_calculate_active_tunnels(const struct ovsrec_bridge *br_int, +void bfd_calculate_active_tunnels(const struct bfd_ovsrec_bridge *br_int, struct sset *active_tunnels); #endif diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c index 6ee72a9fafb4..c0aefa63945a 100644 --- a/ovn/controller/ovn-controller.c +++ b/ovn/controller/ovn-controller.c @@ -680,7 +680,8 @@ main(int argc, char *argv[]) encaps_run(ovs_idl_txn, ovsrec_bridge_table_get(ovs_idl_loop.idl), br_int, sbrec_chassis_table_get(ovnsb_idl_loop.idl), chassis_id); - bfd_calculate_active_tunnels(br_int, &active_tunnels); + bfd_calculate_active_tunnels(bfd_ovsrec_bridge_get(br_int), + &active_tunnels); binding_run(ovnsb_idl_txn, ovs_idl_txn, sbrec_chassis_by_name, sbrec_datapath_binding_by_key, sbrec_port_binding_by_datapath, @@ -742,10 +743,12 @@ main(int argc, char *argv[]) &flow_table, &group_table, &meter_table); if (chassis_id) { - bfd_run(sbrec_chassis_by_name, - sbrec_port_binding_by_datapath, - ovsrec_interface_table_get(ovs_idl_loop.idl), - br_int, chassis, &local_datapaths); + bfd_run( + sbrec_chassis_by_name, + sbrec_port_binding_by_datapath, + bfd_ovsrec_interface_table_get(ovs_idl_loop.idl), + bfd_ovsrec_bridge_get(br_int), + chassis, &local_datapaths); } physical_run( sbrec_chassis_by_name, diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in index ee655f7e7915..60898bdbe4e9 100755 --- a/ovsdb/ovsdb-idlc.in +++ b/ovsdb/ovsdb-idlc.in @@ -14,7 +14,9 @@ from ovs.db.types import StringType, IntegerType, RealType argv0 = sys.argv[0] def parseSchema(filename): - return ovs.db.schema.IdlSchema.from_json(ovs.json.from_file(filename)) + schema = ovs.db.schema.IdlSchema.from_json(ovs.json.from_file(filename)) + replace_cplusplus_keyword(schema) + return schema def annotateSchema(schemaFile, annotationFile): schemaJson = ovs.json.from_file(schemaFile) @@ -182,7 +184,6 @@ def replace_cplusplus_keyword(schema): def printCIDLHeader(schemaFile): schema = parseSchema(schemaFile) - replace_cplusplus_keyword(schema) prefix = schema.idlPrefix print('''\ /* Generated automatically -- do not modify! -*- buffer-read-only: t -*- */ @@ -431,7 +432,6 @@ def printEnum(type, members): def printCIDLSource(schemaFile): schema = parseSchema(schemaFile) - replace_cplusplus_keyword(schema) prefix = schema.idlPrefix print('''\ /* Generated automatically -- do not modify! -*- buffer-read-only: t -*- */ @@ -1493,6 +1493,220 @@ def ovsdb_escape(string): return '\\x%02x' % ord(c) return re.sub(r'["\\\000-\037]', escape, string) +RO = 'RO' +RW = 'RW' +def printCIDLSubset(schemaFile, subsetFile): + schema = parseSchema(schemaFile) + prefix = schema.idlPrefix + subset = eval(open(subsetFile, "rb").read(), None, + {'RO': RO, + 'RW': RW}) + subset_prefix = subset['prefix'] + subset_tables = subset['tables'] + print('''\ +/* Generated automatically -- do not modify! -*- buffer-read-only: t -*- */ + +#ifndef %(SUBSET_PREFIX)sIDL_HEADER +#define %(SUBSET_PREFIX)sIDL_HEADER 1 + +#include %(header)s + +#ifdef __cplusplus +extern "C" { +#endif +''' + % {'SUBSET_PREFIX': subset_prefix.upper(), + 'header': schema.idlHeader}) + + # XXX verify that columns in the subset_tables exist + # XXX verify that permitted columns only reference permitted tables + + for table_name, column_acls in sorted(subset_tables.items()): + table = schema.tables[table_name] + struct_name = "%s%s" % (subset_prefix, table_name.lower()) + prefix_struct_name = "%s%s" % (prefix, table_name.lower()) + + print(" ") + print("/* %s table. */" % table_name) + print("struct %s {" % struct_name) + print("\tstruct ovsdb_idl_row header_;") + rw = False + for column_name, column in sorted_columns(table): + print("\n\t/* %s column. */" % column_name) + if column.extensions.get("members"): + print("\t%s" % column.extensions["members"]) + continue + comment, members = cMembers(subset_prefix, table_name, + column_name, column, False) + if column_name in column_acls: + name_suffix = '' + if column_acls[column_name] == RW: + rw = True + else: + name_suffix = '__accessdenied' + for member in members: + print("\t%s%s%s;%s" + % (member['type'], + member['name'], + name_suffix, + member['comment'])) + print("};") + + args = {'s': struct_name, 'S': struct_name.upper(), + 'sp': prefix_struct_name} + print(''' +/* Allows the IDL owner to obtain a restricted table view. */ +static inline const struct %(s)s_table * +%(s)s_table_get(const struct ovsdb_idl *idl) +{ + return (const struct %(s)s_table *) idl; +} + +/* Obtains a restrict row view from a general-purpose one. */ +static inline const struct %(s)s * +%(s)s_get(const struct %(sp)s *row) +{ + return (const struct %(s)s *) row; +} + +/* Iterators for the restricted table view. */ +#define %(S)s_TABLE_FOR_EACH(ROW, TABLE) \\ + for ((ROW) = %(s)s_table_first(TABLE); \\ + (ROW); \\ + (ROW) = %(s)s_next(ROW))''' % args) + if rw: + print('''\ +#define %(S)s_TABLE_FOR_EACH_SAFE(ROW, NEXT, TABLE) \\ + for ((ROW) = %(s)s_table_first(TABLE); \\ + (ROW) ? ((NEXT) = %(s)s_next(ROW), 1) : 0; \\ + (ROW) = (NEXT))''' % args) + print('''\ +static inline const struct %(s)s * +%(s)s_get_for_uuid( + const struct %(s)s_table *table, + const struct uuid *uuid) +{ + struct ovsdb_idl *idl = (struct ovsdb_idl *) table; + return (const struct %(s)s *) %(sp)s_get_for_uuid(idl, uuid); +} +static inline const struct %(s)s * +%(s)s_table_first(const struct %(s)s_table *table) +{ + struct ovsdb_idl *idl = (struct ovsdb_idl *) table; + return (const struct %(s)s *) %(sp)s_first(idl); +} +static inline const struct %(s)s * +%(s)s_next(const struct %(s)s *row) +{ + return (const struct %(s)s *) %(sp)s_next( + (struct %(sp)s *) row); +} + +unsigned int %(s)s_row_get_seqno(const struct %(s)s *, enum ovsdb_idl_change); +const struct %(s)s *%(s)s_track_get_first(const struct %(s)s_table *); +const struct %(s)s *%(s)s_track_get_next(const struct %(s)s *); +#define %(S)s_FOR_EACH_TRACKED(ROW, IDL) \\ + for ((ROW) = %(s)s_track_get_first(IDL); \\ + (ROW); \\ + (ROW) = %(s)s_track_get_next(ROW)) + +/* Returns true if 'row' was inserted since the last change tracking reset. */ +static inline bool %(s)s_is_new(const struct %(s)s *row) +{ + return %(s)s_row_get_seqno(row, OVSDB_IDL_CHANGE_MODIFY) == 0; +} + +/* Returns true if 'row' was deleted since the last change tracking reset. */ +static inline bool %(s)s_is_deleted(const struct %(s)s *row) +{ + return %(s)s_row_get_seqno(row, OVSDB_IDL_CHANGE_DELETE) > 0; +} + +bool %(s)s_is_updated(const struct %(s)s *, enum %(sp)s_column_id);''' % args) + + for column_name, column in sorted_columns(table): + if (column.extensions.get('synthetic') + or column_name not in column_acls): + continue + print(''' +static inline void +%(s)s_verify_%(c)s(const struct %(s)s *row) +{ + %(sp)s_verify_%(c)s((const struct %(sp)s *) row); +}''' % {'s': struct_name, + 'sp': prefix_struct_name, + 'c': column_name}) + + print("") + for column_name, column in sorted_columns(table): + if (column.extensions.get('synthetic') + or column_name not in column_acls): + continue + if column.type.value: + valueParam = ', enum ovsdb_atomic_type value_type' + else: + valueParam = '' + print('const struct ovsdb_datum *%(s)s_get_%(c)s(const struct %(s)s *, enum ovsdb_atomic_type key_type%(v)s);' % { + 's': struct_name, 'c': column_name, 'v': valueParam}) + + print("") + for column_name, column in sorted_columns(table): + if (column.extensions.get('synthetic') + or column_acls.get(column_name, '') != RW): + continue + type = column.type + + if type.is_smap(): + print('''\ +static inline void +%(s)s_set_%(c)s(const struct %(s)s *row, const struct smap *smap) +{ + %(sp)s_set_%(c)s((const struct %(sp)s *) row, smap); +} +''' + % {'c': column_name, + 's': struct_name, + 'sp': prefix_struct_name}) + continue + + comment, members = cMembers(subset_prefix, table_name, column_name, + column, True) + + print('''\ +static inline void +%(s)s_set_%(c)s(const struct %(s)s *row, %(args)s) +{ + %(sp)s_set_%(c)s((const struct %(sp)s *) row, %(arg_names)s); +} +''' + % {'c': column_name, + 's': struct_name, + 'sp': prefix_struct_name, + 'args': ', '.join(['%(type)s%(name)s' % m for m in members]), + 'arg_names': ', '.join([m['name'] for m in members])}) + + print("") + for column_name, column in sorted_columns(table): + if (column.extensions.get('synthetic') + or column_acls.get(column_name, '') != RW): + continue + if column.type.is_map(): + print('void %(s)s_update_%(c)s_setkey(const struct %(s)s *, ' % {'s': struct_name, 'c': column_name}, end=' ') + print('%(coltype)s, %(valtype)s);' % {'coltype':column.type.key.to_const_c_type(prefix), 'valtype':column.type.value.to_const_c_type(prefix)}) + print('void %(s)s_update_%(c)s_delkey(const struct %(s)s *, ' % {'s': struct_name, 'c': column_name}, end=' ') + print('%(coltype)s);' % {'coltype':column.type.key.to_const_c_type(prefix)}) + if column.type.is_set(): + print('void %(s)s_update_%(c)s_addvalue(const struct %(s)s *, ' % {'s': struct_name, 'c': column_name}, end=' ') + print('%(valtype)s);' % {'valtype':column.type.key.to_const_c_type(prefix)}) + print('void %(s)s_update_%(c)s_delvalue(const struct %(s)s *, ' % {'s': struct_name, 'c': column_name}, end=' ') + print('%(valtype)s);' % {'valtype':column.type.key.to_const_c_type(prefix)}) + + print(''' +#ifdef __cplusplus +} +#endif''') + print("\n#endif /* %sIDL_HEADER */" % subset_prefix.upper()) + def usage(): print("""\ %(argv0)s: ovsdb schema compiler @@ -1539,7 +1753,8 @@ if __name__ == "__main__": commands = {"annotate": (annotateSchema, 2), "c-idl-header": (printCIDLHeader, 1), - "c-idl-source": (printCIDLSource, 1)} + "c-idl-source": (printCIDLSource, 1), + "c-idl-subset": (printCIDLSubset, 2)} if not args[0] in commands: sys.stderr.write("%s: unknown command \"%s\" " -- 2.16.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
