refactor using library crates #15

nrdxp · 2023-09-06T17:03:55Z

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.

Provides a working devshell and package binary.

Thanks to #7 which could not be directly applied due to conflict.

DNS resolution from google produced a different IP every time, which isn't what we want.

Potentially finished but cannot test due to an upstream issue with one of the endpoints: cloudflare/cloudflare-rs#198

Both the public ip crate, and the cloudflare api crate have small issues that prevent them from working properly, yet both have open PRs upstream to fix these issues, so simply patch the crates for now to get this working.

Allows for specifying the API in an intuitive way, and also automatically gives us a user help message.

Also add myself as a crate author

colemickens

Feels like a lot of (lines, complexity, etc) of Nix to add. I'm not opposed, but triggers a feeling of, "oh god, should all Rust projects need this much Nix to be a part of the ecosystem". Not a problem or blocker, just a thought I had.
If this fixes paging, can you note that in the README.

colemickens · 2023-09-08T00:01:53Z

flake.nix

+
+  inputs.std.url = "github:divnix/std/v0.24.0-1";
+  inputs.std.inputs.nixpkgs.follows = "nixpkgs";
+
+  inputs.fenix.url = "github:nix-community/fenix";
+  inputs.crane.url = "github:ipetkov/crane";
+  inputs.crane.inputs.nixpkgs.follows = "nixpkgs";
+  inputs.crane.inputs.flake-compat.follows = "";
+  inputs.crane.inputs.rust-overlay.follows = "";
+
+  inputs.std.inputs.devshell.url = "github:numtide/devshell";
+
+  inputs.nixpkgs.follows = "fenix/nixpkgs";


Mostly for my knowledge, can you fill me in on why these are all necessary?

Why fenix + crane? (I usually have used fenix to get a nightly toolchain, is that somehow needed here)?

What does std provide?

Sorry I didn't even think much about the Nix, I just used the Rust template that I made for Standard, that I am used to, but I can remove the Nix honestly. That was mostly for my own use, but it does setup a nice devshell with rust-analyzer ready to go for those who use terminal based editors.

doesn't bother me, especially if you're maintaining it :)

src/api.rs

src/main.rs

colemickens · 2023-09-08T00:09:27Z

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

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

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

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 · 2023-09-11T15:30:40Z

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 · 2023-09-13T15:14:48Z

Some questions:

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?
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:

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?

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 · 2023-09-13T16:17:40Z

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?

probably
same
yes

colemickens · 2023-09-13T16:24:39Z

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 😉.

I feel like I just signed you up for more work, but I like the answers. 👍

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

Also adds logic to remove stale DNS records.

for sanity & efficiencies sake

nrdxp · 2023-09-18T00:00:32Z

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 · 2023-09-18T02:44:18Z

this is top of my list to review tomorrow.

colemickens · 2023-09-18T14:40:49Z

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 · 2023-09-18T15:20:11Z

src/main.rs

+	let mut handles = Vec::with_capacity(cli.records.len());
+
+	for (record, zone, dns_v4, dns_v6) in records {
+		if let Some(zone) = zone {


This is mostly a nit, but:

check the opposite condition

warn if there's a record specified on CLI that doesn't have a matching Zone in CF

Right now, if the user passes a --record that doesn't exist, this silently skips it. As a user, I'd like to be warned, or errored.

(prevents some right-ward drift too)

IMO, I'd probably elide the known domain suffix check too, in this case, and let CF be the arbiter of that. I don't know if CF allows "invalid" suffixes, maybe some folks use CF public DNS for internal names?

Right now, if the user passes a --record that doesn't exist, this silently skips it. As a user, I'd like to be warned, or errored.

actually it should create a new record now, assuming at least the zone exists.

As for style in general, I have more recfactoring planned as well as time allows.

It does create the record, but if I pass --record thisdoesntmatter.thiszonedoesntexist.com then it silently exits with 0, even though there was no Zone found with a matching name. (aka, the second parameter of the tuple is None, and so this falls through)

(no matching Zone means it can't create the record, of course, but the user might want to know they've messed up)

colemickens · 2023-09-18T15:49:43Z

src/fns.rs

+		params: ListZonesParams::default(),
+	};
+	let zones = client.request(&params);
+	// HACK: make a second call since dns::Zone does not implement Clone upstream


If I'm not mistaken, I think you can avoid this by cloning the zone_id and only capturing that in the async closure. A quick test shows this builds and seems to still work:

diff --git a/src/fns.rs b/src/fns.rs index 81ff477..35e6469 100644 --- a/src/fns.rs +++ b/src/fns.rs @@ -72,19 +72,18 @@ pub async fn get_records( let params = ListZones { params: ListZonesParams::default(), }; - let zones = client.request(&params); - // HACK: make a second call since dns::Zone does not implement Clone upstream - let zones2 = client.request(&params).await?.result; + let zones = client.request(&params).await?.result; - let mut handles = Vec::with_capacity(zones2.len()); - let mut records = Vec::with_capacity(zones2.len() * 10); + let mut handles = Vec::with_capacity(zones.len()); + let mut records = Vec::with_capacity(zones.len() * 10); - for zone in zones.await?.result { + for zone in &zones { let client = client.clone(); + let zone_id = zone.id.to_string(); handles.push(tokio::spawn(async move { client .request(&ListDnsRecords { - zone_identifier: &zone.id, + zone_identifier: zone_id.to_string().as_ref(), params: ListDnsRecordsParams::default(), }) .await @@ -117,10 +116,10 @@ pub async fn get_records( .map(|r| { ( r.to_owned(), - zones2 - .iter() - .find(|z| r.contains(&z.name)) - .map(|z| z.id.to_owned()), + zones.iter().find(|z| r.contains(&z.name)).map(|z| { + log::info!("{}", z.id.to_owned()); + z.id.to_owned() + }), records .iter() .find(|rec| {

I tried a bunch of ways to try and avoid the second call. I may submit a PR upstream to the cloudflare crate to just implement clone on some of the data types eventually. At least as I have it the first and second call happen asynchronously so as to avoid the overhead as much as possible.

colemickens · 2023-09-18T15:51:40Z

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 · 2023-09-18T15:54:24Z

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

I renamed to cfdyndns-old

nrdxp added 14 commits August 30, 2023 14:56

fix: run cargo update to build with latest rust

3e5f816

feat: add Standard Nix framework integration

9cc9c6f

Provides a working devshell and package binary.

feat: use treefmt for project wide formatting

cf7c83a

chore: address clippy lints & use Rust 2021

f4ce952

fix: don't change proxy status

223b780

Thanks to #7 which could not be directly applied due to conflict.

fix: use public-ip crate to resolve current ip

4a3afb3

DNS resolution from google produced a different IP every time, which isn't what we want.

feat: refactor using cloudflare api crate

9c01b44

Potentially finished but cannot test due to an upstream issue with one of the endpoints: cloudflare/cloudflare-rs#198

fix: patch crates to fix upstream issues

4f2e2f2

Both the public ip crate, and the cloudflare api crate have small issues that prevent them from working properly, yet both have open PRs upstream to fix these issues, so simply patch the crates for now to get this working.

fix: use tokio::join to capture ips at once

cbe6f39

fix: use anyhow::bail! instead of panic!

8b4a86f

feat: use clap to set arguments

3830726

Allows for specifying the API in an intuitive way, and also automatically gives us a user help message.

feat: add a verbosity flag for easier log control

aad8553

chore: 0.0.3 -> 0.1.0

6e0dd10

Also add myself as a crate author

Merge branch 'refactor'

6ac5e97

colemickens reviewed Sep 8, 2023

View reviewed changes

feat: refactor to make all requests async

25065bc

nrdxp added 2 commits September 16, 2023 18:15

chore: refactor code

a4ba350

Also adds logic to remove stale DNS records.

fix: remove duplicates and verify domain names

ca79a16

for sanity & efficiencies sake

nrdxp added 2 commits September 17, 2023 18:01

chore: 0.1.0 -> 0.1.1

80a365e

chore: update README.md

56c8d3b

colemickens reviewed Sep 18, 2023

View reviewed changes

colemickens merged commit a52263c into nrdxp:master Sep 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor using library crates #15

refactor using library crates #15

nrdxp commented Sep 6, 2023 •

edited

colemickens left a comment

colemickens Sep 8, 2023

nrdxp Sep 8, 2023

colemickens Sep 13, 2023

colemickens commented Sep 8, 2023

nrdxp commented Sep 11, 2023 •

edited

colemickens commented Sep 13, 2023

nrdxp commented Sep 13, 2023

colemickens commented Sep 13, 2023

nrdxp commented Sep 18, 2023 •

edited

colemickens commented Sep 18, 2023

colemickens commented Sep 18, 2023

colemickens Sep 18, 2023

colemickens Sep 18, 2023

colemickens Sep 18, 2023

nrdxp Sep 18, 2023 •

edited

colemickens Sep 18, 2023 •

edited

colemickens Sep 18, 2023 •

edited

nrdxp Sep 18, 2023 •

edited

colemickens commented Sep 18, 2023

nrdxp commented Sep 18, 2023

refactor using library crates #15

refactor using library crates #15

Conversation

nrdxp commented Sep 6, 2023 • edited

colemickens left a comment

Choose a reason for hiding this comment

colemickens Sep 8, 2023

Choose a reason for hiding this comment

nrdxp Sep 8, 2023

Choose a reason for hiding this comment

colemickens Sep 13, 2023

Choose a reason for hiding this comment

colemickens commented Sep 8, 2023

nrdxp commented Sep 11, 2023 • edited

colemickens commented Sep 13, 2023

nrdxp commented Sep 13, 2023

colemickens commented Sep 13, 2023

nrdxp commented Sep 18, 2023 • edited

colemickens commented Sep 18, 2023

colemickens commented Sep 18, 2023

colemickens Sep 18, 2023

Choose a reason for hiding this comment

colemickens Sep 18, 2023

Choose a reason for hiding this comment

colemickens Sep 18, 2023

Choose a reason for hiding this comment

nrdxp Sep 18, 2023 • edited

Choose a reason for hiding this comment

colemickens Sep 18, 2023 • edited

Choose a reason for hiding this comment

colemickens Sep 18, 2023 • edited

Choose a reason for hiding this comment

nrdxp Sep 18, 2023 • edited

Choose a reason for hiding this comment

colemickens commented Sep 18, 2023

nrdxp commented Sep 18, 2023

nrdxp commented Sep 6, 2023 •

edited

nrdxp commented Sep 11, 2023 •

edited

nrdxp commented Sep 18, 2023 •

edited

nrdxp Sep 18, 2023 •

edited

colemickens Sep 18, 2023 •

edited

colemickens Sep 18, 2023 •

edited

nrdxp Sep 18, 2023 •

edited