[Freeipa-devel] [PATCH] 1088 Recover DNA ranges when deleting a master

[Rob Crittenden]

Petr Viktorin wrote:
> 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?

done

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

sure

>
> 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 shouldn't make a difference. What isn't working?

>> 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

fixed

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

Sure. I kept DNA as upper.

> [...]
>> +    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.

It does in some but it isn't consistent. I removed my calls though.

$ ipa-replica-manage dnarange-show -p badpassword
Connection failed: {'desc': 'Invalid credentials'}

>> +    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.

Fixed.

>
> [...]
>> +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.

ok.

>
> [...]
>> +    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)
>
> [...]

Sure. My original intention was to return the overlapping range but then 
decided against it since it is probably fairly obvious anyway.

>> +    # 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?

No because then we'd still have to validate things and it would be split 
into two places. I can live with these 3 lines of duplicated code, esp 
since we need no error handling around them because it is assumed 
correct by the time it reaches it.

>
> [...]
>> +        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.

Done. I had missed this one.

>
> [...]
>> +        # 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

I simplified things a bit but a for/else won't work here as we need to 
check all ranges all the time. It is perfectly fine to not fit into a 
range, as long as it fits into SOME range.

>> +            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.

Good point, fixed.

> 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?

It depends on what the other range is used for. If it isn't for POSIX 
values then overlap may be perfectly acceptable.

>
> [...]
>> 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

ok

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

ok

>
>> +\- List the DNA ranges
>> +.TP
>> +\fBipadnarange\-set SERVER x\-y\fR
>
> I'd use START-END instead of x-y

ok, that's fine.

>
>> +\- 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.

Added

>
> [...]
>> 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.

Stuck with upper-case DNA again.

>
>> +        """
>> +        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.

sure

>
>> +        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.

Ok, I guess thinking forward to when this gets converted to the 
framework that makes sense. Currently there are no logs.

>
> [...]
>> +        if (nextvalue > maxvalue and maxvalue == 1100 and
>> +            nextvalue == 1101 and remaining == 0):
>
> What are the magic values? Also this redundantly checks if 1101 > 1100.

The magic values are the default values assigned to the DNA configured. 
The redundant check, and the check for remaining, is just to be 
absolutely sure that we are dealing with a default configuration and not 
some custom values. I dropped the 1101 > 1100 check.

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

Ok, I'll drop this since it doesn't affect things with the new LDAP backend.

I also added one change related to the LDAP core changes. In the past if 
you did not have a ticket it would prompt for DM password. This was 
broken after the updates. I added an additional except in test_connection().

rob
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-rcrit-1088-2-dnarange.patch
Type: text/x-diff
Size: 28631 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130306/f1316613/attachment.bin>