On Thu, Jun 14, 2018 at 11:57:19AM -0700, Han Zhou wrote: > On Thu, Jun 14, 2018 at 10:40 AM, Ben Pfaff <b...@ovn.org> wrote: > > > > On Thu, Jun 14, 2018 at 10:17:58AM -0700, Han Zhou wrote: > > > On Thu, Jun 14, 2018 at 10:09 AM, Ben Pfaff <b...@ovn.org> wrote: > > > > > > > > On Wed, Jun 13, 2018 at 08:29:28PM -0700, Han Zhou wrote: > > > > > On Wed, Jun 13, 2018 at 3:37 PM, Ben Pfaff <b...@ovn.org> wrote: > > > > > > > > > > > > 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 <zhou...@gmail.com> > > > > > > Signed-off-by: Ben Pfaff <b...@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 > > > > > > > > > > > > > > > > I wanted to have a quick test but it didn't pass the compile: > > > > > In file included from ovn/controller/bfd.c:17:0: > > > > > ovn/controller/bfd.h:19:44: fatal error: > > > ovn/controller/bfd-vswitch-idl.h: > > > > > No such file or directory > > > > > > > > Hmm. That's a little puzzling. This file should get automatically > > > > generated by "make". I assume that I messed up a dependency > somewhere, > > > > but at first glance I can't see the problem. > > > > > > > > > I didn't debug, but by looking at the code I got the idea. It > ensures > > > the > > > > > purpose is declared through the generated data type and violations > are > > > > > captured at compile time. > > > > > I think this is a brilliant way to achieve the check with such a > small > > > > > change, however, I am not sure how is it supposed to help for > generating > > > > > the dependency. I think instead of caring about whether it is > > > read-only, we > > > > > should care about whether it is write-only. > > > > > > > > > > For example, a engine node A reads on Table T1's column C1, reads & > > > writes > > > > > on Table T2's column C2, and writes on T3's C3. In this case A > depends > > > on > > > > > T1 and T2, but not depends on T3. > > > > > So I think what matters to the dependency generation is whether it > is > > > > > Write-Only. Read-Only is no different from ReadWrite. > > > > > > > > > > If my understanding is correct (that we don't care about the > difference > > > > > between RO and RW), for WO columns, we can simply just don't > monitor its > > > > > change. Then we don't even need this feature from dependency > generation > > > > > point of view. > > > > > Did I miss something? > > > > > > > > It's easy enough to add support for write-only columns here. I'll add > > > > that in the next version. > > > > > > > > > Another thing, however, I think is really important for the > dependency > > > > > generation is the columns that reference other tables, e.g. > bfd_run() > > > > > doesn't has table "ovsrec_port" as input, but in fact it depends on > this > > > > > table by referencing from ovsrec_bridge->ports. We need the table > being > > > > > referenced appear in the input parameters. > > > > > The approach in the patch may not solve that problem directly, but > > > > > something similar may work: we could declare the usage for a > referenced > > > > > column as "direct" or "indirect". "indirect" means the column can be > > > > > accessed only with "get functions" instead of pointers. The "get > > > functions" > > > > > requires an additional parameter that is the instance of the table > being > > > > > referenced. To call the "get function" to access the column, the > caller > > > > > function needs to have the table being referenced passed in as > > > parameter, > > > > > so we will be able to catch the dependency. I am not sure if this > > > approach > > > > > is too heavy, or is tricky to implement. > > > > > > > > I'd think that the patch solves this problem by declaring that bfd.c > > > > depends only on the tables and columns that bfd-vswitch-idl.def allows > > > > it to access. > > > > > > The patch can prevent bfd_run() from accessing ovsrec_port data. > However, > > > the function do require accessing ovsrec_port data: > > > > > > char *port_name = br_int->ports[k]->name; > > > > > > So my point was, how to expose this access in the parameter of > bfd_run(). > > > Sorry if my previous reply was not clear. > > > > I guess I regard access to bfd_ovsrec_bridge as also granting access to > > everything reachable from bfd_ovsrec_bridge. That's a customizable > > amount of access, since what can be reached from bfd_ovsrec_bridge can > > be set in a fine-grained way from the .def file. > > > > It's not ideal to have the access controls in something that is not C, > > but I haven't figured out a way to do it in plain C and I'm not sure > > that it is possible. > > Ok, I got your point. So the dependency generator will look at table schema > for references, and whether a referenced table is regarded as dependency > depends on whether the access is granted by the .def file for those columns > in the source table. > > I think it is good enough for the purpose and it is simpler than the idea I > described. > > +1 to the RFC.
v2 of the RFC applies this concept to every file: https://patchwork.ozlabs.org/patch/932541/ _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev