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

Clean up https://github.com/adulau/pdns-qof/wiki/Additional-Fields #17

Open
djw1149 opened this issue Jan 24, 2020 · 15 comments
Open

Clean up https://github.com/adulau/pdns-qof/wiki/Additional-Fields #17

djw1149 opened this issue Jan 24, 2020 · 15 comments

Comments

@djw1149
Copy link
Contributor

djw1149 commented Jan 24, 2020

The additional fields registry at https://github.com/adulau/pdns-qof/wiki/Additional-Fields contains three fields that have long since been added to the COF draft: sensor_id, zone_time_first, and zone_time_last. I suggest that they be removed from the registry or an additional column be added to the registry table showing the draft version number or RFC number in which each additional field was added to the I-D or RFC (as/if becomes applicable).

@aaronkaplan
Copy link
Collaborator

aaronkaplan commented Jan 24, 2020 via email

@djw1149
Copy link
Contributor Author

djw1149 commented Jan 26, 2020

For the master file format, perhaps we add a reference to RFC1035 section "5.1. Format" in our 3.5.2 and 3.5.3? We already reference "DNS master file RFC 1035..." in our section "3.3.3 rdata".

For the milli/nano resolution, are you saying to add additional variations of all time based fields with the higher resolution data format? i.e. a new time_first, time_last, zone_time_first, and zone_time_last? We actually probably don't need it for zone_time_first and zone_time_last.

Perhaps:

time_first_hires

This field returns the first time that the record / unique tuple (rrname, rrtype, rdata) has been seen by the passive DNS with a higher resolution than the standard time_first field. The timestamp is expressed in milliseconds (decimal) since 1st of January 1970 00:00:00.0000 (the Unix epoch), but this is 1000 times larger than a Unix timestamp. The time zone MUST be UTC. This field is represented as a JSON [RFC4627] number. If BOTH time_first and time_first_hires are present in a record, then INTEGER(time_first_hires / 1000) MUST equal time_first, otherwise the behavior is undefined.

time_last_hires

(Similar to above: s/time_first/time_last/g)

@aaronkaplan
Copy link
Collaborator

See version 07. Good enough?

@djw1149
Copy link
Contributor Author

djw1149 commented Jul 6, 2020

Could you clarify:

if time_first_hires is present then time_first need not be present. (and the implementation is still "conforming")

if time_last_hires is present then time_last need not be present.

@aaronkaplan
Copy link
Collaborator

aaronkaplan commented Jul 6, 2020 via email

@djw1149
Copy link
Contributor Author

djw1149 commented Jul 6, 2020

Aaron, your response makes sense, but a COF object can validly have zone_time_first but not time_first.
Implementation MUST support all the mandatory fields. does not mean that all the mandatory fields MUST be present in every COF object.

@aaronkaplan
Copy link
Collaborator

aaronkaplan commented Jul 6, 2020 via email

@vixie
Copy link
Contributor

vixie commented Jul 6, 2020

i would still prefer that the _hires times be just the fraction, thus not duplicating the time_first and time_last, which should remain mandatory. if we had time_first_nano and time_last_nano it ought to be future proof. sensor_id is of course optional.

@aaronkaplan
Copy link
Collaborator

aaronkaplan commented Jul 6, 2020 via email

@bapril
Copy link
Contributor

bapril commented Jul 6, 2020

okay, hmm... but
a) AFAIK that's what came out of the FIRST pDNS SIG.
b) are we OK to keep time_first, time_last no matter what now? (i.e. no matter if _hires is fraction or not or full or nanoseconds?)

In case (b) is OK for you, I can add a sentence (thanks for pointing it out!) about the mandatory-ness of time_first and time_last (even in the presence of _hires). Idea behind it: don't break existing clients.

The pattern that is currently in service should be supported without a compelling reason to break backwards compatibility. In current implementations the records contain either a pair of time_* or a pair of zone_time_* fields. Duplicating timestamps is unnecessary. I suggest updating the mandatory field list as follows:

(time_first AND time_last) OR (zone_time_first AND zone_time_last)

@aaronkaplan
Copy link
Collaborator

aaronkaplan commented Jul 6, 2020 via email

@bapril
Copy link
Contributor

bapril commented Jul 6, 2020

Sounds like a good compromise, but ... w.r.t to backwards compatibility, I am not really sure if users of the format implement the zone_time_* fields.

Farsight's DNSDB uses this model today. We send either time_[first|last] or zone_time_[first|last], but not both.

The only fields which are mandatory are the time_first, time_last fields.
Hm... a bit of a conundrum because time_first, time_last are mandatory, zone_time_first, zone_time_last (as well as the *_hires ones) are optional. I wonder if it's better to solve this before it goes to the IETF or during that process.... What do you think?

I agree we should get this solved.
I propose restructuring the mandatory fields section such that the zone_time_[first|last] are allowed to appear in place of time_[first|last], as long as one of the pairs is always present.

@aaronkaplan
Copy link
Collaborator

Added an additional note. I would still discourage this since it's really confusing for implementers.

aaronkaplan added a commit to aaronkaplan/pdns-qof that referenced this issue Apr 27, 2021
Not elegant, but works and makes the fsio output compatible.
@aaronkaplan
Copy link
Collaborator

aaronkaplan commented Apr 27, 2021

See #21

@vixie , @djw1149 , @bapril, works for you? I'd like to push for a consolidated version and finally send to the standards track

@djw1149
Copy link
Contributor Author

djw1149 commented Apr 28, 2021

What I think is supposed to be my name is misspelled at

<author fullname="Paul Vixie, Weizman, April, Kaplan, et.al"/>
" <author fullname="Paul Vixie, Weizman, April, Kaplan, et.al"/>" should be <author fullname="Paul Vixie, Waitzman, April, Kaplan, et.al"/>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants