ulygit / asus_rt_ac68u

Configuration and script for Cloudflare DDNS on Asuswrt-Merlin
MIT License
58 stars 24 forks source link

Curl header formatting and last_run_time logic #19

Closed scenix007 closed 2 years ago

scenix007 commented 2 years ago

Curl header formatting and last_run_time logic

Tested Env

RT-AX86U , Asuswrt-Merlin version: 386.3 Curl info:

# curl -V
curl 7.76.1 (arm-buildroot-linux-gnueabi) libcurl/7.76.1 OpenSSL/1.1.1k
Release-Date: 2021-04-14
Protocols: file ftp ftps http https imap imaps mqtt pop3 pop3s smb smbs smtp smtps
Features: alt-svc IPv6 Largefile NTLM SSL TLS-SRP UnixSockets
ulygit commented 2 years ago

I've taken a quick look at your changes, and I have the following concerns.

  1. I'm unable to reproduce any error. I'm running latest Merlin @ 386.3_2 and curl @ 7.76.1. Can you provide the error output you were seeing?
  2. Your changes to the headers are clearly preferable if they work. I'm concerned they won't work for the legacy email+key authentication method. Have you tested with legacy authentication? If support for this authentication has been dropped, I'm happy to adopt your changes. If I recall correctly, that was the reason for using the here-doc.
  3. I haven't taken a very close look at your last_run_time logic, but I don't see where it handles THROTTLED log entries, which should be ignored. I will take a closer look this weekend.

Thanks for your effort here, but I have to be cautious about changes as this can affect the network infrastructure of other users.

ulygit commented 2 years ago

I should add I'm NOT running on RT-AX86U, so it could be something specific to that router. If other RT-AX86U users can confirm there is a bug here, please let me know. Also let me know if there's no bug.

scenix007 commented 2 years ago

Thank you for your reply.

1 . The error message is only one line both when I run ./cloudflare_ddns list and ./cloudflare_ddns 1.1.1.1:

line 119: syntax error: unexpected end of file (expecting "}")`

After switching to the -H formatting, the error disappeared and the script worked (except for the threshold part).

  1. I checked with the legacy email+key authentication method, the curl worked fine. Strange thing is, I have to comment below 3 lines to get it working. Or the CLOUDFLARE_AUTH_HEADERS always gets an empty string no matter how I remove the API_TOKEN from the config file.
#if [ -n $CLOUDFLARE_API_TOKEN ]; then
#    CLOUDFLARE_AUTH_HEADERS="Authorization: Bearer $CLOUDFLARE_API_TOKEN"
#fi

Somehow the -n phrase always returns true in my case...

  1. I just prefer the awk and data command way of handling text files, it is more readable compared to the grep way.

Also, there's no need for you to consider accepting my pull request, I just find it is easy to express my idea through a pull request, feel free to reject it if you want.

ulygit commented 2 years ago

I've taken a second look at this. I appreciate your work on improving the script, but I'm not able to reproduce the error you described, so I can't justify modifying the script. No one else has complained of a similar error, either. I do like that you avoid the nested command substitutions in the last_run_time code, but it's too small an improvement in readability for the risk of impacting the throttling code. This is especially true since your change appears to have ignored THROTTLED log entries.

Rejecting this pull for now, but I'll keep a look out for problems along the lines you described. Thanks.