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

[Petr Viktorin]

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.


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?


-- 
Petr³


diff --git a/freeipa.spec.in b/freeipa.spec.in
index 80de52e..9b56ec1 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -79,7 +79,6 @@ BuildRequires:  libverto-devel
  BuildRequires:  systemd
  BuildRequires:  libunistring-devel
  BuildRequires:  python-lesscpy
-BuildRequires:  python-kerberos
  BuildRequires:  python-cherrypy
  BuildRequires:  python-requests

diff --git a/make-lint b/make-lint
index 40483a6..89e091b 100755
--- a/make-lint
+++ b/make-lint
@@ -114,6 +114,12 @@ class IPATypeChecker(TypeChecker):
          return attrs

      def visit_getattr(self, node):
+
+        # TODO: temporary hack to enable building with older 
python-kerberos
+        # TODO: remove when we BuildRequire python-kerberos 1.1-14
+        if node.attrname == 'authGSSClientInquireCred':
+            return
+
          try:
              inferred = list(node.expr.infer())
          except InferenceError:

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-rcrit-1106-8+pviktori-rest.patch
Type: text/x-patch
Size: 48356 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140314/ab0ee587/attachment.bin>