[vnet][2] DNS library #40972
[vnet][2] DNS library #40972
Conversation
nklaassen
added
the
no-changelog
nklaassen
force-pushed
the
nklaassen/vnet2
branch
from
28914ff to
7bff062
Compare
nklaassen
force-pushed
the
nklaassen/vnet1
branch
from
90fc2d7 to
6dd426d
Compare
nklaassen
force-pushed
the
nklaassen/vnet2
branch
from
1cfa52a to
f63c9db
Compare
| ) | ||
|
|
||
| // Resolver represents an entity that can resolve DNS requests. | ||
| type Resolver interface { |
There was a problem hiding this comment.
I assume net.Resolver won't work here?
There was a problem hiding this comment.
right - net.Resolver is a concrete type that is an actual DNS resolver, I'm using this interface to pass in my custom DNS resolver in the tests and in #41031
nklaassen
force-pushed
the
nklaassen/vnet1
branch
from
6dd426d to
07c8f8a
Compare
nklaassen
force-pushed
the
nklaassen/vnet2
branch
from
f63c9db to
2d32fec
Compare
nklaassen
force-pushed
the
nklaassen/vnet1
branch
from
07c8f8a to
08a8d97
Compare
nklaassen
force-pushed
the
nklaassen/vnet2
branch
from
2d32fec to
4a313d6
Compare
nklaassen
force-pushed
the
nklaassen/vnet1
branch
from
08a8d97 to
c65c36f
Compare
nklaassen
force-pushed
the
nklaassen/vnet2
branch
from
4a313d6 to
60cd009
Compare
nklaassen
force-pushed
the
nklaassen/vnet1
branch
from
c65c36f to
bd00c44
Compare
nklaassen
force-pushed
the
nklaassen/vnet2
branch
from
60cd009 to
493af94
Compare
nklaassen
force-pushed
the
nklaassen/vnet2
branch
from
493af94 to
ae30402
Compare
nklaassen
force-pushed
the
nklaassen/vnet2
branch
2 times, most recently
from
2142e2d to
969dd03
Compare
|
friendly ping @Joerger @fspmarshall |
Co-authored-by: rosstimothy <[email protected]>
lib/vnet/dns/dns.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| maxUDPMessageSize = 65535 |
There was a problem hiding this comment.
nit: would be good to document the significance of this value.
There was a problem hiding this comment.
thanks for encouraging me to actually put a bit more thought into this, since we're only handling DNS we can actually shrink this quite a bit to the (typical) maximum EDNS message size of 4096 bytes.
public-teleport-github-review-bot
bot
removed request for
ibeckermayer and
Joerger
|
|
||
| var ( | ||
| // vnetNotImplemented is an error indicating that VNet is not implemented on the host OS. | ||
| vnetNotImplemented = &trace.NotImplementedError{Message: "VNet is not implemented on " + runtime.GOOS} |
| // ResolveA should return a Result for an A record question. If an empty Result is returned with no error, | ||
| // the question will be forwarded upstream. | ||
| ResolveA(ctx context.Context, domain string) (Result, error) | ||
|
|
||
| // ResolveAAAA should return a Result for an AAAA record question. If an empty Result is returned with no | ||
| // error, the question will be forwarded upstream. | ||
| ResolveAAAA(ctx context.Context, domain string) (Result, error) |
There was a problem hiding this comment.
Since "empty Result" means something here, it would be helpful to codify what "empty" means with a Result.isEmpty() method, or change this API to return a *Result and return nil on "empty".
| }, nil | ||
| } | ||
|
|
||
| // getMessageBuffer returns a buffer of size [maxUDPMessageSize]. Call [returnBuf] to return the buffer to the |
There was a problem hiding this comment.
`// ... Callers MUST call [returnBuf] to return the buffer to the shared pool after use.
| if err != nil { | ||
| return trace.Wrap(err, "failed to read from UDP conn") | ||
| } | ||
| if n >= maxUDPDNSMessageSize { |
There was a problem hiding this comment.
nwill never be> maxUDPDNSMessageSize, because all the buffers aremaxUDPDNSMessageSize- Shouldn't
n == maxUDPDNSMessageSizebe valid, because a message of the max size is allowed? - I don't know exactly how things work here, but if this is a continuous conn then you may need some way to discard the rest of a too-big-message here. Otherwise the next call to
HandleUDPmay just be reading the end of the oversized message.
|
|
||
| // ListendAndServeUDP reads all incoming UDP messages from [conn], handles DNS questions, and writes the | ||
| // responses back to [conn]. | ||
| // This is not called by VNet code and basically exists so we can test the resolver outside of VNet. |
There was a problem hiding this comment.
Smells a bit fishy, like there should be a better way to simulate calling HandleUDP itself, but I don't know it. Maybe more will be revealed in the next PR.
| // Not using the errgroup err, errors were written to channel. | ||
| _ = g.Wait() |
There was a problem hiding this comment.
Wherever you do this I suspect there's a simpler way just using go primitives instead of errgroup
| errs := make(chan error, len(upstreamNameservers)) | ||
| g, ctx := errgroup.WithContext(ctx) | ||
| ctx, cancel = context.WithCancel(ctx) | ||
| defer cancel() |
There was a problem hiding this comment.
I think this cancel's redundant, the parent already has a deferred cancel.
This is the third in a series of PRs implementing Teleport VNet RFD. parent child
This PR implements a DNS server library that will later be used by VNet to serve local address for all Teleport apps. It supports directly answering DNS questions where the answer is known, and forwarding DNS questions to upstream nameservers when the answer isn't known locally. It does not implement any caching, downstream DNS resolvers should do their own caching.