On 01/23/2014 02:17 PM, Petr Viktorin wrote:
On 01/22/2014 08:04 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 01/20/2014 05:21 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 01/17/2014 10:24 PM, Rob Crittenden wrote:
Implement an IPA RESTful Foreman-compatible smart proxy. This exposes
hosts and hostgroups via an unauthenticated REST API. The idea is
that
this service runs on the Foreman server and only listens on local
ports.
It is a CherryPy-based server and that handles the majority of REST
for us.
I included some tests, they can be executed with: nosetests -v
smartproxy/tests
Why is it not a part of ipatests?
I can move it if it's a show-stopper. It seemed specific to this one
directory so I stuck it there. It isn't relevant for most testing and
requires some manual configuration (though CI could handle it).
Not strictly a show stopper, but please move it. At the very least it
should end up in the freeipa-tests package.
Moved.
Thanks!
A lot of the tests (integration, webUI) need manual configuration, so
this would be no exception. Of course the tests should be skipped if the
configuration was not done, and the config instructions should be added
to/linked from http://www.freeipa.org/page/Testing
Hmm, maybe. There are instructions to set up the environment in the man
page. Testing beyond that consists of ./make-test tests/test_smartproxy
I can add that testing bit once the patch is approved I suppose.
rob
Please add python-kerberos >= 1.1-13 to Requires and BuildRequires;
pylint fails with lower versions.
Are there plans to release python-kerberos-1.1-13.fc20, or will this be
f21+ only?
I've noticed gssproxy-0.3.1 is not available in rawhide (but it is in
f20). I've sent a mail to Günther; meanwhile I'll use the f20 package
(may the releng gods be merciful).
Here are my thoughts on the patch.
The URL endpoint /ipa/rest suggests that if we implement a complete REST
API for IPA it would live here. Is the API supposed to be
future-compatible? (The API itself seems to be a good subset of a
complete REST API, but can we easily add an frontend with
authentication, i18n, etc. here later, and keep the limitations for
unauthenticated access?)
Perhaps /ipa/smartproxy would be a better choice?
I assume we're in control of the REST API design. Is that correct?
According to RFC 2616, the POST verb should create a sub-resource of
what's at the given URI. In other words, it would be more correct to use:
/ipa/rest/host POST Create a host entry {'hostname': ...}
and same for hostgroup.
Foreman's DHCP and DNS APIs seem to do this right.
Is returning HTML for errors a Foreman requirement? I think it's a weird
thing to do in an API.
I don't think IPACommand should be a class. It has no state, and it
looks to me like if one of GET/POST/DELETE happens to not be overridden
in an exposed subclass, there's a potential security issue -- any IPA
command can be called. Could you use simple functions instead?
In IPACommand.Command, there's lot of duplicate error handling blocks.
You can handle more error classes at once with:
except (errors.DuplicateEntry, errors.DNSNotARecordError) as e:
Instead of `str(e)`, please report the errors as
"{e.__class__.__name__}: {e}" -- the message is sometimes unusable
without the exception type (e.g. a KeyError from {}['result'] would only
have 'result' as message).
Passing *args, **options to Command closes the door for any (future)
parametrization. Consider making the signature e.g. Command(self,
command, args, options, success_stastus=200).
Instead of `if fqdn == None:` use `if fqdn is None:`.
It would be great if you passed `api` around explicitly, instead of
relying on the global instance.
Some import issues in test_smartproxy/resttest.py:
resttest.py:24:8: Unused import 'sys'
resttest.py:25:8: Unused import 'socket'
resttest.py:26:8: Unused import 'nose'
`requests` is a third-party library, python-requests in Fedora; it
should be in the specfile. (Dogtag pulls it in for now, but when
ipa-server-ca is a separate package things will break.)
According to PEP8:
Imports should be grouped in the following order:
1. standard library imports
2. related third party imports
3. local application/library specific imports [ipa* in our case]
You should put a blank line between each group of imports.
Normally I'm not pedantic about this, but it does help spotting mistakes
like the above.
I'm not a fan of the test code copied from test_xmlrpc, but I guess we
have better things to do than refactoring that :(
But there should at least be a check that skips the tests if the smart
proxy is not available.
In the ipa-rest man page, "Add this to the top of the file, before any
other services" doesn't specify what file should be edited.
Also, maybe the page should mention that `systemd start
ipa-rest.service` starts the server on port 8090.
It's probably an error at my side, but I couldn't get a response from
the server: I got:
<html><body>The server encountered an unexpected condition which
prevented it from fulfilling the request.</body></html>
and in the log I see:
CCacheError: did not receive Kerberos credentials
I'm not familiar with gssproxy, but I don't see how it knows that it
should authenticate as the "rest" user.
Should /var/log/ipa-rest.errors be world-readable?
What is gssproxy.conf.snippet for? The same info is in the man page.
Also please fix:
./ipalib/util.py:61:56: E703 statement ends with a semicolon
./ipalib/util.py:63:54: E703 statement ends with a semicolon
--
Petr³
_______________________________________________
Freeipa-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/freeipa-devel
