Skip to content

Conversation

@gucci-on-fleek
Copy link
Collaborator

Caddy recently added ECH support, but the rfc2136 provider doesn't work properly with it. This PR modifies the rfc2136 module so that setting the ECH records works properly. I've tested these patches on my own server, and everything seems to work as expected now.

The two main changes that I had to make were:

  1. Adding support for the upstream libdns Priority, Weight, and Target struct fields.

  2. Changing the semantics of SetRecords and AppendRecords to align with those discussed in Clarify semantics for SetRecords, GetRecords and Close libdns#145.

I don't have much Go experience, so please let me know if you want me to make any changes. Thanks!

@KoHcoJlb
Copy link
Collaborator

KoHcoJlb commented Mar 9, 2025

Hello, everything LGTM.
One thing about SetRecords though, I believe we can just let DNS server delete respective RRsets for us (2.5.2).

@gucci-on-fleek
Copy link
Collaborator Author

@KoHcoJlb

One thing about SetRecords though, I believe we can just let DNS server delete respective RRsets for us (2.5.2).

Yes, that's definitely a better idea than what I have now. I'll push a fix for that tomorrow. Thanks!

@gucci-on-fleek
Copy link
Collaborator Author

Ok, I've changed SetRecords so that it now uses msg.RemoveRRset() instead of looping over each record and calling msg.Remove(). I've tested the latest commit with the new Caddy ECH support and it works there, but I haven't done any more extensive testing.

Also, I'm already hosting an AXFR testing server for DNSControl, so if you want to add any automated tests, just let me know, and I can easily add another zone to my server.

@KoHcoJlb
Copy link
Collaborator

Can you extract removes into separate loop, otherwise we may be removing previous inserts when records are not sorted by RRsets.
Also, please include these changes into your original commit.

Thanks for the offer, but I do not have time / see particular need for them at the moment.

@gucci-on-fleek
Copy link
Collaborator Author

Can you extract removes into separate loop, otherwise we may be removing previous inserts when records are not sorted by RRsets.

Good catch, done.

Also, please include these changes into your original commit.

Also done.

Also, I'm already hosting an AXFR testing server for DNSControl, so if you want to add any automated tests, just let me know, and I can easily add another zone to my server.

Thanks for the offer, but I do not have time / see particular need for them at the moment.

👍

@KoHcoJlb
Copy link
Collaborator

One last thing I hope) It'll be better to convert records once and reuse them in both calls to remove and insert.

@gucci-on-fleek
Copy link
Collaborator Author

One last thing I hope) It'll be better to convert records once and reuse them in both calls to remove and insert.

Alright, done. Thanks again for the reviews by the way.

@KoHcoJlb
Copy link
Collaborator

Yeah, sorry for bothering you so much, I just wanted this to be implemented to the best of my knowledge, thanks for your contribution.
I will update both this lib and plugin later today.

@gucci-on-fleek
Copy link
Collaborator Author

gucci-on-fleek commented Mar 13, 2025

Yeah, sorry for bothering you so much

Oh, it's really no problem at all. I'm totally new to Go, so I appreciate the feedback.

I will update both this lib and plugin later today.

👍, thanks!

@KoHcoJlb KoHcoJlb merged commit fc4796f into libdns:master Mar 13, 2025
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