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

Wed Apr 30 14:57:13 UTC 2014

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

Of course.

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

Sorry, did it too quickly for my own good.

It also turned out I was still returning 201 on updates which is bad.

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

You're right. I pop off empty values on updates now. This means that one 
can't wipe the description once set though. We may need to set a magic 
value for that.

I beefed up the data passed into the tests so we can confirm that values 
don't disappear.

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

Done, and added 'remove dns entries' as well.

rob
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-rcrit-1106-13-rest.patch
Type: text/x-patch
Size: 52339 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140430/f18dcb26/attachment.bin>