On (02/05/13 20:04), Jakub Hrozek wrote:
On Tue, Apr 30, 2013 at 08:16:05PM +0200, Jakub Hrozek wrote:
> On Mon, Apr 22, 2013 at 02:49:11PM +0200, Jakub Hrozek wrote:
> > On Mon, Apr 22, 2013 at 02:20:59PM +0200, Pavel Březina wrote:
> > > On 04/17/2013 09:04 PM, Jakub Hrozek wrote:
> > > >https://fedorahosted.org/sssd/ticket/1504
> > > >
> > > >Implements dynamic DNS updates for the AD provider. By default, the
> > > >updates also update the reverse zone and run periodically every 24
> > > >hours.
> > >
> > > Hi,
> > > I will do more thorough review in next days, but there are two thing
> > > that struck in my eyes.
> > >
> > > >0001-Active-Directory-dynamic-DNS-updates.patch
> > > >
> > > >
> > > >From 5306abceba7e6eed97763a0cd48bb886ff161835 Mon Sep 17 00:00:00
> > > >2001 From: Jakub Hrozek<jhrozek(a)redhat.com> Date: Tue, 16 Apr
2013
> > > >17:04:43 +0200 Subject: [PATCH] Active Directory dynamic DNS updates
> > > >
> > > >https://fedorahosted.org/sssd/ticket/1504
> > > >
> > > >Implements dynamic DNS updates for the AD provider. By default, the
> > > >updates also update the reverse zone and run periodically every 24
> > > >hours.
> > >
> > > >--- a/src/man/sssd-ad.5.xml +++ b/src/man/sssd-ad.5.xml
> > >
> > > Please, move dyndns options into a separate xml and include it into
> > > sssd-ad and sssd-ipa.
> > >
> >
> > yes, I wanted to do so, but there are two differences I didn't know how
> > to solve without spending too much time on a man page:
> > 1) defaults. We could solve this by saying "In IPA the default is foo
> > but in AD default is bar". It's ugly.
> > 2) the IPA manpage includes the old options.
> >
> > Maybe it could be solved by using a stringparam of xsltproc, I'll take a
> > look. Ideally I wanted to get the feature accepted first so it's
> > testable, then worry about duplication in man pages :)
> >
> > > ad_dyndns.c and ipa_dyndns.c are *very* similar. It must be possible to
> > > write this code only ones. Do you have any specific reasons why
haven't
> > > you done so?
> >
> > They access private data and call static functions directly. As above, I
> > will take a look, but reducing code duplication is something we can do
> > post-beta.
>
> I split the common code into the sdap_dyndns.c module and now both IPA
> and AD timers are just thin wrappers around this request.
>
> I didn't squash the new request into the periodic update task patch,
> sorry. I don't think it's necessary to spend time resolving conflicts
> and actually I think the patches are easier to review this way.
>
> New patches attached.
Attached patches are rebased on top of Pavel's AD site patches.
I tested patches and AD dynamic updates works.
My test cases:
change IP address and restart network (simulation of network failure)
only change IP address (simulation of changing IP by dhcp)
I tried some combination of non default options (dyndns_refresh_interval,
dyndns_update_ptr, dyndns_force_tcp) and dns updates were send.
ACK
LS