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

MacOS implementation of domain specific use of vpn dns #72

Merged
merged 2 commits into from
Jan 16, 2021

Conversation

josmo
Copy link
Contributor

@josmo josmo commented Jan 6, 2021

Why

Currently if you're using vpn-slice you can't easily give a list of domains to use the vpn dns to resolve internally. This update allows the options of --domains-vpn-dns=domain1.com,domain2.com so that those domains will resolve using the vpn dns.

macOS specifics

  • with macOS you can add the domain in /etc/resolver/{domain} to have that domain use specific nameservers (in this case the vpn dns entries)
  • it cleans up the configuration on_disconnect

Notes

  • This is only implemented with macOS however the interface is there to be able to be added to other platforms
  • Python is definitely not something I ever code in so feel free to let me know if there's anything that should be done differently.
  • Also I suck at naming things so if the parameter name should be something else let me know

Related Issues

#37 #15 possibly #31 and could be updated to include #68 automatically in the future

@gmacon
Copy link
Collaborator

gmacon commented Jan 6, 2021

Another related issue is #41, which discusses implementing split DNS on Windows.

Copy link
Collaborator

@gmacon gmacon left a comment

Choose a reason for hiding this comment

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

This looks good overall, I just have a few questions. Thanks for taking the time to build this PR!

vpn_slice/provider.py Outdated Show resolved Hide resolved
vpn_slice/mac.py Outdated Show resolved Hide resolved
vpn_slice/mac.py Outdated Show resolved Hide resolved
rename DomainDNSProvider to SplitDNSProvider
use context manager to open file
use write instead of append
use if not python style
Copy link
Collaborator

@gmacon gmacon left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks again for the PR!

@josmo
Copy link
Contributor Author

josmo commented Jan 6, 2021

Thanks @gmacon

@josmo
Copy link
Contributor Author

josmo commented Jan 12, 2021

@gmacon do I need to do anything else to get this merged? would love to start using it from the release :)

@gmacon gmacon merged commit 7c8c607 into dlenski:master Jan 16, 2021
@gmacon
Copy link
Collaborator

gmacon commented Jan 16, 2021

It would help if I remember to push "Merge Pull Request"... Sorry about that.

@josmo josmo deleted the allow-vpn-dns-for-domains branch January 19, 2021 16:25
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