Re: [PATCH] ddns: add Cloudflare (v4) provider using API token

[Michael Tremer <michael.tremer@ipfire.org>]

Hello Chris,

You don’t need to submit any patches more than once. We will get back to you as soon as there is time.

So let’s get into this...

> On 29 Aug 2025, at 09:16, Chris Anton <chris.v.anton@gmail.com> wrote:
> 
> From: faithinchaos21 <45313722+faithinchaos21@users.noreply.github.com>

I assume that you are faithinchaos21 and this code did not come from someone else?

> Date: Wed, 27 Aug 2025 01:22:46 -0500
> Subject: [PATCH] ddns: add Cloudflare (v4) provider using API token
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> This adds a provider “cloudflare.com-v4” that updates an A record
> via Cloudflare’s v4 API using a Bearer token. The token is accepted
> from either ‘token’ or legacy ‘password’ for UI compatibility.
> 
> Tested on IPFire 2.29 / Core 196:
> - no-op if A already matches WAN IP
> - successful update when WAN IP changes
> - logs include CFv4 breadcrumbs for troubleshooting

To make it short, this patch is sadly very far away from what is acceptable.

Before we get into the actual implementation, there are many design issues here that simply cannot be accepted into DDNS:

* This patch only adds support for IPv4. As far as I am aware, Cloudflare is capable of IPv6, too. Why would this be limited to IPv4 only?

* You are not using the scaffolding and tools that DDNS is providing. A lot of code is being written with plain Python and throwing away all the features that DDNS provides (catching Exceptions, proxy support and many, many more…)

* This all looks very AI-generated to me and is implemented on a green field. You are even importing all the modules that you need and ignore everything else from DDNS. Why not use this snippet as a standalone script then? There was no consideration for what code existed already.

> Signed-off-by: Chris Anton <chris.v.anton@gmail.com>
> ---
> 563f089d0820bd61ad4aecac248d5cc1f2adfc81
> src/ddns/providers.py | 121 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 121 insertions(+)
> 
> diff --git a/src/ddns/providers.py b/src/ddns/providers.py
> index 59f9665..df0f3a9 100644
> --- a/src/ddns/providers.py
> +++ b/src/ddns/providers.py
> @@ -341,6 +341,127 @@ def have_address(self, proto):
> 
>  return False
> 
> +class DDNSProviderCloudflareV4(DDNSProvider):
> +    """
> +    Cloudflare v4 API using a Bearer Token.
> +    Put the API Token in the 'token' OR 'password' field of the DDNS entry.
> +    Optional in ddns.conf:
> +      proxied = false|true   (default false; keep false for WireGuard)
> +      ttl     = 1|60|120...  (default 1 = 'automatic')
> +    """
> +    handle    = "cloudflare.com-v4"
> +    name      = "Cloudflare (v4)"
> +    website   = "https://www.cloudflare.com/"
> +    protocols = ("ipv4",)
> +    supports_token_auth = True
> +    holdoff_failure_days = 0
> +
> +    def _bool(self, key, default=False):
> +        v = str(self.get(key, default)).strip().lower()
> +        return v in ("1", "true", "yes", "on")
> +

This is a good example of a function which totally goes against the coding style of the rest of the code base. It uses str(), chains a lot of functions to gather to make the code look shorter, but very difficult to read.

> +    def update(self):
> +        import json, urllib.request, urllib.error

Just no. We don’t import anything further down the line. DDNS provides a toolkit of what to use and you should stay within it. If some functionally is missing, DDNS’ functionality should be extended so that other providers can re-use the same well-maintained and tested code base.

> +
> +        tok = self.get("token") or self.get("password")
> +        if not tok:
> +            raise DDNSConfigurationError("API Token (password/token)
> is missing.")
> +
> +        proxied = self._bool("proxied", False)
> +        try:
> +            ttl = int(self.get("ttl", 1))
> +        except Exception:
> +            ttl = 1

A TTL of one second is never a good idea.

> +
> +        headers = {
> +            "Authorization": "Bearer {0}".format(tok),
> +            "Content-Type": "application/json",
> +            "User-Agent": "IPFireDDNSUpdater/CFv4",
> +        }

Since you are not using the internal request handler, you are creating your own headers and a completely nonsensical user agent. Why?

> +
> +        # --- find zone ---
> +        parts = self.hostname.split(".")
> +        if len(parts) < 2:
> +            raise DDNSRequestError("Hostname '{0}' is not a valid
> domain.".format(self.hostname))
> +
> +        zone_id = None
> +        zone_name = None
> +        for i in range(len(parts) - 1):
> +            candidate = ".".join(parts[i:])
> +            url =
> f"https://api.cloudflare.com/client/v4/zones?name={candidate}"
> +            try:
> +                req = urllib.request.Request(url, headers=headers,
> method="GET")
> +                with urllib.request.urlopen(req, timeout=20) as r:
> +                    data = json.loads(r.read().decode())
> +            except Exception as e:
> +                raise DDNSUpdateError(f"Failed to query Cloudflare
> zones API: {e}")
> +
> +            if data.get("success") and data.get("result"):
> +                zone_id = data["result"][0]["id"]
> +                zone_name = candidate
> +                break

It is not acceptable to build anything custom that sends a request. You are removing all other kinds of features that I have mentioned above.

To just “guess” the name of the zone is something that I would not consider good style.

> +
> +        if not zone_id:
> +            raise DDNSRequestError(f"Could not find a Cloudflare Zone
> for '{self.hostname}'.")
> +
> +        logger.info("CFv4: zone=%s id=%s", zone_name, zone_id)
> +
> +        # --- get record ---
> +        rec_url =
> f"https://api.cloudflare.com/client/v4/zones/{zone_id}/dns_records?type=A&name={self.hostname}"
> +        try:
> +            req = urllib.request.Request(rec_url, headers=headers,
> method="GET")
> +            with urllib.request.urlopen(req, timeout=20) as r:
> +                rec_data = json.loads(r.read().decode())
> +        except Exception as e:
> +            raise DDNSUpdateError(f"Failed to query Cloudflare DNS
> records API: {e}")
> +
> +        if not rec_data.get("success"):
> +            errs = rec_data.get("errors") or []
> +            if any("Authentication error" in (e.get("message", "") or
> "") for e in errs):
> +                raise DDNSAuthenticationError("Invalid API Token.")
> +            raise DDNSUpdateError(f"Cloudflare API error finding
> record: {errs}")

Same as above, hardcoded defaults like the timeout. Spaghetti code.

> +
> +        results = rec_data.get("result") or []
> +        if not results:
> +            raise DDNSRequestError(f"No A record found for
> '{self.hostname}' in zone '{zone_name}'.")
> +
> +        record_id = results[0]["id"]
> +        stored_ip = results[0]["content"]
> +        logger.info("CFv4: record_id=%s stored_ip=%s", record_id, stored_ip)
> +
> +        # --- compare IPs ---
> +        current_ip = self.get_address("ipv4")
> +        logger.info("CFv4: current_ip=%s vs stored_ip=%s",
> current_ip, stored_ip)
> +        if current_ip == stored_ip:
> +            logger.info("CFv4: no update needed")
> +            return

Why is this done using the API at all? We have functionality to use DNS for this which creates a lot less load than performing several API requests every couple of minutes.

> +
> +        # --- update ---
> +        upd_url =
> f"https://api.cloudflare.com/client/v4/zones/{zone_id}/dns_records/{record_id}"
> +        payload = {
> +            "type": "A",
> +            "name": self.hostname,
> +            "content": current_ip,
> +            "ttl": ttl,
> +            "proxied": proxied,
> +        }
> +        logger.info("CFv4: updating %s -> %s (proxied=%s ttl=%s)",
> self.hostname, current_ip, proxied, ttl)
> +
> +        try:
> +            req = urllib.request.Request(
> +                upd_url, data=json.dumps(payload).encode(),
> headers=headers, method="PUT"
> +            )
> +            with urllib.request.urlopen(req, timeout=20) as r:
> +                upd = json.loads(r.read().decode())
> +        except Exception as e:
> +            raise DDNSUpdateError(f"Failed to send update request to
> Cloudflare: {e}”)
> 

Once again the same custom request logic.

> +        if not upd.get("success"):
> +            raise DDNSUpdateError(f"Cloudflare API error on update:
> {upd.get('errors')}")
> +
> +        logger.info("CFv4: update ok for %s -> %s", self.hostname, current_ip)
> +        return
> +
> 
> class DDNSProtocolDynDNS2(object):
>  """
> 

I would really like to have support for Cloudflare in DDNS, but this is sadly not the way.

Please study my comments thoroughly and then lets come up with a plan together how to implement this properly.

Best,
-Michael