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)
- 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.
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.
- 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.
--
Petr³
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
