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.

Sign up for GitHub

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

Jump to bottom

Add the LOC record #310

Merged
merged 70 commits into from
Jul 26, 2022
Merged

Add the LOC record #310

merged 70 commits into from
Jul 26, 2022

Conversation

RyanGibb
Copy link
Contributor

@RyanGibb RyanGibb commented Apr 26, 2022 •
edited
Loading

See rfc1876

@RyanGibb
gitignore _opam and .vscode
8c4d3db
@RyanGibb
LOC RR
7f2b039
@RyanGibb
LOC parsing
8235f0e
@RyanGibb
meters parsing
6bf0d4f
@RyanGibb
LOC parsing optional args and default values
55a89b0
@RyanGibb
update
4ef2829
@RyanGibb
took a different tack making meter unit optional
c716682 
Using `meters = ... 'm'?` (making 'm' optional) makes `NUMER DOT NUMBER..` parsing ambigious and matches IPv4 addresses to meters.

Instead, handle the cases where m doesn't occur in the parser rather than the lexer.
@RyanGibb
LOC module
f26d350
@RyanGibb
removed parer TODOs
fb30519
@RyanGibb
LOC printing
111e127
@RyanGibb
added LOC encoding according to RFC 1876 Appendix A
46a756c
@RyanGibb
update
a30ec63
@RyanGibb
refactor
36b79f9
@RyanGibb
odns resolver LOC support
b027538
@RyanGibb
refactor
4cd710f
@RyanGibb
fix LOC record ID
498d58c
@RyanGibb
LOC handling
5c41301
@RyanGibb
fix LOC record encoding
824fd39
@RyanGibb
LOC record decoding and printing
f140d3d
@RyanGibb
revert .gitignore changes
1177d02
@RyanGibb
revert .gitignore changes
470d195
@RyanGibb
fix do_txt
16fc5b9
@RyanGibb
update
7869434
@RyanGibb
removed TODOs
8675777
avsm
avsm reviewed May 1, 2022
View reviewed changes
Copy link
Member

@avsm avsm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have had a first read through, and the overall approach looks good to me. Thanks for the PR @RyanGibb! Need to test it out and look through the RFC before a full approval. Some more eyes on this welcome @mirage/core

src/dns.ml Outdated Show resolved Hide resolved
src/dns.mli Outdated Show resolved Hide resolved
@hannesm
Copy link
Member

hannesm commented May 3, 2022

Thanks for your PR, would you mind to add some tests into tests/test.mlcovering both successful packet decoding and unsuccessful packet decoding? Also for encoding the packet... Take a brief look at that test module, it should be pretty straightforward to add that.

About the zone file parsing, there aren't many tests yet -- maybe add some to server.ml.

@RyanGibb
Copy link
Contributor Author

RyanGibb commented May 3, 2022

Yes, sure!

hannesm
hannesm reviewed Jul 26, 2022
View reviewed changes
src/dns.ml Show resolved Hide resolved
hannesm
hannesm reviewed Jul 26, 2022
View reviewed changes
src/dns.ml Outdated Show resolved Hide resolved
hannesm
hannesm reviewed Jul 26, 2022
View reviewed changes
src/dns.ml Outdated Show resolved Hide resolved
hannesm
hannesm reviewed Jul 26, 2022
View reviewed changes
src/dns.ml Outdated Show resolved Hide resolved
hannesm
hannesm reviewed Jul 26, 2022
View reviewed changes
src/dns.ml Outdated Show resolved Hide resolved
hannesm
hannesm reviewed Jul 26, 2022
View reviewed changes
src/dns.ml Outdated Show resolved Hide resolved
hannesm
hannesm reviewed Jul 26, 2022
View reviewed changes
src/dns.ml Outdated Show resolved Hide resolved
hannesm
hannesm reviewed Jul 26, 2022
View reviewed changes
src/dns.ml Outdated Show resolved Hide resolved
hannesm
hannesm reviewed Jul 26, 2022
View reviewed changes
src/dns.ml Outdated Show resolved Hide resolved
hannesm
hannesm reviewed Jul 26, 2022
View reviewed changes
src/dns.ml Outdated Show resolved Hide resolved
hannesm
hannesm reviewed Jul 26, 2022
View reviewed changes
src/dns.ml Outdated Show resolved Hide resolved
hannesm
hannesm reviewed Jul 26, 2022
View reviewed changes
src/dns.mli Show resolved Hide resolved
@hannesm
Copy link
Member

hannesm commented Jul 26, 2022

thank you for your contribution. I added minor comments and suggestions above. IMHO this is mostly ready for being merged.

@RyanGibb @hannesm
hannesm suggestions
099e850 
Co-authored-by: Hannes Mehnert <hannes@mehnert.org>
@RyanGibb
Copy link
Contributor Author

Thank you for the suggestions, and for all the help getting this here!

hannesm
hannesm reviewed Jul 26, 2022
View reviewed changes
src/dns.ml Outdated Show resolved Hide resolved
hannesm
hannesm reviewed Jul 26, 2022
View reviewed changes
src/dns.ml Outdated Show resolved Hide resolved
hannesm
hannesm reviewed Jul 26, 2022
View reviewed changes
src/dns.ml Outdated Show resolved Hide resolved
hannesm
hannesm reviewed Jul 26, 2022
View reviewed changes
src/dns.ml Outdated Show resolved Hide resolved
hannesm
hannesm reviewed Jul 26, 2022
View reviewed changes
src/dns.ml Outdated Show resolved Hide resolved
hannesm
hannesm reviewed Jul 26, 2022
View reviewed changes
src/dns.ml Outdated Show resolved Hide resolved
@hannesm
Copy link
Member

hannesm commented Jul 26, 2022

sorry, in #314 I decided it doesn't make much sense to define int_compare / int32_compare in dns.ml, and got rid of it.

@hannesm hannesm merged commit 3a500ee into mirage:main Jul 26, 2022
@hannesm
Copy link
Member

hannesm commented Jul 26, 2022

CI is green, I squash-merged.

@hannesm hannesm mentioned this pull request Jul 26, 2022
Merged
hannesm added a commit to hannesm/opam-repository that referenced this pull request Jul 26, 2022
@hannesm
[new release] dnssec, dns, dns-tsig, dns-stub, dns-server, dns-resolv…
2b93b6f 
…er, dns-mirage, dns-client, dns-cli and dns-certify (6.3.0)

CHANGES:

* dns-server: demote log level for various messages (mirage/ocaml-dns#309 @hannesm)
* dns-zone: add additional glue: only add if authoritative for nameserver domain
  (mirage/ocaml-dns#309 @hannesm)
* BUGFIX: dns-trie: fix lookup when delegations are present, add tests
  (mirage/ocaml-dns#309 @hannesm)
* ozone: be more explicit when showing errors (mirage/ocaml-dns#311 @psafont)
* dns: avoid polymorphic comparison (mirage/ocaml-dns#314 @hannesm, reported by @RyanGibb)
* FEATURE: dns: add LOC resource records (RFC 1876) (mirage/ocaml-dns#310 @RyanGibb)
hannesm added a commit to hannesm/ocaml-dns that referenced this pull request Feb 27, 2023
@hannesm
A failing test case since mirage#310 was merged
cc3df09
@hannesm hannesm mentioned this pull request Feb 27, 2023
Merged
hannesm added a commit that referenced this pull request Feb 27, 2023
@hannesm
Fix zone parser and lexer
5a42916 
Certain character (and character sequences) were not allowed in the domain name on the left in a zone file since the lexer created tokens that were not accepted by the parser (missing in the `keyword_or_number` rule):
- N S E W (since #310)
- -<number> (NEG_NUMBER, also since #310)
- <number>m (METERS, also since #310)
- DS (since #290)
- CAA (since the beginning of this development)
- TYPEYYY (since the beginning of this development)

Also add test cases and an explanatory comment to keep Dns_zone_parser.keyword_or_number and Dns_zone_lexer.kw_or_cs synchronised.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hannesm hannesm hannesm left review comments

@avsm avsm avsm left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RyanGibb @hannesm @avsm