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

Wed Apr 30 12:11:03 UTC 2014

On 04/30/2014 05:11 AM, Rob Crittenden wrote:
> 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.

Instead of
+    nomaskexception = options.get('nomaskexception', False)
+    if 'nomaskexception' in options:
+        del options['nomaskexception']
you can use:
+    nomaskexception = options.pop('nomaskexception', False)

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

Nope, wrong test. 'Update the host' should use POST.

Also, when updating using POST, unspecified information is removed, e.g. 
doing
      http POST 
':8090/ipa/smartproxy/host/testhost.idm.lab.eng.brq.redhat.com?userclass=a_class'
removes the description etc.
I don't think that's intended.

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

Also, add 'add dns entries' and 'update dns entries' to the permission 
line in the manpage.

-- 
Petr³