[ovs-dev] [PATCH] [RFC] ovn-controller: Experiment with restricting access to columns.

Ben Pfaff blp at ovn.org
Wed Jun 13 22:37:27 UTC 2018


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 <zhouhan at gmail.com>
Signed-off-by: Ben Pfaff <blp at ovn.org>
---
 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 > $@.tmp && mv $@.tmp $@
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



More information about the dev mailing list