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: Support DNSAction.Allow in DynBlocks for testing purposes #6703

Closed

Conversation

lochiiconnectivity
Copy link
Contributor

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:

  • 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)
  • checked that this code was merged to master

@lochiiconnectivity lochiiconnectivity changed the title Support DNSAction.Allow in DynBlocks for testing purposes dnsdist: Support DNSAction.Allow in DynBlocks for testing purposes Jun 2, 2018
@rgacogne rgacogne added this to the dnsdist-1.3.x milestone Jun 4, 2018
Copy link
Member

@rgacogne rgacogne left a 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?

case DNSAction::Action::Refused:
g_dynBlockAction = action;
break;
case DNSAction::Action::Truncate:
Copy link
Member

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:
...

Copy link
Contributor Author

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?

Copy link
Member

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.

@rgacogne
Copy link
Member

rgacogne commented Aug 2, 2018

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!

@rgacogne rgacogne closed this Aug 2, 2018
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.

None yet

2 participants