Re: [Freeipa-devel] [PATCH] 1021 index fqdn for 2.2. branch
On Mon, 2012-05-21 at 20:51 -0400, Simo Sorce wrote: On Mon, 2012-05-21 at 16:40 -0400, Rob Crittenden wrote: We already have an index on fqdn in the master branch. Add this to the 2.2 branch as well. We do a search on host when installing a replica and an unindexed search might fail. ACK Simo. Pushed to ipa-2-2. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 258 Remove LDAP limits from DNS service
On 05/10/2012 09:31 AM, Martin Kosek wrote:
bind-dyndb-ldap persistent search queries LDAP for all DNS records.
The LDAP connection must have no size or time limits to work
properly.
This patch updates limits both for existing service principal
on updated machine and for new service principals added
as a part of DNS installation.
https://fedorahosted.org/freeipa/ticket/2531
Yes, the limits are set correctly on both install and upgrade.
ACK
Minor style nitpicks:
+root_logger.critical(Could not modify principal's %s entry: %s \
+% (dns_principal, str(e)))
The backshashes are unnecessary inside parentheses.
+dnsupdates[dns_service_dn] = {'dn' : dns_service_dn,
+ 'updates' : limit_updates}
Don't put a space before a semicolon.
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 258 Remove LDAP limits from DNS service
On Tue, 2012-05-22 at 12:15 +0200, Petr Viktorin wrote:
On 05/10/2012 09:31 AM, Martin Kosek wrote:
bind-dyndb-ldap persistent search queries LDAP for all DNS records.
The LDAP connection must have no size or time limits to work
properly.
This patch updates limits both for existing service principal
on updated machine and for new service principals added
as a part of DNS installation.
https://fedorahosted.org/freeipa/ticket/2531
Yes, the limits are set correctly on both install and upgrade.
ACK
Minor style nitpicks:
+root_logger.critical(Could not modify principal's %s entry:
%s \
+% (dns_principal, str(e)))
The backshashes are unnecessary inside parentheses.
+dnsupdates[dns_service_dn] = {'dn' : dns_service_dn,
+ 'updates' : limit_updates}
Don't put a space before a semicolon.
I removed the space before the colon.
Pushed to master.
Martin
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 25 ipa-server-install: s/calculated/determined/
https://fedorahosted.org/freeipa/ticket/2704 Output message of the 'read_domain_name' function in ipa-server-install was reworded. -- Regards, Ondrej Hamada FreeIPA team jabber: oh...@jabbim.cz IRC: ohamada From 4a7eda9b2a97b10ee0767696406fda09c1a9de86 Mon Sep 17 00:00:00 2001 From: Ondrej Hamada oham...@redhat.com Date: Tue, 22 May 2012 12:19:53 +0200 Subject: [PATCH] ipa-server-install reword message Output message of the 'read_domain_name' function in ipa-server-install was reworded. https://fedorahosted.org/freeipa/ticket/2704 --- install/tools/ipa-server-install |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/install/tools/ipa-server-install b/install/tools/ipa-server-install index d3327a6803d10012f412fbb8365b80e39e9124c3..2f06a9e879902eb1c2ac340757fcd1762959fe30 100755 --- a/install/tools/ipa-server-install +++ b/install/tools/ipa-server-install @@ -386,7 +386,7 @@ def read_host_name(host_default,no_host_dns=False): return host_name def read_domain_name(domain_name, unattended): -print The domain name has been calculated based on the host name. +print The domain name has been determined based on the host name. print if not unattended: domain_name = user_input(Please confirm the domain name, domain_name) -- 1.7.6.5 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 260 Replace DNS client based on acutil with python-dns
On 05/16/2012 09:44 AM, Martin Kosek wrote:
On Tue, 2012-05-15 at 14:02 +0200, Petr Viktorin wrote:
On 05/11/2012 06:52 PM, Martin Kosek wrote:
python-dns is very feature-rich and it can help us a lot with our DNS
related code. This patch does the first step, i.e. replaces acutil use
with python-dns, which is more convenient to use as you will see in the
patch. More integration will follow in the future.
I send this patch rather early, so that I can get responses to this
patch early and also so that we are able to catch issues in a safe
distance from the next release.
---
IPA client and server tool set used authconfig acutil module to
for client DNS operations. This is not optimal DNS interface for
several reasons:
- does not provide native Python object oriented interface
but but rather C-like interface based on functions and
structures which is not easy to use and extend
- acutil is not meant to be used by third parties besides
authconfig and thus can break without notice
Replace the acutil with python-dns package which has a feature rich
interface for dealing with all different aspects of DNS including
DNSSEC. The main target of this patch is to replace all uses of
acutil DNS library with a use python-dns. In most cases, even
though the larger parts of the code are changed, the actual
functionality is changed only in the following cases:
- redundant DNS checks were removed from verify_fqdn function
in installutils to make the whole DNS check simpler and
less error-prone. Logging was improves for the remaining
checks
- improved logging for ipa-client-install DNS discovery
https://fedorahosted.org/freeipa/ticket/2730
[...]
Fixed.
Martin
I've been testing the patches in various setups and haven't found a
regression so far. ACK on 261, for 260 I have a comment below.
diff --git a/ipa-client/ipaclient/ipadiscovery.py
b/ipa-client/ipaclient/ipadiscovery.py
index
86bef28b2d7fdfc8111b493bddec7ac6888f021a..1889d1918c01fe0c80a3d56b9a7ef304c5a7d97c
100644
--- a/ipa-client/ipaclient/ipadiscovery.py
+++ b/ipa-client/ipaclient/ipadiscovery.py
@@ -310,84 +313,74 @@ class IPADiscovery:
os.rmdir(temp_ca_dir)
-def ipadnssearchldap(self, tdomain):
-servers =
-rserver =
+def ipadns_search_srv(self, domain, srv_record_name, default_port,
+ get_first=True):
+
+Search for SRV records in given domain. When no record is found,
+en empty string is returned
-qname = _ldap._tcp.+tdomain
-# terminate the name
-if not qname.endswith(.):
-qname += .
-results = ipapython.dnsclient.query(qname,
ipapython.dnsclient.DNS_C_IN, ipapython.dnsclient.DNS_T_SRV)
+:param domain: Search domain name
+:param srv_record_name: SRV record name, e.g. _ldap._tcp
+:param default_port: When default_port is not None, it is being
+checked with the port in SRV record and if they don't
+match, the port from SRV record is appended to
+found hostname in this format: hostname:port
+:param get_first: break on first find, otherwise multiple finds
+separated by : may be returned
They are separated by ,.
In the calling code, for splitting a comma-separated string it is better
to use servers.split(',') than ipautil.parse_items(servers). Or, return
a list directly from here.
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 25 ipa-server-install: s/calculated/determined/
On Tue, 2012-05-22 at 14:50 +0200, Petr Viktorin wrote: On 05/22/2012 12:38 PM, Ondrej Hamada wrote: https://fedorahosted.org/freeipa/ticket/2704 Output message of the 'read_domain_name' function in ipa-server-install was reworded. ACK Pushed to master. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 1019 require policycoreutils if SELinux is enabled
On 2012-05-18 17:53, Rob Crittenden wrote: We don't have an explicit requires on the policycoreutils package in the client because SELinux is not required (just recommended). SELinux can be enabled without this package so check for that condition and don't allow installation if it is the case. The resulting install will be rather broken. Also check on the server when installing. This should never happen but in theory it could do the server install then fail in the client because of this. rob All other platform-dependent services have a default defined in ipapython/services.py.in. Shouldn't check_selinux_status have one, too? -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0040 Move install script error handling to a common function
On 2012-04-23 17:05, John Dennis wrote:
On 04/23/2012 05:19 AM, Petr Viktorin wrote:
This fixes https://fedorahosted.org/freeipa/ticket/2071 (Add final debug
message in installers).
I submitted an earlier version of this patch before (0014), but it was
too much to include in 2.2. Hopefully now there's more space for
restructuring. I think it's better to start a new thread with this
approach.
The try/except blocks at the end of installers/management scripts are
replaced by a call to a common function, which includes the final
message.
For each specific error, the error handlers in all scripts was almost
the same, but each script handled a different selection of errors.
Instead of having this copy/pasted code (with subtle differences
creeping in over time), this patch consolidates it all in one place.
I like this approach much better than the earlier patch, great, thanks.
I'm a big fan of calling into common code instead of copying code to my
mind the refactoring to utilize common code is great approach. I also
like the fact the logging configuration is not modified after it's
established.
At some point we may want to revist how the log messages are generated.
For example should all communication to the console pass through the
console handler? Is there a logger established for the script? Should
the format of messages emitted to the console be altered? Should all
command line utilities accept the both the verbose and debug flag? Etc.
But for now this is fantastic start in the right direction.
I have not installed and exercised the patch so I can't comment on any
runtime time issues that might be present, but from code inspection only
it has my ACK.
Thanks John!
Yes, this is just a start.
Patch rebased to curent master
--
Petr³
From 9f9c55a65a87e5b687506d999f100978b27b5c74 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Fri, 20 Apr 2012 04:39:59 -0400
Subject: [PATCH] Move install script error handling to a common function
All of our install/admin scripts had a try/except block calling the
main function and handling common exceptions. These were copy-pasted
from each other and modified to various levels of sophistication.
This refactors them out to a single function, which includes a final
pass/fail message for all of the scripts.
https://fedorahosted.org/freeipa/ticket/2071
---
install/tools/ipa-adtrust-install| 34 ++---
install/tools/ipa-ca-install | 65 +++--
install/tools/ipa-compat-manage | 23 +-
install/tools/ipa-compliance | 14 ++--
install/tools/ipa-csreplica-manage | 18 +
install/tools/ipa-dns-install| 34 ++---
install/tools/ipa-ldap-updater | 20 ++
install/tools/ipa-managed-entries| 19 +
install/tools/ipa-nis-manage | 23 +-
install/tools/ipa-replica-conncheck | 11 +--
install/tools/ipa-replica-install| 64 +++--
install/tools/ipa-replica-manage | 25 +--
install/tools/ipa-replica-prepare| 23 ++
install/tools/ipa-server-certinstall | 14 ++--
install/tools/ipa-server-install | 57 +++
install/tools/ipa-upgradeconfig |9 +--
install/tools/ipactl | 30 ++--
ipaserver/install/installutils.py| 127 ++
ipaserver/install/ldapupdate.py |5 +-
19 files changed, 260 insertions(+), 355 deletions(-)
diff --git a/install/tools/ipa-adtrust-install b/install/tools/ipa-adtrust-install
index 248ea35eaa86dd59ebbc871b86df780cfd71ccf6..0dfc6eba6568f2388fb2bec8319acf593ad838a9 100755
--- a/install/tools/ipa-adtrust-install
+++ b/install/tools/ipa-adtrust-install
@@ -21,8 +21,6 @@
# along with this program. If not, see http://www.gnu.org/licenses/.
#
-import traceback
-
from ipaserver.plugins.ldap2 import ldap2
from ipaserver.install import adtrustinstance
from ipaserver.install.installutils import *
@@ -35,6 +33,8 @@ import krbV
import ldap
from ipapython.ipa_log_manager import *
+log_file_name = /var/log/ipaserver-install.log
+
def parse_options():
parser = IPAOptionParser(version=version.VERSION)
parser.add_option(-p, --ds-password, dest=dm_password,
@@ -86,8 +86,8 @@ def main():
if os.getegid() != 0:
sys.exit(Must be root to setup AD trusts on server)
-standard_logging_setup(/var/log/ipaserver-install.log, debug=options.debug, filemode='a')
-print \nThe log file for this installation can be found in /var/log/ipaserver-install.log
+standard_logging_setup(log_file_name, debug=options.debug, filemode='a')
+print \nThe log file for this installation can be found in %s % log_file_name
root_logger.debug('%s was invoked with options: %s' % (sys.argv[0], safe_options))
root_logger.debug(missing options might be asked for interactively later\n)
@@ -227,26 +227,6 @@ def main():
return 0
-try:
-sys.exit(main())
-except SystemExit, e:
-sys.exit(e)
Re: [Freeipa-devel] [PATCH] 1018 enforce sizelimit when searching for permissions
Martin Kosek wrote:
On Fri, 2012-05-18 at 10:17 -0400, Rob Crittenden wrote:
Rob Crittenden wrote:
Martin Kosek wrote:
On Thu, 2012-05-17 at 16:11 -0400, Rob Crittenden wrote:
We do two searches when looking for permissions. One within the
permission object itself and a second in the ACIs. We weren't enforcing
a sizelimit on either search.
rob
This returns the right result, but I don't think it is right with
respect to truncated flag because of several reasons:
1) You manipulate and set truncated flag in post_callback but this
won't affect the flag in the returned result because the new value is
not propagated outside of the post_callback function. I.e. truncated
flag will be set correctly only when it was raised during original
permission_find.
Truncated is still honored as expected. I even added a test case for this.
Yes, but it only works when the truncated flag is raised by the base
LDAP search, i.e. the search for permission objects (which is a case of
your unit test). If the search does not raise the flag and it is set
later in post callback, it is never propagated to the user. Please check
the attached (crappy) test that shows this issue:
==
FAIL: test_permission[20]: permission_find: Search for permissions by
attr with a limit of 1 (truncated)
--
...
AssertionError: assert_deepequal: expected != got.
test_permission[20]: permission_find: Search for permissions by attr
with a limit of 1 (truncated)
expected = True
got = False
path = ('truncated',)
I am not sure how to solve this right, we may need to add a new return
attribute (truncated) to all LDAPSearch post callbacks so that the post
callback can really modify it - something similar we already do with pre
callbacks which are able to change LDAP search filter, scope etc.
2) The part with ind is strange:
+ # enforce --sizelimit
+ if len(entries) == max_entries:
+ if ind + 1 len(results):
+ truncated = True
+ break
I think it would be much easier to just do
...
if (dn, permission) not in entries:
if len(entries) max_entries:
entries.append((dn, permission))
else:
truncated = True
break
Otherwise you would rise truncated even when the rest of results
does not contain relevant entries that would have not been added anyway.
Yes, that makes sense.
And now updated patch.
We can now also remove the enumerate part, ind is no longer needed.
Martin
You're right, I'd have sworn I tested that...
The only solution is going to be to have the post_callback return
truncated. This is going to be a rather intrusive change.
rob
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
