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

[Günter Kukkukk]

Am 20.02.2014 11:11, schrieb Kai Blin:

Hi Kai,

> On 2014-02-20 10:38, Günter Kukkukk wrote:
> 
> Hi 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.
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.

>> The second patch is *ugly* somewhat - but can be seen as an intermediate fix,
>> until all the Microsoft dns scavenging/aging is fully understood.
>> Then some rework is needed anyway ...
> 
> I'm not sure I understand what we're trying to achieve with this patch. If leftover deleted dynamic records are scavenged, are they a problem?
> 
about a year ago - both the DLZ module and the internal DNS server were programmed in a way
that the sequence
  - create dnsNode and dnsRecord
  - delete dnsRecord
leftover the dnsNode as:
  "Name=myhost, Records=0, Children=0"

AFAIK - Amitay used a lot of time to do it _this_ way (tombstone problem). A bit strange to me...

Unfortunately the internal dns server had a bug those days when doing the same:
The sequence:
  - create dnsNode and dnsRecord
  - delete dnsRecord
was working as expected - BUT adding a new dnsRecord then in _that_ state resulted in an error!

The passed down bool variable "needs_add" was just wrong!
The called function was not able to distinguish between ldb_add() and ldb_modify().

Those days my findings resulted in commit 8b24c43b382740106474e26dec59e1419ba77306
 - why leave those "Name=myhost, Records=0, Children=0" dnsNodes around at all?
 - just delete the dnsNode when all dnsRecords are gone

I guess, you also noticed the reported "200000 dns tombstone replicating" problem.

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.

   make test TESTS=samba.tests.dns

just ONLY confirms that this patch does not break "what we have tested til today".

I'm atm investigating how "all that MS dns scavenging/aging" is working.
Until not fully understood, there's no chance to write any new torture test.

Many messages like:
dnsserver: server operation 'StartScavenging' not implemented     DnssrvOperation2: struct DnssrvOperation2
        in: struct DnssrvOperation2
            dwClientVersion          : DNS_CLIENT_VERSION_LONGHORN (458752)
            dwSettingFlags           : 0x00000000 (0)
            pwszServerName           : *
                pwszServerName           : 'linux4771'
            pszZone                  : NULL
            dwContext                : 0x00000000 (0)
            pszOperation             : *
                pszOperation             : 'StartScavenging'
            dwTypeId                 : DNSSRV_TYPEID_NULL (0)
            pData                    : union DNSSRV_RPC_UNION(case 0)
            Null                     : NULL

need our attention....

Anyway - i leave it up to you whether you push these 2 patches...

Cheers, Günter

>> Both patches pass:
>> make test TESTS=samba.tests.dns
> 
> Yes, but I doubt the existing tests really exercise these code paths, so this isn't really saying much. We really need specific new tests for specific
> new features, or we'll just have a lot of magic untested code that'll break eventually.
> 
> Cheers,
> Kai
> 


--