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

Allow extending dnsmasq by providing custom configuration. #162

Merged
merged 4 commits into from
Jun 6, 2021

Conversation

ThisIsQasim
Copy link

@ThisIsQasim ThisIsQasim commented Apr 6, 2021

cc: @subspacecommunity/subspace-maintainers

Background

Reason for the change

dnsmasq can be used for accomplish a wide variety of tasks. This change lets us mount dnsmasq configurations inside the subspace container to provide additional configuration to dnsmasq.

For example you could override hostnames by creating the following config file and then mounting it inside subspace.

/opt/docker/dnsmasq/01-static-dns.conf

address=/a.example.com/172.16.0.10
address=/b.example.com/172.16.0.11

docker-compose.yml

   volumes:
   - /opt/docker/subspace:/data
   - /opt/docker/dnsmasq:/etc/dnsmasq.d

Changes

  • Include /etc/dnsmasq.d in dnsmasq config at /etc/dnsmasq.conf

Testing

The container works as expected with additional dnsmasq configuration mounted and without.

@sonarcloud
Copy link

sonarcloud bot commented Apr 6, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@jcmontigny-fox
Copy link

This is a very welcome change ! We have that use case also. This simple change would make subspace configuration very flexible.

@gchamon
Copy link

gchamon commented May 27, 2021

Hey @ThisIsQasim

Thanks for the pr and sorry for the long silence.
Could you also expand the readme to include indications that you can provide dnsmasq by mapping the internal folder to an external one with docker?

Then I think it would make it clearer for the user. Your pr is well written but the end user won't see it.

@sonarcloud
Copy link

sonarcloud bot commented Jun 1, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@ThisIsQasim
Copy link
Author

Hi @gchamon,

No need for the apologies.
I have updated the PR with brief instructions in the readme. Does that work or should I expand it some more?

@gchamon
Copy link

gchamon commented Jun 1, 2021

@ThisIsQasim the readme overall, in my opinion (I don't speak here for the rest of the maintainers) needs some rework. This is enough to point to the fact that this config is supported like this.

I didn't have time to test it just yet, but seeing what kind of changes it introduces, the only concern would be for dnsmasq to fail with an edge case of an empty/invalid file or a file without the correct permissions. This I will test later.

@gchamon gchamon requested review from agonbar, a team and gchamon June 1, 2021 12:36
@ThisIsQasim
Copy link
Author

Sounds great. Hit me up if you want me to change something.

Copy link
Collaborator

@agonbar agonbar left a comment

Choose a reason for hiding this comment

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

I tested in my machine, awsome addtion! In one of my setups I was running a pi-hole just to have an easy option to serve a couple of custom domains, though the vpn instead of the public IP.

Many thanks! I'll wait for @gchamon to test this and the we can merge it once he reviews the changes.

@ThisIsQasim
Copy link
Author

In one of my setups I was running a pi-hole just to have an easy option to serve a couple of custom domains, though the vpn instead of the public IP.

Yup that is exactly what my use-case was.

Many thanks! I'll wait for @gchamon to test this and the we can merge it once he reviews the changes.

Happy to be of help and thank you for maintaining this awesome project!

@gchamon
Copy link

gchamon commented Jun 6, 2021

@agonbar approved

@gchamon gchamon merged commit 488a762 into subspacecommunity:master Jun 6, 2021
@gchamon
Copy link

gchamon commented Jun 6, 2021

@all-contributors please add @ThisIsQasim for code

@allcontributors
Copy link

@gchamon

I've put up a pull request to add @ThisIsQasim! 🎉

@subspacecommunity subspacecommunity deleted a comment from allcontributors bot Jun 6, 2021
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