-
Notifications
You must be signed in to change notification settings - Fork 902
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: Support DNSAction.Allow in DynBlocks for testing purposes #6703
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this pull request! Code looks pretty good. I'm a bit concerned about the fact that in this current version, DNSAction.Allow
definitively accepts the query, bypassing any rules. While this is consistent with the behavior of DNSAction.Allow
in other places, it does prevent using it to test dynamic blocks
if you also want the other rules to still apply. Perhaps we need a DNSAction.NoOp
or something like that?
pdns/dnsdist-lua.cc
Outdated
case DNSAction::Action::Refused: | ||
g_dynBlockAction = action; | ||
break; | ||
case DNSAction::Action::Truncate: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure the switch is needed there, but perhaps we could at least collapse it into two single blocks since all blocks are the same except for the default, so something like:
case DNSAction::Action::Allow:
case DNSAction::Action::Drop:
case DNSAction::Action::Refused:
case DNSAction::Action::Truncate:
g_dynBlockAction = action;
break;
default:
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack, will collapse the switch statement.
With regards to the rule processing, how about permitting the use of DNSAction::Action::None here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it looks like DNSAction::Action::None
would work for setDynBlocksAction()
, although it wouldn't for the optional action parameter of addDynBlocks()
since we already use None
there to indicate that the default action should be used. I don't think that's an issue, but we would need to make that clear in the documentation.
I'm closing this pull request because it looks like it has been superseded by #6776, please correct me if I'm wrong and thank you again! |
Short description
Supports DNSAction.Allow in DynBlocks for testing purposes, based on discussion in the IRC channel on 31/05. Also makes documentation more verbose.
Checklist
I have: