This repository has been archived by the owner on Jun 7, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add a non-existence test using NSEC3 #63
base: main
Are you sure you want to change the base?
Add a non-existence test using NSEC3 #63
Changes from all commits
97f0ef5
53c7a5b
8613556
8c497be
c369acd
e8a952a
7e6ae36
8467cb2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will be included in the signed zone file despite the original / unsigned zone file not having a wildcard record?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, it will not, it is added because the server will compute it and generate an NSEC3 record that covers that specific hash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if your leaf nameserver was not
nameservers.com.
but rathersomedomain.com.
you would not need to deal with theseprimaryNN.nameservers.com.
recordsalso, changing the naming scheme to something like
primary${thread_id}-${thread_local_count}
would make this list shorter but you would need to compute the hash in the test code instead of computing them offline / ahead of timeit's probably easier to read the contents of the signed zone file to get the hashes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having to compute the hash in the tests kinda defeats the purpose as eventually we'd have to integrate similar code to Hickory-DNS and it would feel like we're testing the hashing implementation against itself.
BUUUUT, if we were able to use
example.
we could just reuse the examples in the RFC's appendix.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you may want to add a
try_into_nsec3
method using rust-analyzer and use that hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are several assertions in this single unit test. I would suggest splitting it into several
#[test]
s, where each one maps to a sentence / paragraph in the RFC text, and using a fixture (i.e. a setup function, seesrc/resolver/dnssec/fixtures.rs
) to set up the same name server graph in all the tests.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds a bit expensive. Would it make sense to initialize this in a static then?
But then they wouldn't be dropped 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you put anything that internally contains a
Network
or aContainer
in a static variable then its destructor won't run even if the process shut down cleanly. not running the destructors means leaving docker networks / containers behind which is undesirableat the moment, I would not worry about having more
#[test]
functions making things slower. more tests means you have more granularity about what's failing. if you put many assertions in a single tests and the first one fails then you won't know the outcome of the other assertions. tests can be filtered through the CLI so devs fixing issues will only be running the tests that are of interest to them so the total number of tests won't impact their workflow.that being said, there are ways to set up an object that will be shared by
#[test]
functions and then properly tear down when all the#[test]
functions end but it's quite a bit of work to setup. it does involve a sharedstatic
variable and aMutex
. the side effect of such setup is that one test will impact the other because of the caching behavior of resolvers and nameservers; meaning that such resource-efficient test groups should only be used to test things where caching is unimportant (lest we come up with a way to clear all the caches in between test runs; which again adds more complexity)