ddns-scripts: fix IPv6 updates to IPv4-only DDNS providers #28063
Conversation
roganlynch
force-pushed
the
fix-ipv6-to-ipv4-only-ddns-provider
branch
from
ce1a78b to
5576d64
Compare
|
@BKPepe Looking for what it'd take to get this merged. LMK Small Fix - DUCKDNS only has v4 addresses - but supports v6 AAAA records Logic was added to the existing script redundantly as opposed to adding a new function - which seemed in keeping with the existing code style. LMK if there are more hoops to get through. |
|
@feckert would you mind to review this? :) |
GeorgeSapkin
changed the title
|
This needs |
|
@GeorgeSapkin - Done. |
|
OK, but now you need to squash these commits as the second one is not following the guidelines. |
roganlynch
force-pushed
the
fix-ipv6-to-ipv4-only-ddns-provider
branch
from
262e29b to
d7408ad
Compare
|
Thanks for the hand holding. I've combined the commits and added the sign-off. LMK if there's still more administratrivia that needs addressing. @GeorgeSapkin |
|
Well, your combined commit still needs to follow the commit guidelines regarding the commit subject format and the message. It needs be something like And you need to specify how you run-tested this in the PR description. |
roganlynch
force-pushed
the
fix-ipv6-to-ipv4-only-ddns-provider
branch
from
d7408ad to
20b0a6c
Compare
|
@GeorgeSapkin - Okay - "Once More With Feeling"™ LMK & Thanks |
|
I gave the changes a brief look and I feel like this could use a shellcheck. |
Indeed.. the whole thing is a shellcheck nightmare.... This is "in keeping with existing code style" -- ducks & runs I can resubmit with sc2806 quotes in place. Not an issue. LMK |
|
I guess it's a bit out of scope for this PR. If you feel like it you could split this into a shellcheck fixing commit and your changes. |
| if [ $use_ipv6 -eq 0 ]; then | ||
| __PROG="$__PROG -4" | ||
| elif [ $__DNS_HAS_AAAA -eq 1 ]; then | ||
| __PROG="$__PROG -6" # only force IPv6 if host supports it | ||
| else | ||
| __PROG="$__PROG -4" # fallback to IPv4 if host doesn't support IPv6 | ||
| fi |
There was a problem hiding this comment.
And the indentation is really off:
| if [ $use_ipv6 -eq 0 ]; then | |
| __PROG="$__PROG -4" | |
| elif [ $__DNS_HAS_AAAA -eq 1 ]; then | |
| __PROG="$__PROG -6" # only force IPv6 if host supports it | |
| else | |
| __PROG="$__PROG -4" # fallback to IPv4 if host doesn't support IPv6 | |
| fi | |
| if [ $use_ipv6 -eq 0 ]; then | |
| __PROG="$__PROG -4" | |
| elif [ $__DNS_HAS_AAAA -eq 1 ]; then | |
| __PROG="$__PROG -6" # only force IPv6 if host supports it | |
| else | |
| __PROG="$__PROG -4" # fallback to IPv4 if host doesn't support IPv6 | |
| fi |
There was a problem hiding this comment.
Looks like I am off by one indentation level on that conditional block in all three client cases... I will rebase and force it up...
| [ -z "$bind_network" ] && [ "$ip_source" = "network" ] && [ "$ip_network" ] && bind_network="$ip_network" | ||
|
|
||
| # Check if URL host supports IPv6 when use_ipv6 is enabled | ||
| <<<<<<< Updated upstream |
There was a problem hiding this comment.
Looks like something didn't merge properly.
There was a problem hiding this comment.
Yeah - something ate poop... I have git reset and repushed and I think this version is clean
There was a problem hiding this comment.
This git thing reminds me of democracy it's the worst except for all the other things....
roganlynch
force-pushed
the
fix-ipv6-to-ipv4-only-ddns-provider
branch
from
2da4575 to
53fa02b
Compare
|
Probably ought to test this with forced_ipversion=0/1 on all three clients (wget/curl/uclient_fetch) with all the combinations of v4,v6,dualstack clients and servers. However that's beyond my current level of commitment without more automation at my disposal. |
Smoke/Fix Testing Performed ========================== Tested on DuckDNS (only has A records) with personal account Ran with '-x' and with '-v 3' '-v 2' '-v 1' and without flags (happy path for this issue) Dual stack Client -> Single Stack Server (v4) Theory/Assumptions Behind Change: ================================ Making curl use v6 when its target host doesn't support it is DUMB Naively Everyone has a v4 address (client & server) - not true, but true-ish enough (maybe?) Testing Needed: ============== Dual Stack Client -> Dual stack Server Dual Stack Client -> Single Stack Server (v6) Single Stack Client (v4) -> Single Stack Server (v4) Single Stack Client (v4) -> Dual Stack Server Single Stack Client (v6) -> Single Stack Server (v6) Single Stack Client (v6) -> Dual Stack Server Signed-off-by: Rogan Lynch <[email protected]>
roganlynch
force-pushed
the
fix-ipv6-to-ipv4-only-ddns-provider
branch
from
53fa02b to
a08d5a5
Compare
3 participants
This PR fixes issues with DDNS scripts when sending IPv6 address updates to IPv4-only providers.
Current behavior - do_transfer() attempts to connect to provider url without first validating it has an address in that protocol family (leaping before looking) - this fails for DUCKDNS.org as it's only got an A Record
This adds a check to ensure that if v6 updates are requested, that before attempting to connect to a URL some target validation is performed (does it have a AAAA record) or it falls back to using v4 to perform the transfer.
It only enforces this logic when
force_ipversionis set, but does force '-6' when bothuse_ipv6and__DNS_HAS_AAAAare true. we may want to expand this if the behavior is actually desirable to explicitly prefer v4/v6 if we have both.