-
-
Notifications
You must be signed in to change notification settings - Fork 362
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add the basic EDE (RFC8914) cases #604
Conversation
+ complete EDE codes
This reverts commit b3f60db.
when refusing to answer authoritatively. Also remove TODO comments that were already done
…fluous todo comments
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.
Code looks good! Hopefully the review fixes a couple of issues.
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.
Nice work! Here is the first part of the review (missing review of validator.c and all the test cases).
The review comments are mostly nits but there are also some suggestions and some changes that I would like to see.
I'll start working on the second part of the review.
m->s.env->need_to_validate && (!(r->qflags&BIT_CD) || | ||
m->s.env->cfg->ignore_cd) && rep && |
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.
…g superfluous _ede functions in the validator
…and change logic accordingly, and change validator logic to be more careful with EDE overwrites and change logic accordingly
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.
Finished the second part of the review.
Mostly nits (with clickable suggestions) but I believe a couple of tests should expect a different EDE value.
Co-authored-by: gthess <george@nlnetlabs.nl>
…e logic, remove superfluous test cases
…hange coresponding 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.
Seems good!
Only thing that is pending is changing serve-expired-ede
to ede-serve-expired
and checking the global ede option together with that option.
Maybe also renaming the global option to just ede
for easier grouping with the (future) ede options/toggles as discussed.
…g the global ede option together with that option
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.
LGTM, ready for merging!
* nlnet/master: - Fix some lint type warnings. - Fix ede test to not use default pidfile, and use local interface. - Fix to silence test for ede error output to the console from the test setup script. - Fix typos in config_set_option for the 'num-threads' and 'ede-serve-expired' options. - Fix NLnetLabs#678: [FR] modify behaviour of unbound-control rpz_enable zone, by updating unbound-control's documentation. - For NLnetLabs#677: Added tls-system-cert to config parser and documentation. - Changelog note for NLnetLabs#677. Allow using system certificates not only on Windows - Fix NLnetLabs#417: prefetch and ECS causing cache corruption when used together. - Fix NLnetLabs#673: DNS over TLS: error: SSL_handshake syscall: No route to host. - Fix Python build in non-source directory; based on patch by Michael Tokarev. Changelog entry for NLnetLabs#604: Add the basic EDE (RFC8914) cases Add the basic EDE (RFC8914) cases (NLnetLabs#604) - Fix NLnetLabs#670: SERVFAIL problems with unbound 1.15.0 running on OpenBSD 7.1.
this branch is a split from #504 which implements all the easier EDE cases.