Change default value to 1232 #6183
Conversation
As specified by DNS flag day 2020, good and decent default value avoiding fragmentation issues should be 1232. It is quite likely 1500 would work reliably on local ethernet networks. Value 512 is set implicitly and must be used for all clients, which did not include OPT RR with explicit value they support. Since MR coredns#5368 it should work correctly. Signed-off-by: Petr Menšík <[email protected]>
pemensik
force-pushed
the
bufsize-default
branch
from
8667e6f to
25d400d
Compare
|
I think this plugin is somehow too simple. It can only further reduce EDNS accepted size after #5366. It is not able to add EDNS header to forwarded query, which I think documentation for it suggests. It can only reduce accepted response size for queries, which already has EDNS0 header. |
|
I think the example, which uses bufsize with forward plugin, creates confusion. It were originally able to add EDNS0 header to forwarded query. After MR #5368 it cannot do that anymore. There are cases where adding EDNS0 header is desired. But in case the response is larger than originating client query allowed, it would have to return response with TC bit set and EDNS0 header removed. That seems not possible anymore. |
|
Related change in OpenShift: openshift/cluster-dns-operator#370 |
|
It seems to me similar algorithm should be made as in #4085. When incoming query arrives, it should be safe to set internal supported EDNS0 size on the forwarded query. When the answer arrives, OPT RR may need to be removed from response, if it were not in original request. |
There was a problem hiding this comment.
Thanks for the PR, LGTM! would be nice to include it into 1.11.0 release
FYI @chrisohaver
There was a problem hiding this comment.
oh, looks like TestSetupBufsize needs to be updated. Shame on me for approving before triggering pipelines 🙂
|
I’d like to review all recent PRs/issues relating to Edns before they merge. |
|
FYI @pemensik all commits must be signed https://github.com/coredns/coredns/pull/6183/checks?check_run_id=14770904337 🙂 |
|
Okay, I think it is ready for review now. |
Codecov Report
@@ Coverage Diff @@
## master #6183 +/- ##
==========================================
+ Coverage 55.70% 58.97% +3.27%
==========================================
Files 224 248 +24
Lines 10016 16154 +6138
==========================================
+ Hits 5579 9527 +3948
- Misses 3978 6041 +2063
- Partials 459 586 +127
... and 165 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
TestSetupBufsize needs adjustment https://github.com/coredns/coredns/actions/runs/5478594030/jobs/9979672394?pr=6183
Test 0: Bufsize not correctly set for input bufsize. Expected: 512, actual: 1232
Ouch, I have force-pushed previous version with just corrected documentation. I have already fixed this. |
Check also buffer size smaller than legacy value is not accepted. Signed-off-by: Petr Menšík <[email protected]>
Mention also increasing request size is not possible, it can only reduce the accepted size. Signed-off-by: Petr Menšík <[email protected]>
* Change default value to 1232 As specified by DNS flag day 2020, good and decent default value avoiding fragmentation issues should be 1232. It is quite likely 1500 would work reliably on local ethernet networks. Value 512 is set implicitly and must be used for all clients, which did not include OPT RR with explicit value they support. Since MR coredns#5368 it should work correctly. Signed-off-by: Petr Menšík <[email protected]> * Adapt bufsize test to new default value Check also buffer size smaller than legacy value is not accepted. Signed-off-by: Petr Menšík <[email protected]> * Update bufsize documentation Mention also increasing request size is not possible, it can only reduce the accepted size. Signed-off-by: Petr Menšík <[email protected]> --------- Signed-off-by: Petr Menšík <[email protected]> Signed-off-by: jaehnri <[email protected]>
As specified by DNS flag day 2020, good and decent default value avoiding fragmentation issues should be 1232. It is quite likely 1500 would work reliably on local ethernet networks.
Value 512 is set implicitly and must be used for all clients, which did not include OPT RR with explicit value they support.
Since MR #5368 it should work correctly.
1. Why is this pull request needed and what does it do?
Changes EDNS0 default value to recommended value.
2. Which issues (if any) are related?
3. Which documentation changes (if any) need to be made?
4. Does this introduce a backward incompatible change or deprecation?
It removes the confusion around EDNS0 size. It may create behaviour change.