[Freeipa-devel] [PATCH 0104-0109] DNS upgrade: change forwarding policy to "only" if private IPs are used

Fri May 20 10:19:00 UTC 2016

On 11.5.2016 12:08, Martin Basti wrote:
> 
> 
> On 03.05.2016 14:59, Petr Spacek wrote:
>> Hello,
>>
>> DNS upgrade: change forwarding policy to "only" if private IPs are used.
>>
>> https://fedorahosted.org/freeipa/ticket/5710
>>
>> This is the upgrade part. I will add one more patch to print a warning in
>> dnsforwardzone* commands to avoid surprises. Please do not close the ticket
>> yet.
>>
>>
>>
> 
> 1)
> Upgrade failed with 'BindInstance' object has no attribute
> 'named_conf_get_directive'
> IPA server upgrade failed: Inspect /var/log/ipaupgrade.log and run command
> ipa-server-upgrade manually.
> ('IPA upgrade failed.', 1)
> The ipa-server-upgrade command failed. See /var/log/ipaupgrade.log for more
> information
> 
> 2016-05-11T08:26:20Z ERROR Upgrade failed with 'BindInstance' object has no
> attribute 'named_conf_get_directive'
> 2016-05-11T08:26:20Z DEBUG Traceback (most recent call last):
>   File
> "/usr/lib/python2.7/site-packages/ipaserver/install/upgradeinstance.py", line
> 213, in __upgrade
>     self.modified = (ld.update(self.files) or self.modified)
>   File "/usr/lib/python2.7/site-packages/ipaserver/install/ldapupdate.py",
> line 917, in update
>     self._run_updates(all_updates)
>   File "/usr/lib/python2.7/site-packages/ipaserver/install/ldapupdate.py",
> line 889, in _run_updates
>     self._run_update_plugin(update['plugin'])
>   File "/usr/lib/python2.7/site-packages/ipaserver/install/ldapupdate.py",
> line 862, in _run_update_plugin
>     restart_ds, updates = self.api.Updater[plugin_name]()
>   File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 1418, in
> __call__
>     return self.execute(**options)
>   File "/usr/lib/python2.7/site-packages/ipaserver/install/plugins/dns.py",
> line 547, in execute
>     self.update_global_named_conf_forwarder(bind)
>   File "/usr/lib/python2.7/site-packages/ipaserver/install/plugins/dns.py",
> line 508, in update_global_named_conf_forwarder
>     if bind.named_conf_get_directive(
> AttributeError: 'BindInstance' object has no attribute 'named_conf_get_directive'
> 
> 2016-05-11T08:26:20Z DEBUG Traceback (most recent call last):
>   File "/usr/lib/python2.7/site-packages/ipaserver/install/service.py", line
> 447, in start_creation
>     run_step(full_msg, method)
>   File "/usr/lib/python2.7/site-packages/ipaserver/install/service.py", line
> 437, in run_step
>     method()
>   File
> "/usr/lib/python2.7/site-packages/ipaserver/install/upgradeinstance.py", line
> 221, in __upgrade
>     raise RuntimeError(e)
> RuntimeError: 'BindInstance' object has no attribute 'named_conf_get_directive'
> 
> PATCH * Add ipaDNSVersion option to dnsconfig* commands and use new attribute *
> 2)
> +        Int('ipadnsversion?',
> +            label=_('IPA DNS version'),
> +        ),
> 
> Shouldn't be this part of System: Read DNS Configuration permission?
> 
> 3)
> -    def postprocess_result(self, result):
> +    def postprocess_result(self, result, show_version):
>          if not any(param in result['result'] for param in self.params):
>              result['summary'] = unicode(_('Global DNS configuration is empty'))
> 
> show_version param was added but I don't see it used in this patch.
> 
> 4)
> +        Int('ipadnsversion?',
> +            label=_('IPA DNS version'),
> +        ),
> 
> Could we add comment here that this option is accessible only from installers
> and upgrade?
> 
> 5)
> +        for config_option in container_entry.get("ipaConfigString", []):
> +            matched = re.match("^DNSVersion\s+(?P<version>\d+)$",
> +                               config_option, flags=re.I)
> +            if matched:
> +                version = int(matched.group("version"))
> 
> Shouldn't we print error if version cannot be parsed?
> 
> PATCH  * DNS upgrade: separate backup logic to make it reusable *
> 
> LGTM
> 
> PATCH * Add function ipapython.dnsutil.related_to_auto_empty_zone() *
> 
> 7)
> I'm curious why do you need to check superdomains?
> 
> PATCH * DNS upgrade: change forwarding policy to = only for conflicting
> forward zones*
> 
> 8)
> +            self.log.debug('Zone %s was sucessfully modified to use '
> +                           'forward policy "only"', zone['idnsname'][0])
> <---missing empty line---->
> +    def execute(self, **options):
> 
> PATCH * DNS upgrade: change global forwarding policy in LDAP to "only" if
> private IPs are used *
> 9)
> - dnsutil.related_to_auto_empty_zone(zone.get('idnsname')[0])
> +                dnsutil.related_to_auto_empty_zone(
> +                    dnsutil.DNSName(zone.get('idnsname')[0]))
> 
> Should be in previous commit
> 
> 10)
> -            return
> +            return False, []
> This should be fixed in the previous commit
> 
> PATCH * DNS upgrade: change global forwarding policy in named.conf to "only"
> if private IPs are used *
> 11)
> IMO this is an upgrade of configuration and this should be in
> ipaserver/install/server/upgrade.py, upgrade plugins are used only for
> updating of LDAP values
> 
> Unless you really want to use this as precedence, but then it requires broader
> discussion.
> 
> 12)
> 
> bind.named_conf_get_directive
> should be
> bindinstance.named_conf_get_directive
> 
> see 1)

This new patchset completely obsoletes the old one. I had to reshuffle few
things to to make the split between server config & LDAP upgrade possible.

Hopefully I addressed all your comment.

-- 
Petr^2 Spacek
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pspacek-0113-Use-root_logger-for-verify_host_resolvable.patch
Type: text/x-patch
Size: 7354 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160520/fb9dccc8/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pspacek-0114-Move-IP-address-resolution-from-ipaserver.install.in.patch
Type: text/x-patch
Size: 7559 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160520/fb9dccc8/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pspacek-0115-Turn-verify_host_resolvable-into-a-wrapper-around-ip.patch
Type: text/x-patch
Size: 1936 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160520/fb9dccc8/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pspacek-0116-Add-ipaDNSVersion-option-to-dnsconfig-commands-and-u.patch
Type: text/x-patch
Size: 16634 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160520/fb9dccc8/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pspacek-0117-DNS-upgrade-separate-backup-logic-to-make-it-reusabl.patch
Type: text/x-patch
Size: 9393 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160520/fb9dccc8/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pspacek-0118-Add-function-ipapython.dnsutil.related_to_auto_empty.patch
Type: text/x-patch
Size: 1859 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160520/fb9dccc8/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pspacek-0119-DNS-upgrade-change-forwarding-policy-to-only-for-con.patch
Type: text/x-patch
Size: 6202 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160520/fb9dccc8/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pspacek-0120-DNS-upgrade-change-global-forwarding-policy-in-LDAP-.patch
Type: text/x-patch
Size: 3321 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160520/fb9dccc8/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pspacek-0121-DNS-upgrade-change-global-forwarding-policy-in-named.patch
Type: text/x-patch
Size: 5852 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160520/fb9dccc8/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pspacek-0122-DNS-Warn-if-forwarding-policy-conflicts-with-automat.patch
Type: text/x-patch
Size: 4964 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160520/fb9dccc8/attachment-0009.bin>