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      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:7 vorner]:

 > To address the design points first. I like it to be split into two
 parts, I don't know if it's better to be done this way or if more modules
 can be packed into one .so, but it is probably simpler this way anyway.

 By "split into two parts", do you mean (e.g.) having isc.acl.acl and
 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
 - having multiple .so's for multiple modules
 - having a single (or smaller number of) module that contains much
   more classes

 > 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.

 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?

 > 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
 }}}

 If we do this, my linker complains:
 {{{
 *** Warning: Linking the shared library dns.la against the loadable module
 *** acl.so is not portable!
 *** Warning: lib acl.so is a module, not a shared library
 }}}
 (actually I saw that in my development, which is why I didn't do this)
 and, the resulting code fails in tests in a strange way:
 {{{
   File
 "/Users/jinmei/src/isc/git/bind10-983/src/lib/python/isc/acl/tests/dns_test.py",
 line 89, in test_construct
     self.assertRaises(Error, RequestContext, ('example.com', 5300))
   File
 "/opt/local/Library/Frameworks/Python.framework/Versions/3.1/lib/python3.1/unittest.py",
 line 589, in assertRaises
     callableObj(*args, **kwargs)
 SystemError: NULL result without error in PyObject_Call
 }}}

 How exactly do you mean by "dnsacl could not find an exception type
 from libacl"?  Is that a link time error or run time error?  If it's
 the latter, I thought having __init__.py import isc.acl.acl would
 resolve the missing symbol problems.  Or maybe you need to explicitly
 import isc.acl.acl when you import isc.acl.dns.

 If it's a link time error, maybe we'll need some workaround, e.g.,
 eliminating inter .so dependency completely (which might result in
 mostly redundant definitions in each .so module though).

 >  * I don't really understand this comment, what does it mean?
 > {{{
 >         // Install module constants.  Note that we can release our own
 >         // references to these objects because we don't have
 corresponding
 >         // C++ variables.
 > }}}

 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).
 }}}

 >  * 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.

 >  * 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.

 >  * src/lib/python/isc/acl/dns.py seems to have the legal header twice.

 Ah, good catch, thanks.  Removed.

 >  * Should this really be `TypeError`?
 > {{{#!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 simply changed it to ACLError.

 >  * 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?

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