On Mon, Mar 05, 2012 at 01:56:14PM +0100, Petr Spacek wrote: > Hello, > > we are back with another proposal from Adam. See last lines.
Hello, reply is below... > > On 03/05/2012 12:32 PM, Adam Tkac wrote: > >On Thu, Mar 01, 2012 at 07:55:33PM +0100, Petr Spacek wrote: > >>> Hello, > >>> > >>> here is (again) reworked patch for > >>> https://fedorahosted.org/bind-dyndb-ldap/ticket/49 . > >>> > >>> Adam pointed me to existing BIND parser, which I missed. Now is all > >>> parsing& socket magic done inside BIND libraries. Our code is a bit > >>> shorter and syntax is 100% BIND-compatible. (But it means same as > >>> discussed yesterday:-) > >>> > >>> Adam, please review it. > >Thanks for the patch Petr, see my comments below. > > > >>> Petr^2 Spacek > >>> > >>> On 03/01/2012 03:22 PM, Petr Spacek wrote: > >>>> >Hello, > >>>> > > >>>> >here is reworked patch for > >>>> >https://fedorahosted.org/bind-dyndb-ldap/ticket/49 . > >>>> > > >>>> >Changes after yesterday's discussion on IRC with Simo and Mkosek: > >>>> >It follows BIND9 syntax for optional specification of port& adds > >>>> >documentation for this new syntax. > >>>> > > >>>> >Petr^2 Spacek > >>>> > > >>>> >On 02/29/2012 05:33 PM, Martin Kosek wrote: > >>>>> >>I agree that we should keep the BIND syntax and separate port and IP > >>>>> >>address with a space. We will at least avoid possible issues with IP > >>>>> >>address decoding in the future. > >>>>> >> > >>>>> >>Since this is a new attribute we have a good chance to do changes > >>>>> now so > >>>>> >>that it is used correctly. I created an upstream ticket to change the > >>>>> >>behavior and validators in FreeIPA: > >>>>> >> > >>>>> >>https://fedorahosted.org/freeipa/ticket/2462 > >>>>> >> > >>>>> >>Martin > >>>>> >> > >>>>> >>On Wed, 2012-02-29 at 16:44 +0100, Petr Spacek wrote: > >>>>>> >>>On 02/29/2012 04:30 PM, Simo Sorce wrote: > >>>>>>> >>>>Either way looks ok to me. > >>>>>>> >>>>I agree that using a space may be less confusing if this syntax > >>>>>>> never > >>>>>>> >>>>allows to specify multiple addresses. > >>>>>>> >>>>If multiple address can be specified than it may be less ideal > >>>>>>> to use > >>>>>>> >>>>spaces. > >>>>>>> >>>> > >>>>>>> >>>>Simo. > >>>>>> >>> > >>>>>> >>>idnsForwarders is multi-value attribute, so each value contain > >>>>>> single > >>>>>> >>>forwarder address. > >>>>>> >>> > >>>>>> >>>Petr^2 Spacek > >>>>>> >>> > >>>>>>> >>>>On Wed, 2012-02-29 at 15:14 +0100, Petr Spacek wrote: > >>>>>>>> >>>>>And there is the patch, sorry. > >>>>>>>> >>>>> > >>>>>>>> >>>>>Petr^2 > >>>>>>>> >>>>> > >>>>>>>> >>>>>On 02/29/2012 03:10 PM, Petr Spacek wrote: > >>>>>>>>> >>>>>>Hello, > >>>>>>>>> >>>>>> > >>>>>>>>> >>>>>>this patch > >>>>>>>>> fixeshttps://fedorahosted.org/bind-dyndb-ldap/ticket/49 , > >>>>>>>>> >>>>>>but I want to discuss one (unimplemented) change: > >>>>>>>>> >>>>>> > >>>>>>>>> >>>>>>I propose a change in (currently very strange) forwarders > >>>>>>>>> syntax. > >>>>>>>>> >>>>>> > >>>>>>>>> >>>>>>Current syntax: > >>>>>>>>> >>>>>><IP>[.port] > >>>>>>>>> >>>>>> > >>>>>>>>> >>>>>>examples: > >>>>>>>>> >>>>>>1.2.3.4 (without optional port) > >>>>>>>>> >>>>>>1.2.3.4.5553 (optional port 5553) > >>>>>>>>> >>>>>>A::B (IPv6, without optional port) > >>>>>>>>> >>>>>>A::B.5553 > >>>>>>>>> >>>>>>::FFFF:1.2.3.4 (6to4, without optional port) > >>>>>>>>> >>>>>>::FFFF:1.2.3.4.5553 (6to4, with optional port 5553) > >>>>>>>>> >>>>>> > >>>>>>>>> >>>>>>I find this syntax confusing, non-standard and > >>>>>>>>> not-typo-proof. > >>>>>>>>> >>>>>> > >>>>>>>>> >>>>>> > >>>>>>>>> >>>>>>IMHO better choice is to follow BIND forwarders syntax: > >>>>>>>>> >>>>>><IP> [port ip_port] (port is string delimited with spaces) > >>>>>>>>> >>>>>> > >>>>>>>>> > >>>>>>>>> >>>>>>(From:http://www.zytrax.com/books/dns/ch7/queries.html#forwarders) > >>>>>>>>> >>>>>> > >>>>>>>>> >>>>>> > >>>>>>>>> >>>>>>*Current syntax is not documented*, so probably is not used > >>>>>>>>> anywhere. > >>>>>>>>> >>>>>>(And DNS server on non-standard port is probably useful only > >>>>>>>>> for > >>>>>>>>> >>>>>>testing > >>>>>>>>> >>>>>>purposes, but it's another story.) > >>>>>>>>> >>>>>> > >>>>>>>>> >>>>>> > >>>>>>>>> >>>>>>What is you opinion? > >>> From 14056200915a90c2a957e8a2219819bd3b7f9365 Mon Sep 17 00:00:00 2001 > >>> From: Petr Spacek<pspa...@redhat.com> > >>> Date: Thu, 1 Mar 2012 15:08:10 +0100 > >>> Subject: [PATCH] Add support for IPv6 elements in idnsForwarders > >>> attribute > >>> and make syntax compatible with BIND9 forwarders. > >>> Signed-off-by: Petr Spacek<pspa...@redhat.com> > >>> > >>> --- > >>> NEWS | 3 + > >>> README | 8 ++- > >>> src/acl.c | 89 ++++++++++++++++++++++++++++++++++ > >>> src/acl.h | 3 + > >>> src/ldap_helper.c | 136 > >>> ++++++----------------------------------------------- > >>> 5 files changed, 116 insertions(+), 123 deletions(-) > >>> > >>> diff --git a/NEWS b/NEWS > >>> index ced6250..a97a5f3 100644 > >>> --- a/NEWS > >>> +++ b/NEWS > >>> @@ -1,3 +1,6 @@ > >>> +[1] Add support for IPv6 elements in idnsForwarders attribute > >>> + and make syntax compatible with BIND9 forwarders. > >>> + > >>> 1.1.0a2 > >>> ====== > >>> > >>> diff --git a/README b/README > >>> index 914abdc..aedb88c 100644 > >>> --- a/README > >>> +++ b/README > >>> @@ -89,8 +89,12 @@ example zone ldif is available in the doc directory. > >>> with a valid idnsForwarders attribute. > >>> > >>> * idnsForwarders > >>> - Defines multiple IP addresses (TODO: optional port numbers) to > >>> which > >>> - queries will be forwarded. > >>> + Defines multiple IP addresses to which queries will be > >>> forwarded. > >>> + It is multi-value attribute: Each IP address (and optional > >>> port) has to > >>> + be in own value. BIND9 syntax for "forwarders" is required. > >>> + Optional port can be specified by adding " port<number>" after > >>> IP > >>> + address. IPv4 and IPv6 addresses are supported. > >>> + Examples: "1.2.3.4" or "1.2.3.4 port 553" or "A::B" or "A::B > >>> port 553" > > On 03/05/2012 12:32 PM, Adam Tkac wrote: > >In my opinion we should mark the idnsForwarders attribute single-value and > >deal > >with them as with idnsAllow{Query,Transfer} (i.e. specify forwarders > >splitted by > >semi-colon). It will be more consistent with idnsAllow{Query,Transfer}. What > >do > >you think about this? > > It sounds good to me. It's more consistent with real BIND > configuration and other parts of plugin. > > What about upgrade process? It is necessary to change schema and > convert existing entries to new format. If idnsForward is already in use, I'm ok with multival attribute because ordering is not an issue here. Please incorporate changes which I suggested (if you don't dissagree, of course) and then push the patch. Thank you in advance! Regards, Adam -- Adam Tkac, Red Hat, Inc. _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel