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

[Petr Viktorin]

On 04/30/2014 04:57 PM, Rob Crittenden wrote:
> 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.

Fortunately there already is a magic value, the empty string, which IPA 
treats as "unset attribute". It works nicely :)

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

Great!
Enough nitpicking, let's go break master.

ACK, pushed to master: 64dcb1ec76fa706320746720431ef815eb3e9ecd

-- 
Petr³