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

Add a setTCPQueryRate Dynblock to allow IP to fall back to UDP #12731

Open
MiniPierre opened this issue Apr 11, 2023 · 6 comments
Open

Add a setTCPQueryRate Dynblock to allow IP to fall back to UDP #12731

MiniPierre opened this issue Apr 11, 2023 · 6 comments

Comments

@MiniPierre
Copy link

  • Program: dnsdist
  • Issue type: Feature request

Short description

We would like to be able to remove an IP from the BPF maps if it is retrying in TCP after getting a TC response.

Usecase

When a legitimate IP is inserted into the BPF maps and is retrying with TCP, we can see a great burst of TCP requests that takes a lot of computing resources and time when the client is a big DNS resolver. We would like to allow IPs to fall back to UDP when flagged as legitimate.

Description

The idea would be to add a setTCPQueryRate DynBlock that match a TCP requests rate and add the corresponding IP to a BPF whitelist map or exclude it from the actual BPF maps for a given time, allowing it to fall back to UDP.

@Habbie Habbie added the dnsdist label Apr 11, 2023
@MiniPierre MiniPierre changed the title Add a setTCPQueryRate to allow IP to fall back to UDP Add a setTCPQueryRate Dynblock to allow IP to fall back to UDP Apr 11, 2023
@rgacogne rgacogne added this to the dnsdist-helpneeded milestone May 3, 2023
@rgacogne
Copy link
Member

rgacogne commented May 3, 2023

I like the idea, we need to find a generic way to implement that. We can already interact with the BPF maps from Lua, so I'm wondering if it would be possible to implement that in pure Lua, from the maintenance function. Perhaps by looking at the ring buffers, or providing a counting mechanism similar to what we do in MaxQPSIPRule, so that maintenance knows how many queries were received over TCP from a particular IP in the last N seconds.

@MiniPierre
Copy link
Author

I tried to implemented an ersatz mechanism using excludeRange to whitelist IPs but it does not seems to do what I expected.
For example, with the IP 172.16.1.1:

> dbr:excludeRange("172.16.1.1/32")
> dbr:toString()
...
Excluded Subnets: 172.16.1.1/32

Then I try to remove the whitelisting using includeRange, and while I am expecting the subnet to not appear anymore, it just modifies the entry:

> dbr:includeRange("172.16.1.1/32")
> dbr:toString()
...
Excluded Subnets: !172.16.1.1/32

Is it the expected behaviour, and if so is there a way to simply remove the exclusion ?

@MiniPierre
Copy link
Author

Also, if I exclude the same IP with an incorrect network mask, I will have a duplicated entry:

> dbr:excludeRange("172.16.1.1/32")
> dbr:excludeRange("172.16.1.1/33")
> dbr:toString()
...

Excluded Subnets: 172.16.1.1/32, 172.16.1.1/32

@rgacogne
Copy link
Member

rgacogne commented Oct 6, 2023

Is it the expected behaviour, and if so is there a way to simply remove the exclusion ?

Yes, it is the expected behaviour. I agree it might be a bit strange but the idea is that when we insert a new entry it might be more specific than an existing subnet with a different policy (like, we are including 192.0.2.1/32 while 192.0.2.0/24 was already excluded) so it might be wrong to remove the existing entry. I think it would make sense to have a removeRange() Lua binding, though.

Also, if I exclude the same IP with an incorrect network mask, I will have a duplicated entry:

This does really look like a bug, I'm looking into it.

@rgacogne
Copy link
Member

rgacogne commented Oct 6, 2023

Also, if I exclude the same IP with an incorrect network mask, I will have a duplicated entry:

This does really look like a bug, I'm looking into it.

This should be fixed by #13340

@rgacogne
Copy link
Member

rgacogne commented Oct 6, 2023

Is it the expected behaviour, and if so is there a way to simply remove the exclusion ?

Yes, it is the expected behaviour. I agree it might be a bit strange but the idea is that when we insert a new entry it might be more specific than an existing subnet with a different policy (like, we are including 192.0.2.1/32 while 192.0.2.0/24 was already excluded) so it might be wrong to remove the existing entry. I think it would make sense to have a removeRange() Lua binding, though.

#13342

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

No branches or pull requests

3 participants