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 <[email protected]> >>> 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 <[email protected]> > 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 _______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
