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

Han Zhou zhouhan at gmail.com
Thu Jun 14 18:57:19 UTC 2018


On Thu, Jun 14, 2018 at 10:40 AM, Ben Pfaff <blp at 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 <blp at 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 <blp at 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 <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
> > > > >
> > > >
> > > > 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.


More information about the dev mailing list