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

Wed Apr 30 03:11:43 UTC 2014

Petr Viktorin wrote:
> 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.

Good idea. I sort of split the difference.

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

Done both.

>
>> - 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'?)

Yes. I imagine that the Foreman smartproxy is doing this to mask the 
issue as well. I dropped it, doing a GET instead so we'll get the proper 
error in any case. It's a bit racy but it won't be as unnerving as 
running into https://fedorahosted.org/freeipa/ticket/4329

rob

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-rcrit-1106-12-rest.patch
Type: text/x-patch
Size: 50986 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140429/6593d7fb/attachment.bin>