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.

Sign up for GitHub

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

Jump to bottom

ddclient: T5791: Adjust warning messages, minor refactor and smoketest updates #2607

Merged
merged 3 commits into from
Dec 13, 2023
Merged

ddclient: T5791: Adjust warning messages, minor refactor and smoketest updates #2607

merged 3 commits into from
Dec 13, 2023

Conversation

indrajitr
Copy link
Contributor

@indrajitr indrajitr commented Dec 11, 2023 •
edited
Loading

Change Summary

This is a follow-up to #2602 with the following changes:

  • exclude pseudo-interfaces from strict checking
  • adjust warning messages and minor refactoring
  • add a corresponding smoke test
  • cleanup smoke tests a bit

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

Smoketest result

vyos@v15-1210:~$ python3 /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" with protocol
"freedns"


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


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

ok
test_02_dyndns_service_ipv6 (__main__.TestServiceDDNS.test_02_dyndns_service_ipv6) ... 
"expiry-time" must be greater than "wait-time" for Dynamic DNS service
"dynv6"

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" 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_web_options (__main__.TestServiceDDNS.test_06_dyndns_web_options) ... 
"web-options" is applicable only when using HTTP(S) web request to
obtain the IP address

ok
test_07_dyndns_dynamic_interface (__main__.TestServiceDDNS.test_07_dyndns_dynamic_interface) ... 
WARNING: Interface "pppoe587" does not exist for Dynamic DNS service
"namecheap" yet! The service will not operate correctly till the
interface is configured.

ok
test_08_dyndns_vrf (__main__.TestServiceDDNS.test_08_dyndns_vrf) ... ok

----------------------------------------------------------------------
Ran 8 tests in 292.142s

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 December 11, 2023 02:46
sever-sever
sever-sever reviewed Dec 11, 2023
View reviewed changes
src/conf_mode/dns_dynamic.py Outdated
@@ -30,6 +30,9 @@
config_file = r'/run/ddclient/ddclient.conf'
systemd_override = r'/run/systemd/system/ddclient.service.d/override.conf'

# Dynamic interfaces that might not exist when the configuration is loaded
dynamic_interfaces = ('peth', 'pppoe', 'sstpc')
Copy link
Member

@sever-sever sever-sever Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I can’t get what is wrong with “peth” interfaces? As it is configured statically during the boot process.
Do VLANs have the same issue?
Is there something wrong with priorities?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know what's wrong with peth either and don't have a way to validate myself. I am going by the forum feedback https://forum.vyos.io/t/issues-loading-config-on-re-boot-when-it-commits-and-runs-fine-beforehand/13000/1

Copy link
Member

Choose a reason for hiding this comment

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

I suggest do not exclude (not dynamic ) interfaces under we don’t understand the root cause. It reasonable for PPPoE but not for “peth”, vlans, etc.
And this way you just “hide” the problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed 💯

@indrajitr indrajitr marked this pull request as ready for review December 11, 2023 05:05
@vyosbot vyosbot requested a review from a team December 11, 2023 05:05
@indrajitr indrajitr force-pushed the ddclient-improvement-round-3-2023-12-10 branch from 3b3749e to cd20cab Compare December 11, 2023 05:16
@indrajitr indrajitr changed the title ddclient: T5791: Exclude availablity check for pseudo-interface ddclient: T5791: Exclude availablity check for pseudo-interface and update smoketests Dec 11, 2023
@indrajitr indrajitr force-pushed the ddclient-improvement-round-3-2023-12-10 branch from cd20cab to 20713b5 Compare December 11, 2023 05:33
@indrajitr indrajitr changed the title ddclient: T5791: Exclude availablity check for pseudo-interface and update smoketests ddclient: T5791: Adjust warning messaged, minor refactor and smoketest updates Dec 11, 2023
@indrajitr indrajitr changed the title ddclient: T5791: Adjust warning messaged, minor refactor and smoketest updates ddclient: T5791: Adjust warning messages, minor refactor and smoketest updates Dec 11, 2023
c-po
c-po requested changes Dec 11, 2023
View reviewed changes
src/conf_mode/dns_dynamic.py Outdated
if config['address'].startswith(dynamic_interfaces):
Warning(f'Interface "{config["address"]}" does not exist for Dynamic DNS '
f'service "{service}" yet! The service will not operate correctly '
f'till the interface is configured.')
Copy link
Member

Choose a reason for hiding this comment

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

The statement is incorrect. It should be:

'Interface "{config["address"]}" does not yet exist and con not be used for Dynamic DNS '
f'service "{service}" until it is up!')

PPPoE interfaces might be configured but not up as the BRAS connection has not yet been established or is currently disturbed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Message updated.

@indrajitr indrajitr force-pushed the ddclient-improvement-round-3-2023-12-10 branch from 20713b5 to 40b22ff Compare December 11, 2023 23:00
@c-po c-po merged commit 790c028 into vyos:current Dec 13, 2023
@indrajitr indrajitr mentioned this pull request Dec 27, 2023
Merged
12 tasks
@indrajitr indrajitr deleted the ddclient-improvement-round-3-2023-12-10 branch December 27, 2023 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sever-sever sever-sever sever-sever approved these changes

@c-po c-po c-po approved these changes

@dmbaturin dmbaturin Awaiting requested review from dmbaturin dmbaturin is a code owner automatically assigned from vyos/reviewers

@sarthurdev sarthurdev Awaiting requested review from sarthurdev sarthurdev is a code owner automatically assigned from vyos/reviewers

@zdc zdc Awaiting requested review from zdc zdc is a code owner automatically assigned from vyos/reviewers

@jestabro jestabro Awaiting requested review from jestabro jestabro is a code owner automatically assigned from vyos/reviewers

Assignees

@indrajitr indrajitr

Labels
current
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@indrajitr @sever-sever @c-po