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

[Rob Crittenden]

Petr Viktorin wrote:
> On 03/12/2014 07:48 PM, Rob Crittenden wrote:
>> Petr Viktorin wrote:
>>> On 03/10/2014 08:55 PM, Rob Crittenden wrote:
>>>> Rob Crittenden wrote:
>>>>> Petr Viktorin wrote:
>>>>>> On 02/27/2014 10:18 PM, Rob Crittenden wrote:
>>>>>>> Rob Crittenden wrote:
>>>>>>> Updated patch based on feedback from Foreman team. I added a new
>>>>>>> URI,
>>>>>>> /features, which Foreman uses to determine what capabilities a proxy
>>>>>>> has.
>>>>>>>
>>>>>>> rob
>>>>>>
>>>>>> On my VMs, where the first request is handled properly but the server
>>>>>> hangs on the second one. I gave you access to the machines for
>>>>>> investigation.
>>>>>
>>>>> Sent you something out-of-band but in short, I wasn't able to
>>>>> reproduce
>>>>> on your reproducing VMs :-( Ping me tomorrow and we'll try it
>>>>> together.
>>>
>>> It ended up a combination of my mistake and a bug in GSSProxy. At least
>>> you found the bug. https://fedorahosted.org/gss-proxy/ticket/121
>>>
>>>>>> Please add the Python libraries (python-cherrypy, python-requests,
>>>>>> python-kerberos) to BuildRequires. Otherwise the build fails due to
>>>>>> pylint errors.
>>>>>
>>>>> Fixed.
>>>>>
>>>>>>
>>>>>> In the man page:
>>>>>>
>>>>>>> +Create a host or user whose credentials will be used by the
>>>>>>> server to
>>>>>>> make requests and add it to the role:
>>>>>>> +
>>>>>>> + $ ipa user\-add \-\-first=Smartproxy \-\-last=Serversmartproxy
>>>>>>> + $ ipa role\-add\-member \-\-users=smartproxy 'Smartproxy
>>>>>>> management'
>>>>>>
>>>>>> the first command should be
>>>>>>      ipa user-add smartproxy --first=Smartproxy
>>>>>> --last=Serversmartproxy
>>>>>> since by default the username is 'sserversmartproxy'.
>>>>>
>>>>> The problem was a missing space before smartproxy. I have the login
>>>>> name
>>>>> last in this example. Fixed.
>>>>>
>>>>>>
>>>>>> A nitpick regarding the systemd service: according to [0],
>>>>>> Type=forking
>>>>>> should be avoided. Is there a reason against setting Type=simple, and
>>>>>> removing the PID file?
>>>>>
>>>>> Because I wasn't able to get this working with cherrypy daemon mode.
>>>>> AFAICT it forks itself and systemd wouldn't end up with the right
>>>>> pid so
>>>>> can't stop the service.
>>>>
>>>> And now the updated patch. The changes are super-minor.
>>>>
>>>> rob
>>>>
>>>
>>> One last nitpick: the IPAErrors get encoded as JSON but the
>>> Content-Encoding is set to text/html. It's a one-line change so I went
>>> ahead and tested with it. ACK from me if you agree.
>>
>> +1
>>
>> 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.

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

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