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

home/config: improve error message when validating bind hosts #6451

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ishanjain28
Copy link
Contributor

Hey, This PR improves error message when the program sees an invalid address.

The BindHosts field in Config struct in the program is of type netip.Addr. yaml reads the (unquoted)value 2a0a:6040:4050:: incorrectly and it's set to 'Invalid IP' by netip.

dns:
  bind_hosts:
    - 10.1.1.3
    - 2a0a:6040:4050::
  port: 53
  anonymize_client_ip: false
  ratelimit: 200
  ratelimit_subnet_len_ipv4: 24
  ratelimit_subnet_len_ipv6: 56
  ratelimit_whitelist: []
  refuse_any: true

The error message has been updated from

2023/11/23 13:49:07.012617 [error] parsing configuration file: dns.bind_hosts at index 3 is not a valid ip address

to

2023/11/23 13:48:35.042301 [error] parsing configuration file: dns.bind_host at index 3 with value 'invalid IP' is not a valid ip address. try quoting the value

There is no associated issue or wiki discussion since this is a relatively small change.

@codecov-commenter
Copy link

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (1320043) 51.72% compared to head (4530b83) 51.72%.

Files Patch % Lines
internal/home/config.go 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6451   +/-   ##
=======================================
  Coverage   51.72%   51.72%           
=======================================
  Files         176      176           
  Lines       14384    14384           
=======================================
  Hits         7440     7440           
  Misses       6213     6213           
  Partials      731      731           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ainar-g
Copy link
Contributor

ainar-g commented Nov 23, 2023

Thanks for the contribution, but the problem here is deeper, it seems. See go-yaml/yaml#1006. Considering that that repo seems to be half-dead, we'll probably need to make a wrapper type that implements YAMLUnmarshaler for netip.Addr and actually validates the value to get a more obvious message on the decoding stage.

Please file an issue about invalid addresses not being properly handled, so that this doesn't get lost.

Copy link
Contributor

@ainar-g ainar-g left a comment

Choose a reason for hiding this comment

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

See comment above.

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.

3 participants