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

ddclient: T5708: Upgrade to ddclient 3.11.1 #2438

Merged
merged 3 commits into from
Nov 6, 2023

Conversation

indrajitr
Copy link
Contributor

@indrajitr indrajitr commented Nov 4, 2023

Change Summary

  • Migrate to ddclient 3.11.1 and enforce debian/control dependency
  • Add dual stack support for additional protocols
  • Restrict usage of porkbun protocol, VyOS configuration structure isn't compatible with porkbun yet
  • Improve and cleanup error messages
  • web-options is only applicable when using HTTP(S) web request to obtain the IP address, apply guard for that
  • Time interval in seconds to wait between DNS updates would be a bit more intuitive as interval than timeout

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
  • Other (please describe):

Related Task(s)

Related PR(s)

Component(s) name

dns dynamic

Proposed changes

How to test

set service dns dynamic interval 600

set service dns dynamic address eth5 service cloudflare host-name 'dyn.example.com'
set service dns dynamic address eth5 service cloudflare ip-version 'both'
set service dns dynamic address eth5 service cloudflare password 'scrambled_string'
set service dns dynamic address eth5 service cloudflare protocol 'cloudflare'
set service dns dynamic address eth5 service cloudflare ttl '300'
set service dns dynamic address eth5 service cloudflare zone 'example.com'

set service dns dynamic address eth5 service namecheap host-name '@.example.com'
set service dns dynamic address eth5 service namecheap username 'example.com'
set service dns dynamic address eth5 service namecheap password 'scrambled_string'
set service dns dynamic address eth5 service namecheap protocol 'namecheap'

Smoketest result

vyos@vyos15a:~$ /usr/libexec/vyos/tests/smoke/cli/test_service_dns_dynamic.py
test_01_dyndns_service_standard (__main__.TestServiceDDNS.test_01_dyndns_service_standard) ... 
"zone" is not supported for Dynamic DNS service "freedns" on "eth0" with
protocol "freedns"


"ttl" is not supported for Dynamic DNS service "freedns" on "eth0" with
protocol "freedns"


"ttl" is not supported for Dynamic DNS service "zoneedit" on "eth0" with
protocol "zoneedit1"


"ttl" is not supported for Dynamic DNS service "zoneedit" on "eth0" with
protocol "zoneedit1"

ok
test_02_dyndns_service_ipv6 (__main__.TestServiceDDNS.test_02_dyndns_service_ipv6) ... 
"expiry-time" must be greater than "wait-time"

ok
test_03_dyndns_service_dual_stack (__main__.TestServiceDDNS.test_03_dyndns_service_dual_stack) ... 
Both IPv4 and IPv6 at the same time is not supported for Dynamic DNS
service "google" on "eth0" with protocol "googledomains"

ok
test_04_dyndns_rfc2136 (__main__.TestServiceDDNS.test_04_dyndns_rfc2136) ... ok
test_05_dyndns_hostname (__main__.TestServiceDDNS.test_05_dyndns_hostname) ... ok
test_06_dyndns_vrf (__main__.TestServiceDDNS.test_06_dyndns_vrf) ... ok

----------------------------------------------------------------------
Ran 6 tests in 1384.026s

OK

Checklist:

  • I have read the CONTRIBUTING document
  • I have linked this PR to one or more Phabricator Task(s)
  • I have run the components SMOKETESTS if applicable
  • My commit headlines contain a valid Task id
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

@vyosbot vyosbot requested review from a team, dmbaturin, sarthurdev, zdc, jestabro, sever-sever and c-po and removed request for a team November 4, 2023 21:41
@indrajitr indrajitr force-pushed the ddclient-bump-311 branch 3 times, most recently from dc3e6ee to 47c3db0 Compare November 5, 2023 01:02
@indrajitr indrajitr changed the title T5708: Migrate to ddclient 3.11.1 ddclient: T5708: Upgrade to ddclient 3.11.1 Nov 5, 2023
@indrajitr indrajitr marked this pull request as ready for review November 5, 2023 05:10
@vyosbot vyosbot requested a review from a team November 5, 2023 05:10
@indrajitr
Copy link
Contributor Author

indrajitr commented Nov 5, 2023

Quick question to reviewers:

As you can see here there is now an additional guard to prevent setting up web-options when interface is used to detect IP address (because web-options aren't useful in such cases).

This would prevent users from doing something like:

set service dns dynamic address eth5 web-options url https://domains.google.com/checkip

but this would be allowed (web request to obtain the IP address):

set service dns dynamic address web web-options url https://domains.google.com/checkip

Would this be considered too intrusive a validation? I am undecided because I couldn't find a pattern or consensus in the codebase on what the 'best practice' is. (for example, set interfaces ethernet eth5 dhcpv6-options is allowed even if the interface eth5 doesn't have address dhcpv6).

Based on the feedback, I can either keep it or just remove it (and retain the present behavior with less stringent validation).

@c-po
Copy link
Member

c-po commented Nov 5, 2023

You have added a migrator for dns dynamic, you also need to bump the version number here: https://github.com/vyos/vyos-1x/blob/current/interface-definitions/include/version/dns-dynamic-version.xml.i

@c-po
Copy link
Member

c-po commented Nov 5, 2023

Please also add a PR for vyos-documentation as you updated the syntax

@c-po
Copy link
Member

c-po commented Nov 5, 2023

Would this be considered too intrusive a validation?

I like the idea and would like to see an implementation. There are only two questions:

  • Did this ever worked in the past? Or was it only allowed by the CLI?
  • If it was only allowed by the CLI and never worked out properly, please remove the nodes in your migration script, else users having this invalid configuration will get upgrade issues.

Time interval in seconds to wait between DNS updates would be a bit
more intuitive as `interval` than `timeout`.
`web-options` is only applicable when using HTTP(S) web request to
obtain the IP address. Apply guard for that.
- Migrate to ddclient 3.11.1 and enforce debian/control dependency
- Add dual stack support for additional protocols
- Restrict usage of `porkbun` protocol, VyOS configuration structure
  isn't compatible with porkbun yet
- Improve and cleanup error messages
@indrajitr
Copy link
Contributor Author

indrajitr commented Nov 5, 2023

You have added a migrator for dns dynamic, you also need to bump the version number here: current/interface-definitions/include/version/dns-dynamic-version.xml.i

Sorry, missed this one. Updated now.

Please also add a PR for vyos-documentation as you updated the syntax

I'm working on a full documentation update for all the ddclient related changes in last few months. PR should be there in a couple of days.

Would this be considered too intrusive a validation?

I like the idea and would like to see an implementation. There are only two questions:

  • Did this ever worked in the past? Or was it only allowed by the CLI?

Here is the implementation (commit is part of this PR). This never worked and was allowed by the CLI only - but was discarded while building the ddclient.conf

  • If it was only allowed by the CLI and never worked out properly, please remove the nodes in your migration script, else users having this invalid configuration will get upgrade issues.

Yes, the migration script here will remove the vestigial path from the existing configuration.

@dmbaturin dmbaturin merged commit 8e2429e into vyos:current Nov 6, 2023
7 checks passed
@indrajitr
Copy link
Contributor Author

@dmbaturin thank you for merging the PR. A related PR vyos/vyos-build#453 also needs to be applied for some of the protocols to behave correctly.

@indrajitr indrajitr deleted the ddclient-bump-311 branch November 6, 2023 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants