[Freeipa-devel] [PATCH 0170] Detect and warn about invalid forwardzone configuration
Petr Spacek
pspacek at redhat.com
Mon Dec 1 16:44:53 UTC 2014
On 1.12.2014 14:39, Martin Basti wrote:
> On 24/11/14 17:24, Petr Spacek wrote:
>> Hello!
>>
>> Thank you for the patch. It is not ready yet but the overall direction seems
>> good. Please see my comments in-line.
>>
>> On 24.11.2014 14:35, Martin Basti wrote:
>>> Ticket: https://fedorahosted.org/freeipa/ticket/4721
>>> Patch attached
>>>
>>> --
>>> Martin Basti
>>>
>>>
>>> freeipa-mbasti-0170-Detect-and-warn-about-invalid-DNS-forward-zone-confi.patch
>>>
>>>
>>> From a5a19137e3ddf2d2d48cfbdb2968b6f68ac8f772 Mon Sep 17 00:00:00 2001
>>> From: Martin Basti <mbasti at redhat.com>
>>> Date: Fri, 21 Nov 2014 16:54:09 +0100
>>> Subject: [PATCH] Detect and warn about invalid DNS forward zone configuration
>>>
>>> Shows warning if forward and parent authoritative zone do not have
>>> proper NS record delegation, which can cause the forward zone will be
>>> ineffective and forwarding will not work.
>>>
>>> The chande in the test was required due python changes order of elemnts
>>> in dict (python doesnt guarantee an order in dict)
>>>
>>> Ticket: https://fedorahosted.org/freeipa/ticket/4721
>>> ---
>>> ipalib/messages.py | 12 +++
>>> ipalib/plugins/dns.py | 147
>>> +++++++++++++++++++++++++++++---
>>> ipatests/test_xmlrpc/test_dns_plugin.py | 2 +-
>>> 3 files changed, 149 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/ipalib/messages.py b/ipalib/messages.py
>>> index
>>> 102e35275dbe37328c84ecb3cd5b2a8d8578056f..cc9bae3d6e5c7d3a7401dab89fafb1b60dc1e15f
>>> 100644
>>> --- a/ipalib/messages.py
>>> +++ b/ipalib/messages.py
>>> @@ -200,6 +200,18 @@ class
>>> DNSServerDoesNotSupportDNSSECWarning(PublicMessage):
>>> u"If DNSSEC validation is enabled on IPA server(s), "
>>> u"please disable it.")
>>> +class ForwardzoneIsNotEffectiveWarning(PublicMessage):
>>> + """
>>> + **13008** Forwardzone is not effective, forwarding will not work because
>>> + there is authoritative parent zone, without proper NS delegation
>>> + """
>>> +
>>> + errno = 13008
>>> + type = "warning"
>>> + format = _(u"forward zone \"%(fwzone)s\" is not effective (missing
>>> proper "
>>> + u"NS delegation in authoritative zone \"%(authzone)s\"). "
>>> + u"Forwarding may not work.")
>> I think that the error message could be made mode useful:
>>
>> "Forwarding will not work. Please add NS record <fwdzone.relativize(authzone)>
>> to parent zone %(authzone)s" (or something like that).
>>
>>> +
>>> def iter_messages(variables, base):
>>> """Return a tuple with all subclasses
>>> diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
>>> index
>>> c5d96a8c4fcdf101254ecefb60cb83d63bee6310..4f92da645e0faf784c7deb4b8ce386c1440f4229
>>> 100644
>>> --- a/ipalib/plugins/dns.py
>>> +++ b/ipalib/plugins/dns.py
>>> @@ -1725,6 +1725,79 @@ def _normalize_zone(zone):
>>> return zone
>>> +def _find_zone_which_makes_fw_zone_ineffective(fwzonename):
>> Generally, this method finds an authoritative zone for given "fwzonename".
>> What about method name _find_auth_zone_ldap(name) ?
>>
>>> + """
>>> + Check if forward zone is effective.
>>> +
>>> + If parent zone exists as authoritative zone, forward zone will not
>>> + forward queries. It is necessary to delegate authority of forward zone
>> I would clarify it: "forward queries by default."
>>
>>
>>> + to another server (non IPA DNS server).
>> I would personally omit "(non IPA DNS server)" because this mechanism is not
>> related to IPA domain at all.
>>
>>> + Example:
>>> +
>>> + Forward zone: sub.example.com
>>> + Zone: example.com
>>> +
>>> + Forwarding will not work, because server thinks it is authoritative
>>> + and returns NXDOMAIN
>>> +
>>> + Adding record: sub.example.com NS nameserver.out.of.ipa.domain.
>> Again, this is not related to IPA domain at all. I propose this text:
>> "Adding record: sub.example.com NS ns.sub.example.com."
>>
>>> + will delegate authority to another server, and IPA DNS server will
>>> + forward DNS queries.
>>> +
>>> + :param fwzonename: forwardzone
>>> + :return: None if effective, name of authoritative zone otherwise
>>> + """
>>> + assert isinstance(fwzonename, DNSName)
>>> +
>>> + # get all zones
>>> + zones = api.Command.dnszone_find(pkey_only=True,
>>> + sizelimit=0)['result']
>> Ooooh, are you serious? :-) I don't like this approach, I would rather chop
>> left-most labels from "name" and so dnszone_find() one by one. What if you
>> have many DNS zones?
>>
>>
>> This could be thrown away if you iterate only over relevant zones.
>>> + zonenames = [zone['idnsname'][0].make_absolute() for zone in zones]
>>> + zonenames.sort(reverse=True, key=len) # sort, we need longest match
>>> +
>>> + # check if is there a zone which can cause the forward zone will
>>> + # be ineffective
>>> + sub_of_auth_zone = None
>>> + for name in zonenames:
>>> + if fwzonename.is_subdomain(name):
>>> + sub_of_auth_zone = name
>>> + break
>>> +
>>> + if sub_of_auth_zone is None:
>>> + return None
>>> +
>>> + # check if forwardzone is delegated in authoritative zone
>>> + # test if exists and get NS record
>>> + try:
>>> + ns_records = api.Command.dnsrecord_show(
>>> + sub_of_auth_zone, fwzonename, raw=True)['result']['nsrecord']
>>> + except (errors.NotFound, KeyError):
>>> + return sub_of_auth_zone
>> Note: This function should process only deepest (existing) sub-domain in LDAP
>> which is active (idnsZoneActive = TRUE).
>>
>> Example:
>> fwzonename = sub.fwd.example.com.
>> Existing LDAP master zone = fwd.example.com. - DISABLED
>> Existing LDAP master zone = example.com.
>> Existing LDAP master zone = com.
>>
>> 1) Check existence & activity of fwd.example.com. -> does not exist, continue.
>> 2) Check existence & activity of example.com. -> exists, search for NS records.
>> 3) [inner loop - searching for NS records]
>> 3a) sub.fwd.example.com. NS -> does not exist, continue inner loop.
>> 3b) fwd.example.com. NS -> does not exist, continue inner loop.
>> 3c) Inner loop ends: no more (relative) candidate names to try.
>> 4) Exit returning "example.com." - deepest authoritative super-domain of
>> sub.fwd.example.com. in LDAP.
>>
>> AFAIK content of the NS record does not matter so this check can be thrown
>> away. (Check this before doing so, please :-))
>>
>>> + # make sure the target is not IPA DNS server
>>> + # FIXME: what if aliases are used?
>>> + normalized_ns_records = set()
>>> + for record in ns_records:
>>> + if record.endswith('.'):
>>> + normalized_ns_records.add(record)
>>> + else:
>>> + normalized_record = "%s.%s" % (record, sub_of_auth_zone)
>>> + normalized_ns_records.add(normalized_record)
>>> +
>>> + nameservers = [normalize_zone(x) for x in
>>> + api.Object.dnsrecord.get_dns_masters()]
>>> +
>>> + if any(nameserver in normalized_ns_records
>>> + for nameserver in nameservers):
>>> + # NS record is pointing to IPA DNS server
>>> + return sub_of_auth_zone
>>> +
>>> + # authoritative zone contains NS records which delegates forwardzone to
>>> + # different server, forwardzone is effective
>>> + return None
>>> +
>>> +
>>> class DNSZoneBase(LDAPObject):
>>> """
>>> Base class for DNS Zone
>>> @@ -3164,6 +3237,29 @@ class dnsrecord(LDAPObject):
>>> dns_name = entry_name[1].derelativize(dns_domain)
>>> self.wait_for_modified_attrs(entry, dns_name, dns_domain)
>>> + def warning_if_ns_change_cause_fwzone_ineffective(self, keys, result,
>>> + options):
>>> + """after execute"""
>>> + record_name_absolute = keys[-1]
>>> + if not keys[-1].is_absolute():
>>> + record_name_absolute = keys[-1].derelativize(keys[-2])
>>> +
>>> + try:
>>> + api.Command.dnsforwardzone_show(record_name_absolute)
>>> + except errors.NotFound:
>>> + # no forward zone
>>> + return
>>> +
>>> + authoritative_zone = \
>>> + _find_zone_which_makes_fw_zone_ineffective(record_name_absolute)
>>> + if authoritative_zone:
>>> + # forward zone is not effective and forwarding will not work
>>> + messages.add_message(
>>> + options['version'], result,
>>> + messages.ForwardzoneIsNotEffectiveWarning(
>>> + fwzone=record_name_absolute, authzone=authoritative_zone
>>> + )
>>> + )
>>> @register()
>>> @@ -3358,6 +3454,14 @@ class dnsrecord_add(LDAPCreate):
>>> return
>>> raise exc
>>> + def execute(self, *keys, **options):
>>> + result = super(dnsrecord_add, self).execute(*keys, **options)
>>> + if 'nsrecord' in options:
>>> + self.obj.warning_if_ns_change_cause_fwzone_ineffective(keys,
>>> + result,
>>> + options)
>>> + return result
>>> +
>>> def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
>>> assert isinstance(dn, DN)
>>> for attr in getattr(context, 'dnsrecord_precallback_attrs', []):
>>> @@ -3487,7 +3591,10 @@ class dnsrecord_mod(LDAPUpdate):
>>> if self.api.env['wait_for_dns']:
>>> self.obj.wait_for_modified_entries(context.dnsrecord_entry_mods)
>>> -
>>> + if 'nsrecord' in options:
>>> + self.obj.warning_if_ns_change_cause_fwzone_ineffective(keys,
>>> + result,
>>> + options)
>>> return result
>>> def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
>>> @@ -3654,19 +3761,23 @@ class dnsrecord_del(LDAPUpdate):
>>> if self.api.env['wait_for_dns']:
>>> entries = {(keys[0], keys[1]): None}
>>> self.obj.wait_for_modified_entries(entries)
>>> - return result
>>> + else:
>>> + result = super(dnsrecord_del, self).execute(*keys, **options)
>>> + result['value'] = pkey_to_value([keys[-1]], options)
>>> - result = super(dnsrecord_del, self).execute(*keys, **options)
>>> - result['value'] = pkey_to_value([keys[-1]], options)
>>> + if getattr(context, 'del_all', False) and not \
>>> + self.obj.is_pkey_zone_record(*keys):
>>> + result = self.obj.methods.delentry(*keys,
>>> +
>>> version=options['version'])
>>> + context.dnsrecord_entry_mods[(keys[0], keys[1])] = None
>>> - if getattr(context, 'del_all', False) and not \
>>> - self.obj.is_pkey_zone_record(*keys):
>>> - result = self.obj.methods.delentry(*keys,
>>> - version=options['version'])
>>> - context.dnsrecord_entry_mods[(keys[0], keys[1])] = None
>>> + if self.api.env['wait_for_dns']:
>>> +
>>> self.obj.wait_for_modified_entries(context.dnsrecord_entry_mods)
>>> - if self.api.env['wait_for_dns']:
>>> - self.obj.wait_for_modified_entries(context.dnsrecord_entry_mods)
>>> + if 'nsrecord' in options or options.get('del_all', False):
>>> + self.obj.warning_if_ns_change_cause_fwzone_ineffective(keys,
>>> + result,
>>> + options)
>>> return result
>>> def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
>>> @@ -4019,6 +4130,20 @@ class dnsforwardzone_add(DNSZoneBase_add):
>>> return dn
>>> + def execute(self, *keys, **options):
>>> + result = super(dnsforwardzone_add, self).execute(*keys, **options)
>>> + authoritative_zone =
>>> _find_zone_which_makes_fw_zone_ineffective(keys[-1])
>>> + if authoritative_zone:
>>> + # forward zone is not effective and forwarding will not work
>>> + messages.add_message(
>>> + options['version'], result,
>>> + messages.ForwardzoneIsNotEffectiveWarning(
>>> + fwzone=keys[-1], authzone=authoritative_zone
>>> + )
>>> + )
>>> +
>>> + return result
>>> +
>>> @register()
>>> class dnsforwardzone_del(DNSZoneBase_del):
>>> diff --git a/ipatests/test_xmlrpc/test_dns_plugin.py
>>> b/ipatests/test_xmlrpc/test_dns_plugin.py
>>> index
>>> fb53853147ecf663cf7015867131445f32364cfb..224a80d98273480f40fd78dea0f27e34ea36492f
>>> 100644
>>> --- a/ipatests/test_xmlrpc/test_dns_plugin.py
>>> +++ b/ipatests/test_xmlrpc/test_dns_plugin.py
>>> @@ -1060,7 +1060,7 @@ class test_dns(Declarative):
>>>
>>> 'srv_part_target' : u'foo.bar.',
>>>
>>> 'srvrecord': [u"1 100 1234 %s" \
>>> %
>>> zone1_ns]}),
>>> - expected=errors.ValidationError(name='srv_target',
>>> + expected=errors.ValidationError(name='srv_weight',
>>> error=u'Raw value of a DNS record was already set by ' +
>>> u'"srv_rec" option'),
>>> ),
>>> -- 1.8.3.1
>> The same check should be done also on zone de/activation.
>>
> Updated patch attached.
>
> --
> Martin Basti
>
>
> freeipa-mbasti-0170.2-Detect-and-warn-about-invalid-DNS-forward-zone-confi.patch
Hello,
first of all - the code looks reasonable, I have only couple nitpicks. See below.
> From 2a5cf557840e8f578444039326ad90c76bdfb75a Mon Sep 17 00:00:00 2001
> From: Martin Basti <mbasti at redhat.com>
> Date: Fri, 21 Nov 2014 16:54:09 +0100
> Subject: [PATCH] Detect and warn about invalid DNS forward zone configuration
>
> Shows warning if forward and parent authoritative zone do not have
> proper NS record delegation, which can cause the forward zone will be
> ineffective and forwarding will not work.
>
> Ticket: https://fedorahosted.org/freeipa/ticket/4721
> ---
> ipalib/messages.py | 13 ++
> ipalib/plugins/dns.py | 334 ++++++++++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 336 insertions(+), 11 deletions(-)
>
> diff --git a/ipalib/messages.py b/ipalib/messages.py
> index 102e35275dbe37328c84ecb3cd5b2a8d8578056f..b44beca729f5483a7241e4c98a9f724ed663e70f 100644
> --- a/ipalib/messages.py
> +++ b/ipalib/messages.py
> @@ -200,6 +200,19 @@ class DNSServerDoesNotSupportDNSSECWarning(PublicMessage):
> u"If DNSSEC validation is enabled on IPA server(s), "
> u"please disable it.")
>
> +class ForwardzoneIsNotEffectiveWarning(PublicMessage):
> + """
> + **13008** Forwardzone is not effective, forwarding will not work because
> + there is authoritative parent zone, without proper NS delegation
> + """
> +
> + errno = 13008
> + type = "warning"
> + format = _(u"forward zone \"%(fwzone)s\" is not effective because of "
> + u"missing proper NS delegation in authoritative zone "
> + u"\"%(authzone)s\". Please add NS record "
> + u"\"%(ns_rec)s\" to parent zone \"%(authzone)s\".")
> +
>
> def iter_messages(variables, base):
> """Return a tuple with all subclasses
> diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
> index c5d96a8c4fcdf101254ecefb60cb83d63bee6310..b381dcc5e42761bb6d8d7ec35ef403c6e9b4d629 100644
> --- a/ipalib/plugins/dns.py
> +++ b/ipalib/plugins/dns.py
> @@ -1725,6 +1725,222 @@ def _normalize_zone(zone):
> return zone
>
>
> +def _get_auth_zone_ldap(name):
> + """
> + Find authoritative zone in LDAP for name
> + :param name:
> + :return:
> + """
> + assert isinstance(name, DNSName)
> + ldap = api.Backend.ldap2
> +
> + # Create all possible parent zone names
> + labels = name.make_absolute().ToASCII().split('.')
Please use python-dns interface and do not work with domain names as with strings.
> + zone_names = ['.', ] # always add root zone
> + # decrease len by 1, we already have root zone
> + for i in xrange(len(labels) - 1):
> + zone_name_abs = '.'.join(labels[i:])
> + zone_names.append(zone_name_abs)
> + # compatibility with IPA < 4.0, zone name can be relative
> + zone_names.append(zone_name_abs[:-1])
> +
> + # Create filters
> + objectclass_filter = ldap.make_filter({'objectclass':'idnszone'})
> + zonenames_filter = ldap.make_filter({'idnsname': zone_names})
> + zoneactive_filter = ldap.make_filter({'idnsZoneActive': 'true'})
> + complete_filter = ldap.combine_filters(
> + [objectclass_filter, zonenames_filter, zoneactive_filter],
> + rules=ldap.MATCH_ALL
> + )
> +
> + try:
> + entries, truncated = ldap.find_entries(
> + filter=complete_filter,
> + attrs_list=['idnsname'],
> + base_dn=DN(api.env.container_dns, api.env.basedn),
> + scope=ldap.SCOPE_ONELEVEL
> + )
What about truncated return value? It would be better to throw a exception and
optionally catch it in _warning* functions if you don't want throw fatal
errors from warning-generator.
> + except errors.NotFound:
> + return None
> +
> + # always use absolute zones
> + matched_auth_zones = [entry.single_value['idnsname'].make_absolute()
> + for entry in entries]
> +
> + # return longest match
> + return max(matched_auth_zones, key=len)
> +
> +
> +def _get_longest_match_ns_delegation_ldap(zone, name):
> + """
> + Finds record in LDAP which has the longest match with name.
> +
> + NOTE: does not search in zone apex, returns None if there is no NS
> + delegation outside of zone apex
> +
> + Example:
> + zone: example.com.
> + name: ns.sub.example.com.
> +
> + records:
> + extra.ns.sub.example.com.
> + sub.example.com.
> + example.com
> +
> + result: sub.example.com.
> +
> + :param zone: zone name
> + :param name:
> + :return: record name if success, or None if no such record exists, or
> + record is zone apex record
> + """
> + assert isinstance(zone, DNSName)
> + assert isinstance(name, DNSName)
> +
> + ldap = api.Backend.ldap2
> +
> + # get zone DN
> + zone_dn = api.Object.dnszone.get_dn(zone)
> +
> + if name.is_absolute():
> + relative_record_name = name.relativize(zone.make_absolute())
> + else:
> + relative_record_name = name
> +
> + # Name is zone apex
> + if relative_record_name.is_empty():
> + return None
> +
> + relative_record_name = relative_record_name.ToASCII()
Again, please do not work with DNS names as with strings.
> + labels = relative_record_name.split('.')
> +
> + # create list of possible record names
> + possible_record_names = ['.'.join(labels[i:]) for i in xrange(len(labels))]
> +
> + # search filters
> + name_filter = ldap.make_filter({'idnsname': [possible_record_names]})
> + objectclass_filter = ldap.make_filter({'objectclass': 'idnsrecord'})
> + complete_filter = ldap.combine_filters(
> + [name_filter, objectclass_filter],
> + rules=ldap.MATCH_ALL
> + )
> +
> + try:
> + entries, truncated = ldap.find_entries(
> + filter=complete_filter,
> + attrs_list=['idnsname', 'nsrecord'],
> + base_dn=zone_dn,
> + scope=ldap.SCOPE_ONELEVEL
> + )
What about truncated?
> + except errors.NotFound:
> + return None
> +
> + matched_records = []
> +
> + # test if entry contains NS records
> + for entry in entries:
> + print entry
> + if entry.get('nsrecord'):
> + matched_records.append(entry.single_value['idnsname'])
> +
> + if not matched_records:
> + return None
> +
> + # return longest match
> + return max(matched_records, key=len)
> +
> +
> +def _find_subtree_forward_zones_ldap(name, child_zones_only=False):
> + """
> + Search for forwardzone <name> and all child forwardzones
> + Filter: (|(*.<name>.)(<name>.))
> + :param name:
> + :param child_zones_only: search only for child zones
> + :return: list of zonenames, or empty list if no zone was found
> + """
> + assert isinstance(name, DNSName)
> + ldap = api.Backend.ldap2
> +
> + # prepare for filter "*.<name>."
> + search_name = u".%s" % name.make_absolute().ToASCII()
> + # we need to search zone with and without last dot, due compatibility
> + # with IPA < 4.0
> + search_names = [search_name, search_name[:-1]]
> +
> + # Create filters
> + objectclass_filter = ldap.make_filter({'objectclass':'idnsforwardzone'})
> + zonenames_filter = ldap.make_filter({'idnsname': search_names}, exact=False,
> + trailing_wildcard=False)
> + if not child_zones_only:
> + # find also zone with exact name
> + exact_name = name.make_absolute().ToASCII()
Again, please do not work with DNS names as with strings.
> + # we need to search zone with and without last dot, due compatibility
> + # with IPA < 4.0
> + exact_names = [exact_name, exact_name[-1]]
> + exact_name_filter = ldap.make_filter({'idnsname': exact_names})
> + zonenames_filter = ldap.combine_filters([zonenames_filter,
> + exact_name_filter])
> +
> + zoneactive_filter = ldap.make_filter({'idnsZoneActive': 'true'})
> + complete_filter = ldap.combine_filters(
> + [objectclass_filter, zonenames_filter, zoneactive_filter],
> + rules=ldap.MATCH_ALL
> + )
> +
> + try:
> + entries, truncated = ldap.find_entries(
> + filter=complete_filter,
> + attrs_list=['idnsname'],
> + base_dn=DN(api.env.container_dns, api.env.basedn),
> + scope=ldap.SCOPE_ONELEVEL
> + )
What about truncated?
> + except errors.NotFound:
> + return []
> +
> + result = [entry.single_value['idnsname'].make_absolute()
> + for entry in entries]
> +
> + return result
> +
> +
> +def _get_zone_which_makes_fw_zone_ineffective(fwzonename):
> + """
> + Check if forward zone is effective.
> +
> + If parent zone exists as authoritative zone, the forward zone will not
> + forward queries by default. It is necessary to delegate authority
> + to forward zone with a NS record.
> +
> + Example:
> +
> + Forward zone: sub.example.com
> + Zone: example.com
> +
> + Forwarding will not work, because the server thinks it is authoritative
> + for zone and will return NXDOMAIN
> +
> + Adding record: sub.example.com NS ns.sub.example.com.
> + will delegate authority, and IPA DNS server will forward DNS queries.
> +
> + :param fwzonename: forwardzone
> + :return: None if effective, name of authoritative zone otherwise
> + """
> + assert isinstance(fwzonename, DNSName)
> +
> + auth_zone = _get_auth_zone_ldap(fwzonename)
> + if not auth_zone:
> + return None
> +
> + delegation_record_name = _get_longest_match_ns_delegation_ldap(
> + auth_zone, fwzonename)
> +
> + if delegation_record_name:
> + return None
> +
> + return auth_zone
> +
> +
> class DNSZoneBase(LDAPObject):
> """
> Base class for DNS Zone
> @@ -2376,6 +2592,30 @@ class dnszone(DNSZoneBase):
> )
> )
>
> + def _warning_fw_zone_is_not_effective(self, result, *keys, **options):
> + """
> + Warning if any operation with zone causes, a child forward zone is
> + not effective
> + """
> + zone = keys[-1]
> + affected_fw_zones = _find_subtree_forward_zones_ldap(
> + zone, child_zones_only=True)
> + if not affected_fw_zones:
> + return
> +
> + for fwzone in affected_fw_zones:
> + authoritative_zone = \
> + _get_zone_which_makes_fw_zone_ineffective(fwzone)
> + if authoritative_zone:
> + # forward zone is not effective and forwarding will not work
> + messages.add_message(
> + options['version'], result,
> + messages.ForwardzoneIsNotEffectiveWarning(
> + fwzone=fwzone, authzone=authoritative_zone,
> + ns_rec=fwzone.relativize(authoritative_zone)
> + )
> + )
> +
> @register()
> class dnszone_add(DNSZoneBase_add):
> __doc__ = _('Create new DNS zone (SOA record).')
> @@ -2445,6 +2685,7 @@ class dnszone_add(DNSZoneBase_add):
> self.obj._warning_forwarding(result, **options)
> self.obj._warning_dnssec_experimental(result, *keys, **options)
> self.obj._warning_name_server_option(result, context, **options)
> + self.obj._warning_fw_zone_is_not_effective(result, *keys, **options)
> return result
>
> def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
> @@ -2475,6 +2716,13 @@ class dnszone_del(DNSZoneBase_del):
>
> msg_summary = _('Deleted DNS zone "%(value)s"')
>
> + def execute(self, *keys, **options):
> + result = super(dnszone_del, self).execute(*keys, **options)
> + nkeys = keys[-1] # we can delete more zones
> + for key in nkeys:
> + self.obj._warning_fw_zone_is_not_effective(result, key, **options)
> + return result
> +
> def post_callback(self, ldap, dn, *keys, **options):
> super(dnszone_del, self).post_callback(ldap, dn, *keys, **options)
>
> @@ -2595,12 +2843,22 @@ class dnszone_disable(DNSZoneBase_disable):
> __doc__ = _('Disable DNS Zone.')
> msg_summary = _('Disabled DNS zone "%(value)s"')
>
> + def execute(self, *keys, **options):
> + result = super(dnszone_disable, self).execute(*keys, **options)
> + self.obj._warning_fw_zone_is_not_effective(result, *keys, **options)
> + return result
> +
>
> @register()
> class dnszone_enable(DNSZoneBase_enable):
> __doc__ = _('Enable DNS Zone.')
> msg_summary = _('Enabled DNS zone "%(value)s"')
>
> + def execute(self, *keys, **options):
> + result = super(dnszone_enable, self).execute(*keys, **options)
> + self.obj._warning_fw_zone_is_not_effective(result, *keys, **options)
> + return result
> +
>
> @register()
> class dnszone_add_permission(DNSZoneBase_add_permission):
> @@ -3164,6 +3422,30 @@ class dnsrecord(LDAPObject):
> dns_name = entry_name[1].derelativize(dns_domain)
> self.wait_for_modified_attrs(entry, dns_name, dns_domain)
>
> + def warning_if_ns_change_cause_fwzone_ineffective(self, result, *keys,
> + **options):
> + """after execute"""
Please add more descriptive comment to doc string.
> + record_name_absolute = keys[-1]
a variable with zone name instead of keys[-2] would make it more readable
> + if not keys[-1].is_absolute():
record_name_absolute.is_absolute() would be better
> + record_name_absolute = keys[-1].derelativize(keys[-2])
again, please replace keys[x] with sensible names
> +
> + affected_fw_zones = _find_subtree_forward_zones_ldap(
> + record_name_absolute)
> + if not affected_fw_zones:
> + return
> +
> + for fwzone in affected_fw_zones:
Would it be possible to generalize & use _warning_fw_zone_is_not_effective() here?
> + authoritative_zone = \
> + _get_zone_which_makes_fw_zone_ineffective(fwzone)
> + if authoritative_zone:
> + # forward zone is not effective and forwarding will not work
> + messages.add_message(
> + options['version'], result,
> + messages.ForwardzoneIsNotEffectiveWarning(
> + fwzone=fwzone, authzone=authoritative_zone,
> + ns_rec=fwzone.relativize(authoritative_zone)
> + )
> + )
>
>
> @register()
> @@ -3487,7 +3769,10 @@ class dnsrecord_mod(LDAPUpdate):
>
> if self.api.env['wait_for_dns']:
> self.obj.wait_for_modified_entries(context.dnsrecord_entry_mods)
> -
> + if 'nsrecord' in options:
> + self.obj.warning_if_ns_change_cause_fwzone_ineffective(result,
> + *keys,
> + **options)
> return result
>
> def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
> @@ -3654,19 +3939,23 @@ class dnsrecord_del(LDAPUpdate):
> if self.api.env['wait_for_dns']:
> entries = {(keys[0], keys[1]): None}
> self.obj.wait_for_modified_entries(entries)
> - return result
> + else:
> + result = super(dnsrecord_del, self).execute(*keys, **options)
> + result['value'] = pkey_to_value([keys[-1]], options)
>
> - result = super(dnsrecord_del, self).execute(*keys, **options)
> - result['value'] = pkey_to_value([keys[-1]], options)
> + if getattr(context, 'del_all', False) and not \
> + self.obj.is_pkey_zone_record(*keys):
> + result = self.obj.methods.delentry(*keys,
> + version=options['version'])
> + context.dnsrecord_entry_mods[(keys[0], keys[1])] = None
>
> - if getattr(context, 'del_all', False) and not \
> - self.obj.is_pkey_zone_record(*keys):
> - result = self.obj.methods.delentry(*keys,
> - version=options['version'])
> - context.dnsrecord_entry_mods[(keys[0], keys[1])] = None
> + if self.api.env['wait_for_dns']:
> + self.obj.wait_for_modified_entries(context.dnsrecord_entry_mods)
>
> - if self.api.env['wait_for_dns']:
> - self.obj.wait_for_modified_entries(context.dnsrecord_entry_mods)
> + if 'nsrecord' in options or options.get('del_all', False):
> + self.obj.warning_if_ns_change_cause_fwzone_ineffective(result,
> + *keys,
> + **options)
> return result
>
> def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
> @@ -3998,6 +4287,19 @@ class dnsforwardzone(DNSZoneBase):
> # managed_permissions: permissions was apllied in dnszone class, do NOT
> # add them here, they should not be applied twice.
>
> + def _warning_fw_zone_is_not_effective(self, result, *keys, **options):
> + fwzone = keys[-1]
> + authoritative_zone = _get_zone_which_makes_fw_zone_ineffective(
> + fwzone)
> + if authoritative_zone:
> + # forward zone is not effective and forwarding will not work
> + messages.add_message(
> + options['version'], result,
> + messages.ForwardzoneIsNotEffectiveWarning(
> + fwzone=fwzone, authzone=authoritative_zone,
> + ns_rec=fwzone.relativize(authoritative_zone)
> + )
> + )
>
> @register()
> class dnsforwardzone_add(DNSZoneBase_add):
> @@ -4019,6 +4321,11 @@ class dnsforwardzone_add(DNSZoneBase_add):
>
> return dn
>
> + def execute(self, *keys, **options):
> + result = super(dnsforwardzone_add, self).execute(*keys, **options)
> + self.obj._warning_fw_zone_is_not_effective(result, *keys, **options)
> + return result
> +
>
> @register()
> class dnsforwardzone_del(DNSZoneBase_del):
> @@ -4083,6 +4390,11 @@ class dnsforwardzone_enable(DNSZoneBase_enable):
> __doc__ = _('Enable DNS Forward Zone.')
> msg_summary = _('Enabled DNS forward zone "%(value)s"')
>
> + def execute(self, *keys, **options):
> + result = super(dnsforwardzone_enable, self).execute(*keys, **options)
> + self.obj._warning_fw_zone_is_not_effective(result, *keys, **options)
> + return result
> +
>
> @register()
> class dnsforwardzone_add_permission(DNSZoneBase_add_permission):
> -- 1.8.3.1
NACK
Thank you for your patience with me ;-)
--
Petr^2 Spacek
More information about the Freeipa-devel
mailing list