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 UUIDs to rules #6030

Merged
merged 7 commits into from Jan 20, 2018

Conversation

Projects
None yet
3 participants
@rgacogne
Member

rgacogne commented Dec 1, 2017

Short description

Having UUID assigned to rules makes it possible to track a given rule, as opposed to the existing rule numbers that changed every time a rule was deleted or moved around. A rule now keeps the same UUID for the lifetime of the dnsdist process and can even keep this UUID persistent across restart if the UUID is provided when the rule is added:

addAction(AllRule(), AllowAction(), {uuid="123e4567-e89b-12d3-a456-426655440000"})

This is especially useful if the rules are managed via a central controller using the console, or to display metrics retrieved via the API.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled and 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 added this to the dnsdist-1.3.0 milestone Dec 1, 2017

@rgacogne rgacogne force-pushed the rgacogne:dnsdist-consistent-ids branch from 6c19262 to c3700a0 Dec 11, 2017

@rgacogne rgacogne force-pushed the rgacogne:dnsdist-consistent-ids branch from c3700a0 to 770f828 Dec 18, 2017

@rgacogne rgacogne force-pushed the rgacogne:dnsdist-consistent-ids branch from 770f828 to 922c7dc Jan 9, 2018

dnsdist: Add UUIDs to rules
Having UUID assigned to rules makes it possible to track a given rule,
as opposed to the existing rule numbers that changed everytime a rule
was deleted or moved around. A rule now keeps the same UUID for the
lifetime of the dnsdist process and can even keep this UUID persistent
across restart if the UUID is provided when the rule is added:

```
addAction(AllRule(), AllowAction(), {uuid="123e4567-e89b-12d3-a456-426655440000"})
```

This is especially useful if the rules are managed via a central
controller using the console, or to display metrics retrieved via
the API.

@rgacogne rgacogne force-pushed the rgacogne:dnsdist-consistent-ids branch from 922c7dc to 4d5959e Jan 12, 2018

@rgacogne rgacogne referenced this pull request Jan 16, 2018

Merged

dnsdist: run self-answered responses through a ruleset #6185

4 of 6 tasks complete
@@ -892,32 +892,76 @@ std::shared_ptr<DNSRule> makeRule(const luadnsrule_t& var)
return std::make_shared<NetmaskGroupRule>(nmg, true);
}
static boost::uuids::uuid getRuleID(std::string& id)

This comment has been minimized.

@zeha

zeha Jan 19, 2018

Collaborator

get... for something that also makes something out of thin air sounds wrong to me. Weak suggestion: makeRuleID

}
});
g_lua.writeFunction("rmResponseRule", [](unsigned int num) {
g_lua.writeFunction("rmResponseRule", [](boost::variant<unsigned int, std::string> num) {

This comment has been minimized.

@zeha

zeha Jan 19, 2018

Collaborator

rename num to id as done in the help strings?

rules.erase(std::remove_if(rules.begin(),
rules.end(),
[uuid](const DNSDistResponseRuleAction& a) { return a.d_id == uuid; }),
rules.end());

This comment has been minimized.

@zeha

zeha Jan 19, 2018

Collaborator

there's now no feedback if no rule matches, right?

auto rule=makeRule(dnsrule);
return std::make_shared<std::pair< luadnsrule_t, std::shared_ptr<DNSAction> > >(rule, action);
DNSDistRuleAction ra({rule, action, uuid});
return std::make_shared<DNSDistRuleAction>(ra);

This comment has been minimized.

@zeha

zeha Jan 19, 2018

Collaborator

the given uuid is not checked against uniqueness, right?

boost_required_version=1.42

This comment has been minimized.

@zeha

zeha Jan 19, 2018

Collaborator

maybe add a comment on why? (before someone puts back the old code...)

@@ -187,9 +187,10 @@ JSON Objects
:property string action: The action taken when the rule matches (e.g. "to pool abuse")
:property dict action-stats: A list of statistics whose content varies depending on the kind of rule
:property integer id: The identifier (or order) of this rule
:property integer id: The order of this rule

This comment has been minimized.

@zeha

zeha Jan 19, 2018

Collaborator

s/order/position/?

@zeha

This comment has been minimized.

Collaborator

zeha commented Jan 19, 2018

FTR, my comments are mostly nits and can be fixed later.

I'd welcome a merge soon, so I can rebase my then conflicted PRs.

Would also prepare a followup PR to clean up some of the nits.

@zeha

This comment has been minimized.

Collaborator

zeha commented Jan 19, 2018

Also did a lightweight test of it.

@zeha zeha force-pushed the rgacogne:dnsdist-consistent-ids branch from c5a59af to dcdc32c Jan 19, 2018

@zeha

This comment has been minimized.

Collaborator

zeha commented Jan 19, 2018

I've added cherry-pickable commits that should fix most of my nits, plus one that adds the new optional argument to addLua(Response)Action.

@Habbie

This comment has been minimized.

Member

Habbie commented Jan 19, 2018

Mergeable when green on Travis.

@Habbie Habbie merged commit 4eecc7a into PowerDNS:master Jan 20, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@rgacogne rgacogne deleted the rgacogne:dnsdist-consistent-ids branch Feb 12, 2018

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