Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: dns caching #137

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

ohaiibuzzle
Copy link
Contributor

@ohaiibuzzle ohaiibuzzle commented Aug 14, 2024

Add a simple TTL cache for the DNS Resolver. Seperated from #129. Re-do from #132

The idea is that since SpoofDPI currently have to perform DNS lookup on every request it make, it would get slow if a domain is requested multiple times over multiple connections (easiest example being something like a social timeline where each image is a new connection). Adding a cache here allows some of them to be kept for later requests which reduces latency.

Note that this cache implementation is NOT optimal. If there is a flood of uncached requests, some may miss the cache even when they have been written by other routines. The issue is that I've not found a way to fix the issue without effectively render the resolver single threaded :(

Also after merge you may want to set a different default TTL (like 300s). 60s might be a bit on the aggressive side for DNS caching

@ohaiibuzzle ohaiibuzzle mentioned this pull request Aug 16, 2024
dns/dns_cache.go Outdated Show resolved Hide resolved
@ohaiibuzzle
Copy link
Contributor Author

@TeaDove sorry for the (suuuuuuuuuuper) late response. I've take that into account and rebase the code. Please check if the implementation is acceptable

@Ledorub
Copy link
Collaborator

Ledorub commented Sep 8, 2024

Can it lead to issues when client switches from IPv4-only connection to IPv6-only or vice versa? I assume that in such circumstances SpoofDPI will return an address from the cache, which will be unreachable from the client.

@ohaiibuzzle
Copy link
Contributor Author

In theory, it is an issue, and we could account for that by also caching the queried type, but like, this shouldn’t happen unless you are hopping network to network in a very short amount of time.

Still, something to consider

@LiquidTheDangerous
Copy link
Collaborator

@ohaiibuzzle It seems that caching by qType makes no sense. In any case, if the name is resolved, then we can dial a connection with ipv4 or ipv6 address

@ohaiibuzzle
Copy link
Contributor Author

ohaiibuzzle commented Sep 14, 2024

From what I can tell the matter that is being brought up here is a novel-ish situation where if the cache is on v4, then somehow during cache expiry you jump from a v4-only network to v6-only (or back), it will serve a v4 address to a v6-only network (or vice versa) and fail

@LiquidTheDangerous

@ohaiibuzzle
Copy link
Contributor Author

ohaiibuzzle commented Sep 19, 2024

@LiquidTheDangerous We could but a daemon scrubbing routine still have to exist to prevent a situation where a cache item that is rarely accessed is indefinitely stored (because it never gets accessed again to be cleared)

@LiquidTheDangerous
Copy link
Collaborator

LiquidTheDangerous commented Sep 19, 2024

@ohaiibuzzle You can look at this branch, tell please how suitable it is for implementing a cache. Create a new handler and insert it into the chain #250

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants