nrdxp / cfdyndns

CloudFlare Dynamic DNS Client
MIT License
12 stars 15 forks source link

refactor using library crates #15

Closed nrdxp closed 1 year ago

nrdxp commented 1 year ago

Resolves #11 Fixes #12

This is a pretty hefty refactor which uses upstream crates to query cloudflare's API and to acquire the users public IP. In addition a basic clap API has been defined to make the required options discoverable directly from the command line. In addition, this adds ipv6 handling and will set both v4 and v6 addresses if found.

Not sure if you want to merge this, or if, as you mentioned elsewhere, you just want to hand over maintainership. Either way is fine by me. Just opening this to have that discussion.

colemickens commented 1 year ago

I'm happy to give you maintainership, whether that means:

  1. redirecting the repo via GitHub mechanisms
  2. adding a huge header noting the change in authoritative repo to the README and github description
  3. adding you as a maintainer here, to colemickens/cfdyndns

Or I can merge this in a bit and keep limping along :upside_down_face:.

Let me know what you prefer, I don't have a strong preference, maybe a smidge of a note of origination if we do move the repo, but it's not really something I'm going to lose sleep over.

nrdxp commented 1 year ago

I'm okay with any of the options, although I'd probably prefer to transfer to my user if that's okay with you, as I have more work I'd like to do. Of course I'll mention the proper attributions in the README

colemickens commented 1 year ago

Some questions:

  1. Am I correct that this will silently ignore any cli.records that don't have corresponding records in cloudflare? I'm not sure whether it's better to loop over cli.records, or whether to just check that something got updated for each record?
  2. Which I guess dovetails into the other question -- what if there are both A and AAAA existing records, but we've only managed to resolve one or the other?

Maybe I'm over-thinking it, but it feels like there's a chance that users could potentially end up in a situation where there's a stale/mismatched record -- imagine I have a laptop, I've previously setup A and AAAA records, and then run cfdyndns from a network where I only have ipv4 connectivity.

It's hard to know what to do without requiring the user to be more explicit about A/AAAA records:

I guess it just depends on how comprehensive you want it to be. And I say all of this without really even remembering if the existing code adds missing records, and knowing that it doesn't have any support for AAAA.

nrdxp commented 1 year ago
  1. Am I correct that this will silently ignore any cli.records that don't have corresponding records in cloudflare? I'm not sure whether it's better to loop over cli.records, or whether to just check that something got updated for each record?

Yes, but so did the existing code. I didn't change substantially the code semantics at all, although some rethinking is likely in order at this point, and iterating over the cli.records sounds both more correct, and more efficient. Happy to implement that at this stage.

2. Which I guess dovetails into the other question -- what if there are both A and AAAA existing records, but we've only managed to resolve one or the other?

Only the IP that has been resolved on the localhost will be updated. So if I resolved ipv4 and there is an existing AAAA record, the latter will not be touched. I do see the utility in removing an existing AAAA record though, if only ipv4 is resolved, if only to ensure no stale records remain around that could potentially leave an unintended/insecure route open. To sum up:

  • should it purge AAAA records if ipv6 didn't resolve?
  • same question for ipv4/A records
  • should it then also add records if they resolve but don't already exist?
colemickens commented 1 year ago

Yes, but so did the existing code. I didn't change substantially the code semantics at all,

Ah, well, note to my older, less wise self :wink:.

I feel like I just signed you up for more work, but I like the answers. :+1:

Also, forgot to say but happy to transfer the repo, and it doesn't have to wait for the merge. Let me know.

nrdxp commented 1 year ago

If you wouldn't mind redirecting at this point to my fork via github that would be great. I implemented the suggestions from our last conversation. So any non-existing records will now be created when a public ip is resolved, and stale records which no longer resolve locally will be deleted. These features are included in the v0.1.1 tag.

colemickens commented 1 year ago

this is top of my list to review tomorrow.

colemickens commented 1 year ago

If you wouldn't mind redirecting at this point to my fork via github that would be great. I implemented the suggestions from our last conversation. So any non-existing records will now be created when a public ip is resolved, and stale records which no longer resolve locally will be deleted. These features are included in the v0.1.1 tag.

Just saw this. I was going to transfer the repo to you. Not sure what will happen if I do that if you already have a repo named cfdyndns.

I'll look over this and then, merge and see what happens if I try to transfer.

colemickens commented 1 year ago

I used this as a chance to check my own Rust knowledge. I'm happy to merge as-is.

Do you want to rename your fork so the transfer is ... easier to assume what will happen?

nrdxp commented 1 year ago

Do you want to rename your fork so the transfer is ... easier to assume what will happen?

I renamed to cfdyndns-old