Skip to content
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 record update attempt with invalid IP #203

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

irfanhakim-as
Copy link

@irfanhakim-as irfanhakim-as commented Nov 26, 2024

Issues addressed

Changes

This PR is a two-parter:

Part 1

  • This is a safe change and non-disruptive in the sense that it does not change any existing behaviour other than the fact that it ensures when the endpoint that is being used to determine the public IPv4/6 address is unavailable or not returning the intended address correctly, it fails instead of returning the error response as the supposed IP address (which causes unwanted, repetitive attempts to update DNS records with this error response).

  • It fixes the linked issue only partially - it fixes the unintended attempts of updating the DNS records with error response values it thought was the IPv4/6 address, but it still is affected by downtimes or currently broken (primary/fallback) Cloudflare endpoints (https://1.1.1.1/cdn-cgi/trace and https://1.0.0.1/cdn-cgi/trace).

  • Commits: f0d9510...cbb5ead

  • Test image: ghcr.io/irfanhakim-as/cloudflare-ddns:1.0.2-fix-wrong-ip-r1

Part 2

  • Includes Part 1, is safe, but adds a few more modifications that changes an existing behaviour slightly.

  • It fixes the linked issue completely - by adding support for acquiring IPv4/6 address from single-value endpoints (i.e. https://ipv6.icanhazip.com) in addition to key-value endpoints as before (i.e. https://[2606:4700:4700::1111]/cdn-cgi/trace).

  • This includes adding 2 new global variables (ipv4_endpoints and ipv6_endpoints), which are used to specify the primary and fallback endpoints for both IP types. These global variables could potentially be used as new config options (by supplying 2-item list of primary and fallback endpoints for each IP type) in the future.

  • The previous default fallback endpoints were updated from https://1.0.0.1/cdn-cgi/trace and https://[2606:4700:4700::1001]/cdn-cgi/trace to https://ipv4.icanhazip.com and https://ipv6.icanhazip.com respectively.

  • Commits: f0d9510...cbb5ead and cbb5ead...ba37a97

  • Test image: ghcr.io/irfanhakim-as/cloudflare-ddns:1.0.2-fix-wrong-ip-r2

Notes

I'm not sure about the "usual" or "proper" way of contributing to this repo, i.e. if I need to update the version number in the main script, but feel free to let me know if there's anything I can do to smoothen this PR in case you intend to merge it!

Reduces code repetition and ensures failures get raised
Allows acquiring IPv4/6 address from key-value (i.e. https://1.1.1.1/cdn-cgi/trace) and single-value (i.e. https://ipv4.icanhazip.com)
These vars could potentially be added as new config options to set the primary and fallback endpoints
Sensible fallback endpoint in case Cloudflare's are unavailable
Copy link

@KhooHaoYit KhooHaoYit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have service disruption twice because it wasn't updating my domain ip but thanks to your PR I could temporary fix the issue by mounting the file and override it in the container

Thanks man, I appeciate your PR

@madereddy
Copy link

Had the same issue where underlying IP rotated and it failed to update the IPs. This fixed my issue as well.

@AverageHelper
Copy link

AverageHelper commented Dec 3, 2024

Here for the same outage as well. Thanks for the PR!!

@makutaku
Copy link

makutaku commented Dec 3, 2024

Shouldn't this be handled like a transaction, meaning, it should not clear existing A record unless it can successfully update it ?

Copy link

@DarrenOfficial DarrenOfficial left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works like a charm

@orelgs
Copy link

orelgs commented Jan 21, 2025

How can I get this fix?
I tried updating my stack to use this image: ghcr.io/irfanhakim-as/cloudflare-ddns:1.0.2-fix-wrong-ip-r2
But the container logs are just:
exec /usr/local/bin/python: exec format error

@irfanhakim-as
Copy link
Author

How can I get this fix? I tried updating my stack to use this image: ghcr.io/irfanhakim-as/cloudflare-ddns:1.0.2-fix-wrong-ip-r2 But the container logs are just: exec /usr/local/bin/python: exec format error

I'm not sure why you're getting that, it does seem that you're pulling the right image ghcr.io/irfanhakim-as/cloudflare-ddns:1.0.2-fix-wrong-ip-r2.

Do note that while I did find some things in the (source) script that could use some improvement, this PR of mine aims to only fix the things I've mentioned in my description of the PR - as I try to make the PR as least disruptive as possible.

That being said, I've not seen the error you've mentioned deploying the exact same image for the past few months using my Helm chart.

@orelgs
Copy link

orelgs commented Jan 22, 2025 via email

@irfanhakim-as
Copy link
Author

irfanhakim-as commented Jan 22, 2025

Any chance you could upload it to docker or something?I basically just replaced the "image" section in my already existing docker compose file for cloudflare-ddnsThat's how I got the error

I don't really use Docker/Dockerhub anymore, though it shouldn't matter if I upload there - what matters is if you're able to actually pull the image and are currently using it in your docker-compose.

Can you try pulling the image on your system using docker:

docker pull ghcr.io/irfanhakim-as/cloudflare-ddns:1.0.2-fix-wrong-ip-r2

If it's downloaded successfully, list down your container images and check its IMAGE ID value:

docker images | grep ghcr.io/irfanhakim-as/cloudflare-ddns | awk '{print $3}'

Its ID should be:

5d1f94290180

If you got the correct image/ID, and your docker-compose is also specifying/using the exact same image, then I'm not sure why you got that error other than perhaps a configuration issue of sort in your deployment. You can share your setup for us to check, including docker-compose manifest or any configurations, but if you do be sure to redact any sensitive information beforehand.

Though, my expectation is, if you get that error with my test image (ghcr.io/irfanhakim-as/cloudflare-ddns:1.0.2-fix-wrong-ip-r2), you should also be getting the exact same error with the "official" image (ghcr.io/timothymiller/cloudflare-ddns/cloudflare-ddns@sha256:ded91de91844e26feb7dbe7f902d754e77857514f1a75fd3a18a9ca1a6722a74).

@orelgs
Copy link

orelgs commented Jan 22, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants