[PATCH] Active Directory dynamic DNS updates

[Jakub Hrozek]

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.

[Pavel Březina]

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 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.

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?

[Jakub Hrozek]

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 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.

[Jakub Hrozek]

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 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:
> 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.

[Jakub Hrozek]

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 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:
> > 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.

[Lukas Slebodnik]

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 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:
> > > 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

[Jakub Hrozek]

On Fri, May 03, 2013 at 08:11:37PM +0200, Lukas Slebodnik wrote:
> 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 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:
> > > > 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

Thank you for the thorough testing.

Pushed to master.

[Jakub Hrozek]

On Mon, Apr 22, 2013 at 02:49:11PM +0200, Jakub Hrozek wrote:
> > > --- 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:
> 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 :)

I opened https://fedorahosted.org/sssd/ticket/1907 to track reducing the
duplication.