[Freeipa-devel] [PATCH] 1088 Recover DNA ranges when deleting a master
Petr Viktorin
pviktori at redhat.com
Mon Mar 4 14:59:45 UTC 2013
On 03/01/2013 11:57 PM, Rob Crittenden wrote:
> Implement the design at http://freeipa.org/page/V3/Recover_DNA_Ranges
Could you add the link to the commit message?
> Note that this required some new ACIs in cn=config which is not
> replicated so the range-set commands won't work against older instances.
> It should be gracefully handled though.
I think noting this in the man page would be helpful.
On new installs, the ACI on cn=Posix IDs,cn=Distributed Numeric
Assignment Plugin,cn=plugins,cn=config is added before the entry itself.
I didn't test everything as I didn't get the access.
> It also doesn't work so well if you try it using a delegated
> administrator, see ticket https://fedorahosted.org/freeipa/ticket/3480
>
> rob
It should be possible to shrink existing ranges, i.e. ones that overlap
with themselves:
$ ipa-replica-manage dnarange-show
vm-075.idm.lab.eng.brq.redhat.com: 1401000005-1401100499
$ ipa-replica-manage dnarange-set vm-075.idm.lab.eng.brq.redhat.com
1401000005-1401100498
New range overlaps the DNA range on vm-075.idm.lab.eng.brq.redhat.com
> freeipa-rcrit-1088-dnarange.patch
>
>
>>From 9a654b0b3730f8d9058dfbf25a93a58e1f4939e7 Mon Sep 17 00:00:00 2001
> From: Rob Crittenden<rcritten at redhat.com>
> Date: Fri, 1 Mar 2013 15:02:14 -0500
> Subject: [PATCH] Extend ipa-replica-manage to be able to manage DNA ranges.
>
> Attempt to automatically save DNA ranges when a master is removed.
> This is done by trying to find a master that does not yet define
> a DNA on-deck range. If one can be found then the range on the deleted
> master is added.
>
> If one cannot be found then it is reported as an error.
>
> Some validation of the ranges are done to ensure that they do overlap
> an IPA local range and do not overlap existing DNA ranges configured
> on other masters.
The patch adds some trailing whitespace, please trim it.
I also found some nitpicks, see below.
>
> https://fedorahosted.org/freeipa/ticket/3321
> ---
[...]
> diff --git a/install/tools/ipa-replica-manage b/install/tools/ipa-replica-manage
> index 859809bf1c301913c3eb7fc92d1ed58675609b8c..6c0b45620dd2deabfc11ef2249b18205fb23b1fd 100755
> --- a/install/tools/ipa-replica-manage
> +++ b/install/tools/ipa-replica-manage
[...]
>
> +def showDNARanges(hostname, master, realm, dirman_passwd, nextrange=False):
Style issue: please don't use camel case for functions.
[...]
> + try:
> + repl = replication.ReplicationManager(realm, hostname, dirman_passwd)
> + except Exception, e:
> + sys.exit("Connection failed: %s" % ipautil.convert_ldap_error(e))
ipaldap should convert LDAP errors to IPA ones, there's no need to call
convert_ldap_error. Same in other places.
> + dn = DN(('cn', 'masters'), ('cn', 'ipa'), ('cn', 'etc'), repl.suffix)
> + try:
> + entries = repl.conn.get_entries(dn, repl.conn.SCOPE_ONELEVEL)
> + except:
Don't use a bare except. Same in other places.
[...]
> +def setDNARange(hostname, range, realm, dirman_passwd, next_range=False):
> + """
> + Given a DNA range try to change it on the designated master.
> +
> + The range must not overlap with any other ranges and must be within
> + one of the IPA local ranges as defined in cn=ranges.
> +
> + Setting an on-deck range of 0-0 removes the range.
> +
> + Return True if range was saved, False if not
> +
> + hostname: hostname of the master to set the range on
> + range: The DNA range to set
> + realm: our realm, needed to create a connection
> + dirman_passwd: the DM password, needed to create a connection
Please also mention next_range.
[...]
> + def range_intersection(s1, s2, r1, r2):
> + overlap = xrange(max(s1, r1), min(s2, r2) + 1)
> + return len(overlap) > 0
That looks complicated. How about:
def ranges_intersect(s1, s2, r1, r2):
return max(s1, r1) <= min(s2, r2)
[...]
> + # Normalize the range
> + (dna_next, dna_max) = range.split('-', 1)
Can this be done before validate_range, so id doesn't have to be
duplicated there?
[...]
> + dn = DN(('cn', 'masters'), ('cn', 'ipa'), ('cn', 'etc'), ipautil.realm_to_suffix(realm))
I think you should use repl.suffix instead of generating it again.
[...]
> + # Verify that this is within one of the IPA domain ranges.
> + dn = DN(('cn','ranges'),('cn','etc'),repl.suffix)
Style issue: no spaces after commas
> + try:
> + entries = repl.conn.get_entries(dn, repl.conn.SCOPE_ONELEVEL,
> + "(objectclass=ipaDomainIDRange)")
> + except errors.NotFound, e:
> + sys.exit('Unable to load IPA ranges: %s' % e.message)
> +
> + failed = 0
> + for ent in entries:
This loops more than necessary and is somewhat hard to follow. Consider
using for-else here:
for ...:
...
if okay:
break
else:
raise error
> + entry_start = int(ent.single_value('ipabaseid'))
> + entry_max = entry_start + int(ent.single_value('ipaidrangesize'))
> + if not range_intersection(dna_next, dna_max, entry_start, entry_max):
I think we want the DNA range to be fully contained in the idrange,
rather than just overlap a part of it.
Please also adjust the man page when you change this.
> + failed += 1
> +
> + if failed == len(entries):
> + sys.exit("New range does not fit within existing IPA ranges. See ipa help idrange command")
> + # If this falls within any of the AD ranges then it fails.
> + try:
> + entries = repl.conn.get_entries(dn, repl.conn.SCOPE_BASE,
> + "(objectclass=ipatrustedaddomainrange)")
If we add more types of ranges in the future, should they also be
checked? Would (!(objectclass=ipaDomainIDRange)) be more appropriate here?
[...]
> diff --git a/install/tools/man/ipa-replica-manage.1 b/install/tools/man/ipa-replica-manage.1
> index 836743902278ec2273f3ce7a7fbf3992370c4828..d23e2566eb9a22c70991cbdca0140eb1d268533c 100644
> --- a/install/tools/man/ipa-replica-manage.1
> +++ b/install/tools/man/ipa-replica-manage.1
> @@ -16,13 +16,13 @@
> .\"
> .\" Author: Rob Crittenden<rcritten at redhat.com>
> .\"
> -.TH "ipa-replica-manage" "1" "Mar 14 2008" "FreeIPA" "FreeIPA Manual Pages"
> +.TH "ipa-replica-manage" "1" "Mar 1 2013" "FreeIPA" "FreeIPA Manual Pages"
> .SH "NAME"
> ipa\-replica\-manage \- Manage an IPA replica
> .SH "SYNOPSIS"
> -ipa\-replica\-manage [\fIOPTION\fR]... [connect|disconnect|del|list|re\-initialize|force\-sync]
> +ipa\-replica\-manage [\fIOPTION\fR]... [COMMAND}
Please straighten the curly brace at the end
> .SH "DESCRIPTION"
> -Manages the replication agreements of an IPA server.
> +Manages the replication agreements of an IPA server. The available commands are:
> .TP
> \fBconnect\fR [SERVER_A] <SERVER_B>
> \- Adds a new replication agreement between SERVER_A/localhost and SERVER_B
> @@ -54,6 +54,18 @@ Manages the replication agreements of an IPA server.
> \fBlist\-clean\-ruv\fR
> \- List all running CLEANALLRUV and abort CLEANALLRUV tasks.
> .TP
> +\fBipadnarange\-show [SERVER]\fR
The subcommand is dnarange-show, no ipa at the start. Same for the others.
> +\- List the DNA ranges
> +.TP
> +\fBipadnarange\-set SERVER x\-y\fR
I'd use START-END instead of x-y
> +\- Set the DNA range on a master
> +.TP
> +\fBipadnanextrange\-show [SERVER]\fR
> +\- List the next DNA ranges
> +.TP
> +\fBipadnanextrange\-set SERVER x\-y\fR
here too
[...]
> +The DNA range and on\-deck (next) values can be managed using the dnarange\-set and dnanextrange\-set commands. The rules for managing these ranges are:
> +\- The range must overlap a local range as defined by the ipa idrange command.
> +
> +\- The range cannot overlap the DNA range or on\-deck range on another IPA master.
> +
> +\- The primary DNA range cannot be removed.
> +
> +\- An on\-deck range range can be removed by setting it to 0\-0. The assumption is that the range will be manually moved or merged elsewhere.
Also, a range can't overlap ranges of trusted AD domains.
[...]
> index 804d046bf2553daa4aded5c23436a98636e20da0..9076c8396041a95c7ea01ef15aa77991516d30e6 100644
> --- a/ipaserver/install/replication.py
> +++ b/ipaserver/install/replication.py
> @@ -1308,3 +1308,123 @@ class ReplicationManager(object):
> print "This may be safely interrupted with Ctrl+C"
>
> wait_for_task(self.conn, dn)
> +
> + def getDNARange(self, hostname):
Style issue: please don't use camel-case for methods.
> + """
> + Return the DNA range on this server as a tuple, (next, max), or
> + (None, None) if no range has been assigned yet.
> + """
> + dn = DN(('cn', 'Posix IDs'), ('cn', 'Distributed Numeric Assignment Plugin'), ('cn', 'plugins'), ('cn', 'config'))
I'd put this in a global constant.
> + try:
> + entry = self.conn.get_entry(dn)
> + except Exception, e:
> + print "Unable to read DNA configuration: %s" % e.message
I think it's better to communicate the error by raising an exception,
rather than pretending the range hasn't been set yet.
With prints, the error won't appear in logs, and can't be checked by the
caller.
Same elsewhere.
[...]
> + if (nextvalue > maxvalue and maxvalue == 1100 and
> + nextvalue == 1101 and remaining == 0):
What are the magic values? Also this redundantly checks if 1101 > 1100.
I'd expect the DNS plugin to ensure that dnaRemainingValues == 0 if
nextvalue > maxvalue, do we need to check explicitly?
[...]
> diff --git a/ipaserver/ipaldap.py b/ipaserver/ipaldap.py
> index 4a46532642013204720ba467966c59de31a92301..cb9a7e98fd0c486abe5b8b92aff711fa69f23fa9 100644
> --- a/ipaserver/ipaldap.py
> +++ b/ipaserver/ipaldap.py
> @@ -1775,6 +1775,8 @@ class IPAdmin(LDAPClient):
> if removes:
> if not force_replace:
> modlist.append((ldap.MOD_DELETE, key, removes))
> + elif new_values == []: # delete an empty value
> + modlist.append((ldap.MOD_DELETE, key, removes))
I don't understand this change. AFAIK updateEntry/generateModList is
only used in ldapupdater now, and it's going away as soon as I can find
time to remove it. If you need to change it I'd like to know why.
--
Petr³
More information about the Freeipa-devel
mailing list