Small fix to ddns update and unit test to verify. #73

nputnam · 2026-01-25T18:06:06Z

Patch for a regression introduced in the previous ddns commit.

To be more thorough in the testing I setup a lab with latest Kea/Bind for a Debian 13. I then exercised the setup to verify it worked as expected, DDNS updates were added/delete/updated as expected. From there I did a packet capture and compared with what Dora was sending. This commit has the fix for the regression and a unit test to validate. I replayed the Kea/Bind9 workflows with Dora acting as the DHCP server and everything worked as expected.

coveralls · 2026-01-25T18:11:25Z

coverage: 72.673% (-0.07%) from 72.745%
when pulling d54cece on nputnam:ddns_small_fix
into ad2c8c3 on bluecatengineering:master.

leshow · 2026-01-25T22:59:37Z

Thank you for your work testing and fixing the issue!

leshow · 2026-01-25T23:04:48Z

libs/ddns/src/update.rs

    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);


You said this was a regression, was this previously add_prerequisite and your PR changed to add_update?

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

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_answer which 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.

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

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

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);

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.

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

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);

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.

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:

// Prerequisite 1: name exists let mut prerequisite = Record::update0(name.clone(), 0, RecordType::ANY); prerequisite.set_dns_class(DNSClass::ANY); message.add_prerequisite(prerequisite); // wrong ... // Prerequisite 2: DHCID matches let dhcid_record = Record::from_rdata(...); message.add_prerequisite(dhcid_record); // wrong ... // Update: add A record let a_record = Record::from_rdata(name, ttl, A(leased).into_rdata()); message.add_update(a_record);

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)

leshow · 2026-01-25T23:13:06Z

Just for fun I asked copilot to review

Copilot

Pull request overview

This PR fixes a regression in DDNS (Dynamic DNS) updates where a prerequisite record was incorrectly added to the update section instead of the prerequisite section. The fix ensures DDNS UPDATE messages conform to RFC 2136 and work correctly with Kea/Bind9.

Changes:

Fixed prerequisite record placement in the update() function by changing add_update() to add_pre_requisite()
Added comprehensive unit test to validate the fix and prevent future regressions
Reorganized test module imports to support the new test

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

libs/ddns/src/update.rs

Copilot · 2026-01-25T23:16:11Z

libs/ddns/src/update.rs

+        //let from_kea = BASE64_STANDARD.decode("wbYoAAABAAEAAgABA2xhYgAABgABCG91dHJpZGVywAwA/wD+AAAAAAAAwBUAAQABAAAEsAAECkVFNMAVADEAAQAABLAAIwABAasTowCr6hY874Nyfq0krHOxnvk5GwMgYIi6N1UY6lTRCGtlYS1iaW5kAAD6AP8AAAAAAD0LaG1hYy1zaGEyNTYAAABpbnyOASwAIB0XNv7B7IFpFMfsWXNH4jrSjqApS61geEUuVlin/bPBMsUAAAAA").unwrap();
+        //let message = Message::from_vec(from_kea.as_slice());
+        //println!("{:#?}", message);


Consider removing the commented-out code. If this reference code is valuable for future debugging or documentation, it should either be moved to a comment explaining the test's origin, or removed entirely to keep the codebase clean.

Suggested change

//let from_kea = BASE64_STANDARD.decode("wbYoAAABAAEAAgABA2xhYgAABgABCG91dHJpZGVywAwA/wD+AAAAAAAAwBUAAQABAAAEsAAECkVFNMAVADEAAQAABLAAIwABAasTowCr6hY874Nyfq0krHOxnvk5GwMgYIi6N1UY6lTRCGtlYS1iaW5kAAD6AP8AAAAAAD0LaG1hYy1zaGEyNTYAAABpbnyOASwAIB0XNv7B7IFpFMfsWXNH4jrSjqApS61geEUuVlin/bPBMsUAAAAA").unwrap();

//let message = Message::from_vec(from_kea.as_slice());

//println!("{:#?}", message);

Is there value in keeping this and doing an assert or just deleting the comment?

I think it's a useful reference but that is loosely held. No issue to delete it.

libs/ddns/src/update.rs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

nputnam · 2026-01-26T00:56:25Z

Just for fun I asked copilot to review

Neat. I'll fix the unnecessary clone but I'm not sure what to do about the commenting out of the packet I used for testing. Open to any better suggestion than leaving it.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

leshow · 2026-01-26T01:51:59Z

I got copilot to fix the clone, had some issues with cargo fmt but that's resolved. I'm going to merge as-is. If we can use the kea output in a test assert_eq! that would be cool but I'll leave it for now.

Thanks again for your effort

nputnam · 2026-01-26T01:57:20Z

Thank you! I'll come up with a plan for adding some options to the ddns config, does creating an issue here work?

leshow · 2026-01-26T04:32:40Z

sure, create an issue with a proposal and I'll have a look. Note: I reverted the update_present as well based on my reading of the RFC:

The updater next attempts to confirm that the FQDN is not being used
by some other client by preparing an UPDATE request in which there
are two prerequisites. The first prerequisite is that the FQDN
exists. The second is that the desired FQDN has attached to it a
DHCID RR whose contents match the client identity.

https://datatracker.ietf.org/doc/html/rfc4703#section-5.3.2

Small fix to ddns update and unit test to verify.

5012f15

rustfmt

02b2590

leshow reviewed Jan 25, 2026

View reviewed changes

leshow requested a review from Copilot January 25, 2026 23:12

Copilot started reviewing on behalf of leshow January 25, 2026 23:13 View session

Copilot AI reviewed Jan 25, 2026

View reviewed changes

Update libs/ddns/src/update.rs

c4f2e87

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

leshow and others added 5 commits January 25, 2026 20:06

Update libs/ddns/src/update.rs

7771c29

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

update_present should use add_pre_requisite

ea6eea9

Reorder imports in update.rs test module

49cc18d

Change update to prerequisite for dhcid_record

4970521

cargo fmt

d54cece

leshow merged commit edd9fde into bluecatengineering:master Jan 26, 2026
6 checks passed

	//let from_kea = BASE64_STANDARD.decode("wbYoAAABAAEAAgABA2xhYgAABgABCG91dHJpZGVywAwA/wD+AAAAAAAAwBUAAQABAAAEsAAECkVFNMAVADEAAQAABLAAIwABAasTowCr6hY874Nyfq0krHOxnvk5GwMgYIi6N1UY6lTRCGtlYS1iaW5kAAD6AP8AAAAAAD0LaG1hYy1zaGEyNTYAAABpbnyOASwAIB0XNv7B7IFpFMfsWXNH4jrSjqApS61geEUuVlin/bPBMsUAAAAA").unwrap();
	//let message = Message::from_vec(from_kea.as_slice());
	//println!("{:#?}", message);

Small fix to ddns update and unit test to verify. #73

Small fix to ddns update and unit test to verify. #73

Uh oh!

Conversation

nputnam commented Jan 25, 2026

Uh oh!

coveralls commented Jan 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leshow commented Jan 25, 2026

Uh oh!

leshow Jan 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leshow Jan 25, 2026

Choose a reason for hiding this comment

Uh oh!

nputnam Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

leshow Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leshow Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

nputnam Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

nputnam Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

leshow Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leshow Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leshow commented Jan 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

Uh oh!

leshow Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

nputnam Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nputnam commented Jan 26, 2026

Uh oh!

leshow commented Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

nputnam commented Jan 26, 2026

Uh oh!

leshow commented Jan 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Jan 25, 2026 •

edited

Loading

leshow Jan 25, 2026 •

edited

Loading

leshow Jan 26, 2026 •

edited

Loading

leshow Jan 26, 2026 •

edited

Loading

leshow Jan 26, 2026 •

edited

Loading

leshow commented Jan 26, 2026 •

edited

Loading