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

added src boolean to NetmaskGroupRule to match destination address #4116

Merged
merged 4 commits into from Aug 19, 2016

Conversation

Projects
None yet
3 participants
@skoef
Contributor

skoef commented Jul 5, 2016

In order to solve #4104, I've created this possible solution to the problem. Also split NetmaskGroupRule into NMGRule and its subclass NetmaskGroupRule as @cmouse suggested.

@@ -779,8 +779,8 @@ vector<std::function<void(void)>> setupLua(bool client, const std::string& confi
return std::shared_ptr<DNSRule>(new SuffixMatchNodeRule(smn, quiet ? *quiet : false));
});
g_lua.writeFunction("NetmaskGroupRule", [](const NetmaskGroup& nmg) {
return std::shared_ptr<DNSRule>(new NetmaskGroupRule(nmg));
g_lua.writeFunction("NetmaskGroupRule", [](const NetmaskGroup& nmg, bool src = true) {

This comment has been minimized.

@rgacogne

rgacogne Jul 6, 2016

Member

With the Lua wrapper we use, you need to make src a boost::optional<bool> if you want it to be optional, which is a good idea to keep compatibility. You'll then have to check whether it is set before using it. There is an example just above in SuffixMatchNodeRule.

@Habbie Habbie added this to the dnsdist-1.1.0 milestone Jul 7, 2016

@rgacogne

This comment has been minimized.

Member

rgacogne commented Aug 16, 2016

I'm not sure what the use of the NMGRule class with the current design. Perhaps two separate classes NetmaskGroupRule and DestinationNetmaskGroupRule both inheriting from NMGRule would make more sense?

@skoef

This comment has been minimized.

Contributor

skoef commented Aug 17, 2016

I believe that was my initial suggestion on IRC, but @cmouse made this suggestion which I went for. What to do now?

@rgacogne

This comment has been minimized.

Member

rgacogne commented Aug 19, 2016

Alright, it does not matter much, let's merge that. Thank you!

@rgacogne rgacogne merged commit 0e5bb7e into PowerDNS:master Aug 19, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@skoef skoef deleted the skoef:netmaskGroupRuleLocalAddr branch Aug 19, 2016

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