[Freeipa-devel] [PATCH] 1106 IPA REST smart proxy

Tue Apr 29 16:23:33 UTC 2014

On 04/29/2014 04:27 PM, Rob Crittenden wrote:
> Petr Viktorin wrote:
>> On 04/23/2014 08:52 PM, Rob Crittenden wrote:
>>> Petr Viktorin wrote:
>>>> On 04/09/2014 11:29 PM, Rob Crittenden wrote:
>>>>> Rob Crittenden wrote:
>>>>>> Petr Viktorin wrote:
>>>>>>> On 03/14/2014 07:58 PM, Rob Crittenden wrote:
>>>>>>>> Petr Viktorin wrote:
>>>>>>>>> On 03/12/2014 07:48 PM, Rob Crittenden wrote:
>>>>>>> [...]
>>>>>>>>>>
>>>>>>>>>> Here are a couple more enhancements I'm considering, this seems
>>>>>>>>>> simpler
>>>>>>>>>> than inter-diff since it is so small.
>>>>>>>>>
>>>>>>>>> Not really. Having a patch file with a sequence+revision number
>>>>>>>>> you
>>>>>>>>> can
>>>>>>>>> refer to has its merits. Especially in a hairy thread like this
>>>>>>>>> one.
>>>>>>>>> Also one of our MUAs wrapped the lines, I had to undo that
>>>>>>>>> manually.
>>>>>>>>>
>>>>>>>>>> Here is why I made the changes, in order:
>>>>>>>>>>
>>>>>>>>>> I doubled the calls to create the connection but one isn't in a
>>>>>>>>>> try/except!? Remove the obvious one.
>>>>>>>>>>
>>>>>>>>>> We currently completely eat GSSAPI errors, I figure we should log
>>>>>>>>>> failures.
>>>>>>>>>>
>>>>>>>>>> IPA stores the principal in the request context so using that
>>>>>>>>>> will
>>>>>>>>>> save
>>>>>>>>>> a GSSAPI call (and as we learned, a lock in gssproxy).
>>>>>>>>>>
>>>>>>>>>> I included your content-type change.
>>>>>>>>>
>>>>>>>>> These changes look good.
>>>>>>>>> I'm almost done testing but I need to call it a week.
>>>>>>>>
>>>>>>>> Awesome, thanks.
>>>>>>>
>>>>>>> ACK on the functionality.
>>>>>>>
>>>>>>>>> Sorry for not catching that last time, but your patch doesn't
>>>>>>>>> add a
>>>>>>>>> *versioned* BuildRequres on python-kerberos, instead it adds a
>>>>>>>>> duplicate
>>>>>>>>> unversioned one. So lint (and thus the build) will fail if the old
>>>>>>>>> python-kerberos version is installed.
>>>>>>>>>
>>>>>>>>> A possible a solution to the build trouble would be to just add a
>>>>>>>>> lint
>>>>>>>>> exception now, and open a ticket to remove it later. That way the
>>>>>>>>> build
>>>>>>>>> succeeds despite the older version, and the new python-kerberos is
>>>>>>>>> only
>>>>>>>>> needed when installing freeipa-server-foreman-smartproxy.
>>>>>>>>> That should make everyone happy, including Martin.
>>>>>>>>> Unfortunately our lint exception mechanism doesn't work on
>>>>>>>>> modules, so
>>>>>>>>> this needs a somewhat nastier hack.
>>>>>>>>> The attaching a patch that does this (and I'm pasting a simple
>>>>>>>>> diff
>>>>>>>>> below). Does that look okay to push?
>>>>>>>>
>>>>>>>> I'm trying to find a better solution to all this. I may end up
>>>>>>>> taking
>>>>>>>> Martin's suggestion of rawhide-only to avoid this sort of thing.
>>>>>>>
>>>>>>> Looks like you'll still need to silence pylint on f20 in that case.
>>>>>>>
>>>>>>>> The deal with the smartproxy is that you can/should be able to run
>>>>>>>> it on
>>>>>>>> any IPA-enrolled client, so you can run it directly on the Foreman
>>>>>>>> box,
>>>>>>>> with the IPA server somewhere else. What this means is that someone
>>>>>>>> could probably fairly easily package this up for other
>>>>>>>> distributions
>>>>>>>> and
>>>>>>>> if we end up with a Fedora-only python-kerberos patch then
>>>>>>>> smartproxy is
>>>>>>>> Fedora-only as well.
>>>>>>>>
>>>>>>>> So I'm trying to get some movement out of upstream on this but it's
>>>>>>>> been
>>>>>>>> crickets for weeks. I think in the context of the calendar server
>>>>>>>> PyKerberos is small potatoes so doesn't get much lovin'. I'll
>>>>>>>> amp up
>>>>>>>> the
>>>>>>>> nagging to get some sort of response, even if it is "stop nagging
>>>>>>>> us!"
>>>>>>>>
>>>>>>>> rob
>>>>>>>
>>>>>>> Good luck!
>>>>>>
>>>>>> Ok, taking a different tack on this. Rather than running it as a
>>>>>> separate server process, run it as a WSGI app inside Apache. This
>>>>>> required a fair bit of re-tooling and complicates the set up a little
>>>>>> bit. I think I've got it all covered in the man page.
>>>>>>
>>>>>> On the python-kerberos front I've got bugs opened in Ubuntu and
>>>>>> Debian
>>>>>> to see if we can get the patch accepted their until (if) upstream
>>>>>> ever
>>>>>> takes a look.
>>>>>
>>>>> I decided to run the new WSGI app in a different process group, using
>>>>> the smartproxy we use for delegation. This simplifies the connection
>>>>> code, rather than using ldap2 like I was using, we use the RPC
>>>>> interface. And it provides to process separation. As a side-effect it
>>>>> will make running this code on platforms without GSSProxy a bit
>>>>> easier.
>>>>>
>>>>> rob
>>>>>
>>>>
>>>> Works great here!
>>>>
>>>>
>>>> The python-kerberos dependency issue still needs to be solved.
>>>
>>> Build is on the way to updates-testing if you can give it a go.
>>>
>>>>
>>>> The man page says:
>>>>     Copy ipa-smartproxy-apache.conf to
>>>> /etc/httpd/conf.d/ipa-smartproxy.conf.
>>>> It would be nice to put the whole path here so people don't have to
>>>> search for the file.
>>>
>>> Done.
>>>
>>>>
>>>> The "Configure Apache to use smartproxy" line looks like a step to be
>>>> performed. It could use some emphasis to make it look like a header.
>>>
>>> I combined it with the subsequent sentence so hopefully it is a bit
>>> clearer.
>>>
>>> I also added a bit on testing so you can confirm that things are
>>> working.
>>>
>>>
>>>> Side note, cherrypy's routing makes requests like this possible:
>>>>      http POST
>>>> :8090/ipa/smartproxy/host/testhost.idm.lab.eng.brq.redhat.com/some_description/True/06-00-00-00-00-00/some_userclass
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> Should that be allowed?
>>>
>>> It is definitely ugly but AFAICT it isn't illegal. The zero
>>> content-length bothers me more than this horrible-looking URI. It
>>> definitely requires some understanding of the ordering of parameters to
>>> get this call right.
>>>
>>> rob
>>
>> Everything works now!
>> Except one thing: json_encode_binary recently got a mandatory version
>> argument. For code that's part of IPA, it should be fine to just use
>> API_VERSION here.
>>
>> I tested with that added; ACK if you agree.
>
> ACK on your change.
>
> Sorry, one more set of changes, some fairly significant. This brings our
> proxy in line with the Foreman proxy functionality-wise. I tested this
> against a Foremane 1.5.0 RC1 build and it is fine. I didn't go as far as
> actually deploying any hosts but did confirm that the create/delete
> functions work ok.

I've eyeballed the changes, no testing yet. Here are my thoughts.

> Significant changes:
> - random removed as an option. It is always true unless a user-provided
> password is sent.
> - Added an option to Command so that the real IPA error can be raised to
> make internal error handling possible.

Please use a bare `raise` for simple re-raising of exceptions in an 
except: clause. It'll preserve the traceback.

The control flow for the masking/re-raising seems convoluted. I've seen 
code like the following before:

     if nomaskexception:
         nonmasked_exceptions = Exception
     else:
         nonmasked_exceptions = ()

     try:
         something()
     except nonmasked_exceptions as e:
         raise
     except ConcreteException as e:
         raise_masked_exception(e)

Also it might be useful to factor raise_masked_exception() out; see below.

> - Rather than returning an DuplicateEntry error when POSTing to an
> existing host, do a host_mod instead.

Could you add a test for this?

It looks like ip_address has no effect in this case, should we fail if 
it's given instead of silently ignoring it? Same for rebuilding new hosts.

> - Add config option to pass updatedns=True when deleting a host.

In DELETE, the exception handling seems overly broad. With a 
raise_masked_exception() function and nomaskexception, you could handle 
the specific error. It should help debugging if nothing else.

This way of masking errors seems clumsy when nesting commands; I wonder 
if they can be handled better at a higher level ('request.error_response'?)

-- 
Petr³