rec: implement DNS cookie support for outgoing requests #15412
Conversation
Pull Request Test Coverage Report for Build 14754294555Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
| contacted again, it should use the same bound IP. | ||
|
|
||
| - Although it is perfectly fine for a client cookie to live for a long time, this design will | ||
| flush entries older that a certain period of time, to avoid an ever growing CookieStore. |
There was a problem hiding this comment.
Avoid an ever-growing cookie store? I'm not sure the cookie monster will like this.
…ies disabled and enabled
|
Rebased to fix conflicts |
Co-authored-by: Miod Vallat <[email protected]>
|
Did a few bulk tests both with cookie enabled and disabled and saw no issues. So ready for review. |
| @@ -15,12 +15,15 @@ IMPORTS | |||
| FROM SNMPv2-CONF; | |||
|
|
|||
| rec MODULE-IDENTITY | |||
| LAST-UPDATED "202408280000Z" | |||
| LAST-UPDATED "202504290000Z" | |||
There was a problem hiding this comment.
(not for this PR really)
should this .txt be in git at all? it is generated
There was a problem hiding this comment.
We refer to it from docs, so it needs to be somewhere.
Co-authored-by: Peter van Dijk <[email protected]>
…mments why binding to a given local address is needed
There was a problem hiding this comment.
This looks solid. I reviewed all code -except- the tests (which somebody should definitely still do, that's where the proof is).
I wonder about the case where an auth
- responds normally to non-EDNS queries
- responds correctly to EDNS queries
- responds FORMERR to EDNS queries with a cookie, with the distinctive cases of (1) it tells you it hates the cookie opt [by having EDNS in the reply] (2) it tells you it has broken EDNS support [by not having EDNS in the reply]
pdns/recursordist/metrics_table.py
Outdated
| { | ||
| 'name': 'cookies-supported', | ||
| 'lambda': '[] { return g_Counters.sum(rec::Counter::cookiesSupported); }', | ||
| 'desc': 'Number of authoritative server IPs marked as supporting cookies', |
There was a problem hiding this comment.
given that entries eventually expire, this sentence is not completely accurate. I don't have better wording though (and I suspect this problem is not limited to this PR)
There was a problem hiding this comment.
Number of of authoritative server IPs currently known to support cookies?
There was a problem hiding this comment.
but a single IP might still be counted twice if we don't talk to it for a while, right?
There was a problem hiding this comment.
Right... Number of cookies probes resulting in succes?
There was a problem hiding this comment.
ah yes, I suppose so! in that case it goes with the cookie* stats, not the cookies* stats, and the same for Unsupported, so cookieProbeSupported and cookieProbeUnsupported
|
Thanks! I did all the trivial things. Will review the EDNS supported/FormErr etc logic soon. |
Short description
Draft, as I need to do some bulk tests.
Checklist
I have: