-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/27/2011 01:31 PM, Jakub Hrozek wrote:
> On 01/26/2011 09:50 PM, Simo Sorce wrote:
>> On Mon, 2011-01-24 at 15:51 +0100, Jakub Hrozek wrote:
>>> -----BEGIN PGP SIGNED MESSAGE-----
>>> Hash: SHA1
>>>
>>> On 01/21/2011 05:54 PM, Rob Crittenden wrote:
>>>> Jakub Hrozek wrote:
>>>>> -----BEGIN PGP SIGNED MESSAGE-----
>>>>> Hash: SHA1
>>>>>
>>>>> On 01/20/2011 11:53 PM, Simo Sorce wrote:
>>>>>> On Thu, 20 Jan 2011 17:27:37 -0500
>>>>>> Dmitri Pal<d...@redhat.com>  wrote:
>>>>>>
>>>>>>> Michael Gregg wrote:
>>>>>>>> Jakub Hrozek wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> as discussed in https://bugzilla.redhat.com/show_bug.cgi?id=671019
>>>>>>>> to delete a DNS RR one has to remove its record types one by one.
>>>>>>>>
>>>>>>>> This patch modifies the behaviour so that if the user runs
>>>>>>>> dnsrecord-del<zone>  <record-name>  with no other parameters, the
>>>>>>>> whole record is removed.
>>>>>>>>
>>>>>>>> Alternative solutions might be to expose the internal command that
>>>>>>>> is able to delete the record (although I think it is
>>>>>>>> counterintuitive to have one command to remove record types and one
>>>>>>>> for the whole record) or have a special flag (--del-all?) to remove
>>>>>>>> the whole record.
>>>>>>>>
>>>>>>>> The patch also fixes the unit tests as they didn't reflect all the
>>>>>>>> recent changes.
>>>>>>>
>>>>>>>> Going with this patch sounds good, but to make sure, I polled
>>>>>>>> several
>>>>>>> people here, and they all seemed to think that having to add a
>>>>>>> --del-all or --del-record flag at the end would be better as it would
>>>>>>> be less prone to failure where admins would accidentally delete a
>>>>>>> entire record because they didn't specify anything after the "<zone>
>>>>>>> <record>"
>>>>>>>
>>>>>>>> So, maybe we do need a --del-all or --del-record operator.
>>>>>>>
>>>>>>> Agree.
>>>>>>
>>>>>> +1
>>>>>> Someone may simply push enter accidentally while checking what to write
>>>>>> after the command. It would be rather unfortunate.
>>>>>>
>>>>>> Simo.
>>>>>>
>>>>>>
>>>>>
>>>>> Attached is a new version of the patch that implements --del-all. It
>>>>> also reports failure when deleting a nonexistent RR (new ticket 829).
>>>>
>>>> nack, this isn't working properly for me.
>>>>
>>>> Here is how I tested:
>>>>
>>>> - add a new zone, newzone1
>>>> - ipa dnsrecord-add newzone1 as --a-rec 3.4.5.6
>>>> - ipa dnsrecord-add newzone1 as
>>>>   Record name: as
>>>>   A record: 3.4.5.6
>>>> - ipa dnsrecord-show newzone1 as
>>>>   Record name: as
>>>>   A record: 3.4.5.6
>>>> - ipa dnsrecord-del newzone1 as --del-all
>>>> [ no output ]
>>>> - ipa dnsrecord-show newzone1 as
>>>> ipa: ERROR: as: DNS resource record not found
>>>>
>>>> So a couple of problems:
>>>>
>>>> 1. An error should have been thrown when I tried a delete without a
>>>> specific record type.
>>>
>>> I agree but I was reluctant to do this because it was perfectly OK to
>>> call "dnsrecord-add" with no options. That would create an empty DNS
>>> record. The interface was orthogonal so "dnsrecord-del" with no options
>>> would remove the record if it was empty. But I don't think an empty DNS
>>> record makes any sense.
>>>
>>> I changed the behaviour such that:
>>> * dnsrecord-add with no attributes is no longer allowed. You have to
>>> specify at least one RR type.
> 
>> Apparently this is not effective, I was able to add an empty DNS
>> record. 
> 
> 
> Thanks for catching this. A fixed patch is attached.

Apparently the fix was not enough, I got fooled by one callback in the
framework and it worked only by accident. Thanks for testing, Simo.

New patch attached.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk1C6o0ACgkQHsardTLnvCU1xQCg6agQW+AvHRDgPsClnHiPlPNV
gS0AoOU4CsXgRGJ7gnNXivwMGZRw2ORA
=DbJb
-----END PGP SIGNATURE-----
From f252c185b90091fab9c1ac6a0f67973543d22589 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Thu, 20 Jan 2011 07:54:14 -0500
Subject: [PATCH] Delete the whole DNS record with no parameters

Error out when deleting a nonexistent DNS record

Also fixes the DNS unit tests.

https://fedorahosted.org/freeipa/ticket/816
https://fedorahosted.org/freeipa/ticket/829
---
 API.txt                              |    3 +-
 ipalib/plugins/dns.py                |   52 +++++++++++++++++++++++++++++++--
 tests/test_xmlrpc/test_dns_plugin.py |   38 +++++++++++++-----------
 3 files changed, 71 insertions(+), 22 deletions(-)

diff --git a/API.txt b/API.txt
index 42ba61f..4b84a2d 100644
--- a/API.txt
+++ b/API.txt
@@ -580,9 +580,10 @@ output: Output('summary', (<type 'unicode'>, <type 'NoneType'>), 'User-friendly
 output: Entry('result', <type 'dict'>, Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
 output: Output('value', <type 'unicode'>, "The primary_key value of the entry, e.g. 'jdoe' for a user")
 command: dnsrecord_del
-args: 2,41,3
+args: 2,42,3
 arg: Str('dnszoneidnsname', cli_name='dnszone', label=Gettext('Zone name', domain='ipa', localedir=None), query=True, required=True)
 arg: Str('idnsname', attribute=True, cli_name='name', label=Gettext('Record name', domain='ipa', localedir=None), multivalue=False, primary_key=True, query=True, required=True)
+option: Flag('del_all', autofill=True, default=False, label=Gettext('Delete all associated records', domain='ipa', localedir=None))
 option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui', flags=['no_output'])
 option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui', flags=['no_output'])
 option: Str('version?', exclude='webui', flags=['no_option', 'no_output'])
diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
index af2ba4d..b4f56d4 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -120,6 +120,13 @@ _record_validators = {
     u'APL': _validate_ipnet,
 }
 
+def has_cli_options(entry, no_option_msg):
+    entry = dict((t, entry.get(t, [])) for t in _record_attributes)
+    numattr = reduce(lambda x,y: x+y,
+                     map(lambda x: len(x), entry.values()))
+    if numattr == 0:
+        raise errors.OptionError(no_option_msg)
+    return entry
 
 def dns_container_exists(ldap):
     try:
@@ -380,6 +387,13 @@ class dnsrecord(LDAPObject):
             return self.api.Object[self.parent_object].get_dn(*keys[:-1], **options)
         return super(dnsrecord, self).get_dn(*keys, **options)
 
+    def attr_to_cli(self, attr):
+        try:
+            cliname = attr[:-len('record')].upper()
+        except IndexError:
+            cliname = attr
+        return cliname
+
 api.register(dnsrecord)
 
 
@@ -407,7 +421,9 @@ class dnsrecord_cmd_w_record_options(Command):
                 )
 
     def record_options_2_entry(self, **options):
-        return dict((t, options.get(t, [])) for t in _record_attributes)
+        entries = dict((t, options.get(t, [])) for t in _record_attributes)
+        entries.update(dict((k, []) for (k,v) in entries.iteritems() if v == None ))
+        return entries
 
 
 class dnsrecord_mod_record(LDAPQuery, dnsrecord_cmd_w_record_options):
@@ -456,7 +472,9 @@ class dnsrecord_mod_record(LDAPQuery, dnsrecord_cmd_w_record_options):
         if self.obj.is_pkey_zone_record(*keys):
             entry_attrs[self.obj.primary_key.name] = [u'@']
 
-        self.post_callback(keys, entry_attrs)
+        retval = self.post_callback(keys, entry_attrs)
+        if retval:
+            return retval
 
         return dict(result=entry_attrs, value=keys[-1])
 
@@ -487,12 +505,18 @@ class dnsrecord_add(LDAPCreate, dnsrecord_cmd_w_record_options):
     """
     Add new DNS resource record.
     """
+    no_option_msg = 'No options to add a specific record provided.'
+
     def get_options(self):
         for option in super(dnsrecord_add, self).get_options():
             yield option
         for option in self.get_record_options():
             yield option
 
+    def args_options_2_entry(self, *keys, **options):
+        has_cli_options(options, self.no_option_msg)
+        return super(dnsrecord_add, self).args_options_2_entry(*keys, **options)
+
     def exc_callback(self, keys, options, exc, call_func, *call_args, **call_kwargs):
         if call_func.func_name == 'add_entry':
             if isinstance(exc, errors.DuplicateEntry):
@@ -509,6 +533,7 @@ class dnsrecord_delentry(LDAPDelete):
     """
     Delete DNS record entry.
     """
+    msg_summary = _('Deleted record "%(value)s"')
     NO_CLI = True
 
 api.register(dnsrecord_delentry)
@@ -518,6 +543,24 @@ class dnsrecord_del(dnsrecord_mod_record):
     """
     Delete DNS resource record.
     """
+    no_option_msg = 'Neither --del-all nor options to delete a specific record provided.'
+    takes_options = (
+            Flag('del_all',
+                default=False,
+                label=_('Delete all associated records'),
+            ),
+    )
+
+    def execute(self, *keys, **options):
+        if options.get('del_all', False):
+            return self.obj.methods.delentry(*keys)
+
+        return super(dnsrecord_del, self).execute(*keys, **options)
+
+    def record_options_2_entry(self, **options):
+        entry = super(dnsrecord_del, self).record_options_2_entry(**options)
+        return has_cli_options(entry, self.no_option_msg)
+
     def update_old_entry_callback(self, entry_attrs, old_entry_attrs):
         for (a, v) in entry_attrs.iteritems():
             if not isinstance(v, (list, tuple)):
@@ -526,14 +569,15 @@ class dnsrecord_del(dnsrecord_mod_record):
                 try:
                     old_entry_attrs[a].remove(val)
                 except (KeyError, ValueError):
-                    pass
+                    raise errors.NotFound(reason='%s record with value %s not found' %
+                                          (self.obj.attr_to_cli(a), val))
 
     def post_callback(self, keys, entry_attrs):
         if not self.obj.is_pkey_zone_record(*keys):
             for a in _record_attributes:
                 if a in entry_attrs and entry_attrs[a]:
                     return
-            self.obj.methods.delentry(*keys)
+            return self.obj.methods.delentry(*keys)
 
 api.register(dnsrecord_del)
 
diff --git a/tests/test_xmlrpc/test_dns_plugin.py b/tests/test_xmlrpc/test_dns_plugin.py
index 5fb08fd..0be29ff 100644
--- a/tests/test_xmlrpc/test_dns_plugin.py
+++ b/tests/test_xmlrpc/test_dns_plugin.py
@@ -45,7 +45,7 @@ class test_dns(Declarative):
 
     cleanup_commands = [
         ('dnszone_del', [dnszone1], {}),
-        ('dnsrecord_del', [dnszone1, dnsres1], {}),
+        ('dnsrecord_del', [dnszone1, dnsres1], {'del_all' : True}),
     ]
 
     tests = [
@@ -86,7 +86,8 @@ class test_dns(Declarative):
                     'dn': u'idnsname=%s,cn=dns,%s' % (dnszone1, api.env.basedn),
                     'idnsname': [dnszone1],
                     'idnszoneactive': [u'TRUE'],
-                    'idnssoamname': [u'ns1.%s' % dnszone1],
+                    'idnssoamname': [u'ns1.%s.' % dnszone1],
+                    'nsrecord': [u'ns1.%s.' % dnszone1],
                     'idnssoarname': [u'root.%s.' % dnszone1],
                     'idnssoaserial': [fuzzy_digits],
                     'idnssoarefresh': [fuzzy_digits],
@@ -122,7 +123,8 @@ class test_dns(Declarative):
                     'dn': u'idnsname=%s,cn=dns,%s' % (dnszone1, api.env.basedn),
                     'idnsname': [dnszone1],
                     'idnszoneactive': [u'TRUE'],
-                    'idnssoamname': [u'ns1.%s' % dnszone1],
+                    'nsrecord': [u'ns1.%s.' % dnszone1],
+                    'idnssoamname': [u'ns1.%s.' % dnszone1],
                     'idnssoarname': [u'root.%s.' % dnszone1],
                     'idnssoaserial': [fuzzy_digits],
                     'idnssoarefresh': [fuzzy_digits],
@@ -143,7 +145,8 @@ class test_dns(Declarative):
                 'result': {
                     'idnsname': [dnszone1],
                     'idnszoneactive': [u'TRUE'],
-                    'idnssoamname': [u'ns1.%s' % dnszone1],
+                    'nsrecord': [u'ns1.%s.' % dnszone1],
+                    'idnssoamname': [u'ns1.%s.' % dnszone1],
                     'idnssoarname': [u'root.%s.' % dnszone1],
                     'idnssoaserial': [fuzzy_digits],
                     'idnssoarefresh': [u'5478'],
@@ -157,8 +160,8 @@ class test_dns(Declarative):
 
 
         dict(
-            desc='Search for zones with name server %r' % (u'ns1.%s' % dnszone1),
-            command=('dnszone_find', [], {'idnssoamname': u'ns1.%s' % dnszone1}),
+            desc='Search for zones with name server %r' % (u'ns1.%s.' % dnszone1),
+            command=('dnszone_find', [], {'idnssoamname': u'ns1.%s.' % dnszone1}),
             expected={
                 'summary': None,
                 'count': 1,
@@ -167,7 +170,8 @@ class test_dns(Declarative):
                     'dn': u'idnsname=%s,cn=dns,%s' % (dnszone1, api.env.basedn),
                     'idnsname': [dnszone1],
                     'idnszoneactive': [u'TRUE'],
-                    'idnssoamname': [u'ns1.%s' % dnszone1],
+                    'nsrecord': [u'ns1.%s.' % dnszone1],
+                    'idnssoamname': [u'ns1.%s.' % dnszone1],
                     'idnssoarname': [u'root.%s.' % dnszone1],
                     'idnssoaserial': [fuzzy_digits],
                     'idnssoarefresh': [u'5478'],
@@ -200,7 +204,8 @@ class test_dns(Declarative):
                     'dn': u'idnsname=%s,cn=dns,%s' % (dnszone1, api.env.basedn),
                     'idnsname': [dnszone1],
                     'idnszoneactive': [u'FALSE'],
-                    'idnssoamname': [u'ns1.%s' % dnszone1],
+                    'nsrecord': [u'ns1.%s.' % dnszone1],
+                    'idnssoamname': [u'ns1.%s.' % dnszone1],
                     'idnssoarname': [u'root.%s.' % dnszone1],
                     'idnssoaserial': [fuzzy_digits],
                     'idnssoarefresh': [fuzzy_digits],
@@ -233,7 +238,8 @@ class test_dns(Declarative):
                     'dn': u'idnsname=%s,cn=dns,%s' % (dnszone1, api.env.basedn),
                     'idnsname': [dnszone1],
                     'idnszoneactive': [u'TRUE'],
-                    'idnssoamname': [u'ns1.%s' % dnszone1],
+                    'nsrecord': [u'ns1.%s.' % dnszone1],
+                    'idnssoamname': [u'ns1.%s.' % dnszone1],
                     'idnssoarname': [u'root.%s.' % dnszone1],
                     'idnssoaserial': [fuzzy_digits],
                     'idnssoarefresh': [fuzzy_digits],
@@ -254,7 +260,7 @@ class test_dns(Declarative):
 
         dict(
             desc='Try to delete non-existent record %r in zone %r' % (dnsres1, dnszone1),
-            command=('dnsrecord_del', [dnszone1, dnsres1], {}),
+            command=('dnsrecord_del', [dnszone1, dnsres1], {'del_all' : True}),
             expected=errors.NotFound(reason='DNS resource record not found'),
         ),
 
@@ -285,6 +291,7 @@ class test_dns(Declarative):
                 'result': [
                     {
                         'dn': u'idnsname=%s,cn=dns,%s' % (dnszone1, api.env.basedn),
+                        'nsrecord': (u'ns1.dnszone.test.',),
                         'idnsname': [u'@'],
                     },
                     {
@@ -329,14 +336,11 @@ class test_dns(Declarative):
 
         dict(
             desc='Delete record %r in zone %r' % (dnsres1, dnszone1),
-            command=('dnsrecord_del', [dnszone1, dnsres1], {}),
+            command=('dnsrecord_del', [dnszone1, dnsres1], {'del_all': True }),
             expected={
                 'value': dnsres1,
-                'summary': None,
-                'result': {
-                    'idnsname': [dnsres1],
-                    'arecord': [u'10.10.0.1'],
-                }
+                'summary': u'Deleted record "%s"' % dnsres1,
+                'result': {'failed': u''},
             },
         ),
 
@@ -347,7 +351,7 @@ class test_dns(Declarative):
             expected={
                 'value': dnszone1,
                 'summary': None,
-                'result': True,
+                'result': {'failed': u''},
             },
         ),
 
-- 
1.7.3.5

Attachment: jhrozek-freeipa-039-05-dsn-record-del.patch.sig
Description: PGP signature

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to