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: Add a KeyValueStoreLookup action based on CDB or LMDB #8139

Merged
merged 26 commits into from Aug 29, 2019

Conversation

@rgacogne
Copy link
Member

commented Jul 29, 2019

Short description

A lookup into a key value store can be done via the KeyValueStoreLookupAction action,
using the usual selectors to match the incoming queries for which the lookup should be done.

The first step is to get a KeyValueStore object via one of the following functions:

  • newCDBKVStore for a CDB database ;
  • newLMDBKVStore for a LMDB one.

Then the key used for the lookup can be selected via one of the following functions:

  • the exact qname with KeyValueLookupKeyQName ;
  • a suffix match via KeyValueLookupKeySuffix, meaning that several lookups will be done, removing one label from the qname at a time, until a match has been found or there is no label left ;
  • the source IP with KeyValueLookupKeySourceIP ;
  • the value of an existing tag with KeyValueLookupKeyTag.

For example, to do a suffix-based lookup into a LMDB KVS database, the following rule can be used:

kvs = newLMDBKVStore('/path/to/lmdb/database', 'database name')
addAction(AllRule(), KeyValueStoreLookupAction(kvs, KeyValueLookupKeySuffix(), 'kvs-suffix-result'))

For a query whose qname is "sub.domain.powerdns.com.", and for which only the "\8powerdns\3com\0" key exists in the database, this would result in the following lookups:

  • \3sub\6domain\8powerdns\3com\0
  • \6domain\8powerdns\3com\0
  • \8powerdns\3com\0

Then a match is found for the last key, and the corresponding value is stored into the 'kvs-suffix-result' tag. This tag can now be used in subsequent rules to take an action based on the result of the lookup.

addAction(TagRule('kvs-suffix-result', 'this is the value obtained from the lookup'), SpoofAction('2001:db8::1'))

If the value found in the LMDB database for the key '\8powerdns\3com\0' was 'this is the value obtained from the lookup', then the query is immediately answered with a AAAA record.

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)

@rgacogne rgacogne added this to the dnsdist-1.5.0 milestone Jul 29, 2019

modules/tinydnsbackend/cdb.cc Outdated Show resolved Hide resolved
modules/lmdbbackend/Makefile.am Outdated Show resolved Hide resolved

@rgacogne rgacogne changed the title dnsdist: Add a KeyValueStoreLookup action based on CDB or LMDB dnsdist: Add a KeyValueStoreLookup action based on CDB or LMDB Jul 29, 2019

@lgtm-com

This comment has been minimized.

Copy link

commented Jul 29, 2019

This pull request introduces 2 alerts when merging ea3055a into b3eb306 - view on LGTM.com

new alerts:

  • 2 for Comparison result is always the same
@lgtm-com

This comment has been minimized.

Copy link

commented Jul 30, 2019

This pull request introduces 2 alerts when merging 75877cc into 7f86fed - view on LGTM.com

new alerts:

  • 2 for Comparison result is always the same

@rgacogne rgacogne force-pushed the rgacogne:dnsdist-kvs branch from 75877cc to cd8a9cc Aug 5, 2019

@rgacogne

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2019

Rebased to fix a conflict in dnsdist-lua-actions.cc.

@lgtm-com

This comment has been minimized.

Copy link

commented Aug 5, 2019

This pull request introduces 2 alerts when merging cd8a9cc into 7fc7689 - view on LGTM.com

new alerts:

  • 2 for Comparison result is always the same
@chbruyand
Copy link
Member

left a comment

Great work!

pdns/dnsdistdist/dnsdist-kvs.hh Outdated Show resolved Hide resolved
pdns/dnsdistdist/dnsdist-kvs.hh Outdated Show resolved Hide resolved
pdns/dnsdistdist/dnsdist-kvs.cc Show resolved Hide resolved
pdns/dnsdist-lua-bindings.cc Outdated Show resolved Hide resolved
rgacogne added 18 commits Jul 4, 2019
dnsdist: Remove ambiguity between our make_unique and the C++14 one
Since LGTM seems to be somehow compiling with C++14 enabled.

@rgacogne rgacogne force-pushed the rgacogne:dnsdist-kvs branch from cd8a9cc to 70ba95d Aug 7, 2019

@lgtm-com

This comment has been minimized.

Copy link

commented Aug 7, 2019

This pull request introduces 2 alerts when merging 70ba95d into 016a165 - view on LGTM.com

new alerts:

  • 2 for Comparison result is always the same
@gregcockroft

This comment has been minimized.

Copy link

commented Aug 12, 2019

2 suggestions

It would be nice to limit the depth of the KeyValueLookupKeySuffix match.
Right now a lookup for "cnn.com" looks up "cnn.com" and "com".
This would save a low level lookup in almost all cases.

Matching on wire format names makes debugging harder. It would be nice if this was optional. A flag so that KeyValueLookupKeySuffix::getKeys could use toString() instead of toDNSString().

@rgacogne

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

Thanks for the feedback!

It would be nice to limit the depth of the KeyValueLookupKeySuffix match.
Right now a lookup for "cnn.com" looks up "cnn.com" and "com".
This would save a low level lookup in almost all cases.

That makes complete sense, I'll likely add that soon.

Matching on wire format names makes debugging harder. It would be nice if this was optional. A flag so that KeyValueLookupKeySuffix::getKeys could use toString() instead of toDNSString().

Agreed, the reason I used wire format was to remove the ambiguity between something like \3www\3sub\4main\3tld\0 and \3www\8sub.main\3tld\0, but I guess we might not care, or not care by default. Adding a flag would makes sense as well.

@rgacogne

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

I just pushed a commit adding suffix matching limits and plain text lookups.

@lgtm-com

This comment has been minimized.

Copy link

commented Aug 13, 2019

This pull request introduces 2 alerts when merging b5b3fc9 into 8f86426 - view on LGTM.com

new alerts:

  • 2 for Comparison result is always the same
rgacogne added 4 commits Aug 14, 2019
@lgtm-com

This comment has been minimized.

Copy link

commented Aug 14, 2019

This pull request introduces 2 alerts when merging 752db0d into da24df4 - view on LGTM.com

new alerts:

  • 2 for Comparison result is always the same
@rgacogne

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2019

I moved the lmdb-safe files to ext/lmdb-safe/.

rgacogne added 2 commits Aug 27, 2019
dnsdist: Add conditionally added CDB/LMDB files to EXTRA_DIST
So they are included in the `make dist` tarball even if CDB or LMDB
support is not enabled at that time.

@rgacogne rgacogne merged commit a1c95af into PowerDNS:master Aug 29, 2019

26 of 27 checks passed

LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: C/C++ No new or fixed alerts
Details
LGTM analysis: Python No new or fixed alerts
Details
ci/circleci: build-auth Your tests passed on CircleCI!
Details
ci/circleci: build-auth-docs Your tests passed on CircleCI!
Details
ci/circleci: build-dnsdist Your tests passed on CircleCI!
Details
ci/circleci: build-dnsdist-docs Your tests passed on CircleCI!
Details
ci/circleci: build-recursor Your tests passed on CircleCI!
Details
ci/circleci: build-recursor-docs Your tests passed on CircleCI!
Details
ci/circleci: test-auth-algorithms Your tests passed on CircleCI!
Details
ci/circleci: test-auth-api Your tests passed on CircleCI!
Details
ci/circleci: test-auth-regress-bind Your tests passed on CircleCI!
Details
ci/circleci: test-auth-regress-gmysql Your tests passed on CircleCI!
Details
ci/circleci: test-auth-regress-gpgsql Your tests passed on CircleCI!
Details
ci/circleci: test-auth-regress-gsqlite3 Your tests passed on CircleCI!
Details
ci/circleci: test-auth-regress-ldap Your tests passed on CircleCI!
Details
ci/circleci: test-auth-regress-lmdb Your tests passed on CircleCI!
Details
ci/circleci: test-auth-regress-mydns Your tests passed on CircleCI!
Details
ci/circleci: test-auth-regress-odbc-mssql Your tests passed on CircleCI!
Details
ci/circleci: test-auth-regress-odbc-sqlite3 Your tests passed on CircleCI!
Details
ci/circleci: test-auth-regress-tinydns Your tests passed on CircleCI!
Details
ci/circleci: test-dnsdist-regression Your tests passed on CircleCI!
Details
ci/circleci: test-ixfrdist-regression Your tests passed on CircleCI!
Details
ci/circleci: test-recursor-api Your tests passed on CircleCI!
Details
ci/circleci: test-recursor-bulk Your tests passed on CircleCI!
Details
ci/circleci: test-recursor-regression Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rgacogne rgacogne deleted the rgacogne:dnsdist-kvs branch Aug 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.