From 07159c8d87c62c7cd7ef79579a1f575e0cc931f8 Mon Sep 17 00:00:00 2001 From: Chris Narkiewicz Date: Wed, 7 Sep 2022 14:53:30 +0100 Subject: [PATCH] Do not expand query UDP buffer size if already set to a smaller value (#5602) Signed-off-by: Krzysztof Narkiewicz Signed-off-by: Krzysztof Narkiewicz Co-authored-by: Krzysztof Narkiewicz --- plugin/bufsize/bufsize.go | 5 +- plugin/bufsize/bufsize_test.go | 136 +++++++++++++++++++-------------- 2 files changed, 81 insertions(+), 60 deletions(-) diff --git a/plugin/bufsize/bufsize.go b/plugin/bufsize/bufsize.go index f3c228d07..00556c2ba 100644 --- a/plugin/bufsize/bufsize.go +++ b/plugin/bufsize/bufsize.go @@ -1,4 +1,4 @@ -// Package bufsize implements a plugin that modifies EDNS0 buffer size. +// Package bufsize implements a plugin that clamps EDNS0 buffer size preventing packet fragmentation. package bufsize import ( @@ -17,10 +17,9 @@ type Bufsize struct { // ServeDNS implements the plugin.Handler interface. func (buf Bufsize) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) { - if option := r.IsEdns0(); option != nil { + if option := r.IsEdns0(); option != nil && int(option.UDPSize()) > buf.Size { option.SetUDPSize(uint16(buf.Size)) } - return plugin.NextOrFailure(buf.Name(), buf.Next, ctx, w, r) } diff --git a/plugin/bufsize/bufsize_test.go b/plugin/bufsize/bufsize_test.go index 45fef84e7..eb267ddbc 100644 --- a/plugin/bufsize/bufsize_test.go +++ b/plugin/bufsize/bufsize_test.go @@ -4,7 +4,6 @@ import ( "context" "testing" - "github.com/coredns/coredns/plugin" "github.com/coredns/coredns/plugin/test" "github.com/coredns/coredns/plugin/whoami" @@ -12,69 +11,92 @@ import ( ) func TestBufsize(t *testing.T) { - em := Bufsize{ - Size: 512, + const maxBufSize = 1024 + + setUpWithRequestBufsz := func(bufferSize uint16) (Bufsize, *dns.Msg) { + p := Bufsize{ + Size: maxBufSize, + Next: whoami.Whoami{}, + } + r := new(dns.Msg) + r.SetQuestion(dns.Fqdn("."), dns.TypeA) + r.Question[0].Qclass = dns.ClassINET + if bufferSize > 0 { + r.SetEdns0(bufferSize, false) + } + return p, r } - tests := []struct { - next plugin.Handler - qname string - inputBufsize uint16 - outgoingBufsize uint16 - expectedErr error - }{ - // This plugin is responsible for limiting outgoing query's bufize - { - next: whoami.Whoami{}, - qname: ".", - inputBufsize: 1200, - outgoingBufsize: 512, - expectedErr: nil, - }, - // If EDNS is not enabled, this plugin should not add it - { - next: whoami.Whoami{}, - qname: ".", - outgoingBufsize: 512, - expectedErr: nil, - }, - } + t.Run("Limit response buffer size", func(t *testing.T) { + // GIVEN + // plugin initialized with maximum buffer size + // request has larger buffer size than allowed + p, r := setUpWithRequestBufsz(maxBufSize + 128) - for i, tc := range tests { - req := new(dns.Msg) - req.SetQuestion(dns.Fqdn(tc.qname), dns.TypeA) - req.Question[0].Qclass = dns.ClassINET - em.Next = tc.next + // WHEN + // request is processed + _, err := p.ServeDNS(context.Background(), &test.ResponseWriter{}, r) - if tc.inputBufsize != 0 { - req.SetEdns0(tc.inputBufsize, false) + // THEN + // no error + // OPT RR present + // request buffer size is limited + if err != nil { + t.Errorf("unexpected error %s", err) } - - _, err := em.ServeDNS(context.Background(), &test.ResponseWriter{}, req) - - if err != tc.expectedErr { - t.Errorf("Test %d: Expected error is %v, but got %v", i, tc.expectedErr, err) + option := r.IsEdns0() + if option == nil { + t.Errorf("OPT RR not present") } - - if tc.outgoingBufsize != 0 { - for _, extra := range req.Extra { - if option, ok := extra.(*dns.OPT); ok { - b := option.UDPSize() - if b != tc.outgoingBufsize { - t.Errorf("Test %d: Expected outgoing bufsize is %d, but got %d", i, tc.outgoingBufsize, b) - } - } else { - t.Errorf("Test %d: Not found OPT RR.", i) - } - } + if option.UDPSize() != maxBufSize { + t.Errorf("buffer size not limited") } + }) - if tc.inputBufsize == 0 { - for _, extra := range req.Extra { - if _, ok := extra.(*dns.OPT); ok { - t.Errorf("Test %d: Found OPT RR on reply to query with no OPT RR.", i) - } - } + t.Run("Do not increase response buffer size", func(t *testing.T) { + // GIVEN + // plugin initialized with maximum buffer size + // request has smaller buffer size than allowed + const smallerBufferSize = maxBufSize - 128 + p, r := setUpWithRequestBufsz(smallerBufferSize) + + // WHEN + // request is processed + _, err := p.ServeDNS(context.Background(), &test.ResponseWriter{}, r) + + // THEN + // no error + // request buffer size is not expanded + if err != nil { + t.Errorf("unexpected error %s", err) } - } + option := r.IsEdns0() + if option == nil { + t.Errorf("OPT RR not present") + } + if option.UDPSize() != smallerBufferSize { + t.Errorf("buffer size should not be increased") + } + }) + + t.Run("Buffer size should not be set", func(t *testing.T) { + // GIVEN + // plugin initialized with maximum buffer size + // request has no EDNS0 option set + p, r := setUpWithRequestBufsz(0) + + // WHEN + // request is processed + _, err := p.ServeDNS(context.Background(), &test.ResponseWriter{}, r) + + // THEN + // no error + // OPT RR is not appended + if err != nil { + t.Errorf("unexpected error %s", err) + } + if r.IsEdns0() != nil { + t.Errorf("EDNS0 enabled for incoming request") + } + }) }