[PR #2199] [CLOSED] move server file configuration parameter to relevant stores #2871

New issue

Closed

opened 2026-03-16 11:12:48 +03:00 by kerem · 0 comments

kerem commented

2026-03-16 11:12:48 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/hickory-dns/hickory-dns/pull/2199
Author: @divagant-martian
Created: 4/29/2024
Status: ❌ Closed

Base: main ← Head: remove-file-config-shortcut

📝 Commits (10+)

7364810 move file paramter to relevant stores
91faeaf make stores mandatory since all None paths are error paths
e2e7531 update configuration tests
dd1a0d0 make clippy happy
4de0fd8 remove declaration of deprecated test file that was removed
5b9add0 Merge branch 'main' into remove-file-config-shortcut
34b5c49 address review: rename ZoneConfig::new to ZoneConfig::new_with_file_zone
a155da3 Merge branch 'main' into remove-file-config-shortcut
c8cb2fb Merge branch 'main' into remove-file-config-shortcut
e2dddab Merge branch 'main' into remove-file-config-shortcut

📊 Changes

23 files changed (+90 additions, -244 deletions)

View changed files

📝 bin/src/hickory-dns.rs (+7 -68)
📝 bin/tests/named_test_rsa_dnssec.rs (+0 -36)
📝 crates/server/src/config/mod.rs (+17 -11)
📝 crates/server/src/store/file/config.rs (+9 -0)
📝 crates/server/tests/config_tests.rs (+7 -8)
📝 tests/test-data/test_configs/all_supported_dnssec.toml (+6 -6)
📝 tests/test-data/test_configs/dns_over_https.toml (+1 -1)
📝 tests/test-data/test_configs/dns_over_quic.toml (+1 -1)
📝 tests/test-data/test_configs/dns_over_tls.toml (+1 -1)
📝 tests/test-data/test_configs/dns_over_tls_rustls_and_openssl.toml (+1 -1)
📝 tests/test-data/test_configs/dnssec_with_update.toml (+5 -5)
➖ tests/test-data/test_configs/dnssec_with_update_deprecated.toml (+0 -71)
📝 tests/test-data/test_configs/example.toml (+6 -6)
📝 tests/test-data/test_configs/example_allow_networks.toml (+1 -1)
📝 tests/test-data/test_configs/example_deny_allow_networks.toml (+1 -1)
📝 tests/test-data/test_configs/example_deny_networks.toml (+1 -1)
📝 tests/test-data/test_configs/example_forwarder.toml (+5 -5)
📝 tests/test-data/test_configs/example_recursor.toml (+5 -5)
📝 tests/test-data/test_configs/ipv4_and_ipv6.toml (+1 -1)
📝 tests/test-data/test_configs/ipv4_only.toml (+1 -1)

...and 3 more files

📄 Description

Small part of #2195

As stated in the issue, the file parameter as a shortcut allows to create configurations that are not quite valid and generate warnings in logs. This PR removes the higher level path parameter down to the two store configurations where it's valid. This has some consequences triggering other changes:

it's no longer possible to load sqlite configuration using the shortcut with the sqlite feature and setting is_update_allowed. This was previously logged as deprecated.
get_file returns an Option<PathBuf>. It seems to me this should have always been the case. Also note this method is not used anywhere in this repository and to external users it previously generated a panic in valid None cases.
Noting that config validation always required either the shortcut or the store config, and the file has been moved to the relevant kinds of stores, the store configuration is now mandatory since all None cases map to an error.
Is to note that removing the shortcut and keeping the store serialization as an internally tagged enum changes the minimum effort required to write a configuration. Ideally, the enumeration would be externally tagged imo. I gave the untagged option a try and I'm not convinced it's that much better. basic-toml does not support externally tagged enums which makes me wonder why is the more wildly used toml crate not used instead.

@japaric I hope this isn't colliding with any started work and helps instead. The great description in the issue made it an attractive way to do a first contribution. Would appreciate a review as well

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/hickory-dns/hickory-dns/pull/2199 **Author:** [@divagant-martian](https://github.com/divagant-martian) **Created:** 4/29/2024 **Status:** ❌ Closed **Base:** `main` ← **Head:** `remove-file-config-shortcut` --- ### 📝 Commits (10+) - [`7364810`](https://github.com/hickory-dns/hickory-dns/commit/736481041a8d0b7dfd4be61c833682eacb253b28) move file paramter to relevant stores - [`91faeaf`](https://github.com/hickory-dns/hickory-dns/commit/91faeafbe9847da9ac8c0a54c80c78393bb0a646) make stores mandatory since all None paths are error paths - [`e2e7531`](https://github.com/hickory-dns/hickory-dns/commit/e2e7531fbcfb141d552f29cda53755d6180276dd) update configuration tests - [`dd1a0d0`](https://github.com/hickory-dns/hickory-dns/commit/dd1a0d0a19b73c5f9b4936957fe99762924222a6) make clippy happy - [`4de0fd8`](https://github.com/hickory-dns/hickory-dns/commit/4de0fd8c52c2f29d26a7ada973b6891b68854921) remove declaration of deprecated test file that was removed - [`5b9add0`](https://github.com/hickory-dns/hickory-dns/commit/5b9add04d848615516756079150ec663229de505) Merge branch 'main' into remove-file-config-shortcut - [`34b5c49`](https://github.com/hickory-dns/hickory-dns/commit/34b5c49d2b11baadbdc869d83e3732a4271b71d3) address review: rename ZoneConfig::new to ZoneConfig::new_with_file_zone - [`a155da3`](https://github.com/hickory-dns/hickory-dns/commit/a155da31d2862b7e437b1e3ed5fd182b10849e1c) Merge branch 'main' into remove-file-config-shortcut - [`c8cb2fb`](https://github.com/hickory-dns/hickory-dns/commit/c8cb2fb2b938ea48f61c3cb20f80ed51136a174a) Merge branch 'main' into remove-file-config-shortcut - [`e2dddab`](https://github.com/hickory-dns/hickory-dns/commit/e2dddab25ca63805b85d391d75aac41121d29c2e) Merge branch 'main' into remove-file-config-shortcut ### 📊 Changes **23 files changed** (+90 additions, -244 deletions) <details> <summary>View changed files</summary> 📝 `bin/src/hickory-dns.rs` (+7 -68) 📝 `bin/tests/named_test_rsa_dnssec.rs` (+0 -36) 📝 `crates/server/src/config/mod.rs` (+17 -11) 📝 `crates/server/src/store/file/config.rs` (+9 -0) 📝 `crates/server/tests/config_tests.rs` (+7 -8) 📝 `tests/test-data/test_configs/all_supported_dnssec.toml` (+6 -6) 📝 `tests/test-data/test_configs/dns_over_https.toml` (+1 -1) 📝 `tests/test-data/test_configs/dns_over_quic.toml` (+1 -1) 📝 `tests/test-data/test_configs/dns_over_tls.toml` (+1 -1) 📝 `tests/test-data/test_configs/dns_over_tls_rustls_and_openssl.toml` (+1 -1) 📝 `tests/test-data/test_configs/dnssec_with_update.toml` (+5 -5) ➖ `tests/test-data/test_configs/dnssec_with_update_deprecated.toml` (+0 -71) 📝 `tests/test-data/test_configs/example.toml` (+6 -6) 📝 `tests/test-data/test_configs/example_allow_networks.toml` (+1 -1) 📝 `tests/test-data/test_configs/example_deny_allow_networks.toml` (+1 -1) 📝 `tests/test-data/test_configs/example_deny_networks.toml` (+1 -1) 📝 `tests/test-data/test_configs/example_forwarder.toml` (+5 -5) 📝 `tests/test-data/test_configs/example_recursor.toml` (+5 -5) 📝 `tests/test-data/test_configs/ipv4_and_ipv6.toml` (+1 -1) 📝 `tests/test-data/test_configs/ipv4_only.toml` (+1 -1) _...and 3 more files_ </details> ### 📄 Description Small part of #2195 As stated in the issue, the file parameter as a shortcut allows to create configurations that are not quite valid and generate warnings in logs. This PR removes the higher level path parameter down to the two store configurations where it's valid. This has some consequences triggering other changes: - it's no longer possible to load sqlite configuration using the shortcut with the `sqlite` feature and setting `is_update_allowed`. This was previously logged as deprecated. - `get_file` returns an `Option<PathBuf>`. It seems to me this should have always been the case. Also note this method is not used anywhere in this repository and to external users it previously generated a panic in valid `None` cases. - Noting that config validation always required either the shortcut or the store config, and the file has been moved to the relevant kinds of stores, the store configuration is now mandatory since all `None` cases map to an error. - Is to note that removing the shortcut and keeping the store serialization as an internally tagged enum changes the minimum effort required to write a configuration. Ideally, the enumeration would be externally tagged imo. [I gave the untagged option a try](https://github.com/hickory-dns/hickory-dns/pull/2200/commits/46231c68117de8b2f52b7171272bf2eb1061c301) and I'm not convinced it's that much better. `basic-toml` [does not support externally tagged enums](https://github.com/dtolnay/basic-toml/issues/8) which makes me wonder why is the more wildly used `toml` crate not used instead. @japaric I hope this isn't colliding with any started work and helps instead. The great description in the issue made it an attractive way to do a first contribution. Would appreciate a review as well --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>