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

Han Zhou zhouhan at gmail.com
Thu Jun 28 00:37:16 UTC 2018

On Wed, Jun 27, 2018 at 2:09 PM, Mark Michelson <mmichels at redhat.com> wrote:

> On 06/26/2018 10:22 PM, Han Zhou wrote:
>> On Wed, Jun 20, 2018 at 11:04 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 "__noaccess" or "__writeonly".
>>> (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.
>>> Type-safe indexes are supplied in the same way.
>>> Comments?
>> Hi Ben,
>> Thanks for the great work and sorry for slow response.
>> Regarding the idl generation, the .def files define how each field of a
>> record can be accessed: no access, writeonly, etc, but it didn't specify
>> whether intersions or deletions are allowed on the table. Apparently
>> insertions and deletions should be considered as write operations to the
>> table. The insert/delete interfaces should not be generated for RO tables.
>> Another general comment is, this patch defines views of tables at module
>> level. In the incremental processing, we probably want to define different
>> engine nodes (like my earlier patches do), which may not be exact 1-1
>> match
>> to the modules. Each node should have its own view of the data, so I am
>> not
>> sure if this module level view satisfies our goal of incremental
>> processing. Do you have any thoughts on this? Or are you suggesting making
>> engine nodes 1-1 mapping with modules?
> I interpreted the module level views to be what fits with the current
> ovn-controller. You can think of this as an example to follow rather than a
> permanent fixture.
> With incremental processing, I think you would remove some of the existing
> module-level views and replace them with views that represent engine nodes.
> Once incremental processing is fully implemented, I think you'd see a 1-1
> mapping of views to engine nodes, rather than the current 1-1 mapping of
> views to modules.

Makes sense.

>> Please find my other comments inlined.
I removed the contents of the original patch below since it is big and may
hide my inlined comments.
And I also corrected a typo in my previous comments (sorry for that) to
avoid confusion.

>> Thanks,
>> Han
>>> -    struct sbrec_port_binding *target =
>> sbrec_port_binding_index_init_row(
>>> -        sbrec_port_binding_by_datapath);
>>> +    struct bfd_sbrec_port_binding *target
>>> +        = bfd_sbrec_port_binding_by_datapath_new_row(
>> Would it be better to follow same naming convention as the original
>> interface, i.e. ...init_row instead of ...new_row?
>> +            sbrec_port_binding_by_datapath);
>>>       /* Go through whole graph to figure out all chassis which may
>>> deliver
>>>        * packets to gateway. */
>>> @@ -140,7 +135,8 @@ bfd_travel_gw_related_chassis(
>>>           dp = dp_binding->dp;
>>>           free(dp_binding);
>>>           for (size_t i = 0; i < dp->n_peer_dps; i++) {
>>> -            const struct sbrec_datapath_binding *pdp = dp->peer_dps[i];
>>> +            const struct bfd_sbrec_datapath_binding *pdp
>>> +                = bfd_sbrec_datapath_binding_get(dp->peer_dps[i]);
>> I have a concern for this kind of conversion for DB records that is
>> contained in other data structures. There are 3 problems:
>> 1. Since local_datapaths is just a hmap, how to link it to the structure
>> of
>> the hmap node when generating the dependency remains unclear to me.
>> 2. From the input of this function, it depends on local_datapaths, which
>> contains the original version of datapath record, so the function should
>> be
>> considered as potentially writing the datapath record, no matter if the
>> conversion happens here or not, while in fact this function only reads the
>> record with the restricted view. So the real dependency is not reflected
>> correctly from the interface of the function.
>> 3. Looking at the how the data structure is generated, it is in
>> binding_run(), where a different view of the table is used as input
>> (binding_sbrec_datapath_binding), and it is then type converted to the
>> generic type sbrec_datapath_binding. This also loses track of the real
>> dependency.
>> Maybe you already have idea how to address these problems in the
>> dependency
>> auto-generation, but here is my rough idea:
>> When node B depends on node A, and A has ovsdb table record T as its
>> output
>> while B take T as input, it requires some coversion between B's view of T
>> (TB) and A's view of T. (TA) This coversion can be performed in a generic
>> way in the incremental processing engine framework, so that output of B is
>> converted as input of A according to the definition of the input and the
>> output data structure of engine nodes themselves.
>> Basically, each node has its own version of data structure, even if they
>> in
>> fact contains same data (e.g. node B's output has TB in a structure S-B,
>> while node A's input has TA in a structure S-A, and S-A is converted from
>> S-B. It is simply type convertion without runtime cost. This can be
>> achieved with some json *template* similar as the .def files to define
>> dependencies explicitely with output <-> input conversion mappings.
>> (instead of auto-generate the dependency, we can simplify it with
>> auto-validation, but probably you have better idea, like you always did
>> :))
>> Regarding the problem 1), what I think we can do is to make sure naming
>> convention is followed for parameters with type hmap, smap, etc. For
>> example, struct hmap *local_datapaths indicates the type of the node is
>> struct local_datapath. This doesn't prevent someone from using struct X to
>> access a hmap node supposed to be struct Y, but I think it is generic
>> problem even without considering the incremental processing.
>> There is much more details to be figured out, but it leads to the
>> discussion of the implementation of dependency auto-generation mechanism.

More information about the dev mailing list