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

add no-strict feature to ignore InvalidDirectives #18

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

philgebhardt
Copy link

Fix for #17

It's sometimes desireable to ignore invalid directives so that the
supplied configuration can still be used. This change adds a feature
flag no-strict that when enabled, does not return an
Err(InvalidDirective(_)) when a directive isn't valid. Instead, it
skips over it, allowing the configuration to be used.

Fix for hickory-dns#17

It's sometimes desireable to ignore invalid directives so that the
supplied configuration can still be used. This change adds a feature
flag `no-strict` that when enabled, does not return an
`Err(InvalidDirective(_))` when a directive isn't valid. Instead, it
skips over it, allowing the configuration to be used.
@tailhook
Copy link
Collaborator

Hm, can we make it a runtime feature rather than compile-time?

@philgebhardt
Copy link
Author

@tailhook I suppose it could be implemented as a runtime feature. However that presents the problem of propagating such behavior up to dependent crates (such as https://github.com/bluejekyll/trust-dns). Implementing the switch at compile time gives the end user the ability to control the behavior, regardless of how this crate is used. Of course I'm biased toward the decision of compile time for this reason, but I'd expect this leads less configuration overall

@tailhook
Copy link
Collaborator

tailhook commented Nov 23, 2019

What's your specific use case? Do you put custom stuff into the resolv-conf? Or is it some directive on exotic system is not implemented?

@Coding-Badly
Copy link

There is a third use case... Most / all other software that uses resolv-conf ignores invalid entries. A system with a "poison" resolve-conf can seem perfectly normal. When a Rust application built with this crate is introduced to a system with a simple typographical mistake in resolve-conf, the Rust application appears to be buggy.

@djc
Copy link
Member

djc commented Oct 28, 2020

Just a third-party opinion: while being strict about parsing is quite useful (and very much in line with the Rust ecosystem), I also feel like defaulting to matching existing tools in some amount of liberty makes sense. I think it makes sense to make this optional at runtime, but default to being more liberal. (Whatever is decided here, as a trust-dns maintainer I'm fine with exposing some API in trust-dns to allow access to useful runtime options.)

@Coding-Badly
Copy link

@tailhook, what problem do you hope to solve by making strict a run-time option?

@tailhook
Copy link
Collaborator

tailhook commented Oct 29, 2020

From the top of my head:

  1. Features in cargo are additive, it means that if I'm having crate A that needs strict parsing and crate B that doesn't. Crate A will get non-strict parsing (whenever A and B are used in the same application).
  2. If I would make some command-line that manipulates resolv-conf files I would do strict validation on writing, and might use loose validation on reading.
  3. In my apps, I'd like loose check by default, perhaps with printing warnings (given the current state of the world), but strict for extra security (since name resolution is often an important part of the system security).

@tailhook
Copy link
Collaborator

That said I think it would be cool if would look like this:

Config::parse(xxx).or_else(|e| {
  log::warn("Resolv.conf issues: {}", e);
  e.loose_config()
})

I.e. something similar to how mutex poisoning work, where you can recover original value from error object.

@Coding-Badly
Copy link

Coding-Badly commented May 7, 2021

...it would be cool if would look like this...

It would.

But that does not solve the problem presented by @philgebhardt. The application developer has determined that ignoring resolv.conf errors is the right choice. That decision needs to propagate throughout the application. The most effective way to make that happen is for this crate to ignore resolv.conf errors.

brian-cook added 2 commits May 16, 2021 15:31
…xclusive. Inverting the the [no-]strict feature means that strict is the option and we no longer need to include a feature when building agent.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants