BIND 10 #983: Python wrappers for ACLs

[BIND 10 Development]

#983: Python wrappers for ACLs
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  vorner                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20110712
                  Component:         |            Resolution:
  Unclassified                       |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  5.0
Feature Depending on Ticket:  ACL    |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 Replying to [comment:8 jinmei]:
 > By "split into two parts", do you mean (e.g.) having isc.acl.acl and

 Yes, I mean this.

 > isc.acl.dns?  BTW, as far as I know .so cannot have multiple python
 > modules.  The relationship between modules and .so's (or more commonly
 > .py's) is one to one mappings.  So, our choice is either

 It is true for .py files, but .so files have some more powerful tools it
 seems (at last the API reference suggests so), I think it could be
 possible to create a module object „by hand“ and plug it into another
 module or something. I didn't think about it much and I'm not sure it is
 really possible. But as I said previously, I think having multiple .so
 files is fine and is definitely much easier, so let's leave it this way.

 > > The thing about `load_request_acl` ‒ I'm little bit worried here. I
 expect we'll add a way to write custom checks sometime in future and I'd
 really like it to be possible to write them in python as well. Then we
 would need to expose the loader itself to register it and would need to
 change the interface. But maybe I worry too soon.
 >
 > In fact, I wondered how the python part is expected to be extendable.
 > So, as a future possibility I have no problem with exposing the
 > register and introducing a mechanism to use python version checks as
 > we see the real need for this.  But these would even be bigger than a
 > single task, so would certainly be beyond the scope of this ticket.

 Yes, agreed here.

 > One thing we can do within this ticket would be to introduce a wrapper
 > for (Request)Loader which would currently only has the load() method.
 > That way, if and when we introduce more extensions the code for
 > loading() won't have to be modified.  If you like I'm okay with doing
 > this in this ticket.  Do you?

 That was what I originally thought, so it would be nice to have the
 limited class wrapper, where we can add the register in future.

 > > The code itself looks clean, with few small details:
 > >  * I fixed a problem in Makefile ‒ it failed here, because dnsacl
 could not find an exception type from libacl. And I think the libacl
 should not depend on dns acl.
 >
 > The latter one is okay (it was an error of mine).  As for the former,
 > do you mean this one?
 > {{{
 > +dns_la_LIBADD += acl.la
 > }}}

 Yes, this solves my problem. If it isn't there, it fails with this (during
 tests):

 {{{
 Running test: dns_test.py
 Traceback (most recent call last):
   File
 "/home/vorner/work/bind10/src/lib/python/isc/acl/tests/dns_test.py", line
 19, in <module>
     from isc.acl.dns import *
   File "/home/vorner/work/bind10/src/lib/python/isc/acl/dns.py", line 52,
 in <module>
     from dns import *
 ImportError: /home/vorner/work/bind10/src/lib/python/isc/acl/.libs/dns.so:
 undefined symbol: _ZN3isc3acl6python14po_LoaderErrorE
 make[7]: *** [check-local] Error 1
 }}}

 I don't know if having the library loaded suffices here. I think my linker
 looks only into libraries that are noted as dependencies. Don't you load
 the library already before? I think the test imports the acl.acl module
 first. What do you suggest I try?

 > Would this be better?
 > {{{
 >         // Install module constants.  Note that we can let Py_BuildValue
 >         // "steal" the references to these object (by specifying false
 to
 >         // installToModule), because, unlike the exception cases above,
 >         // we don't have corresponding C++ variables (see the note in
 >         // pycppwrapper_util for more details).
 > }}}

 Yes, probably.

 > >  * In `PyInit_acl`, can some of the installToModule thing fail?
 Shouldn't it be checked for NULL? Or is this some kind of our thing that
 throws?
 >
 > installToModule will throw if it fails.  More important in this
 > context, if po_XXXError is NULL PyObjectContainer(po_XXXError) will
 > throw.  In fact, that's the design principle of this framework - to
 > make the wrapper code concise (not requiring too much error handling
 > within the wrapper) and safer (minimize the risk of null pointer
 > dereference or leaking reference on failure).  See pycppwrapper_util.h
 > for more details.

 OK, I see. The naming conventions looked a lot like the original python
 API, so I thought it belongs there.

 > >  * Why are the included files with documentation `.cc`? Shouldn't they
 be .h?
 >
 > Because they contain definitions and cannot be shared by multiple
 > .cc's.  I don't think including a .cc is so uncommon, but it only
 > matters within this wrapper implementation and is a kind of a
 > preference matter, so I wouldn't strongly push it.

 OK, it is just unusual to me I guess. No problem there.

 > > {{{#!C++
 > > PyErr_SetString(PyExc_TypeError,
 > >                     "RequestACL cannot be directly constructed");
 > > }}}
 >
 > Probably not.  Choosing a reasonable python exception in the wrapper
 > code is always a difficult question to me.  In many cases none of the
 > standard python exceptions is really suitable.  In this specific case,

 I updated a test to match the exception.

 > >  * The functions that construct the ACLs take strings. I get it is the
 same as with the logging, but the logging is not really exposed, it is
 used from within the ModuleConfig thing. This might be used externally, do
 you think it makes sense to accept the list argument directly (and call
 the conversion to string to be used from within the C++ code)?
 >
 > Ah, okay.  I thought applications would have ACL configurations in the
 > form of a json object (in which case it should be easy and trivial to
 > convert them to strings), but they are actually converted to a more
 > native form.  So, yes, I think that makes sense.  But I'm afraid it's
 > too low level for the C++ binding code to convert such complicated
 > python objects.  I'd introduce a top layer python script that has a
 > hidden C++ backend like the standard socket module, i.e, defining
 > dns.py and _dns.so and having the former import the latter (it would
 > make it trickier to share the .py in the in-source mode and in the
 > installed version, though).  Would you agree with that?

 Well, I didn't really mean writing C++ code to do the conversion. My idea
 was to take the object passed as a parameter, somehow load the json.dumps
 function (which is the python function used to convert to string) and call
 it with `PyObject_CallObject`, so effectively using the python function
 from C++ code. It should even handle parameter parsing.

 Having a python wrapper sounds reasonably as well, but it looks more
 tricky to me.

 Anyway, it probably isn't so much a problem, so if the solutions both look
 complicated, I'm OK with the current interface ‒ I just thought it could
 be more convenient.

-- 
Ticket URL: <http://bind10.isc.org/ticket/983#comment:10>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development