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

[Günter Kukkukk]

Am 05.03.2014 04:49, schrieb Günter Kukkukk:
> Hello Kai,
> 
> comments inline
> 
> Am 24.02.2014 07:33, schrieb Kai Blin:
>> 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?
> 
> both patches are in _no_ way related to usual TTL dns lifetime!
> So no _DNS_ test _can_ verify the Microsoft specific internal dwTimeStamp ADS storage.
> (dnsp_DnssrvRpcRecord)
> 
> Sure, after doing some DNS call one could then use LDB queries to verify ADS "dwTimeStamp",
> unfortunately i atm feel not able to add those LDB queries to the dns test module... :-(
> 
> Anyway - my local stuff is working - and I'm hunting the much more complex scavenging/aging/tombstone
> problem.
> 
> Cheers, Günter
> 
>>
>> ...
>>
>>> 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.
>>
>> Cheers,
>> Kai
>>
> 
> 

Sorry to reply to myself..

The one year old commit 8b24c43b382740106474e26dec59e1419ba77306 WAS JUST WRONG!

The dnsNode must NOT be deleted when the dnsRecord count drops to zero!
There are at least 3 timely steps needed to finally do the deletion from ADS - not implemented atm.
But the current behavior (and also the missing "dwTimeStamp") should be fixed ASAP.

So other patches are welcomed ....

Cheers, Günter

--