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

server outgoing signature changes #184

Merged
merged 6 commits into from
Aug 14, 2019
Merged

server outgoing signature changes #184

merged 6 commits into from
Aug 14, 2019

Conversation

hannesm
Copy link
Member

@hannesm hannesm commented Aug 7, 2019

we used to accumulate hacks and further maps and workaround just to ensure that "to each IP address, establish a single connection" -- esp. at startup of the primary (where it may want to inform a single IP about 100 zones; or when it received a signed SOA request (with a root key) for the root zone, replying with NOTIFY for all authoritative zones) and secondary (where it may want to request the SOA of a single primary for multiple zones). now, instead of having the result (ip * cstruct.t) list, using (ip * cstruct.t list) list allows the effectful layer to send all the notify/requests to that single ip address at once! (i.e. no fold and establish connections necessary :)

similarly, the secondary API got slightly revised, turns out handle_* may only ever result in one reply and optionally one other packet to the authoritative (i.e. when a notify is received, a notify_ack is replied with, and a SOA request is sent (or directly ixfr/axfr if the notify contained a signed soa)).

now, the ad-hoc tcp_packet_transit map is gone, there are retransmissions in the timer function which care exactly about that (that something is sent, but not acknowledged on the protocol level).

this cleans up #169 and #170, could get some unit tests about the above described startup and update struggle, and looks to me as the last blocker of a release.

@cfcs if you could review this as now, that'd be great. i'll surely push some more documentation and unit tests directly on this PR before merging, think a bit more, and likely tag a release (unless you find blockers -- i guess #165 would be good to have fixed as well, I'll propose in a separate PR in a few minutes).

@hannesm hannesm requested a review from cfcs August 7, 2019 21:09
Copy link
Contributor

@cfcs cfcs left a comment

Choose a reason for hiding this comment

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

Looks good to me.

- handle_buf/handle_packet return type is packet option
- closed return type is cstruct.t list
- timer return type is (ip * cstruct.t list) list
…imer now returns (ip * packet list) tuples, this behaves much better)
- avoid Int64.add and their overflows (use In64.sub instead)
- preserve the <now> timestamp, instead of the old <ts> value
- reduce retransmission of SOA to 3 seconds instead of 5 during startup
  (and add a TODO item for exponential backoff)
@hannesm hannesm merged commit 4485675 into mirage:master Aug 14, 2019
@hannesm hannesm deleted the notify branch August 14, 2019 19:03
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

Successfully merging this pull request may close these issues.

2 participants