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

T6294: Service dns forwarding add the ability to configure ZonetoCache #3896

Merged
merged 1 commit into from
Sep 11, 2024
Merged

T6294: Service dns forwarding add the ability to configure ZonetoCache #3896

merged 1 commit into from
Sep 11, 2024

Conversation

HollyGurza
Copy link
Contributor

@HollyGurza HollyGurza commented Jul 29, 2024 •
edited
Loading

Change Summary

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

Proposed changes

How to test

conf
set service dns forwarding allow-from '127.0.0.0/24'
set service dns forwarding listen-address '127.0.0.1'
set service dns forwarding zone-to-cache test source url 'https://www.internic.net/domain/root.zone'
commit

vyos@vyos# sudo rec_control dump-cache cache.tmp
dumped 15052 records

sudo journalctl | grep DNS
...
Jul 29 09:46:40 vyos systemd[1]: Started pdns-recursor.service - PowerDNS Recursor.
Jul 29 09:46:40 vyos pdns-recursor[9685]: msg="Getting zone by URL" subsystem="ztc" level="0" tid="2" ts="1722246400.869" zone="test"
Jul 29 09:46:55 vyos pdns-recursor[9685]: msg="ZONEMD digest validation" subsystem="ztc" level="0" tid="2" ts="1722246415.065" validationDone="0" validationSuccess="0" zone="test"
Jul 29 09:46:55 vyos pdns-recursor[9685]: msg="Loaded zone into cache" subsystem="ztc" level="0" tid="2" ts="1722246415.092" refresh="86400" zone="test"

Smoketest result

vyos@vyos:~$ python3 /usr/libexec/vyos/tests/smoke/cli/test_service_dns_forwarding.py
test_basic_forwarding (__main__.TestServicePowerDNS.test_basic_forwarding) ... 
service_dns_forwarding: ConfigError('DNS forwarding requires a listen-address')


service_dns_forwarding: ConfigError('DNS forwarding requires a listen-address')

ok
test_dns64 (__main__.TestServicePowerDNS.test_dns64) ... 
service_dns_forwarding: ConfigError('DNS 6to4 prefix must be of length /96')

ok
test_dnssec (__main__.TestServicePowerDNS.test_dnssec) ... ok
test_domain_forwarding (__main__.TestServicePowerDNS.test_domain_forwarding) ... ok
test_ecs_add_for (__main__.TestServicePowerDNS.test_ecs_add_for) ... ok
test_ecs_ipv4_bits (__main__.TestServicePowerDNS.test_ecs_ipv4_bits) ... ok
test_edns_subnet_allow_list (__main__.TestServicePowerDNS.test_edns_subnet_allow_list) ... ok
test_exclude_throttle_adress (__main__.TestServicePowerDNS.test_exclude_throttle_adress) ... ok
test_external_nameserver (__main__.TestServicePowerDNS.test_external_nameserver) ... ok
test_listening_port (__main__.TestServicePowerDNS.test_listening_port) ... ok
test_multiple_ns_records (__main__.TestServicePowerDNS.test_multiple_ns_records) ... ok
test_no_rfc1918_forwarding (__main__.TestServicePowerDNS.test_no_rfc1918_forwarding) ... ok
test_serve_stale_extension (__main__.TestServicePowerDNS.test_serve_stale_extension) ... ok
test_zone_to_cache_axfr (__main__.TestServicePowerDNS.test_zone_to_cache_axfr) ... ok
test_zone_to_cache_options (__main__.TestServicePowerDNS.test_zone_to_cache_options) ... ok
test_zone_to_cache_url (__main__.TestServicePowerDNS.test_zone_to_cache_url) ... ok
test_zone_to_cache_wrong_source (__main__.TestServicePowerDNS.test_zone_to_cache_wrong_source) ... 
service_dns_forwarding: ConfigError('Invalid configuration for zone "smoketest": Please select one source\ntype "url" or "axfr".')

ok

----------------------------------------------------------------------
Ran 17 tests in 68.981s

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

@HollyGurza HollyGurza requested a review from a team as a code owner July 29, 2024 09:49
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 29, 2024 •
edited
Loading

👍
No issues in PR Title / Commit Title

@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 29, 2024 •
edited
Loading

✅ No issues found in unused-imports check.. Please refer the workflow run

@github-actions GitHub Actions
Copy link

CI integration 👍 passed!

Details

CI logs

  • CLI Smoketests 👍 passed
  • Config tests 👍 passed
  • RAID1 tests 👍 passed

dmbaturin
dmbaturin requested changes Aug 2, 2024
View reviewed changes
Copy link
Member

@dmbaturin dmbaturin left a comment

Choose a reason for hiding this comment

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

I think the UI and help strings can be improved.

interface-definitions/service_dns_forwarding.xml.in Outdated Show resolved Hide resolved
interface-definitions/service_dns_forwarding.xml.in Outdated Show resolved Hide resolved
interface-definitions/service_dns_forwarding.xml.in Outdated Show resolved Hide resolved
interface-definitions/service_dns_forwarding.xml.in Outdated Show resolved Hide resolved
interface-definitions/service_dns_forwarding.xml.in Outdated Show resolved Hide resolved
interface-definitions/service_dns_forwarding.xml.in Outdated Show resolved Hide resolved
interface-definitions/service_dns_forwarding.xml.in Outdated Show resolved Hide resolved
interface-definitions/service_dns_forwarding.xml.in Outdated Show resolved Hide resolved
interface-definitions/service_dns_forwarding.xml.in Outdated Show resolved Hide resolved
interface-definitions/service_dns_forwarding.xml.in Outdated Show resolved Hide resolved
@c-po
Copy link
Member

c-po commented Aug 5, 2024

@Mergifyio rebase

@mergify Mergify
Copy link
Contributor

mergify bot commented Aug 5, 2024

rebase

✅ Branch has been successfully rebased

c-po
c-po requested changes Aug 20, 2024
View reviewed changes
interface-definitions/service_dns_forwarding.xml.in Show resolved Hide resolved
interface-definitions/service_dns_forwarding.xml.in
<valueless/>
</properties>
</leafNode>
<leafNode name="interval">
Copy link
Member

Choose a reason for hiding this comment

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

From a CLI perspective (both completion help and constraint) I think it would be easier to just make this seconds only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the first version used seconds, but was reworked after this comment:
#3896 (comment)

@dmbaturin what do you think about this?

Copy link
Member

Choose a reason for hiding this comment

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

Let's just do seconds for ease of validation. We can later add suffixed values, once we have a validator for them. It shouldn't be a problem because a number without any suffix is interpreted as seconds anyway, so we'll only need to relax the validation to allow human-readable values like 1d12h.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

interface-definitions/service_dns_forwarding.xml.in Outdated
<properties>
<help>Retry interval after zone retrieval errors</help>
<valueHelp>
<format>u32:1-2147483647</format>
Copy link
Member

Choose a reason for hiding this comment

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

Retry every 24855 days? I think the bounds are to relaxed. I'd go for 1 - 86400 so from one second to once a day.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

interface-definitions/service_dns_forwarding.xml.in Outdated
<properties>
<help>Maximum zone size in megabytes</help>
<valueHelp>
<format>u32:0-1024</format>
Copy link
Member

Choose a reason for hiding this comment

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

I assume 0 refers to "ignore the size". If this is the case, please add a dedicated value help explaining the special meaning of 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member

Choose a reason for hiding this comment

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

Plase make it even more obvious:

<valueHelp>
  <format>u32:0</format>
  <description>No restriction</description>
</valueHelp>
<valueHelp>
  <format>u32:1-1024</format>
  <description>Size in megabytes</description>
</valueHelp>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

interface-definitions/service_dns_forwarding.xml.in Outdated
</leafNode>
<leafNode name="zonemd">
<properties>
<help>ZONEMD mode</help>
Copy link
Member

Choose a reason for hiding this comment

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

What's ZONEMD mode? Maybe add a bit more context. to the help string.

Please also add

<completionHelp>
  <list>ignore validate require</list>
</completionHelp>

to improve the user experience on the CLI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@HollyGurza HollyGurza requested a review from c-po August 21, 2024 10:31
c-po
c-po previously requested changes Aug 30, 2024
View reviewed changes
interface-definitions/service_dns_forwarding.xml.in Outdated
<properties>
<help>Maximum zone size in megabytes</help>
<valueHelp>
<format>u32:0-1024</format>
Copy link
Member

Choose a reason for hiding this comment

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

Plase make it even more obvious:

<valueHelp>
  <format>u32:0</format>
  <description>No restriction</description>
</valueHelp>
<valueHelp>
  <format>u32:1-1024</format>
  <description>Size in megabytes</description>
</valueHelp>

@dmbaturin dmbaturin dismissed c-po’s stale review September 11, 2024 11:26

Roman already addressed the issue.

@dmbaturin dmbaturin merged commit de8d388 into vyos:current Sep 11, 2024
14 of 16 checks passed
@24fpsDaVinci
Copy link

when I try to add root zone . as per https://docs.powerdns.com/recursor/lua-config/ztc.html#example
I get node.tag auto complete, is this correct behavior? I couldnt find any reference to "node.tag" in pdns-recursor docs

vyos@vyos# set service dns forwarding zone-cache . 
Possible completions:
 > node.tag             Load a zone into the recursor cache

then /run/pdns-recursor/recursor.conf.lua ends up being

zoneToCache("node.tag", "url", "https://www.internic.net/domain/root.zone", { dnssec = "validate", zonemd = "validate", maxReceivedMBytes = 0, retryOnErrorPeriod = 60, refreshPeriod = 86400, timeout = 20 })

@HollyGurza
Copy link
Contributor Author

when I try to add root zone . as per https://docs.powerdns.com/recursor/lua-config/ztc.html#example I get node.tag auto complete, is this correct behavior? I couldnt find any reference to "node.tag" in pdns-recursor docs

vyos@vyos# set service dns forwarding zone-cache . 
Possible completions:
 > node.tag             Load a zone into the recursor cache

then /run/pdns-recursor/recursor.conf.lua ends up being

zoneToCache("node.tag", "url", "https://www.internic.net/domain/root.zone", { dnssec = "validate", zonemd = "validate", maxReceivedMBytes = 0, retryOnErrorPeriod = 60, refreshPeriod = 86400, timeout = 20 })

Thank you for testing, I think it's incorrect behavior.
created a bug report for this: https://vyos.dev/T6893

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

@dmbaturin dmbaturin dmbaturin approved these changes

@c-po c-po c-po left review comments

@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

@HollyGurza HollyGurza

Labels
current
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@HollyGurza @c-po @24fpsDaVinci @dmbaturin @sever-sever