Skip to content

evdns: Add support for DNS requests via TCP#1004

Merged
azat merged 1 commit intolibevent:masterfrom
seleznevae:dns_via_tcp
May 25, 2020
Merged

evdns: Add support for DNS requests via TCP#1004
azat merged 1 commit intolibevent:masterfrom
seleznevae:dns_via_tcp

Conversation

@seleznevae
Copy link
Copy Markdown
Contributor

@seleznevae seleznevae commented Apr 28, 2020

Added support for DNS requests via TCP. By default, requests are
done via UDP. In case truncated response is received new attempt
is done via TCP connection. Added 2 new macros DNS_QUERY_USEVC and
DNS_QUERY_IGNTC to force all requests to be done via TCP and to
disable switch to TCP in case of truncated responses.

Also added possibility for DNS server to listen and receive requests
on TCP port. Current implementation of TCP support in DNS server seems
rather preliminary and maybe changes after discussion and code review.

Fixes: #979

@seleznevae
Copy link
Copy Markdown
Contributor Author

seleznevae commented Apr 28, 2020

Some comments about the pull request.
One of the main goals was to preserve current API as much as possible and not to create new exported functions.

Fallback to TCP in case of truncated DNS requests is done automatically. To imitate the old behaviour macros DNS_QUERY_IGNTC should be used. To force all DNS requests to be done via TCP one should use the flag DNS_QUERY_USEVC. Names DNS_QUERY_IGNTC, DNS_QUERY_USEVC were chosen to imitate similar flags in c-ares and glibc.

Support for TCP connections in DNS server is done primarly for tests. I wasn't sure how better to implement it (in terms of API). At the moment the same function evdns_add_server_port_with_base is used to create TCP or UDP servers (to handle TCP and UDP requests at the same type one should create 2 servers). There are some other implementation variants: like create a special type for TCP servers or a new type that will handle TCP and UDP servers at the same type. I hoped the right decision about the API will be found during the discussion. Therefore to simplify things at the moment all TCP and UDP data are in the same class evdns_server_port and functions that work work with server have mixed code for TCP and UDP (which in some cases might be rather confusing). After the discussion it will be possibly e.g. to create 2 internal structure like evdns_tcp_server_port and evdns_udp_server_port which might have some mutual part evdns_server_port and make more clear distinction in code that process TCP and UDP cases.

@azat
Copy link
Copy Markdown
Member

azat commented Apr 28, 2020

Many thanks for this patch set!
I need to look through the code carefully, but I like the idea in general for sure (especially that everything is configurable).

@azat
Copy link
Copy Markdown
Member

azat commented Apr 28, 2020

Refs: #684

Copy link
Copy Markdown
Member

@azat azat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broadly looks good, but some comments need to be addressed first

Copy link
Copy Markdown
Member

@azat azat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, can you rebase against upstream/master and squash fixup comments? Also maybe I will something else while looking before merging, objections on rebasing? (will push --force into this topic branch, to update the PR HEAD, so that it will be marked as merged)

@seleznevae
Copy link
Copy Markdown
Contributor Author

Hi! I've squashed all changes to one commit, rebased on master and forced pushed.

@azat azat merged commit 0f6ee89 into libevent:master May 25, 2020
@azat
Copy link
Copy Markdown
Member

azat commented May 25, 2020

Applied, many thanks! (I fixed some small issues, you can take a look at the topic branch that contains it, and also I added the description of the PR to the topic branch merge commit)

@seleznevae
Copy link
Copy Markdown
Contributor Author

Hi! Thanks for your help and support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

Support of DNS requests via TCP

3 participants