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

Ability to disable "Special Use Domain Name" blocking #1618

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

Conversation

mapl
Copy link

@mapl mapl commented Oct 2, 2024

No description provided.

Some RFCs have optional recommendations, which are configurable as described below.
However, you can completely deactivate the blocking of SUDN by setting enable to false
Copy link
Collaborator

Choose a reason for hiding this comment

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

There should be a warning included that this should only be done if your upstream DNS server is local since this shouldldn't be disabled for remote upstreams.

Copy link
Author

Choose a reason for hiding this comment

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

Add a warning as desired

@mapl
Copy link
Author

mapl commented Oct 16, 2024

I am not sure about the failed unit test. Please give me a hint there.

@kwitsch
Copy link
Collaborator

kwitsch commented Oct 16, 2024

I am not sure about the failed unit test. Please give me a hint there.

Have only skimmed over the changes since I'm on mobile but it seems like you never set the enabled config option to false before checking if it's disabled.

@mapl
Copy link
Author

mapl commented Oct 17, 2024

I hope I fixed the unit test now

Copy link

codecov bot commented Oct 18, 2024

Codecov Report

Attention: Patch coverage is 25.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 93.91%. Comparing base (fe84ab8) to head (e6c14f8).
Report is 145 commits behind head on main.

Files with missing lines Patch % Lines
resolver/sudn_resolver.go 0.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1618      +/-   ##
==========================================
+ Coverage   93.88%   93.91%   +0.02%     
==========================================
  Files          78       80       +2     
  Lines        6361     6569     +208     
==========================================
+ Hits         5972     6169     +197     
- Misses        300      316      +16     
+ Partials       89       84       -5     

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

@kwitsch
Copy link
Collaborator

kwitsch commented Oct 18, 2024

I hope I fixed the unit test now

Fixed 🙂👍

Only one test is still missing: you test the enabled state for the config but not the resolver. The sudn resolver already has an option for disabling it for a subset. Those tests could be used as a starting point (an adaptation of the last sudn resolver test should be enough).

@mapl
Copy link
Author

mapl commented Oct 19, 2024

I added a test. Hopefully it works out. :)

@mapl
Copy link
Author

mapl commented Nov 18, 2024

I hope I fixed the unit test now

Fixed 🙂👍

Only one test is still missing: you test the enabled state for the config but not the resolver. The sudn resolver already has an option for disabling it for a subset. Those tests could be used as a starting point (an adaptation of the last sudn resolver test should be enough).

Anything more what I can do?

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.

2 participants