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

WIP: Add dnstap-compatible protobuf support to dnsdist. #5201

Closed
wants to merge 1 commit into from

Conversation

@jvalentini
Copy link

@jvalentini jvalentini commented Mar 24, 2017

Short description

This adds support for producing dnstap protobuf log messages to dnsdist.

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)
Copy link
Member

@rgacogne rgacogne left a comment

Hi, thank you for this PR! I've added some comments, and of course we also need to make Travis happy before even thinking about merging it.
I see that you are deleting a lot of trailing white spaces. While I'd be happy to get rid of these, please move that to one or more separate commits. Mixing it with actual changes makes reviewing the changes unnecessary hard and will make the use of 'git blame' painful later.

@@ -74,7 +74,7 @@ PDNS_ENABLE_SANITIZERS

PDNS_CHECK_PANDOC

LDFLAGS="$RELRO_LDFLAGS $LDFLAGS"
LDFLAGS="$RELRO_LDFLAGS $LDFLAGS -lfstrm"

This comment has been minimized.

@rgacogne

rgacogne Mar 27, 2017
Member

The configure needs to detect whether libfstrm is available, and dnsdist should be able to compile correctly if it isn't.

@@ -1,132 +0,0 @@
/*

This comment has been minimized.

@rgacogne

rgacogne Mar 27, 2017
Member

You are moving dnsrulactions.cc from the dnsdistdist dir to the pdns one, while we try to keep files that are only used by dnsdist in the dnsdistdist directory. I know I haven't been very consistent about that in the past, sorry.


#define DNSTAP_CONTENT_TYPE "protobuf:dnstap.Dnstap"

FrameStreamLogger::FrameStreamLogger(const std::string socket_path): socket_path(socket_path)

This comment has been minimized.

@rgacogne

rgacogne Mar 27, 2017
Member

Just a nit, but socket_path could be a reference.

{
struct sockaddr_un local;
local.sun_family = AF_UNIX;
strcpy(local.sun_path, socket_path.c_str());

This comment has been minimized.

@rgacogne

rgacogne Mar 27, 2017
Member

Please check that you do not overflow local.sun_path

local.sun_family = AF_UNIX;
strcpy(local.sun_path, socket_path.c_str());

fwopt = fstrm_writer_options_init();

This comment has been minimized.

@rgacogne

rgacogne Mar 27, 2017
Member

It looks like it can fail and return nullptr, please add a check for that case

// Frame successfully queued.
break;
} else if (res == fstrm_res_again) {
// Queue is full.

This comment has been minimized.

@rgacogne

rgacogne Mar 27, 2017
Member

queueData() is busy-looping for as long as the queue is full, effectively blocking queries/responses processing which is not good. I think we should just give up on fstrm_res_again (don't forget to free the allocated memory then)

#include <thread>

#include "iputils.hh"

This comment has been minimized.

@rgacogne

rgacogne Mar 27, 2017
Member

The includes from <atomic> to "iputils.hh" seem unnecessary

#include "dolog.hh"
#endif

#define MAXHOSTNAMELEN 256

This comment has been minimized.

@rgacogne

rgacogne Mar 27, 2017
Member

it would be better not to redefine an existing standard macro

void DnstapProtoBufMessage::serialize(std::string& data) const
{
#ifdef HAVE_PROTOBUF
proto_message.SerializeToString(&data);

This comment has been minimized.

@rgacogne

rgacogne Mar 27, 2017
Member

You are duplicating of lot of code between DnstapProtoBufMessage and DefaultProtoBufMessage, how about creating a base class instead of an empty interface?

setUUID(uuid);

char identity[MAXHOSTNAMELEN+1];
if (gethostname(identity, MAXHOSTNAMELEN) == 0) {

This comment has been minimized.

@rgacogne

rgacogne Mar 27, 2017
Member

Please don't call gethostname() for every update() call. Either pass the host name to the constructor or compute it only once there and store it. You might even want to allow setting the identity as a parameter to newFrameStreamLogger().

{
#ifdef HAVE_PROTOBUF
std::string msg = "qname: " + qname.toString() + "; qtype: " + std::to_string(qtype) + "; qclass: " + std::to_string(qclass);
proto_message.set_extra(msg);

This comment has been minimized.

@gibson042

gibson042 Mar 30, 2017
Contributor

It seems highly opinionated to put data in extra; shouldn't this be a no-op?

@pieterlexis
Copy link
Member

@pieterlexis pieterlexis commented Jun 1, 2017

@jvalentini ping?

@jvalentini
Copy link
Author

@jvalentini jvalentini commented Jun 6, 2017

Sorry for the delay. I should be able to start looking at this now. Thanks for the feedback so far!

@johnhtodd
Copy link

@johnhtodd johnhtodd commented Jun 20, 2017

My organization has added tagging to protobuf messages so that downstream telemetry systems can get some ideas about what happened during the internal Lua processing cycle. (see: #5396) I'm happy to see dnstap abilities, and we'd probably have no problem moving to them but we would want to see the same type of tagging visible in the dnstap protobuf streams as well. Is this worth doing on the first pass or should we just wait to have this (and our patch) added, and then extend tagging afterwards to include dnstap messages?

@jvalentini jvalentini force-pushed the jvalentini:dnstap-support branch from 65f6c86 to 51d76d9 Jul 11, 2017
@pieterlexis
Copy link
Member

@pieterlexis pieterlexis commented Aug 14, 2017

ping

@rgacogne
Copy link
Member

@rgacogne rgacogne commented Nov 24, 2017

Hello @jvalentini! We are seeing quite some interest for this feature, so I was wondering if you expected to be able to work on it, or if we should perhaps consider picking up from there? Thanks!

@jvalentini
Copy link
Author

@jvalentini jvalentini commented Nov 24, 2017

@rgacogne If there's significant interest it's probably best to have someone take this over as it's been hard to find time. It's also somewhat complicated by the fact that this is my first c++ project and a lot of the feedback are things I'm not familiar with, which makes any progress slow going. Thanks!

@rgacogne
Copy link
Member

@rgacogne rgacogne commented Mar 22, 2018

This PR has been superseded by #6170, which has been merged. A huge thanks again for this awesome work!

@rgacogne rgacogne closed this Mar 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.