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 EDNSOptionRule #6803

Merged
merged 3 commits into from Aug 8, 2018

Conversation

Projects
None yet
3 participants
@Habbie
Member

Habbie commented Jul 24, 2018

Short description

This adds EDNSOptionRule, used as follows (8 is edns-client-subnet):

warnlog(string.format("Script starting %s", "up!"))

newServer({address="8.8.8.8", pool="g1"})
newServer({address="8.8.4.4", pool="g2"})

addAction(EDNSOptionRule(8), PoolAction("g1"))
addAction(AllRule(), PoolAction("g2"))

getServer(0):setUp()
getServer(1):setUp()

Needs:

  • tests
  • documentation
  • a good review of the pointer mangling that I cargo culted from another function in dnsdist-ecs.cc

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)
bool isEDNSOptionInOpt(const char* optStart, const size_t optLen, const uint16_t optionCodeToFind)
{
/* we need at least:
root label (1), type (2), class (2), ttl (4) + rdlen (2)*/

This comment has been minimized.

@zeha

zeha Jul 26, 2018

Collaborator

i've seen this comment so often already, maybe it's time to stick the lengths into a #define or think about some other code sharing...

This comment has been minimized.

@rgacogne

rgacogne Jul 30, 2018

Member

Agreed, at least dnsdist-ecs.cc might need some refactoring.

This comment has been minimized.

@Habbie

Habbie Aug 7, 2018

Member

I agree, but let's do that after this PR?

@rgacogne rgacogne added this to the dnsdist-1.3.x milestone Jul 30, 2018

@rgacogne

Code looks good, a "negative" test would be nice.

addAction(EDNSOptionRule(8), DropAction())
"""
def testDropped(self):

This comment has been minimized.

@rgacogne

rgacogne Jul 30, 2018

Member

It would be nice to add a test proving that a query without the corresponding option is not dropped :)

This comment has been minimized.

@Habbie

Habbie Aug 7, 2018

Member

pushed

_config_template = """
newServer{address="127.0.0.1:%s"}
addAction(EDNSOptionRule(8), DropAction())

This comment has been minimized.

@rgacogne

rgacogne Jul 30, 2018

Member

We might want to export EDNSOptionCode.Cookie, EDNSOptionCode.ECS and so on to Lua.

This comment has been minimized.

@Habbie

Habbie Aug 7, 2018

Member

pushed

bool isEDNSOptionInOpt(const char* optStart, const size_t optLen, const uint16_t optionCodeToFind)
{
/* we need at least:
root label (1), type (2), class (2), ttl (4) + rdlen (2)*/

This comment has been minimized.

@rgacogne

rgacogne Jul 30, 2018

Member

Agreed, at least dnsdist-ecs.cc might need some refactoring.

@rgacogne rgacogne modified the milestones: dnsdist-1.3.x, dnsdist-1.4.0 Jul 30, 2018

@rgacogne

LGTM!

@Habbie Habbie changed the title from [WIP] dnsdist: add EDNSOptionRule to dnsdist: add EDNSOptionRule Aug 7, 2018

@rgacogne

This comment has been minimized.

Member

rgacogne commented Aug 8, 2018

This PR now has a conflict following my merge of #6831, sorry about that :-/

@Habbie Habbie force-pushed the Habbie:ednsoptionrule branch from 0522129 to cd1333c Aug 8, 2018

@Habbie

This comment has been minimized.

Member

Habbie commented Aug 8, 2018

This PR now has a conflict following my merge of #6831, sorry about that :-/

Rebased and force pushed; switched to string like the rest of #6831 so the code is better for it!

Please re-review; I'll squash after.

@Habbie Habbie force-pushed the Habbie:ednsoptionrule branch from cd1333c to 6bb92a0 Aug 8, 2018

@Habbie

This comment has been minimized.

Member

Habbie commented Aug 8, 2018

Squashed!

@@ -69,6 +69,20 @@ void setupLuaVars()
{"Additional",3 }
});
g_lua.writeVariable("EDNSOptionCode", std::unordered_map<string,int>{
{"NSID", 3 },

This comment has been minimized.

@rgacogne

rgacogne Aug 8, 2018

Member

Just a nit, but perhaps this would be better:

{ "NSID",          EDNSOptionCode::NSID },
...

This comment has been minimized.

@Habbie

Habbie Aug 8, 2018

Member

yes!

This comment has been minimized.

@Habbie

Habbie Aug 8, 2018

Member

pushed

@rgacogne

LGTM!

@rgacogne rgacogne merged commit 64c0fe7 into PowerDNS:master Aug 8, 2018

4 checks passed

LGTM analysis: C/C++ No alert changes
Details
LGTM analysis: JavaScript No alert changes
Details
LGTM analysis: Python No alert changes
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment