[PATCH] internal DNS server: add missing timestamps on dyn. records and fix adding new dnsRecords

Andrew Bartlett abartlet at samba.org
Mon Feb 24 14:27:05 MST 2014


On Mon, 2014-02-24 at 07:33 +0100, Kai Blin wrote:
> On 2014-02-22 07:00, Günter Kukkukk wrote:
> 
> Hiu Günter,
> 
> >>> the missing timestamp on dyn. DNS records should be fixed ASAP - otherwise
> >>> those records are treated as STATIC DNS entries, which never get scavenged/aged!
> >>
> >> You spent some time digging there. Can you add a test that'll pass against the MS AD DNS server for this, so we can make sure not to break it by
> >> accident?
> >>
> >
> > i think, this time stamp patch is in _no_ way invasive at all - it shouldn't do any harm.
> 
> I wasn't afraid that this patch breaks anything, but that's not the 
> point of a test necessarily. As far as I understand, test-driven 
> development at this stage looks like:
> 
> 1. find a bug
> 2. write a test triggering the bug, the test fails
> 3. fix the bug
> 4. now the test passes
> 
> The main idea is to never run into the same bug again, not to prove that 
> the code written in 3 doesn't break anything.
> 
> > All dns tools i used so far put a time stamp on (normal) dyn. dns update records.
> > There's MS docu around that only "drastic" tools also allow aging of _static_ dns records.
> >
> > AFAIK - the internal dns server is the only one not using time stamps:
> > http://picpaste.com/pics/samba-dns-Skec94fz.1393042765.png
> >
> > The samba bind DLZ module does the same for good reasons - but yes - we atm have
> > no tests around. Which would be a really MAJOR test case to simulate what Microsoft
> > is doing here.... with scavenging/aging/tombstone handling.
> > This patch alone can't be simulated at all.
> 
> Right, but the "add a dynamic DNS entry, verify it has a >0 TTL" part 
> should be testable, right?
> 
> ...
> 
> > THIS patch just "reverts the complete dnsNode deletion" - BUT in addition fixes the bug
> > that new dnsRecords could not be added to an empty dnsNode.
> >
> > At least in this area, the internal dns server now behaves the same as the DLZ module.
> 
> Right, but is this the valid behaviour?
> 
> >     make test TESTS=samba.tests.dns
> >
> > just ONLY confirms that this patch does not break "what we have tested til today".
> 
> Right, and if we keep adding new code without tests, we'll lose the 
> confidence of being able to say this.
> 
> > Anyway - i leave it up to you whether you push these 2 patches...
> 
> As much as I hate to block on this one, I don't really feel comfortable 
> to accept these patches without tests. It has less to do with these 
> specific patches and much more to do with a decision on the principle.

Kai,

Thanks for expressing this so well.

Andrew Bartlett

-- 
Andrew Bartlett
http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba






More information about the samba-technical mailing list