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

hosts-file-enabled cannot be set from config file #413

Closed
linuxgemini opened this issue Aug 20, 2024 · 6 comments
Closed

hosts-file-enabled cannot be set from config file #413

linuxgemini opened this issue Aug 20, 2024 · 6 comments
Assignees
Labels
bug Something isn't working

Comments

@linuxgemini
Copy link
Contributor

Hello,

hosts-file-enabled uses cmd.decodableBool declared in internal/cmd/options.go:

// decodableBool is a boolean that can be unmarshaled from a flags.
//
// TODO(e.burkov): This is a workaround for go-flags, see
// https://github.com/AdguardTeam/dnsproxy/issues/182.
type decodableBool struct {
value bool
}
// type check
var _ goFlags.Unmarshaler = (*decodableBool)(nil)
// UnmarshalFlag implements the [goFlags.Unmarshaler] interface for
// *decodableBool.
func (b *decodableBool) UnmarshalFlag(text string) (err error) {
b.value, err = strconv.ParseBool(text)
return err
}

While the option works as intended as a launch flag --hosts-file-enabled=false, setting it inside a YAML config (hosts-file-enabled: false) will not work:

Aug 20 16:22:47 hostname dnsproxy[738715]: dnsproxy config path: /etc/dnsproxy/config.yaml
Aug 20 16:22:47 hostname dnsproxy[738715]: parsing config file /etc/dnsproxy/config.yaml: unmarshalling file: yaml: unmarshal errors:
Aug 20 16:22:47 hostname dnsproxy[738715]:   line 84: cannot unmarshal !!bool `false` into cmd.decodableBool

Setting it as a string (hosts-file-enabled: "false") also fails:

Aug 20 16:24:47 hostname dnsproxy[738747]: dnsproxy config path: /etc/dnsproxy/config.yaml
Aug 20 16:24:47 hostname dnsproxy[738747]: parsing config file /etc/dnsproxy/config.yaml: unmarshalling file: yaml: unmarshal errors:
Aug 20 16:24:47 hostname dnsproxy[738747]:   line 84: cannot unmarshal !!str `false` into cmd.decodableBool

I don't understand why exactly this occurs (I'm not really proficient in Go), one might omit the YAML flag (seen below) and use it as a command line argument:

// HostsFileEnabled controls whether hosts files are used for resolving or
// not.
HostsFileEnabled decodableBool `yaml:"hosts-file-enabled" long:"hosts-file-enabled" description:"If specified, use hosts files for resolving" default:"true"`

@ainar-g ainar-g added the bug Something isn't working label Aug 20, 2024
adguard pushed a commit that referenced this issue Aug 20, 2024
Updates #413.

Squashed commit of the following:

commit bd9b59b
Author: Eugene Burkov <[email protected]>
Date:   Tue Aug 20 18:37:14 2024 +0300

    cmd: rm dup todo

commit edcd627
Author: Eugene Burkov <[email protected]>
Date:   Tue Aug 20 18:33:40 2024 +0300

    cmd: unmarshal from yaml
@EugeneOne1
Copy link
Member

EugeneOne1 commented Aug 20, 2024

@linuxgemini, hello and thanks for the detailed report. We've just committed the fix (559d2ef) for this issue to the master branch. Could you please build the dnsproxy binary from that commit and check if it now successfully decodes the value?

@timkgh
Copy link

timkgh commented Aug 20, 2024

The default also should not be true. Let users opt-in rather than opt-out by default. If you run dnsproxy in a cloud VM that has all sorts of local things (e.g. private IPs for hosts that would otherwise resolve through public DNS to public IPs) in /etc/hosts, dnsproxy shouldn't really pick those up if you're using it just as a generic/public DNS proxy and it might be a surprising behavior.

@ameshkov
Copy link
Member

ameshkov commented Aug 21, 2024

Eventually, we have in mind making dnsproxy a drop-in replacement for dnsmasq and that's why we're trying to follow its behavior.

PS: Of course we're very far from it now, but nevertheless.

@cyformatician
Copy link

cyformatician commented Aug 21, 2024

I agree with @timkgh, at least until users / package maintainers become familiar with this option ... I also think it's best to pull the .73.0 release and replace it with a working release that includes the fix as I had issues with my openwrt router and internet due to dnsproxy crashing (with/without the hosts option disabled in config) which brought me here..

Wed Aug 21 13:09:44 2024 daemon.info dnsproxy[21942]: 2024/08/21 09:09:44.078661 INFO dnsproxy starting version=v0.73.0 revision="" branch="" commit_time=""
Wed Aug 21 13:09:44 2024 daemon.info dnsproxy[21942]: 2024/08/21 09:09:44.078879 ERROR running dnsproxy err="configuring proxy: creating default handler: open etc/hosts: no such file or directory"
Wed Aug 21 13:09:44 2024 daemon.info dnsproxy[21942]: jail: jail (21943) exited with exit: 1

Wed Aug 21 13:09:49 2024 daemon.info dnsproxy[21956]: jail: exec-ing /usr/bin/dnsproxy
Wed Aug 21 13:09:49 2024 daemon.info dnsproxy[21956]: 2024/08/21 09:09:49.128674 INFO dnsproxy starting version=v0.73.0 revision="" branch="" commit_time=""
Wed Aug 21 13:09:49 2024 daemon.info dnsproxy[21956]: 2024/08/21 09:09:49.128875 ERROR running dnsproxy err="configuring proxy: creating default handler: open etc/hosts: no such file or directory"
Wed Aug 21 13:09:49 2024 daemon.info dnsproxy[21956]: jail: jail (21957) exited with exit: 1

Wed Aug 21 13:09:54 2024 daemon.info dnsproxy[21977]: jail: exec-ing /usr/bin/dnsproxy
Wed Aug 21 13:09:54 2024 daemon.info dnsproxy[21977]: 2024/08/21 09:09:54.178876 INFO dnsproxy starting version=v0.73.0 revision="" branch="" commit_time=""
Wed Aug 21 13:09:54 2024 daemon.info dnsproxy[21977]: 2024/08/21 09:09:54.179094 ERROR running dnsproxy err="configuring proxy: creating default handler: open etc/hosts: no such file or directory"
Wed Aug 21 13:09:54 2024 daemon.info dnsproxy[21977]: jail: jail (21978) exited with exit: 1

For impacted openwrt users - temporary fix would be to edit /etc/init.d/dnsproxy and add the following to load_config_list() section then restart dnsproxy:

append_param "--hosts-file-enabled" "false"

@linuxgemini
Copy link
Contributor Author

@linuxgemini, hello and thanks for the detailed report. We've just committed the fix (559d2ef) for this issue to the master branch. Could you please build the dnsproxy binary from that commit and check if it now successfully decodes the value?

Hi @EugeneOne1, sorry for the late reply; build from current master works yes.

@EugeneOne1
Copy link
Member

@linuxgemini, good to hear! We'll release the patch version later today. I'll close this issue for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants