-
Notifications
You must be signed in to change notification settings - Fork 333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Double sending webhooks #50
Conversation
Proposed fix for double sending webhooks even if IP Address did not change. What it does is it saves the ip address on a file ".cf-wan_ip_<name of record>.txt" and fetches this text file every update making sure that it is comparing a valid IP address.
@psycho-neon |
Are you using global or per site token? If its global, it should work "test.com" but if per site token, should be empty string (""). I am using global token tho, i will try to test a per-site/zone token. |
Nice catch! So the problem was originated from the IP verifying step all along? |
Global token. |
You can try putting "@" instead of "test.com" for the zone apex. I've checked the cf api documentation and it might work. I don't see any reason on the code to why it doesn't catches the record tho. |
Is there some sort of rate-limit implemented in your pull, it worked fine then stopped submitting requests after 3-5+ updates. I have mine on a CRON job running every minute and I did some trial and error by setting the IP on Cloudflare to 8.8.8.8 then it stopped working suddenly. |
Cloudflare logs are also empty, not update via API on the audit log. Something is stopping the request going through but I'm not sure what it is since there's not much info to go off. @psycho-neon |
there is no rate limit if i remember correctly. though you have to delete the entry |
@psycho-neon Because this appears unusual. I've been running the scripts on five different Linux machines, updating several hostnames every five minutes, and I've never had this problem before. @rossiscool123 |
@rossiscool123 |
yes i did stumble upon it a few days back. Then I sorted it out just by caching the actual ip on a file instead of getting it from cloudflare. I do not understand it either why it is happening. Though when I use my custom python script, it isn't doing this double/resending of webhooks. Could be a bad http request or timeout. But I like bash more, its native, so stayed on this script. |
@MeiMeiLaiLao @psycho-neon |
Hi, have this fixed been tested? @psycho-neon |
Yes, I've tested this myself and using it daily. Never fails. @rossiscool123 can confirm also. He is using it I believe. |
_this is a repost because i forget to censor my ip lmao_Alright, thanks for the insights from @rossiscool123, and @psycho-neon. I did some digging and I think we have a more throughout understanding of this mysterious bug now. As mentioned by @psycho-neon, this bug is most likely triggered by HTTP timeout, and I have the info to back up that. It's important to take note of these points in mind first:
[[ ! $ip =~ ^$ipv4_regex$ ]]
Now, I think what happened is most likely this:
Below shown is the script's logs after trying to get the I suggest we implement multiple fixes for this:
# Use regex to check for proper IPv4 format.
if [[ ! $ip =~ ^$ipv4_regex$ ]]; then
logger -s "DDNS Updater: Failed to find a valid IP."
exit 2
fi
update=curl --write-out "%{http_code}" <...>
update_code=$(echo $update | tail -c 4)
# If else logic to match client and server side HTTP code
<...> TL: DR; The webhook message duplication bug is most likely caused by a client-side network error. The network outage occurred somewhere before or while the script is getting the detailed info of a DNS record. If Again, this may or may not be the exact problem but it should give us an idea of what happened. |
Also, we can use infinite for-loop to check the dns record constantly until it returns a valid one. |
@Sailboat265 Great insights on this strange bug that existed for so long. I think we can proceed with a patch and see if it fixes the problem. @psycho-neon Having an infinite loop might cause bottle neck or issues down the line. I think we can look into other means to check dns validity. |
@Sailboat265 your wrong af. The insights given are not close even to actual. Issue was the request dropped, just so simple. @psycho-neon why have you decided to save it to a file? Why the hassle? Best use my fix #51 simple quick nice |
Lol, he did mention about the dropped request. The f you talking about? Doubling the request doesn't fix the issue. Your fix doesn't make any sense at all. It can still drop at any point. |
@psycho-neon failed curl request doesnot equal to request dropped. They is not the same thing My fix use send two http request so if the first one failed then i got second one still ready. Ez fix |
@psycho-neon they wont drop together not a chance. So it the best solution @K0p1-Git please review and merge asap |
@MohairVomNipa Sure thing there, smart guy. Why don't you explain to us, in details, what caused the "request dropped" error huh? |
It seems like this change to store the ip address in a file is over complicating the entire script. This should be simple and just fetch the ip address and update it Cloudflare. If you are hitting max attempts maybe fix your cron to not flood it so much? Fix the problem not apply a band-aid. |
Well in that case, we still can use regex to verify and make sure the extracted IPs are all valid before continuing to the next step. It's worth mentioning that I don't think this issue is related to API rate limiting. Cloudflare's Global API rate limit is 1200 requests per five minutes. It will also return a HTTP 429 if we ever actually cross that limit. However, during some testings, I have never seen HTTP 429 being returned. Even up until now, I have only got 000 return code from curl. Additionally, here are some information you should know:
|
@psycho-neon put me finger bottom emijo haha my code better than you you cry. Imagine losing to 13 yo lmao @ndrone dude i agree with u so much storing to a file is too small pepe and brain. U go see my code,I send request again if first fail. More economic efficiency and nice code. I ran my code using ./cloudflsare.sh then it work ok. No issue |
@K0p1-Git why u so busy? Please ceck and merge my code asap. If not merge fast then got many bug out again |
Not really dude. Could you please explain to me this? What's your reason behind doubling these clauses? Next I'm going to teach you some manners:
|
@MohairVomNipa I am looking into the crux of the issue, at this point it leading towards the request being drop/failed/limited as the script uses existing tools it makes it difficult to pinpoint. However, doubling the request sent is not a valid way of fixing this issue. Like @ndrone mentioned, we are not looking for a band-aid fix. I am indeed busy, soldier first, developer second. I am grateful for those that help maintain and optimize this script that is now used by many. I will however not tolerate negative behavior, please watch yourself and be respectful when it comes to discussion. |
Sorry I have not been following this change close enough. But I believe you are probably on the right path here. We probably should be validating the curl command is successful before continuing any further. And have a regex to validate IPv4 and IPv6. |
Looking through, a good fix as of now will be:
Adding a local file for IP imo is redundant, and might be over complicating the script. |
@psycho-neon @psycho-neon @K0p1-Git i dont know why is all you so angry at me. I already solve the bug and all you keep yelling. please see and try my code u will see it work #!/bin/bash change to "bin/sh" when necessaryauth_email="" # The email used to login 'https://dash.cloudflare.com' ########################################### Check if we have a public IP########################################### exit 0 ipv4_regex='([01]?[0-9]?[0-9]|2[0-4][0-9]|25[0-5]).([01]?[0-9]?[0-9]|2[0-4][0-9]|25[0-5]).([01]?[0-9]?[0-9]|2[0-4][0-9]|25[0-5]).([01]?[0-9]?[0-9]|2[0-4][0-9]|25[0-5])' Use regex to check for proper IPv4 format.if [[ ! $ip =~ ^$ipv4_regex$ ]]; then ########################################### Check and set the proper auth header########################################### ########################################### Seek for the A record########################################### logger "DDNS Updater: Check Initiated" logger "DDNS Updater: Check Initiated" ########################################### Check if the domain has an A record########################################### if [[ $record == ""count":0" ]]; then ########################################### Get existing IP########################################### Compare if they're the sameif [[ $ip == $old_ip ]]; then old_ip=$(echo "$record" | sed -E 's/."content":"(([0-9]{1,3}.){3}[0-9]{1,3})"./\1/') Compare if they're the sameif [[ $ip == $old_ip ]]; then ########################################### Set the record identifier from result########################################### ########################################### Change the IP@Cloudflare using the API########################################### ########################################### Report the status########################################### |
i dont know why cannot text become beautiful code just now
|
@psycho-neon u nid to copy paste the code ok then save to file ok? u then run then can work fine program no error |
Dude, just stop please? If its working for you, fine. I already saw what you did and asked you questions, yet you didn't answer them. What's the point then? |
I will make a new PR consisting of the following:
If all of these criteria failed, then we will just exit the script completely and wait for the next cron sched. |
User has been reported |
Proposed fix for double sending webhooks even if IP Address did not change. What it does is it saves the ip address on a file ".cf-wan_ip_(record_name).txt" and fetches this text file every update making sure that it is comparing a valid IP address.