On 13.6.2016 14:57, Martin Basti wrote:
> Patches attached.
> 
> https://fedorahosted.org/freeipa/ticket/2008
> 
> 
> Missing parts: dns-server config, some warnings from design, some corner,
> cleanup of old unused location records cases, this will be covered in future
> patches



> freeipa-mbasti-0506-DNS-Locations-DNS-data-management.patch
> 
> 
> From f8eb14afe97111e58a8e5bce401a7ab8620888a6 Mon Sep 17 00:00:00 2001
> From: Martin Basti <mba...@redhat.com>
> Date: Wed, 8 Jun 2016 17:53:58 +0200
> Subject: [PATCH 04/11] DNS Locations: DNS data management
> 
> Adding module that allows to work with IPA DNS system records:
> * getting system records
> * updating system records
> * work with DNS locations
> 
> https://fedorahosted.org/freeipa/ticket/2008
> ---
>  ipalib/dns.py                 |   2 +
>  ipalib/dns_data_management.py | 395 
> ++++++++++++++++++++++++++++++++++++++++++
>  ipaserver/plugins/dns.py      |   1 +
>  3 files changed, 398 insertions(+)
>  create mode 100644 ipalib/dns_data_management.py
> 
> diff --git a/ipalib/dns.py b/ipalib/dns.py
> index 
> 54a4c24a08c974d8e905e4630d91d2381c39778d..55e45a094e5b4171962abdc3f81e1647107ef96d
>  100644
> --- a/ipalib/dns.py
> +++ b/ipalib/dns.py
> @@ -18,6 +18,8 @@
>  # You should have received a copy of the GNU General Public License
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  
> +from __future__ import absolute_import
> +
>  import re
>  
>  from ipalib import errors
> diff --git a/ipalib/dns_data_management.py b/ipalib/dns_data_management.py
> new file mode 100644
> index 
> 0000000000000000000000000000000000000000..939978d980c4c1292dd516245457795fc116d401
> --- /dev/null
> +++ b/ipalib/dns_data_management.py
> @@ -0,0 +1,395 @@
> +#
> +# Copyright (C) 2016  FreeIPA Contributors see COPYING for license
> +#
> +
> +from __future__ import absolute_import
> +
> +from collections import defaultdict
> +from dns import (
> +    rdataclass,
> +    rdatatype,
> +    zone,
> +)
> +from dns.rdtypes.IN.SRV import SRV
> +from dns.rdtypes.ANY.TXT import TXT
> +
> +from ipalib import errors
> +from ipalib.dns import record_name_format
> +from ipapython.dnsutil import DNSName, resolve_rrsets
> +
> +IPA_DEFAULT_MASTER_SRV_REC = (
> +    # srv record name, port
> +    (DNSName(u'_ldap._tcp'), 389),
> +    (DNSName(u'_kerberos._tcp'), 88),
> +    (DNSName(u'_kerberos._udp'), 88),
> +    (DNSName(u'_kerberos-master._tcp'), 88),
> +    (DNSName(u'_kerberos-master._udp'), 88),
> +    (DNSName(u'_kpasswd._tcp'), 464),
> +    (DNSName(u'_kpasswd._udp'), 464),
> +)
> +
> +IPA_DEFAULT_ADTRUST_SRV_REC = (
> +    # srv record name, port
> +    (DNSName(u'_ldap._tcp.Default-First-Site-Name._sites.dc._msdcs'), 389),
> +    (DNSName(u'_ldap._tcp.dc._msdcs'), 389),
> +    (DNSName(u'_kerberos._tcp.Default-First-Site-Name._sites.dc._msdcs'), 
> 88),
> +    (DNSName(u'_kerberos._udp.Default-First-Site-Name._sites.dc._msdcs'), 
> 88),
> +    (DNSName(u'_kerberos._tcp.dc._msdcs'), 88),
> +    (DNSName(u'_kerberos._udp.dc._msdcs'), 88),
> +)
> +
> +
> +class IPADomainIsNotManagedByIPAError(Exception):
> +    pass
> +
> +
> +class IPASystemRecords(object):
> +
> +    # fixme do it configurable
> +    PRIORITY_HIGH = 0
> +    PRIORITY_LOW = 50
> +
> +    def __init__(self, api_instance):
> +        self.api_instance = api_instance
> +        self.domain_abs = 
> DNSName(self.api_instance.env.domain).make_absolute()
> +        self.servers_data = {}
> +        self.__init_data()
> +
> +    def reload_data(self):
> +        """
> +        After any change made to IPA servers, this method must be called to
> +        update data in the object, otherwise invalid records may be
> +        created/updated
> +        """
> +        self.__init_data()
> +
> +    def __get_server_attrs(self, hostname):
> +        """
> +        Returns weight, location, and intersection of its roles and allowed
> +        roles
> +        :param hostname: server hostname
> +        :param allowed_roles: *set* of server roles that are allowed, if None
> +        all roles will be used
> +        :return: (weight, location, intersection of roles and allowed roles)

There is no param allowed_roles :-)


> +        """
> +        server_result = 
> self.api_instance.Command.server_show(hostname)['result']
> +        weight = int(server_result.get('ipalocationweight', [u'100'])[0])
> +        location = server_result.get('ipalocation_location', [None])[0]
> +        roles = set(server_result.get('enabled_role_servrole', ()))
> +
> +        return weight, location, roles
> +
> +    def __init_data(self):
> +        self.servers_data = {}
> +
> +        servers_result = self.api_instance.Command.server_find(
> +            pkey_only=True)['result']
> +        servers = [s['cn'][0] for s in servers_result]
> +        for s in servers:
> +            weight, location, roles = self.__get_server_attrs(s)
> +            self.servers_data[s] = {
> +                'weight': weight,
> +                'location': location,
> +                'roles': roles,
> +            }
> +
> +    def __get_srv_records(
> +        self, zone_obj, hostname, rname_port_map,
> +        weight=100, priority=0, location=None
> +    ):
> +        assert isinstance(hostname, DNSName)
> +        assert isinstance(priority, int)
> +        assert isinstance(weight, int)
> +
> +        if location:
> +            suffix = (
> +                location + DNSName('_locations') + self.domain_abs
> +            )
> +        else:
> +            suffix = self.domain_abs
> +
> +        for name, port in rname_port_map:
> +            rd = SRV(
> +                rdataclass.IN, rdatatype.SRV,
> +                priority,
> +                weight,
> +                port,
> +                hostname.make_absolute()
> +            )
> +
> +            r_name = name.derelativize(suffix)
> +
> +            rdataset = zone_obj.get_rdataset(
> +                r_name, rdatatype.SRV, create=True)
> +            rdataset.add(rd, ttl=86400)  # FIXME: use TTL from config
> +
> +    def __get_ca_records_from_hostname(self, zone_obj, hostname):
Name __get is misleading because it does not return a new object - it in fact
modifies one one of its arguments. I would rather start the name with __add_...

> +        assert isinstance(hostname, DNSName) and hostname.is_absolute()
> +        r_name = DNSName('ipa-ca') + self.domain_abs
> +        rrsets = resolve_rrsets(hostname, (rdatatype.A, rdatatype.AAAA))
> +        for rrset in rrsets:
> +            for rd in rrset:
> +                rdataset = zone_obj.get_rdataset(
> +                    r_name, rd.rdtype, create=True)
> +                rdataset.add(rd, ttl=86400)  # FIXME: use TTL from config
> +
> +    def __get_kerberos_txt_rec(self, zone_obj):
The same as above.


> +        # FIXME: with external DNS, this should generate records for all
> +        # realmdomains
> +        r_name = DNSName('_kerberos') + self.domain_abs
> +        rd = TXT(rdataclass.IN, rdatatype.TXT, [self.api_instance.env.realm])
> +        rdataset = zone_obj.get_rdataset(
> +            r_name, rdatatype.TXT, create=True
> +        )
> +        rdataset.add(rd, ttl=86400)  # FIXME: use TTL from config
> +
> +    def _get_base_dns_records_for_server(
The same as above.

> +            self, zone_obj, hostname, roles=None, include_master_role=True,
> +            include_kerberos_realm=True,
> +    ):
> +        server = self.servers_data[hostname]
> +        if roles:
> +            eff_roles = server['roles'] & set(roles)
> +        else:
> +            eff_roles = server['roles']
> +        hostname_abs = DNSName(hostname).make_absolute()
> +
> +        if include_kerberos_realm:
> +            self.__get_kerberos_txt_rec(zone_obj)
> +
> +        # get master records
> +        if include_master_role:
> +            self.__get_srv_records(
> +                zone_obj,
> +                hostname_abs,
> +                IPA_DEFAULT_MASTER_SRV_REC,
> +                weight=server['weight']
> +            )
> +
> +        if 'CA server' in eff_roles:
> +            self.__get_ca_records_from_hostname(zone_obj, hostname_abs)
> +
> +        if 'AD trust controller' in eff_roles:
> +            self.__get_srv_records(
> +                zone_obj,
> +                hostname_abs,
> +                IPA_DEFAULT_ADTRUST_SRV_REC,
> +                weight=server['weight']
> +            )
> +
> +    def _get_location_dns_records_for_server(
> +            self, zone_obj, hostname, locations,
> +            roles=None, include_master_role=True):
> +        """
> +        Function returns list of DNS records for server with proper locations
> +        mappings
In fact it returns zone.

> +        :param api_instance: instance of API
> +        :param hostname: server hostname
> +        :return: dict with keys as record name, and list of rrsets as values
> +        """
> +        server = self.servers_data[hostname]
> +        if roles:
> +            eff_roles = server['roles'] & roles
> +        else:
> +            eff_roles = server['roles']
> +        hostname_abs = DNSName(hostname).make_absolute()
> +
> +        # generate locations specific records
> +        for location in locations:
> +            if location == self.servers_data[hostname]['location']:
> +                priority = self.PRIORITY_HIGH
> +            else:
> +                priority = self.PRIORITY_LOW
> +
> +            if include_master_role:
> +                self.__get_srv_records(
> +                    zone_obj,
> +                    hostname_abs,
> +                    IPA_DEFAULT_MASTER_SRV_REC,
> +                    weight=server['weight'],
> +                    priority=priority,
> +                    location=location
> +                )
> +
> +            if 'AD trust controller' in eff_roles:
> +                self.__get_srv_records(
> +                    zone_obj,
> +                    hostname_abs,
> +                    IPA_DEFAULT_ADTRUST_SRV_REC,
> +                    weight=server['weight'],
> +                    priority=priority,
> +                    location=location
> +                )
> +
> +        return zone_obj
> +
> +    def __prepare_records_update_dict(self, node):
> +        update_dict = defaultdict(list)
> +        for rdataset in node.rdatasets:
AFAIK "rdataset in node" would be better and would not touch internals of node.

> +            for rdata in rdataset:
> +                option_name = (record_name_format % rdatatype.to_text(
> +                    rdata.rdtype).lower())
> +                update_dict[option_name].append(rdata.to_text())
> +        return update_dict
> +
> +    def __update_dns_records(
> +            self, record_name, nodes, set_cname_template=True
> +    ):
> +        update_dict = self.__prepare_records_update_dict(nodes)
> +        cname_template = {
> +            'addattr': [u'objectclass=idnsTemplateObject'],
> +            'setattr': [
> +                u'idnsTemplateAttribute;cnamerecord=%s'
> +                u'.\{substitutionvariable_ipalocation\}._locations' %
> +                record_name.relativize(self.domain_abs)
> +            ]
> +        }
> +        try:
> +            if set_cname_template:
> +                # only srv records should have configured cname templates
> +                update_dict.update(cname_template)
> +            self.api_instance.Command.dnsrecord_mod(
> +                self.domain_abs, record_name,
> +                **update_dict
> +            )
> +        except errors.NotFound:
> +            # because internal API magic, addattr and setattr doesn't work 
> with
> +            # dnsrecord-add well, use dnsrecord-mod instead later
> +            update_dict.pop('addattr', None)
> +            update_dict.pop('setattr', None)
> +
> +            self.api_instance.Command.dnsrecord_add(
> +                self.domain_abs, record_name, **update_dict)
> +
> +            if set_cname_template:
> +                try:
> +                    self.api_instance.Command.dnsrecord_add(

This ^^^ should be dnsrecord_mod.


> +                        self.domain_abs,
> +                        record_name, **cname_template)
> +                except errors.EmptyModlist:
> +                    pass
> +        except errors.EmptyModlist:
> +            pass
> +
> +    def get_base_records(
> +            self, servers=None, roles=None, include_master_role=True,
> +            include_kerberos_realm=True
> +    ):
> +        """
> +        Generate IPA service records for specific servers and roles
> +        :param servers: list of server which will be used in records,
> +        if None all IPA servers will be used
> +        :param roles: roles for which DNS records will be generated,
> +        if None all roles will be used
> +        :param include_master_role: generate records required by IPA master
> +        role
> +        :return: dns.zone.Zone object that contains base DNS records
> +        """
> +
> +        zone_obj = zone.Zone(self.domain_abs, relativize=False)
> +        if servers is None:
> +            servers = self.servers_data.keys()
> +
> +        for server in servers:
> +            self._get_base_dns_records_for_server(zone_obj, server,
> +                roles=roles, include_master_role=include_master_role,
> +                include_kerberos_realm=include_kerberos_realm
> +            )
> +        return zone_obj
> +
> +    def get_locations_records(
> +            self, servers=None, roles=None, include_master_role=True):
> +        """
> +        Generate IPA location records for specific servers and roles.
> +        :param servers: list of server which will be used in records,
> +        if None all IPA servers will be used
> +        :param roles: roles for which DNS records will be generated,
> +        if None all roles will be used
> +        :param include_master_role: generate records required by IPA master
> +        role
> +        :return: dns.zone.Zone object that contains location DNS records
> +        """
> +        zone_obj = zone.Zone(self.domain_abs, relativize=False)
> +        if servers is None:
> +            servers_result = self.api_instance.Command.server_find(
> +                pkey_only=True)['result']
> +            servers = [s['cn'][0] for s in servers_result]
> +
> +        locations_result = 
> self.api_instance.Command.location_find()['result']
> +        locations = [l['idnsname'][0] for l in locations_result]
> +
> +        for server in servers:
> +            self._get_location_dns_records_for_server(
> +                zone_obj, server,
> +                locations, roles=roles,
> +                include_master_role=include_master_role)
> +        return zone_obj
> +
> +    def update_base_records(self):
> +        """
> +        Update base DNS records for IPA services
> +        :return: [(record_name, node), ...], [(record_name, node, error), 
> ...]
> +        where the first list contains successfully updated records, and the
> +        second list contains failed updates with particular exceptions
> +        """
> +        fail = []
> +        success = []
> +        names_requiring_cname_templates = set(
> +            rec[0].derelativize(self.domain_abs) for rec in (
> +                IPA_DEFAULT_MASTER_SRV_REC +
> +                IPA_DEFAULT_ADTRUST_SRV_REC
> +            )
> +        )
> +
> +        base_zone = self.get_base_records()
> +        for record_name, node in base_zone.items():
> +            set_cname_template = record_name in 
> names_requiring_cname_templates
> +            try:
> +                self.__update_dns_records(
> +                    record_name, node, set_cname_template)
> +            except errors.PublicError as e:
> +                fail.append((record_name, node, e))
> +            else:
> +                success.append((record_name, node))
> +        return success, fail
> +
> +    def update_locations_records(self):
> +        """
> +        Update locations DNS records for IPA services
> +        :return: [(record_name, node), ...], [(record_name, node, error), 
> ...]
> +        where the first list contains successfully updated records, and the
> +        second list contains failed updates with particular exceptions
> +        """
> +        fail = []
> +        success = []
> +
> +        location_zone = self.get_locations_records()
> +        for record_name, nodes in location_zone.items():
> +            try:
> +                self.__update_dns_records(
> +                    record_name, nodes,
> +                    set_cname_template=False)
> +            except errors.PublicError as e:
> +                fail.append((record_name, nodes, e))
> +            else:
> +                success.append((record_name, nodes))
> +        return success, fail

I think that methods update_base_records() and update_locations_records() can
be unified because they are basically the same. Condition "record_name in
names_requiring_cname_templates" should evaluate to False for all location
records (I hope :-) so you can save some code and just pass result of
get_base_records()/get_locations_records() into the common function.



> freeipa-mbasti-0507-DNS-Locations-permission-allow-to-read-status-of-ser.patch
> 
> 
> From 6fb4de027ac56e32047c55d8690af078951c8c35 Mon Sep 17 00:00:00 2001
> From: Martin Basti <mba...@redhat.com>
> Date: Sat, 11 Jun 2016 17:49:00 +0200
> Subject: [PATCH 05/11] DNS Locations: permission: allow to read status of
>  services
> 
> New permission was added: "System: Read Status of Services on IPA Servers"
> This permission is needed for detection which records should be created
> on which servers.
> 
> https://fedorahosted.org/freeipa/ticket/2008
> ---
>  ACI.txt                     | 2 ++
>  ipaserver/plugins/server.py | 5 +++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/ACI.txt b/ACI.txt
> index 
> 6f691f2a7b01f834006e3c796c14c256ee87faa6..62665c04f5c27ece0cec588d93f763db5f8cb054
>  100644
> --- a/ACI.txt
> +++ b/ACI.txt
> @@ -228,6 +228,8 @@ dn: cn=usermap,cn=selinux,dc=ipa,dc=example
>  aci: (targetfilter = "(objectclass=ipaselinuxusermap)")(version 3.0;acl 
> "permission:System: Remove SELinux User Maps";allow (delete) groupdn = 
> "ldap:///cn=System: Remove SELinux User
> Maps,cn=permissions,cn=pbac,dc=ipa,dc=example";)
>  dn: cn=masters,cn=ipa,cn=etc,dc=ipa,dc=example
>  aci: (targetattr = "cn || createtimestamp || entryusn || ipalocation || 
> ipalocationweight || modifytimestamp || objectclass")(targetfilter = 
> "(objectclass=ipaLocationMember)")(version 3.0;acl "permission:System: Read 
> Locations of IPA Servers";allow (compare,read,search) groupdn = 
> "ldap:///cn=System: Read Locations of IPA
> Servers,cn=permissions,cn=pbac,dc=ipa,dc=example";)
> +dn: cn=masters,cn=ipa,cn=etc,dc=ipa,dc=example
> +aci: (targetattr = "cn || createtimestamp || entryusn || ipaconfigstring || 
> modifytimestamp || objectclass")(targetfilter = 
> "(objectclass=ipaLocationMember)")(version 3.0;acl "permission:System: Read 
> Status of Services on IPA Servers";allow (compare,read,search) groupdn = 
> "ldap:///cn=System: Read Status of Services on IPA

How is "(targetfilter = (objectclass=ipaLocationMember)" going to work with
non-upgraded servers?

Correct me if I'm wrong but I guess that CentOS 6 replicas will not have
ipaLocationMember object class. Does it mean that the command ran on CentOS 7
will not see and generate records for CentOS 6 replicas?


> Servers,cn=permissions,cn=pbac,dc=ipa,dc=example";)
>  dn: cn=services,cn=accounts,dc=ipa,dc=example
>  aci: (targetfilter = "(objectclass=ipaservice)")(version 3.0;acl 
> "permission:System: Add Services";allow (add) groupdn = "ldap:///cn=System: 
> Add Services,cn=permissions,cn=pbac,dc=ipa,dc=example";)
>  dn: cn=services,cn=accounts,dc=ipa,dc=example
> diff --git a/ipaserver/plugins/server.py b/ipaserver/plugins/server.py
> index 
> a2c2752d94913eda0636cd5a360921eb002282d3..eb40b97ad22b95a2054b6280140e48973b1ec229
>  100644
> --- a/ipaserver/plugins/server.py
> +++ b/ipaserver/plugins/server.py
> @@ -75,6 +75,11 @@ class server(LDAPObject):
>              },
>              'default_privileges': {'DNS Administrators'},
>          },
> +        'System: Read Status of Services on IPA Servers': {
> +            'ipapermright': {'read', 'search', 'compare'},
> +            'ipapermdefaultattr': {'objectclass', 'cn', 'ipaconfigstring'},
> +            'default_privileges': {'DNS Administrators'},
> +        }

Interesting, where is the (targetfilter = (objectclass=ipaLocationMember) ?
How it gets to ACI.txt?

>      }
>  
>      takes_params = (
> -- 2.5.5


> freeipa-mbasti-0509-DNS-Locations-command-dns-update-system-records.patch
> 
> 
> From 3dd971a197f5106cd021135056a15eb9c7521948 Mon Sep 17 00:00:00 2001
> From: Martin Basti <mba...@redhat.com>
> Date: Fri, 10 Jun 2016 17:03:25 +0200
> Subject: [PATCH 07/11] DNS Locations: command dns-update-system-records
> 
> command dns-update-system-records updates/fixes DNS records for IPA
> services:
> * updating A, AAAA records for CA
> * updating SRV records for LDAP, kerberos and AD trust
> * updating TXT record in _kerberos with proper realm
> * updating dns locations if used
> 
> https://fedorahosted.org/freeipa/ticket/2008
> ---
>  API.txt                       |  10 ++++
>  VERSION                       |   4 +-
>  ipaclient/plugins/dns.py      |  26 +++++++++-
>  ipalib/dns_data_management.py |  24 ++++++++-
>  ipaserver/plugins/dns.py      | 112 
> ++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 172 insertions(+), 4 deletions(-)
> 
> diff --git a/API.txt b/API.txt
> index 
> 68ce3560d17c1fb6b6c50a91b5bf6ba810204922..c9268493a4c74a0a1557fc2930f618d1916c184d
>  100644
> --- a/API.txt
> +++ b/API.txt
> @@ -961,6 +961,16 @@ option: Str('version?')
>  output: Output('result', type=[<type 'bool'>])
>  output: Output('summary', type=[<type 'unicode'>, <type 'NoneType'>])
>  output: PrimaryKey('value')
> +command: dns_update_system_records
> +args: 0,2,6
> +option: Flag('dry_run', autofill=True, default=False)
> +option: Str('version?')
> +output: Output('failed_ipa_records', type=[<type 'list'>, <type 'tuple'>])
> +output: Output('failed_location_records', type=[<type 'list'>, <type 
> 'tuple'>])
> +output: Output('ipa_records', type=[<type 'list'>, <type 'tuple'>])
> +output: Output('location_records', type=[<type 'list'>, <type 'tuple'>])
> +output: Output('summary', type=[<type 'unicode'>, <type 'NoneType'>])
> +output: Output('value', type=[<type 'bool'>])
>  command: dnsconfig_mod
>  args: 0,11,3
>  option: Str('addattr*', cli_name='addattr')
> diff --git a/VERSION b/VERSION
> index 
> 7c3e46a98607f3b94a0c98406ed13aa278440875..c39e800ce9dadc7ba7f9fbe08f870903271a9031
>  100644
> --- a/VERSION
> +++ b/VERSION
> @@ -90,5 +90,5 @@ IPA_DATA_VERSION=20100614120000
>  #                                                      #
>  ########################################################
>  IPA_API_VERSION_MAJOR=2
> -IPA_API_VERSION_MINOR=180
> -# Last change: mbabink - Server Roles: provide an API for setting CA renewal 
> master
> +IPA_API_VERSION_MINOR=181
> +# Last change: mbasti - added command dns-update-system-records
> diff --git a/ipaclient/plugins/dns.py b/ipaclient/plugins/dns.py
> index 
> 4defb7c33d301c60c78b22aa6d71c9da19650ec3..b784c04116bec0bfcc37e83f2e0bcf42b011a40c
>  100644
> --- a/ipaclient/plugins/dns.py
> +++ b/ipaclient/plugins/dns.py
> @@ -22,7 +22,7 @@ from __future__ import print_function
>  
>  import six
>  
> -from ipaclient.frontend import MethodOverride
> +from ipaclient.frontend import MethodOverride, CommandOverride
>  from ipalib import errors
>  from ipalib.dns import (get_record_rrtype,
>                          has_cli_options,
> @@ -342,3 +342,27 @@ class dnsforwardzone_mod(MethodOverride):
>                  _("Server will check DNS forwarder(s)."))
>              self.Backend.textui.print_plain(
>                  _("This may take some time, please wait ..."))
> +
> +
> +@register(override=True)
> +class dns_update_system_records(CommandOverride):
> +    def output_for_cli(self, textui, output, *args, **options):
> +        failures = {'failed_ipa_records', 'failed_location_records'}
> +        for key in (
> +                'ipa_records', 'failed_ipa_records',
> +                'location_records', 'failed_location_records'
> +        ):
> +            if output.get(key):
> +                textui.print_line(self.output[key].doc)
> +                if key in sorted(failures, key=lambda x: x[0]):
> +                    for val, err in output[key]:
> +                        textui.print_indented(
> +                            u'{val} ({err})'.format(val=val, err=err),
> +                            indent=1
> +                        )
> +                else:
> +                    for val in sorted(output[key]):
> +                        textui.print_indented(val, indent=1)
> +                textui.print_line(u'')
> +        textui.print_summary(output['summary'])
> +        return int(not output['value'])
> diff --git a/ipalib/dns_data_management.py b/ipalib/dns_data_management.py
> index 
> 939978d980c4c1292dd516245457795fc116d401..0d0466c18dea9830cfcfa89441d632f48bff3910
>  100644
> --- a/ipalib/dns_data_management.py
> +++ b/ipalib/dns_data_management.py
> @@ -4,6 +4,8 @@
>  
>  from __future__ import absolute_import
>  
> +import six
> +
>  from collections import defaultdict
>  from dns import (
>      rdataclass,
> @@ -17,6 +19,10 @@ from ipalib import errors
>  from ipalib.dns import record_name_format
>  from ipapython.dnsutil import DNSName, resolve_rrsets
>  
> +if six.PY3:
> +    unicode=str
> +
> +
>  IPA_DEFAULT_MASTER_SRV_REC = (
>      # srv record name, port
>      (DNSName(u'_ldap._tcp'), 389),
> @@ -229,7 +235,7 @@ class IPASystemRecords(object):
>              for rdata in rdataset:
>                  option_name = (record_name_format % rdatatype.to_text(
>                      rdata.rdtype).lower())
> -                update_dict[option_name].append(rdata.to_text())
> +                update_dict[option_name].append(unicode(rdata.to_text()))
>          return update_dict
>  
>      def __update_dns_records(
> @@ -393,3 +399,19 @@ class IPASystemRecords(object):
>              self.update_base_records(),
>              self.update_locations_records()
>          )
> +
> +    @classmethod
> +    def records_list_from_node(cls, name, node):
> +        records = []
> +        for dataset in node:
nitpick: variable names are inconsistent with the rest, you have used rdataset
in the code above

> +            for rd in dataset:
> +                records.append(
> +                    u'{name} {ttl} {rdclass} {rdtype} {rdata}'.format(
> +                        name=name.ToASCII(),
> +                        ttl=dataset.ttl,
> +                        rdclass=rdataclass.to_text(rd.rdclass),
> +                        rdtype=rdatatype.to_text(rd.rdtype),
> +                        rdata=rd.to_text()
> +                    )
> +                )
> +        return records
> diff --git a/ipaserver/plugins/dns.py b/ipaserver/plugins/dns.py
> index 
> c0cd27713b20efe03f2707cfbddac344db74a085..130b1f26817a0f255150a0903dc924476c6e9549
>  100644
> --- a/ipaserver/plugins/dns.py
> +++ b/ipaserver/plugins/dns.py
> @@ -40,6 +40,10 @@ from ipalib.dns import (get_record_rrtype,
>  from ipalib.request import context
>  from ipalib import api, errors, output
>  from ipalib import Command
> +from ipalib.dns_data_management import (
> +    IPASystemRecords,
> +    IPADomainIsNotManagedByIPAError,
> +)
>  from ipalib.capabilities import (
>      VERSION_WITHOUT_CAPABILITIES,
>      client_has_capability)
> @@ -4429,3 +4433,111 @@ class 
> dnsforwardzone_add_permission(DNSZoneBase_add_permission):
>  @register()
>  class dnsforwardzone_remove_permission(DNSZoneBase_remove_permission):
>      __doc__ = _('Remove a permission for per-forward zone access 
> delegation.')
> +
> +
> +@register()
> +class dns_update_system_records(Command):
> +    __doc__ = _('Update location and IPA server DNS records')
> +
> +    has_output = (
> +        output.summary,
> +        output.Output(
> +            'ipa_records', (list, tuple),
> +            _('IPA DNS records')
> +        ),
> +        output.Output(
> +            'failed_ipa_records', (list, tuple),
> +            _('Failed to update IPA DNS records')
> +        ),
> +        output.Output(
> +            'location_records', (list, tuple),
> +            _('IPA location records')
> +        ),
> +        output.Output(
> +            'failed_location_records', (list, tuple),
> +            _('Failed to update IPA location records')
> +        ),
> +        output.Output(
> +            'value', bool,
> +            _('Result of the command'), ['no_display']
> +        )
> +    )
> +
> +    takes_options = (
> +        Flag(
> +            'dry_run',
> +            label=_('Dry run'),
> +            doc=_('Do not update recors only return expected records')
> +        )
> +    )
> +
> +    def execute(self, *args, **options):
> +
> +        def output_to_list(iterable):
> +            rec_list = []
> +            for name, node in iterable:
> +                rec_list.extend(IPASystemRecords.records_list_from_node(
> +                    name, node))
> +            return rec_list
> +
> +        def output_to_list_with_failed(iterable):
> +            err_rec_list = []
> +            for name, node, error in iterable:
> +                err_rec_list.extend([
> +                        (v, unicode(error)) for v in
> +                        IPASystemRecords.records_list_from_node(name, node)
> +                    ])
> +            return err_rec_list
> +
> +        result = {
> +            'summary': None,
> +            'ipa_records': [],
> +            'failed_ipa_records': [],
> +            'location_records': [],
> +            'failed_location_records': [],
> +            'value': True,
> +        }
> +
> +        system_records = IPASystemRecords(self.api)
> +
> +        if options.get('dry_run'):
> +            result['ipa_records'] = output_to_list(
> +                system_records.get_base_records().items())
> +            result['location_records'] = output_to_list(
> +                system_records.get_locations_records().items())
> +            result['summary'] = unicode(_(u'List of IPA system DNS 
> records.'))
> +        else:
> +            try:
> +                (
> +                    (success_base, failed_base),
> +                    (success_loc, failed_loc),
> +                ) = system_records.update_dns_records()
> +            except IPADomainIsNotManagedByIPAError:
> +                result['value'] = False
> +                result['summary'] = unicode(_(
> +                    u'IPA does not manage IPA domain zone itself (or IPA DNS 
> '
> +                    u'subsystem is not installed), please add '
> +                    u'records to your DNS server manually'
> +                ))
> +                result['ipa_records'] = output_to_list(
> +                    system_records.get_base_records().items())
> +            else:
> +                result['ipa_records'] = output_to_list(success_base)
> +                result['failed_ipa_records'] = output_to_list_with_failed(
> +                    failed_base)
> +                result['location_records'] = output_to_list(success_loc)
> +                result['failed_location_records'] = 
> output_to_list_with_failed(
> +                    failed_loc
> +                )
> +                if failed_base or failed_loc:
> +                    result['value'] = False
> +                    result['summary'] = unicode(_(
> +                        u"Not all IPA system DNS records were updated, 
> please "
> +                        u"update records or fix the failures and re-run "

Did you mean "update the record manually or re-run"...?


> +                        u"the update of records again"))
> +                else:
> +                    result['summary'] = unicode(_(
> +                        u'IPA system DNS records has been sucessfully 
> updated'
> +                    ))
> +
> +        return result
> -- 2.5.5
> 
> 
> freeipa-mbasti-0510-DNS-Locations-use-dns_update_service_records-in-inst.patch
> 
> 
> From a38e1bac3c1f3a5f3c476bbd125ecf4431e87b26 Mon Sep 17 00:00:00 2001
> From: Martin Basti <mba...@redhat.com>
> Date: Sun, 12 Jun 2016 18:05:48 +0200
> Subject: [PATCH 08/11] DNS Locations: use dns_update_service_records in
>  installers
> 
> use the dns_update_system_records command to set proper DNS records
> 
> https://fedorahosted.org/freeipa/ticket/2008
> ---
>  install/tools/ipa-csreplica-manage  |  2 +-
>  install/tools/ipa-replica-manage    |  1 -
>  ipaserver/install/bindinstance.py   | 99 
> ++++++-------------------------------
>  ipaserver/install/ca.py             |  2 +-
>  ipaserver/install/dns.py            |  3 ++
>  ipaserver/install/server/upgrade.py |  7 +--
>  6 files changed, 23 insertions(+), 91 deletions(-)
> 
> diff --git a/install/tools/ipa-csreplica-manage 
> b/install/tools/ipa-csreplica-manage
> index 
> f271863b8f8a6addf630bf8d570adf039db64d89..a0a61b54013315c85c9c04c1746cd6d005b119d2
>  100755
> --- a/install/tools/ipa-csreplica-manage
> +++ b/install/tools/ipa-csreplica-manage
> @@ -286,7 +286,7 @@ def del_master(realm, hostname, options):
>          if bindinstance.dns_container_exists(options.host, api.env.basedn,
>                                               
> dm_password=options.dirman_passwd):
>              bind = bindinstance.BindInstance()
> -            bind.remove_ipa_ca_dns_records(hostname, realm.lower())
> +            bind.update_system_records()
>      except Exception as e:
>          print("Failed to cleanup %s DNS entries: %s" % (hostname, e))
>          print("You may need to manually remove them from the tree")
> diff --git a/install/tools/ipa-replica-manage 
> b/install/tools/ipa-replica-manage
> index 
> 095cca68890cd0f6c4cba9faa2395ffa3b980fc0..5a546e33c3bb741b45503580d3602d6be12f1fa5
>  100755
> --- a/install/tools/ipa-replica-manage
> +++ b/install/tools/ipa-replica-manage
> @@ -898,7 +898,6 @@ def cleanup_server_dns_entries(realm, hostname, suffix, 
> options):
>                                               
> dm_password=options.dirman_passwd):
>              bind = bindinstance.BindInstance()
>              bind.remove_master_dns_records(hostname, realm, realm.lower())
> -            bind.remove_ipa_ca_dns_records(hostname, realm.lower())

Shoudn't you replace both lines with call to bind.update_system_records()?


>              bind.remove_server_ns_records(hostname)
>  
>              keysyncd = dnskeysyncinstance.DNSKeySyncInstance()
> diff --git a/ipaserver/install/bindinstance.py 
> b/ipaserver/install/bindinstance.py
> index 
> 78e75359266bbefe7954242b98922272fb0c9194..e33da070147e39431a3982897f659d3ff7036896
>  100644
> --- a/ipaserver/install/bindinstance.py
> +++ b/ipaserver/install/bindinstance.py
> @@ -692,7 +692,6 @@ class BindInstance(service.Service):
>              self.step("setting up records for other masters", 
> self.__add_others)
>          # all zones must be created before this step
>          self.step("adding NS record to the zones", self.__add_self_ns)
> -        self.step("setting up CA record", self.__add_ipa_ca_record)
>  
>          self.step("setting up kerberos principal", self.__setup_principal)
>          self.step("setting up named.conf", self.__setup_named_conf)
> @@ -858,15 +857,7 @@ class BindInstance(service.Service):
>          else:
>              host_in_rr = normalize_zone(fqdn)
>  
> -        srv_records = (
> -            ("_ldap._tcp", "0 100 389 %s" % host_in_rr),
> -            ("_kerberos._tcp", "0 100 88 %s" % host_in_rr),
> -            ("_kerberos._udp", "0 100 88 %s" % host_in_rr),
> -            ("_kerberos-master._tcp", "0 100 88 %s" % host_in_rr),
> -            ("_kerberos-master._udp", "0 100 88 %s" % host_in_rr),
> -            ("_kpasswd._tcp", "0 100 464 %s" % host_in_rr),
> -            ("_kpasswd._udp", "0 100 464 %s" % host_in_rr),
> -        )
> +        srv_records = ()
>          if self.ntp:
>              srv_records += (
>                  ("_ntp._udp", "0 100 123 %s" % host_in_rr),

This deserves FIXME becaise we have to fix it as soon as NTP role is merged.


> @@ -942,36 +933,6 @@ class BindInstance(service.Service):
>              # there is a CNAME record in ipa-ca, we can't add A/AAAA records
>              pass
>  
> -    def __add_ipa_ca_record(self):
> -        self.__add_ipa_ca_records(self.fqdn, self.ip_addresses,
> -                                  self.ca_configured)
> -
> -        if self.first_instance:
> -            ldap = self.api.Backend.ldap2
> -            try:
> -                entries = ldap.get_entries(
> -                    DN(('cn', 'masters'), ('cn', 'ipa'), ('cn', 'etc'),
> -                       self.api.env.basedn),
> -                    ldap.SCOPE_SUBTREE, 
> '(&(objectClass=ipaConfigObject)(cn=CA))',
> -                    ['dn'])
> -            except errors.NotFound:
> -                root_logger.debug('No server with CA found')
> -                entries = []
> -
> -            for entry in entries:
> -                fqdn = entry.dn[1]['cn']
> -                if fqdn == self.fqdn:
> -                    continue
> -
> -                host, zone = fqdn.split('.', 1)
> -                if dns_zone_exists(zone, self.api):
> -                    addrs = get_fwd_rr(zone, host, api=self.api)
> -                else:
> -                    addrs = dnsutil.resolve_ip_addresses(fqdn)
> -                    # hack, will go away with locations
> -                    addrs = [str(addr) for addr in addrs]
> -
> -                self.__add_ipa_ca_records(fqdn, addrs, True)
>  
>      def __setup_principal(self):
>          dns_principal = "DNS/" + self.fqdn + "@" + self.realm
> @@ -1088,28 +1049,15 @@ class BindInstance(service.Service):
>          self.zonemgr = 'hostmaster.%s' % self.domain
>  
>          self.__add_self()
> -        self.__add_ipa_ca_record()
> +        self.update_system_records()
>  
> -    def add_ipa_ca_dns_records(self, fqdn, domain_name, ca_configured=True):
> -        host, zone = fqdn.split(".", 1)
> -        if dns_zone_exists(zone, self.api):
> -            addrs = get_fwd_rr(zone, host, api=self.api)
> -        else:
> -            addrs = dnsutil.resolve_ip_addresses(fqdn)
> -            # hack, will go away with locations
> -            addrs = [str(addr) for addr in addrs]
> -
> -        self.domain = domain_name
> -
> -        self.__add_ipa_ca_records(fqdn, addrs, ca_configured)
> -
> -    def convert_ipa_ca_cnames(self, domain_name):
> +    def remove_ipa_ca_cnames(self, domain_name):
>          # get ipa-ca CNAMEs
>          cnames = get_rr(domain_name, IPA_CA_RECORD, "CNAME", api=self.api)
>          if not cnames:
>              return
>  
> -        root_logger.info('Converting IPA CA CNAME records to A/AAAA records')
> +        root_logger.info('Removing IPA CA CNAME records')
>  
>          # create CNAME to FQDN mapping
>          cname_fqdn = {}
> @@ -1136,34 +1084,21 @@ class BindInstance(service.Service):
>              fqdn = cname_fqdn[cname]
>              if fqdn not in masters:
>                  root_logger.warning(
> -                    "Cannot convert IPA CA CNAME records to A/AAAA records, "
> -                    "please convert them manually if necessary")
> +                    "Cannot remove IPA CA CNAME please remove them manually "
> +                    "if necessary")
>                  return
>  
>          # delete all CNAMEs
>          for cname in cnames:
>              del_rr(domain_name, IPA_CA_RECORD, "CNAME", cname, api=self.api)
>  
> -        # add A/AAAA records
> -        for cname in cnames:
> -            fqdn = cname_fqdn[cname]
> -            self.add_ipa_ca_dns_records(fqdn, domain_name, None)
> -
>      def remove_master_dns_records(self, fqdn, realm_name, domain_name):
>          host, zone = fqdn.split(".", 1)
>          self.host = host
>          self.fqdn = fqdn
>          self.domain = domain_name
> -        suffix = ipautil.realm_to_suffix(realm_name)
>  
>          resource_records = (
> -            ("_ldap._tcp", "SRV", "0 100 389 %s" % self.host_in_rr),
> -            ("_kerberos._tcp", "SRV", "0 100 88 %s" % self.host_in_rr),
> -            ("_kerberos._udp", "SRV", "0 100 88 %s" % self.host_in_rr),
> -            ("_kerberos-master._tcp", "SRV", "0 100 88 %s" % 
> self.host_in_rr),
> -            ("_kerberos-master._udp", "SRV", "0 100 88 %s" % 
> self.host_in_rr),
> -            ("_kpasswd._tcp", "SRV", "0 100 464 %s" % self.host_in_rr),
> -            ("_kpasswd._udp", "SRV", "0 100 464 %s" % self.host_in_rr),
>              ("_ntp._udp", "SRV", "0 100 123 %s" % self.host_in_rr),
>          )
>  
> @@ -1179,18 +1114,7 @@ class BindInstance(service.Service):
>                  record = get_reverse_record_name(rzone, rdata)
>                  del_rr(rzone, record, "PTR", normalize_zone(fqdn),
>                         api=self.api)
> -
> -    def remove_ipa_ca_dns_records(self, fqdn, domain_name):
> -        host, zone = fqdn.split(".", 1)
> -        if dns_zone_exists(zone, self.api):
> -            addrs = get_fwd_rr(zone, host, api=self.api)
> -        else:
> -            addrs = dnsutil.resolve_ip_addresses(fqdn)
> -            # hack, will go away with locations
> -            addrs = [str(addr) for addr in addrs]
> -
> -        for addr in addrs:
> -            del_fwd_rr(domain_name, IPA_CA_RECORD, addr, api=self.api)
> +        self.update_system_records()
>  
>      def remove_server_ns_records(self, fqdn):
>          """
> @@ -1224,6 +1148,15 @@ class BindInstance(service.Service):
>                  root_logger.debug("record %s in zone %s", record, zone)
>                  del_ns_rr(zone, record, ns_rdata, api=self.api)
>  
> +    def update_system_records(self):
> +        self.print_msg("Updating DNS system records")
> +        result = self.api.Command.dns_update_system_records()
> +        if not result['value']:
> +            root_logger.error("Update of following records failed")
> +            for attr in ('failed_ipa_records', 'failed_location_records'):
> +                for record, error in result.get(attr, []):
> +                    root_logger.error("%s (%s)", record, error)
> +
>      def check_global_configuration(self):
>          """
>          Check global DNS configuration in LDAP server and inform user when it
> diff --git a/ipaserver/install/ca.py b/ipaserver/install/ca.py
> index 
> ac72c76883fda00e2f258a9ecfe9925722041fe9..bce804ac1c4e3eaf8dd08bed894ad45ea2d73ae1
>  100644
> --- a/ipaserver/install/ca.py
> +++ b/ipaserver/install/ca.py
> @@ -253,7 +253,7 @@ def install_step_1(standalone, replica_config, options):
>          # Install CA DNS records
>          if bindinstance.dns_container_exists(host_name, basedn, dm_password):
>              bind = bindinstance.BindInstance(dm_password=dm_password)
> -            bind.add_ipa_ca_dns_records(host_name, domain_name)
> +            bind.update_system_records()
>  
>  
>  def uninstall():
> diff --git a/ipaserver/install/dns.py b/ipaserver/install/dns.py
> index 
> 0fb869a7bab5b38f0a543b74220b2bb74888357f..2ea11739e07c73132bddee01309af618532e9815
>  100644
> --- a/ipaserver/install/dns.py
> +++ b/ipaserver/install/dns.py
> @@ -359,6 +359,9 @@ def install(standalone, replica, options, api=api):
>      dnskeysyncd.start_dnskeysyncd()
>      bind.start_named()
>  
> +    # this must be done when bind is started and operational
> +    bind.update_system_records()
> +

We need to investigate if this short delay is enough or if we need active
waiting for BIND to come up.

Will it work if first master is going to configure first zone and its host
name is not resolvable? We have to test this.



>      if standalone:
>          
> print("==============================================================================")
>          print("Setup complete")
> diff --git a/ipaserver/install/server/upgrade.py 
> b/ipaserver/install/server/upgrade.py
> index 
> cd2ad2e112fde7e13b584cb550af4bcf65e781ad..c757813a07045295d2a6175795dbdc7f93369d36
>  100644
> --- a/ipaserver/install/server/upgrade.py
> +++ b/ipaserver/install/server/upgrade.py
> @@ -1094,12 +1094,9 @@ def add_ca_dns_records():
>  
>      bind = bindinstance.BindInstance()
>  
> -    bind.convert_ipa_ca_cnames(api.env.domain)
> +    bind.remove_ipa_ca_cnames(api.env.domain)
>  
> -    # DNS is enabled, so let bindinstance find out if CA is enabled
> -    # and let it add the record in that case
> -    bind.add_ipa_ca_dns_records(api.env.host, api.env.domain,
> -                                ca_configured=None)
> +    bind.update_system_records()
>  
>      sysupgrade.set_upgrade_state('dns', 'ipa_ca_records', True)
>  
> -- 2.5.5
> 
> 
> freeipa-mbasti-0511-DNS-Locations-adtrustinstance-simplify-dns-managemen.patch
> 
> 
> From e72248311a047f10f16c79b150df5e496aedeb01 Mon Sep 17 00:00:00 2001
> From: Martin Basti <mba...@redhat.com>
> Date: Mon, 13 Jun 2016 12:09:27 +0200
> Subject: [PATCH 09/11] DNS Locations: adtrustinstance simplify dns management
> 
> The path how to get IPA domain in code was somehow obfuscated, this
> patch simplifies and make clear what happened there with domain name.
> 
> https://fedorahosted.org/freeipa/ticket/2008
> ---
>  install/tools/ipa-adtrust-install    |  2 +-
>  ipaserver/install/adtrustinstance.py | 14 ++++++--------
>  2 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/install/tools/ipa-adtrust-install 
> b/install/tools/ipa-adtrust-install
> index 
> 36caa5c2d429c6cf107df03e82aa80e15d8efe01..5babcdb7cb169e4a944acca55739064e0464d41e
>  100755
> --- a/install/tools/ipa-adtrust-install
> +++ b/install/tools/ipa-adtrust-install
> @@ -387,7 +387,7 @@ def main():
>      smb = adtrustinstance.ADTRUSTInstance(fstore)
>      smb.realm = api.env.realm
>      smb.autobind = ipaldap.AUTOBIND_ENABLED
> -    smb.setup(api.env.host, api.env.realm, api.env.domain,
> +    smb.setup(api.env.host, api.env.realm,
>                netbios_name, reset_netbios_name,
>                options.rid_base, options.secondary_rid_base,
>                options.no_msdcs, options.add_sids,
> diff --git a/ipaserver/install/adtrustinstance.py 
> b/ipaserver/install/adtrustinstance.py
> index 
> fb88e5596b649c49939913389bb78dc79e905322..a5c68604c5e97a6c6959147bb8c687d82176edee
>  100644
> --- a/ipaserver/install/adtrustinstance.py
> +++ b/ipaserver/install/adtrustinstance.py
> @@ -135,7 +135,6 @@ class ADTRUSTInstance(service.Service):
>          self.fqdn = None
>          self.host_netbios_name = None
>          self.realm = None
> -        self.domain_name = None
>  
>          service.Service.__init__(self, "smb", service_desc="CIFS",
>                                   dm_password=None, ldapi=True)
> @@ -162,7 +161,6 @@ class ADTRUSTInstance(service.Service):
>          self.fqdn = self.fqdn or api.env.host
>          self.host_netbios_name = make_netbios_name(self.fqdn)
>          self.realm = self.realm or api.env.realm
> -        self.domain_name = self.domain_name or api.env.domain
>  
>          self.cifs_principal = "cifs/" + self.fqdn + "@" + self.realm
>          self.suffix = ipautil.realm_to_suffix(self.realm)
> @@ -177,7 +175,7 @@ class ADTRUSTInstance(service.Service):
>                           ('cn', 'etc'),
>                           self.suffix)
>  
> -        self.smb_dom_dn = DN(('cn', self.domain_name),
> +        self.smb_dom_dn = DN(('cn', api.env.domain),
>                               api.env.container_cifsdomains,
>                               self.suffix)
>  
> @@ -426,7 +424,7 @@ class ADTRUSTInstance(service.Service):
>              self.smb_dom_dn,
>              {
>                  'objectclass': [self.OBJC_DOMAIN, "nsContainer"],
> -                'cn': [self.domain_name],
> +                'cn': [api.env.domain],
>                  self.ATTR_FLAT_NAME: [self.netbios_name],
>                  self.ATTR_SID: [self.__gen_sid_string()],
>                  self.ATTR_GUID: [str(uuid.uuid4())],
> @@ -581,7 +579,7 @@ class ADTRUSTInstance(service.Service):
>          their values are used. Otherwise default values are used.
>          """
>  
> -        zone = self.domain_name
> +        zone = api.env.domain
>          host_in_rr = normalize_zone(self.fqdn)
>  
>          priority = 0
> @@ -787,12 +785,12 @@ class ADTRUSTInstance(service.Service):
>                               LDAPI_SOCKET = self.ldapi_socket,
>                               FQDN = self.fqdn)
>  
> -    def setup(self, fqdn, realm_name, domain_name, netbios_name,
> +    def setup(self, fqdn, realm_name, netbios_name,
>                reset_netbios_name, rid_base, secondary_rid_base,
> -              no_msdcs=False, add_sids=False, smbd_user="samba", 
> enable_compat=False):
> +              no_msdcs=False, add_sids=False, smbd_user="samba",
> +              enable_compat=False):
>          self.fqdn = fqdn
>          self.realm = realm_name
> -        self.domain_name = domain_name
>          self.netbios_name = netbios_name
>          self.reset_netbios_name = reset_netbios_name
>          self.rid_base = rid_base
> -- 2.5.5
> 
> 
> freeipa-mbasti-0512-DNS-Locations-use-automatic-records-update-in-ipa-ad.patch
> 
> 
> From 606e0f3808883709a8734d7121ee1581050b9b8d Mon Sep 17 00:00:00 2001
> From: Martin Basti <mba...@redhat.com>
> Date: Mon, 13 Jun 2016 12:28:58 +0200
> Subject: [PATCH 10/11] DNS Locations: use automatic records update in 
>  ipa-adtrust-install
> 
> DNS records for adtrust is added by call dns_update_system_records
> 
> https://fedorahosted.org/freeipa/ticket/2008
> ---
>  ipaserver/install/adtrustinstance.py | 45 
> +++++++++---------------------------
>  1 file changed, 11 insertions(+), 34 deletions(-)
> 
> diff --git a/ipaserver/install/adtrustinstance.py 
> b/ipaserver/install/adtrustinstance.py
> index 
> a5c68604c5e97a6c6959147bb8c687d82176edee..51d5f4f1c4b23ad418544939ee6182bc80dd517b
>  100644
> --- a/ipaserver/install/adtrustinstance.py
> +++ b/ipaserver/install/adtrustinstance.py
> @@ -32,10 +32,10 @@ import six
>  
>  from ipaserver.install import service
>  from ipaserver.install import installutils
> -from ipaserver.install.bindinstance import get_rr, add_rr, del_rr, \
> -                                           dns_zone_exists
> +from ipaserver.install.bindinstance import dns_zone_exists
Is dns_zone_exists still used in adtrustinstance? Why?

>  from ipaserver.install.replication import wait_for_task
>  from ipalib import errors, api
> +from ipalib.dns_data_management import IPASystemRecords
>  from ipalib.util import normalize_zone
>  from ipapython.dn import DN
>  from ipapython import sysrestore
> @@ -580,17 +580,6 @@ class ADTRUSTInstance(service.Service):
>          """
>  
>          zone = api.env.domain
> -        host_in_rr = normalize_zone(self.fqdn)
> -
> -        priority = 0
> -
> -        ipa_srv_rec = (
> -            ("_ldap._tcp", [self.srv_rec(host_in_rr, 389, priority)], 389),
> -            ("_kerberos._tcp", [self.srv_rec(host_in_rr, 88, priority)], 88),
> -            ("_kerberos._udp", [self.srv_rec(host_in_rr, 88, priority)], 88),
> -        )
> -        win_srv_suffix = (".Default-First-Site-Name._sites.dc._msdcs",
> -                          ".dc._msdcs")
>  
>          err_msg = None
>  
> @@ -610,27 +599,15 @@ class ADTRUSTInstance(service.Service):
>              self.print_msg(err_msg)
>              self.print_msg("Add the following service records to your DNS " \
>                             "server for DNS zone %s: " % zone)
> -            for suff in win_srv_suffix:
> -                for srv in ipa_srv_rec:
> -                    self.print_msg("%s%s IN SRV %s"  % (srv[0], suff, " 
> ".join(srv[1])))
> -            self.print_msg("")
> -            return
> -
> -        for (srv, rdata, port) in ipa_srv_rec:
> -            cifs_rdata = list()
> -            for fqdn in self.cifs_hosts:
> -                cifs_srv = self.srv_rec(fqdn, port, priority)
> -                cifs_rdata.append(cifs_srv)
> -            cifs_rdata.extend(rdata)
> -
> -            for suff in win_srv_suffix:
> -                win_srv = srv+suff
> -                win_rdata = get_rr(zone, win_srv, "SRV")
> -                if win_rdata:
> -                    for rec in win_rdata:
> -                        del_rr(zone, win_srv, "SRV", rec)
> -                for rec in cifs_rdata:
> -                    add_rr(zone, win_srv, "SRV", rec)
> +            system_records = IPASystemRecords(api)
> +            adtrust_recors = system_records.get_base_records(
> +                [self.fqdn], ["AD trust controller"],
> +                include_master_role=False, include_kerberos_realm=False)
> +            for r_name, node in adtrust_recors.items():
> +                for rec in IPASystemRecords.records_list_from_node(r_name, 
> node):
> +                    self.print_msg(rec)
> +        else:
> +            api.Command.dns_update_system_records()
>  
>      def __configure_selinux_for_smbd(self):
>          try:
> -- 2.5.5
> 
> 
> freeipa-mbasti-0513-DNS-Locations-server-mod-add-automatic-records-updat.patch
> 
> 
> From cd2f01d1df07b338ca31fdadd9e053979cb3e67b Mon Sep 17 00:00:00 2001
> From: Martin Basti <mba...@redhat.com>
> Date: Mon, 13 Jun 2016 13:32:25 +0200
> Subject: [PATCH 11/11] DNS Locations: server-mod: add automatic records update
> 
> For any location or server weight change is required to update records
> 
> https://fedorahosted.org/freeipa/ticket/2008
> ---
>  ipalib/messages.py          | 10 ++++++++++
>  ipaserver/plugins/server.py |  8 ++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/ipalib/messages.py b/ipalib/messages.py
> index 
> e863bdd495b55921c9e487794f5c9573a6166038..fcb86b38561d0f4a269b459080b81b6e9b58b93e
>  100644
> --- a/ipalib/messages.py
> +++ b/ipalib/messages.py
> @@ -395,6 +395,16 @@ class 
> DNSForwardPolicyConflictWithEmptyZone(PublicMessage):
>      )
>  
>  
> +class SystemDNSRecordsUpdateFailed(PublicMessage):
> +    errno = 13022
> +    type = "warning"
> +    format = _(
> +        "Automatic update of DNS system records failed with error: %(emsg)s. 
> "
> +        "Please re-run update of system records manually to get list of "
> +        "missing records."
> +    )
> +
> +
>  def iter_messages(variables, base):
>      """Return a tuple with all subclasses
>      """
> diff --git a/ipaserver/plugins/server.py b/ipaserver/plugins/server.py
> index 
> eb40b97ad22b95a2054b6280140e48973b1ec229..23901e3daf36312d38a51d1b0964644793065c1d
>  100644
> --- a/ipaserver/plugins/server.py
> +++ b/ipaserver/plugins/server.py
> @@ -232,6 +232,14 @@ class server_mod(LDAPUpdate):
>          assert isinstance(dn, DN)
>          self.obj.convert_location(entry_attrs, **options)
>          self.obj.get_enabled_roles(entry_attrs)
> +
> +        if 'ipalocation' or 'ipalocationweight' in entry_attrs:
> +            result = self.api.Command.dns_update_system_records()
> +            if not result.get('value'):
> +                self.add_message(messages.SystemDNSRecordsUpdateFailed(
> +                    emsg=result.get('summary', _("Unknown error"))
> +                ))
> +
>          return dn
>  
>  
> -- 2.5.5


Besides nits above it seems good. I will test it tomorrow when you send next
version of patches.

-- 
Petr^2 Spacek

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to