rec: Log protobuf messages for cache hits. Add policy tags in gettag() #4016

Merged
merged 1 commit into from Jun 22, 2016

Projects

None yet

2 participants

@rgacogne
Member
rgacogne commented Jun 20, 2016 edited

gettag() can now return an optional policy tags table in addition to
the existing tag integer.
Question and response protobuf messages are now sent even on cache hits.
When protobuf logging is enabled, the protobuf response message is
added to the cache and retrieved together with the response.

@Habbie Habbie and 1 other commented on an outdated diff Jun 20, 2016
pdns/lua-recursor4.cc
@@ -343,6 +343,12 @@ RecursorLua4::RecursorLua4(const std::string& fname)
d_lw->registerFunction("getRecords", &DNSQuestion::getRecords);
d_lw->registerFunction("setRecords", &DNSQuestion::setRecords);
+ d_lw->writeFunction("addPolicyTag", [](vector<string>* tags, const string& tag) {
+ if (tags) {
+ tags->push_back(tag);
+ }
+ });
+
@Habbie
Habbie Jun 20, 2016 Member

This weird idiom (If I see it correctly, inside gettag I am supposed to call addPolicyTag(policytags, 'foo')) is because we don't have any state object to speak of, like dq?

@rgacogne
rgacogne Jun 20, 2016 Member

Yes. I couldn't find a way to make policytags directly writable from Lua, but I'm open to all suggestion!

@Habbie
Habbie Jun 20, 2016 Member

Options I have in mind:

  1. make table.insert work on it, would be just about as weird though
  2. change from return 5 to return 5, {"foo", "ipsum"}

I don't think 1 is important. 2 might be nice but what you have now is fine with me!

@rgacogne
rgacogne Jun 20, 2016 Member

I always forget that LuaWrapper handles returning multiple values. I'll try to make option 2 work, I like it a lot better than the current one.

@Habbie
Member
Habbie commented Jun 20, 2016

conflicts.. probably because of the other merge of your stuff I just did

@rgacogne
Member

Rebased to fix a conflict.

@rgacogne
Member

New version, this time gettag() can return an optional table of policy tags in addition to the existing tag.

@Habbie
Member
Habbie commented Jun 20, 2016

Change looks good. Feel free to squash. I did not review the 'log protobuf for cache hits' part.

@Habbie Habbie and 1 other commented on an outdated diff Jun 21, 2016
pdns/lua-recursor4.cc
{
- if(d_gettag)
- return d_gettag(remote, ednssubnet, local, qname, qtype);
+ if(d_gettag) {
+ auto ret = d_gettag(remote, ednssubnet, local, qname, qtype);
+
+ if (policyTags) {
+ boost::optional<std::unordered_map<int, std::string>> tags = std::get<1>(ret);
@Habbie
Habbie Jun 21, 2016 Member

I'm surprised this can't be auto - or can it?

@rgacogne
rgacogne Jun 21, 2016 Member

It can, I'm not sure which option is easier to read though.

@Habbie
Habbie Jun 21, 2016 Member

Alright, do what makes sense to you :)

@Habbie
Member
Habbie commented Jun 21, 2016

I see no obvious defects in the protobuf side of things, but I did not read every line. If you have tested it, please feel free to merge.

@rgacogne rgacogne rec: Log protobuf messages for cache hits. Add policy tags in gettag()
gettag()` can now return an optional policy tags table in addition to
the existing `tag` integer.
Question and response protobuf messages are now sent even on cache hits.
When protobuf logging is enabled, the protobuf response message is
added to the cache and retrieved together with the response.
02b47f4
@rgacogne
Member

Minor update using auto in RecursorLua4::gettag(), will merge once travis is happy.

@rgacogne rgacogne merged commit 43fbbb3 into PowerDNS:master Jun 22, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rgacogne rgacogne deleted the rgacogne:rec-protobuf-cached branch Jun 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment