-
Notifications
You must be signed in to change notification settings - Fork 21
Small fix to ddns update and unit test to verify. #73
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
Changes from all commits
5012f15
02b2590
c4f2e87
7771c29
ea6eea9
49cc18d
4970521
d54cece
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -125,7 +125,7 @@ pub fn update( | |||||||
|
|
||||||||
| let mut prerequisite = Record::update0(name.clone(), 0, RecordType::ANY); | ||||||||
| prerequisite.set_dns_class(DNSClass::NONE); | ||||||||
| message.add_update(prerequisite); | ||||||||
| message.add_pre_requisite(prerequisite); | ||||||||
|
|
||||||||
| let a_record = Record::from_rdata(name.clone(), ttl, A(leased).into_rdata()); | ||||||||
| let dhcid_record = Record::from_rdata( | ||||||||
|
|
@@ -161,7 +161,7 @@ pub fn update_present( | |||||||
| let mut prerequisite = Record::update0(name.clone(), 0, RecordType::ANY); | ||||||||
| // use ANY to check only update if this name is present | ||||||||
| prerequisite.set_dns_class(DNSClass::ANY); | ||||||||
| message.add_update(prerequisite); | ||||||||
| message.add_pre_requisite(prerequisite); | ||||||||
|
|
||||||||
| // add dhcid to prereqs, will only update if dhcid is present | ||||||||
| let dhcid_record = Record::from_rdata( | ||||||||
|
|
@@ -172,7 +172,7 @@ pub fn update_present( | |||||||
| rdata: NULL::with(duid.rdata(&name)?), | ||||||||
| }, | ||||||||
| ); | ||||||||
| message.add_update(dhcid_record); | ||||||||
| message.add_pre_requisite(dhcid_record); | ||||||||
|
|
||||||||
| let a_record: Record = Record::from_rdata(name, ttl, A(leased).into_rdata()); | ||||||||
| message.add_update(a_record); | ||||||||
|
|
@@ -296,9 +296,15 @@ pub enum UpdateError { | |||||||
|
|
||||||||
| #[cfg(test)] | ||||||||
| mod test { | ||||||||
| use super::*; | ||||||||
| use crate::dhcid::IdType; | ||||||||
| use dora_core::hickory_proto::op::MessageType::Query; | ||||||||
| use dora_core::hickory_proto::op::{OpCode, UpdateMessage}; | ||||||||
| use dora_core::hickory_proto::rr::DNSClass::IN; | ||||||||
| use dora_core::hickory_proto::rr::rdata::NULL; | ||||||||
| use dora_core::hickory_proto::rr::{RData, Record}; | ||||||||
| use std::net::Ipv6Addr; | ||||||||
|
|
||||||||
| use super::*; | ||||||||
| #[test] | ||||||||
| fn test_rev_ip() { | ||||||||
| assert_eq!( | ||||||||
|
|
@@ -317,4 +323,62 @@ mod test { | |||||||
| "1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.f.2.1.b.0.a.0.8.b.d.0.1.0.0.2.ip6.arpa." | ||||||||
| ) | ||||||||
| } | ||||||||
|
|
||||||||
| #[test] | ||||||||
| fn test_update_message() { | ||||||||
| // Message captured from a running Kea/Bind9 DDNS update (for reference). | ||||||||
| //let from_kea = BASE64_STANDARD.decode("wbYoAAABAAEAAgABA2xhYgAABgABCG91dHJpZGVywAwA/wD+AAAAAAAAwBUAAQABAAAEsAAECkVFNMAVADEAAQAABLAAIwABAasTowCr6hY874Nyfq0krHOxnvk5GwMgYIi6N1UY6lTRCGtlYS1iaW5kAAD6AP8AAAAAAD0LaG1hYy1zaGEyNTYAAABpbnyOASwAIB0XNv7B7IFpFMfsWXNH4jrSjqApS61geEUuVlin/bPBMsUAAAAA").unwrap(); | ||||||||
| //let message = Message::from_vec(from_kea.as_slice()); | ||||||||
| //println!("{:#?}", message); | ||||||||
|
Comment on lines
+330
to
+332
|
||||||||
| //let from_kea = BASE64_STANDARD.decode("wbYoAAABAAEAAgABA2xhYgAABgABCG91dHJpZGVywAwA/wD+AAAAAAAAwBUAAQABAAAEsAAECkVFNMAVADEAAQAABLAAIwABAasTowCr6hY874Nyfq0krHOxnvk5GwMgYIi6N1UY6lTRCGtlYS1iaW5kAAD6AP8AAAAAAD0LaG1hYy1zaGEyNTYAAABpbnyOASwAIB0XNv7B7IFpFMfsWXNH4jrSjqApS61geEUuVlin/bPBMsUAAAAA").unwrap(); | |
| //let message = Message::from_vec(from_kea.as_slice()); | |
| //println!("{:#?}", message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there value in keeping this and doing an assert or just deleting the comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a useful reference but that is loosely held. No issue to delete it.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You said this was a regression, was this previously add_prerequisite and your PR changed to add_update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I think I understand the issue. The original code added the dhcid record to prereq, it was supposed to be update. Your first fix changed both to update but only dhcid record is meant to be in that section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, yes. I've read the RFC a couple times and I think this is the way it's should be. Originally I had this as an
add_answerwhich is the same thing as far as the bytes over the wire are concerned but this is more clear from a spec point of view.For what it's worth I swear the previous version also worked but this maybe more correct.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
afaik the prereq section is effectively the condition that's required and the update is the action that's going to happen.
It's possible it worked with both in that case. Not having any conditions on the update action. But I think the name here is supposed to be the condition.
It's possible the same is the case for dhcid, it could go in either. In one case it means only update if dhcid matches (same device is renewing). In the other case it means add the dhcid along with the a/aaaa record, so it's like making a new entry for the client?
But it's been a while since I looked at the RFC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the same change needs to be made in update_present to change the add_update back to add_prerequisite?
we've got this in update_present
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I've read the RFC parts that deal with the dhcpid resolution. What do you think about adding some more options to the configuration dns updates? I can see having forward/reverse being a selection as well as the dhcpid inclusion or not being something that's configurable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is likely. I couldn't explicitly reproduce the behavior in my test setup. I did the delete/create behavior but I didn't wait for the refresh cycle to happen to see what I think would trigger this behavior? I could try that next and create another PR for that.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://datatracker.ietf.org/doc/html/rfc4703#section-5.3.2
I'm just checking this part out. I think the correct flow for the conflict resolution (
update_present) is this:Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're interested in pursuing this, that would be great. I can revert update_present to it's previous state and then the question would be if that dhcid belongs in prereq or update in the case of a conflict. (can be done in another PR)