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

Support for identifying devices by id such as mac address #5646

Merged
merged 5 commits into from Aug 29, 2017

Conversation

Projects
None yet
4 participants
@neilcook
Contributor

neilcook commented Aug 24, 2017

Short description

This PR provides support for a deviceId field, set by Lua, and included in protobuf messages. I essentially just shadowed what was already done for requestorID.

DeviceId would typically be parsed from an EDNS option such as 65001 or the new edns0-clientid draft, and would usually be a mac address but could also be an IP 4/6 address.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code and tested it
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@aerique aerique added this to the rec-4.1.0 milestone Aug 24, 2017

@pieterlexis

This comment has been minimized.

Show comment
Hide comment
@pieterlexis

pieterlexis Aug 24, 2017

Member

Please add some words about the change in gettag here in this file. I think it can go in the same block that begins with "It can also return a table whose keys and values are strings to fill the ...."

Member

pieterlexis commented Aug 24, 2017

Please add some words about the change in gettag here in this file. I think it can go in the same block that begins with "It can also return a table whose keys and values are strings to fill the ...."

@Habbie

This comment has been minimized.

Show comment
Hide comment
@Habbie

Habbie Aug 24, 2017

Member

Please add support to https://github.com/PowerDNS/pdns/blob/master/contrib/ProtobufLogger.py as well - looks like requestorId is missing there as well.

Member

Habbie commented Aug 24, 2017

Please add support to https://github.com/PowerDNS/pdns/blob/master/contrib/ProtobufLogger.py as well - looks like requestorId is missing there as well.

@neilcook

This comment has been minimized.

Show comment
Hide comment
@neilcook
Contributor

neilcook commented Aug 24, 2017

@aerique aerique requested review from Habbie, pieterlexis and rgacogne and removed request for Habbie Aug 24, 2017

@pieterlexis

One nit, looks good to me. I assume you've tested it.

msg.id,
messageidstr,
initialrequestidstr))
deviceId = binascii.hexlify(bytearray(msg.deviceId))

This comment has been minimized.

@pieterlexis

pieterlexis Aug 28, 2017

Member

What happens if msg.deviceId is empty?

@pieterlexis

pieterlexis Aug 28, 2017

Member

What happens if msg.deviceId is empty?

This comment has been minimized.

@neilcook

neilcook Aug 29, 2017

Contributor

python protobuf returns empty python string

@neilcook

neilcook Aug 29, 2017

Contributor

python protobuf returns empty python string

@Habbie

This comment has been minimized.

Show comment
Hide comment
@Habbie

Habbie Aug 28, 2017

Member
Exception in thread Connection Handler:
Traceback (most recent call last):
  File "/usr/local/Cellar/python/2.7.13_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/threading.py", line 801, in __bootstrap_inner
    self.run()
  File "/usr/local/Cellar/python/2.7.13_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/threading.py", line 754, in run
    self.__target(*self.__args, **self.__kwargs)
  File "ProtobufLogger.py", line 30, in run
    self.printQueryMessage(msg)
  File "ProtobufLogger.py", line 43, in printQueryMessage
    self.printSummary(message, 'Query')
  File "ProtobufLogger.py", line 160, in printSummary
    initialrequestidstr))
TypeError: not enough arguments for format string

on the very first log message. This is without deviceid in the message.

Member

Habbie commented Aug 28, 2017

Exception in thread Connection Handler:
Traceback (most recent call last):
  File "/usr/local/Cellar/python/2.7.13_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/threading.py", line 801, in __bootstrap_inner
    self.run()
  File "/usr/local/Cellar/python/2.7.13_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/threading.py", line 754, in run
    self.__target(*self.__args, **self.__kwargs)
  File "ProtobufLogger.py", line 30, in run
    self.printQueryMessage(msg)
  File "ProtobufLogger.py", line 43, in printQueryMessage
    self.printSummary(message, 'Query')
  File "ProtobufLogger.py", line 160, in printSummary
    initialrequestidstr))
TypeError: not enough arguments for format string

on the very first log message. This is without deviceid in the message.

@neilcook

This comment has been minimized.

Show comment
Hide comment
@neilcook

neilcook Aug 29, 2017

Contributor

These changes are now tested:

Without deviceId:

[2017-08-29 11:16:45.855169] Query of size 51: 127.0.0.1 -> 127.0.0.1 (UDP), id: 35564, uuid: 407f62d12a484a3191ce3ded52b85fdf requestorid:  deviceid: 
[2017-08-29 11:16:45.855198] Response of size 55: 127.0.0.1 -> 127.0.0.1 (UDP), id: 35564, uuid: 407f62d12a484a3191ce3ded52b85fdf requestorid:  deviceid: 

With deviceId (and requestorId):

[2017-08-29 10:59:25.878745] Query of size 51: 127.0.0.1 -> 127.0.0.1 (UDP), id: 12963, uuid: 55c71a178a2d4f49a1cbecb5756dac5a requestorid: pietje deviceid: 414243444546
[2017-08-29 10:59:25.878774] Response of size 55: 127.0.0.1 -> 127.0.0.1 (UDP), id: 12963, uuid: 55c71a178a2d4f49a1cbecb5756dac5a requestorid: pietje deviceid: 414243444546
Contributor

neilcook commented Aug 29, 2017

These changes are now tested:

Without deviceId:

[2017-08-29 11:16:45.855169] Query of size 51: 127.0.0.1 -> 127.0.0.1 (UDP), id: 35564, uuid: 407f62d12a484a3191ce3ded52b85fdf requestorid:  deviceid: 
[2017-08-29 11:16:45.855198] Response of size 55: 127.0.0.1 -> 127.0.0.1 (UDP), id: 35564, uuid: 407f62d12a484a3191ce3ded52b85fdf requestorid:  deviceid: 

With deviceId (and requestorId):

[2017-08-29 10:59:25.878745] Query of size 51: 127.0.0.1 -> 127.0.0.1 (UDP), id: 12963, uuid: 55c71a178a2d4f49a1cbecb5756dac5a requestorid: pietje deviceid: 414243444546
[2017-08-29 10:59:25.878774] Response of size 55: 127.0.0.1 -> 127.0.0.1 (UDP), id: 12963, uuid: 55c71a178a2d4f49a1cbecb5756dac5a requestorid: pietje deviceid: 414243444546
@Habbie

Habbie approved these changes Aug 29, 2017

@neilcook

This comment has been minimized.

Show comment
Hide comment
@neilcook

neilcook Aug 29, 2017

Contributor

JFYI I don't have permission to merge to pdns

Contributor

neilcook commented Aug 29, 2017

JFYI I don't have permission to merge to pdns

@Habbie Habbie merged commit b6113e6 into PowerDNS:master Aug 29, 2017

1 check passed

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