ddns-scripts: use ip_source as bind_network default#13183
ddns-scripts: use ip_source as bind_network default#13183neheb merged 1 commit intoopenwrt:masterfrom
Conversation
|
Needs a PKG_RELEASE bump. |
mrjoel
force-pushed
the
mrjoel/ddns-default-bindnetwork
branch
from
c2d0d1c to
408968b
Compare
|
Sorry for the delay, rebased onto current master and added the PKG_RELEASE bump. |
|
ping @feckert |
| [ $# -ne 1 ] && write_log 12 "Error in 'do_transfer()' - wrong number of parameters" | ||
|
|
||
| # Use ip_network as default for bind_network if not separately specified | ||
| [ -z "$bind_network" -a "$ip_source" = "network" -a "$ip_network" ] && bind_network="$ip_network" |
There was a problem hiding this comment.
I am aware that in the whole script -a or -o is used.
But can you please change that to && this is more POSIX comform.
[ -z "$bind_network" ] && [ "$ip_source" = "network" ] && [ "$ip_network" ] && bind_network="$ip_network"
But I think this is wrong----------------------------------------------------------------^ ?
I also do not verify if this is correct, because I do not use this.
@Ansuel Can you say something about this
There was a problem hiding this comment.
A better question would be...
@mrjoel what does this fix? Why this change?
There was a problem hiding this comment.
Found the issue... It would be good to add to the commit description
Fixes: #13182
There was a problem hiding this comment.
@feckert looks like this fix a specific problem...
bind_network is indeed a separate ddns config but is needed when ip_network is used...
So i think we can accept this when the if is fixed to follow POSIX and a "Fixed" tag is added to the commit description (Commit description is different than the pr description...)
There was a problem hiding this comment.
Updated for better POSIX idioms and added issue link to commit message.
Fixes: openwrt#13182 on https://github.com/openwrt/packages Signed-off-by: Joel Johnson <mrjoel@lixil.net>
mrjoel
force-pushed
the
mrjoel/ddns-default-bindnetwork
branch
from
408968b to
5b36293
Compare
|
Anyway i think you can remove "on https://github.com/openwrt/packages"... Github has some problem detecting the issue if there is text after the id |
Hmm, I added it to remind myself if nothing else the issue (with the |
|
Sorry for test... Try to update the pr description with |
|
Ok thx as you can see now this pr is linked with the issue... and will be auto closed on merge |
|
Hello- this change broke DDNS updates for my setup after upgrading from 19.07 to 21.02RC1. I am using curl and https to update duckdns according to a dynamic IP address assigned via PPPoE. With the code introduced by this commit, the curl command times out. If I comment out the line added in this commit, it works fine for me as it did in 19.07. The difference shows up as the mandatory addition of the curl command line option "--interface ethX.X" which leads to timeouts - if that option is removed then the curl command works fine. In my case, I want to have an empty value for bind_network. @mrjoel I think you might be able to accomplish what this commit does by setting the value of bind_interface to "wan6" in your example config. Can you please test that and report back if it works for you? Assuming that solves the issue, I would like to request reversion of this commit. I should add that I might be a bit confused about the purpose of the bind_network variable. In the wiki page https://openwrt.org/docs/guide-user/base-system/ddns it says "Network to use for communication when detecting IP and sending updates. :!: Needs Wget or cURL package to be installed ! Wget will bind to the IP and cURL to physical interface of given network." The part about it being used "when detecting IP" does not seem to match what happens in the code because it is only ever used in the function do_transfer() and is never mentioned in the function get_registered_ip() If bind_network does not affect the detection of the IP, then I recommend removing or rephrasing this information in the wiki. Also it is not included in the list of variables documented in the comments starting on line 120 in this file packages/net/ddns-scripts/files/usr/lib/ddns/dynamic_dns_updater.sh - should it be added? Dan |
|
@mrjoel Have you been able to test the alternate fix proposed above? |
|
@dbro I don't see any reference to a I'm doing a further scouring and writeup of cases and handling for each that I'll post shortly. In the meantime, can you confirm if you're using IPv6 or only IPv4 with DuckDNS? I also happen to be using DuckDNS as one of my ddns services, and as they note on their site it's a bit of an oddball for IPv6 support (all requests have to go over IPv4 with an IPv6 URL parameter value). |
|
Hi @mrjoel yes, sorry I should have written "bind_network" instead of "bind_interface". In my setup I am using only an IPv4 address with DuckDNS. Hope that helps. |
5 participants
Run tested: OpenWRT snapshot with IPv6 connectivity
Description:
Fixes: #13182