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

rec: Support exporting more record types via protobuf #6708

Merged
merged 1 commit into from
Oct 4, 2018

Conversation

rgacogne
Copy link
Member

@rgacogne rgacogne commented Jun 5, 2018

Short description

Several users have reported the need to export the content of of more record types found in responses. This pull request adds support for optionally exporting the content of MX, NS, PTR, SPF, SRV and TXT record types, in addition to the existing A, AAAA and CNAME. The implementation is limited to those types because it special cases them.
An other option would be to implement all xfr*() methods to be able to get the "wire version" of all types. If we decided to go that road, I think it might make sense to extract the xfr*()methods from DNSPacketWriter into a DNSRecordWriter implementing all xfr*() methods, that would be called by DNSPacketWriter but could also be used from the protobuf logger.

This PR is based on #6698 and will need a rebase after it has been merged.

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)

@chbruyand
Copy link
Member

chbruyand commented Jul 27, 2018

I'd vote in favor of the DNSRecordWriter.

As far as I understand, your current implementation is not really currently limited to exporting the types listed in your comment as you check known types against QType::names.

@rgacogne
Copy link
Member Author

As far as I understand, your current implementation is not really currently limited to exporting the types listed in your comment as you check known types against QType::names.

It is, because RecProtoBufMessage::addRR() only knows how to deal with those types.

@rgacogne
Copy link
Member Author

I'd vote in favor of the DNSRecordWriter.

It would be nicer, yes, but would also be quite more work. My current feeling is to merge this pull request and see if the demand for exporting other types really exists before considering the DNSRecordWriter option.

@rgacogne rgacogne changed the title [WIP] rec: Support exporting more record types via protobuf rec: Support exporting more record types via protobuf Aug 6, 2018
@rgacogne
Copy link
Member Author

rgacogne commented Aug 6, 2018

Rebased since #6698 has been merged in the meantime.

@rgacogne rgacogne merged commit 39afd63 into PowerDNS:master Oct 4, 2018
@rgacogne rgacogne deleted the rec-pb-types branch October 4, 2018 08:16
rgacogne added a commit to rgacogne/pdns that referenced this pull request Oct 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants