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
than inter-diff since it is so small.

Not really. Having a patch file with a sequence+revision
refer to has its merits. Especially in a hairy thread like
Also one of our MUAs wrapped the lines, I had to undo that

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

IPA stores the principal in the request context so using that
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
unversioned one. So lint (and thus the build) will fail if the
python-kerberos version is installed.

A possible a solution to the build trouble would be to just
add a
exception now, and open a ticket to remove it later. That way
succeeds despite the older version, and the new
python-kerberos is
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
below). Does that look okay to push?

I'm trying to find a better solution to all this. I may end up
Martin's suggestion of rawhide-only to avoid this sort of

Looks like you'll still need to silence pylint on f20 in that

The deal with the smartproxy is that you can/should be able to
it on
any IPA-enrolled client, so you can run it directly on the
with the IPA server somewhere else. What this means is that
could probably fairly easily package this up for other
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
crickets for weeks. I think in the context of the calendar
PyKerberos is small potatoes so doesn't get much lovin'. I'll
amp up
nagging to get some sort of response, even if it is "stop


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
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
to see if we can get the patch accepted their until (if) upstream
takes a look.

I decided to run the new WSGI app in a different process group,
the smartproxy we use for delegation. This simplifies the
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


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
It would be nice to put the whole path here so people don't have to
search for the file.


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

I combined it with the subsequent sentence so hopefully it is a bit

I also added a bit on testing so you can confirm that things are

Side note, cherrypy's routing makes requests like this possible:
     http POST

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.


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

I tested with that added; ACK if you agree.

ACK on your change.

Sorry, one more set of changes, some fairly significant. This brings
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
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
         nonmasked_exceptions = ()

     except nonmasked_exceptions as e:
     except ConcreteException as e:

Also it might be useful to factor raise_masked_exception() out; see

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

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

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

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.

Enough nitpicking, let's go break master.

ACK, pushed to master: 64dcb1ec76fa706320746720431ef815eb3e9ecd


Freeipa-devel mailing list

Reply via email to