These are chat archives for CZ-NIC/knot-resolver

16th
Jan 2019
SvenVD
@SvenVD
Jan 16 13:01
@pspacek I created an issue https://gitlab.labs.nic.cz/knot/edns-zone-scanner/issues/1 and I pushed the fixes in a branch. I will not create a merge request yet since these are really fresh patches which need some additional testing, which I will do now
Petr Špaček
@pspacek
Jan 16 13:02
@SvenVD Hi and thank you, I'm glad you found the bug.
Should I have a look at the patches now, or is it too early for review?
SvenVD
@SvenVD
Jan 16 13:03
You may have a look, I think I gathered all the patches I did for one succesful run of the be zone. I will now start with a fresh docker image and only include the patches that I pushed in the branch, to see I really got all my fixes
Petr Špaček
@pspacek
Jan 16 13:05
Okay. Now wondering out loud, without a deep analysis: Wouldn't it be better/easier/more efficient to do normalization in ednscomp2pickle.py during genreport output parsing?
(Assuming that AAAAs obtained from dnspython's resolver and zone file normalized using ldns-read-zone are in the same format...)
SvenVD
@SvenVD
Jan 16 13:07
There is still a difference in ipv6 handling in zonepickle and nsname2ipset.py, they need to agree first
Yes that last assumption is not true
From my experience
Petr Špaček
@pspacek
Jan 16 13:07
Okay, that is the difference between ldns-read-zone and dnspython I did not encounter. Good!
Hmm, maybe using strings is a wrong approach altogether. We should probably be using Python ipaddress objects everywhere, a quick experiment on my systems shows that Python sets handle different representations of the same IP addresses just fine.
SvenVD
@SvenVD
Jan 16 13:11
Yes indeed, that was my thought also but that would require more rewriting
and testing
Petr Špaček
@pspacek
Jan 16 13:12
Right. I will have a look how difficult it would be...
Petr Špaček
@pspacek
Jan 16 14:57
@SvenVD I just opened https://gitlab.labs.nic.cz/knot/edns-zone-scanner/merge_requests/12 which moves all IP address operations to Python IPAddress objects (I hope).
It passes my tests, but these obviously do not catch the problem you had so it needs more testing from your side.
In any case, the vast majority of diff contains just type changes from str to AnIPAddress. If you look at git diff --word-diff you will find out that there are only 4 lines where an actual work is done :-D
SvenVD
@SvenVD
Jan 16 14:59
Alright thanks, can you create a testing container that I can download?
via the registry
Petr Špaček
@pspacek
Jan 16 15:00
Hmm, container build is usually done after merge, let me think.
Would it be enough to supply .diff file?
(Instead of a container.)
You know what? On my test data set it is giving exactly same results as before, so we just might press luck a bit and merge it. It seems not to break use case which worked before, so it should not be a major regression.
SvenVD
@SvenVD
Jan 16 15:02
The thing is that I am running this on a dedicated server, I do not have a python3 development environment ready with all the needed libraries
I just did the changes in side the container and manually copied them over.
But you are touching a lot of files so that's going to be trickier
So for me the fastest would be a container which include the fixes
Petr Špaček
@pspacek
Jan 16 15:03
Okay, let's test it in production! :-)
https://gitlab.labs.nic.cz/knot/edns-zone-scanner/pipelines/44289 is building Docker image, it should be live in minute or two.
SvenVD
@SvenVD
Jan 16 15:21
Ok I ran the new code against my little test zone and it passes
I will now start run against be zone, but this will take hours
so result only tomorrow
Petr Špaček
@pspacek
Jan 16 15:22
I would really appreciare if you could share the test zone - I would integrate it into CI to make sure we do not break this again.
SvenVD
@SvenVD
Jan 16 15:23
I would happy to share but that would be of no use because it depends on NS that we do not have under control. For now it is useful as long as those people do not change the NS or ips that trigger the bug.
Petr Špaček
@pspacek
Jan 16 15:25
Well, for CI it would be enough to use made-up zone and made-up genreport answers - that would speciifcally test normalization phase (and not other parts).
Petr Špaček
@pspacek
Jan 16 15:35
@SvenVD Thank you very much for cooperation. I have to go now, see you later.
SvenVD
@SvenVD
Jan 16 16:12
No problem, thanks for looking into it.