Skip to content

dns/ddclient: add request timeouts and normalize provider file modes - #5741

Open
Flightkick wants to merge 3 commits into
opnsense:masterfrom
Flightkick:ddclient-timeouts-filemodes
Open

Flightkick wants to merge 3 commits into
opnsense:masterfrom
Flightkick:ddclient-timeouts-filemodes

Conversation

@Flightkick

@Flightkick Flightkick commented Sep 26, 2026 •

Copy link
Copy Markdown
  • I have read the contributing guidelines at https://github.com/opnsense/plugins/blob/master/CONTRIBUTING.md

  • I opened an issue first for non-trivial changes and linked it below.

  • AI tools were used to create at least part of the code submitted herewith.

  • Model used: GLM (Mistral Vibe agent)

  • Extent of AI involvement: The audit of the call sites, the drafting of the timeout fix and the file mode analysis, and the preparation of this pull request were performed by an AI agent. I reviewed the resulting diff and verified the changed code compiles.


Describe the problem

The native ddclient backend (lib/poller.py) runs all accounts sequentially in a single loop, but most provider modules issue HTTP requests without a timeout. A hung connection to a provider API therefore stalls Dynamic DNS updates for every other account on the firewall until the TCP stack gives up. The same issue came up in the review of the bunny.net provider PR (#5609).

Describe the proposed solution

As discussed in the comments below, instead of adding timeout= to every provider call site, the poller now wraps requests.sessions.Session.request and applies a default timeout=(5, 30) to every request that does not set one explicitly. Providers that already set their own timeout (allinkl, dnspod_cn) keep their values, and future providers are covered automatically. The patch is process-local to the ddclient poller daemon, and the poller's existing error handling logs a timeout for the affected account without stalling the others.

In addition, account/porkbun.py was missing the executable bit, which the linter policy requires for all files under scripts/ (see #5741 (comment)). Its file mode is changed from 100644 to 100755 to match the rest of the provider modules. No other file mode or content changes are included.

Related issue

None. Trivial change, motivated by the code review discussion on #5609 and the feedback on this pull request.

@AdSchellevis

Copy link
Copy Markdown
Member

This is a bit to fragile as each new provider may forget about the timeout and quite some code is touched here.

In theory it should be possible to monkey-patch requests.sessions.Session.request in the main application or wrap some sort of watcher around the provider to isolate the issue at a single spot. I haven't seen stalls myself, but these could happen without a timeout configured.

@Flightkick

Copy link
Copy Markdown
Author

True that, although new implementations might check the existing ones for reference. Monkey pathing the request method might carry some unintended side-effects with calls that are not idempotent.

Would you prefer if I drop the request fixups and just change streamline the filemodes. Or drop the PR in general if that doesn't really matter either?

@AdSchellevis

Copy link
Copy Markdown
Member

changing the file modes should be fine, for the timeouts I rather have a generic fix, if you change the PR to include both, I'll have another look.

@fichtner

Copy link
Copy Markdown
Member

The file mode changes clash with our linter policy, which exists for a good reason.

@Flightkick

Copy link
Copy Markdown
Author

Understood, thanks both.

For the timeouts I'll drop the per-call-site changes and instead patch requests.sessions.Session.request in the poller, applying a default timeout=(5, 30) to every request that doesn't set one. Providers that already set their own timeout keep their values, and future providers are covered automatically. The poller's existing error handling then logs a timeout per account without stalling the others.

@fichtner on the file modes: Ad's comment suggested the mode changes were fine, but you mention they clash with the linter policy. Could you clarify what you'd expect here? Drop them entirely, or is there a preferred way to align them with the policy?

@fichtner

Copy link
Copy Markdown
Member

The linter wants all files in scripts/ to be executable because in the past some of these scripts have been committed but were not able to be executed by the backend. To avoid such release issues in the future we simply add executable flags to all scripts.

@Flightkick
Flightkick marked this pull request as draft September 28, 2026 10:09
@Flightkick
Flightkick marked this pull request as ready for review September 28, 2026 10:24
@Flightkick

Copy link
Copy Markdown
Author

Thanks for your time, adding clarification and feedback! 🙏
Both the filemode requirements and the generalization of the timeout on the Requests lib have been applied.

Added a comment to clarify the project's filemode requirements to #5609 as well, hoping that one can be merged as well once the filemode is corrected there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants