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

dnsdist: Add warning rates to dynBlockRulesGroup rules #6986

Merged
merged 2 commits into from
Oct 17, 2018

Conversation

rgacogne
Copy link
Member

Short description

After thinking a bit more about the needs described in #6907, I could not find a meaningful scenario where we want different actions for different rates except for the warning case. So for now I took the easy route of just adding a warning threshold, and we can improve on it later if we find that we need more than that.
Closes #6907.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@rgacogne rgacogne force-pushed the dnsdist-warning-dynblocks branch from d9bd2e5 to 1d3ba13 Compare October 3, 2018 09:06
Copy link
Member

@chbruyand chbruyand left a comment

Choose a reason for hiding this comment

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

LGTM!
It may be nice to add a test where only the warning threshold is enabled.

@rgacogne
Copy link
Member Author

It may be nice to add a test where only the warning threshold is enabled.

This is actually not handled, since you can do that by using the regular threshold and a NoOp action, instead of using the warning threshold. Do you think it would make sense to handle that? We could make it work by passing a 0 value to the regular rate, but I'm not sure it would be very intuitive..

Copy link
Member

@chbruyand chbruyand left a comment

Choose a reason for hiding this comment

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

👍

@rgacogne rgacogne merged commit bd0331e into PowerDNS:master Oct 17, 2018
@rgacogne rgacogne deleted the dnsdist-warning-dynblocks branch October 17, 2018 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dnsdist: support several dynBlockRules of the same type with different actions
2 participants