move server file configuration parameter to relevant stores #2199

divagant-martian · 2024-04-29T01:56:21Z

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

japaric

I had not started working on this so thanks for jumping in.

To me, this looks like a step in the right direction. Nice work.

My next ZoneConfig question would be: are all of the existing ZoneType variants compatible with all the store types, e.g. does ZoneType::Hint make sense as an argument to FileAuthority::try_from_config? does ZoneType::Primary make sense as an argument to RecursorAuthority::try_from_config?, etc.

It seems to me that ZoneType should only have two variants: Primary and Secondary and that the {Recursive,Forward}Authority::try_from_config constructors should have no ZoneType parameters. Hence the zones.zone_type setting should move into zones.stores.zone_type, and only for the File and Sqlite variants of StoreConfig.

crates/server/src/config/mod.rs

japaric · 2024-04-29T10:44:05Z

crates/server/src/config/mod.rs

-    pub fn get_file(&self) -> PathBuf {
-        // TODO: Option on PathBuf
-        PathBuf::from(self.file.as_ref().expect("file was none"))
+    pub fn get_file(&self) -> Option<PathBuf> {


AFAICT this is currently not being used in the codebase so I don't know how this utility getter is meant to be used in practice but I think it may make sense to preserve what the underlying StoreConfig variant was and return an enum alongside the path, e.g.

pub struct ZoneFile { pub path: PathBuf, pub kind: ZoneFileKind, } pub enum ZoneFileKind { PlainText, Sqlite, }

I also noticed this isn't used anywhere. Maybe one of the maintainers can shed some light on its purpose.

As I mentioned

to external users it previously generated a panic in valid None cases

so I wonder if anyone uses it at all

Some of this was related to me being overly cautious when I had migrated some file formats in the past. It's probably fine for us to remove a lot of the deprecated use cases.

I agree, it's worth us cleaning this up. I think we can change it how we want, this will be in a breaking change release. So it's ok to clean all this up right now, the public API is possibly desirable for anything using this crate to build a server.

Separately, I wonder if we should stop using the term file for plain_text, and instead refer to it as either TxtZone or just Zone, as Wikipedia refers to it? Most things are going to be files in this context, so it seems wrong to use a generic name for plain text.

Since it was created by BIND alternatively it could be BINDZoneFile. Or RFC1035Zone or something?

Please avoid further proliferation of UPPER acronyms -- the idiomatic spelling would be Bind or Rfc.

(I do agree with the sentiment but feel both the references to bind and the RFC are probably too verbose to this? Maybe just ZoneFile? Unadorned Zone seems a little too ambiguous for my taste.)

I'm ok with just Zone (and agree on the proper naming style)

divagant-martian · 2024-04-29T21:14:04Z

My next ZoneConfig question would be: are all of the existing ZoneType variants compatible with all the store types, e.g. does ZoneType::Hint make sense as an argument to FileAuthority::try_from_config? does ZoneType::Primary make sense as an argument to RecursorAuthority::try_from_config?, etc.

So far I can't really tell since I started to check out this repo yesterday. Happy to continue work towards reducing these discrepancies/states in subsequent PRs, as my understanding of the maintainers' style is more towards very self contained changes.

I'll check about zone type and store compatibility while we wait for maintainer review

djc · 2024-05-02T19:40:20Z

My next ZoneConfig question would be: are all of the existing ZoneType variants compatible with all the store types, e.g. does ZoneType::Hint make sense as an argument to FileAuthority::try_from_config? does ZoneType::Primary make sense as an argument to RecursorAuthority::try_from_config?, etc.

It seems to me that ZoneType should only have two variants: Primary and Secondary and that the {Recursive,Forward}Authority::try_from_config constructors should have no ZoneType parameters. Hence the zones.zone_type setting should move into zones.stores.zone_type, and only for the File and Sqlite variants of StoreConfig.

@bluejekyll want to have a look at this?

bluejekyll · 2024-05-18T18:41:20Z

Getting to this now.

bluejekyll

Overall, I like this change, it cleans things up. Sorry for the lateness of this review.

bluejekyll · 2024-05-18T18:43:47Z

bin/src/hickory-dns.rs

            let recursor =
                RecursiveAuthority::try_from_config(zone_name, zone_type, config, Some(zone_dir));
            let authority = recursor.await?;

            Box::new(Arc::new(authority)) as Box<dyn AuthorityObject>
        }
-        #[cfg(feature = "sqlite")]
-        None if zone_config.is_update_allowed() => {


This was originally to deal with an old update from previous config files. I think I'm ok with this, but it's worth us making sure to note this has a breaking change in the CHANGELOG

bluejekyll · 2024-05-18T18:47:08Z

crates/server/src/config/mod.rs

-    pub fn get_file(&self) -> PathBuf {
-        // TODO: Option on PathBuf
-        PathBuf::from(self.file.as_ref().expect("file was none"))
+    pub fn get_file(&self) -> Option<PathBuf> {


Some of this was related to me being overly cautious when I had migrated some file formats in the past. It's probably fine for us to remove a lot of the deprecated use cases.

bluejekyll · 2024-05-18T18:49:57Z

crates/server/src/config/mod.rs

-    pub fn get_file(&self) -> PathBuf {
-        // TODO: Option on PathBuf
-        PathBuf::from(self.file.as_ref().expect("file was none"))
+    pub fn get_file(&self) -> Option<PathBuf> {


I agree, it's worth us cleaning this up. I think we can change it how we want, this will be in a breaking change release. So it's ok to clean all this up right now, the public API is possibly desirable for anything using this crate to build a server.

bluejekyll · 2024-05-18T18:53:39Z

crates/server/src/config/mod.rs

@@ -186,8 +187,6 @@ pub struct ZoneConfig {
    pub zone: String, // TODO: make Domain::Name decodable
    /// type of the zone
    pub zone_type: ZoneType,
-    /// location of the file (short for StoreConfig::FileConfig{zone_file_path})
-    pub file: Option<String>,


Just leaving a note that we need to add this as a breaking change to the CHANGELOG

bluejekyll · 2024-05-18T19:10:17Z

crates/server/src/config/mod.rs

-    pub fn get_file(&self) -> PathBuf {
-        // TODO: Option on PathBuf
-        PathBuf::from(self.file.as_ref().expect("file was none"))
+    pub fn get_file(&self) -> Option<PathBuf> {


Separately, I wonder if we should stop using the term file for plain_text, and instead refer to it as either TxtZone or just Zone, as Wikipedia refers to it? Most things are going to be files in this context, so it seems wrong to use a generic name for plain text.

Since it was created by BIND alternatively it could be BINDZoneFile. Or RFC1035Zone or something?

divagant-martian added 4 commits April 28, 2024 20:42

move file paramter to relevant stores

7364810

make stores mandatory since all None paths are error paths

91faeaf

update configuration tests

e2e7531

make clippy happy

dd1a0d0

divagant-martian marked this pull request as ready for review April 29, 2024 04:13

remove declaration of deprecated test file that was removed

4de0fd8

japaric reviewed Apr 29, 2024

View reviewed changes

japaric mentioned this pull request Apr 29, 2024

move server file configuration parameter to relevant stores - alternative using untagged serde #2200

Closed

Merge branch 'main' into remove-file-config-shortcut

5b9add0

divagant-martian and others added 2 commits April 29, 2024 16:20

address review: rename ZoneConfig::new to ZoneConfig::new_with_file_zone

34b5c49

Merge branch 'main' into remove-file-config-shortcut

a155da3

djc requested a review from bluejekyll May 2, 2024 19:40

divagant-martian and others added 2 commits May 2, 2024 16:23

Merge branch 'main' into remove-file-config-shortcut

c8cb2fb

Merge branch 'main' into remove-file-config-shortcut

e2dddab

bluejekyll reviewed May 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move server file configuration parameter to relevant stores #2199

move server file configuration parameter to relevant stores #2199

divagant-martian commented Apr 29, 2024 •

edited

japaric left a comment

japaric Apr 29, 2024

divagant-martian Apr 29, 2024

bluejekyll May 18, 2024

bluejekyll May 18, 2024

bluejekyll May 18, 2024

djc May 21, 2024

bluejekyll May 22, 2024

divagant-martian commented Apr 29, 2024

djc commented May 2, 2024

bluejekyll commented May 18, 2024

bluejekyll left a comment

bluejekyll May 18, 2024

bluejekyll May 18, 2024

bluejekyll May 18, 2024

bluejekyll May 18, 2024

bluejekyll May 18, 2024

move server file configuration parameter to relevant stores #2199

Are you sure you want to change the base?

move server file configuration parameter to relevant stores #2199

Conversation

divagant-martian commented Apr 29, 2024 • edited

japaric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divagant-martian commented Apr 29, 2024

djc commented May 2, 2024

bluejekyll commented May 18, 2024

bluejekyll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divagant-martian commented Apr 29, 2024 •

edited