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

Almost-dynamic zone update #15

Merged
merged 7 commits into from
Sep 30, 2022

Conversation

azmeuk
Copy link
Contributor

@azmeuk azmeuk commented Sep 23, 2022

This follows the discussion on #14

This patch adds a DNSServer.add_record method to dynamically add zones. As the DNS server runs in a thread, at the moment it is needed to reload the server after calling add_record.

If the approach is fine to you, I will submit a second PR where the record list will be shared between the threads. I am open to implementations ideas :)

@codecov
Copy link

codecov bot commented Sep 23, 2022

Codecov Report

Merging #15 (389de5b) into main (192a21e) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #15      +/-   ##
==========================================
+ Coverage   97.46%   97.54%   +0.08%     
==========================================
  Files           4        4              
  Lines         197      204       +7     
  Branches       37       37              
==========================================
+ Hits          192      199       +7     
  Misses          3        3              
  Partials        2        2              
Impacted Files Coverage Δ
dnserver/main.py 99.12% <100.00%> (+0.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 192a21e...389de5b. Read the comment docs.

Copy link
Owner

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

A few suggestions.

dnserver/main.py Outdated Show resolved Hide resolved
@samuelcolvin
Copy link
Owner

weird, my review was lost, I'll try again.

dnserver/main.py Outdated Show resolved Hide resolved
dnserver/main.py Outdated Show resolved Hide resolved
@@ -173,3 +181,6 @@ def stop(self):
@property
def is_running(self):
return (self.udp_server and self.udp_server.isAlive()) or (self.tcp_server and self.tcp_server.isAlive())

def add_record(self, zone: Zone):
self.records.zones.append(zone)
Copy link
Owner

Choose a reason for hiding this comment

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

humm, have you actually tried this? I think because of self.records = [Record(zone) for zone in records.zones] this won't actually add another record to the dns server.

Best option would be to store Records directly on ProxyResolver and BaseResolver, with that, extending self.records should actually affect the dns server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

humm, have you actually tried this? I think because of self.records = [Record(zone) for zone in records.zones] this won't actually add another record to the dns server.

Well, there is a unit test for that. Do you think this is not tested enough?

Best option would be to store Records directly on ProxyResolver and BaseResolver, with that, extending self.records should actually affect the dns server.

I just pushed that. Indeed, doing this way actually not requires the server to be restarted when a zone is added.

@azmeuk
Copy link
Contributor Author

azmeuk commented Sep 30, 2022

@samuelcolvin the very last thing I plan to tackle is allowing to create a DNSServer object without zone. Should I add commits for this in this PR or make a new one?

@samuelcolvin
Copy link
Owner

@samuelcolvin the very last thing I plan to tackle is allowing to create a DNSServer object without zone. Should I add commits for this in this PR or make a new one?

Surely that's as simple as passing an empty list of zones/records?

If the change is bigger than that, best to create a new PR.

@azmeuk
Copy link
Contributor Author

azmeuk commented Sep 30, 2022

Surely that's as simple as passing an empty list of zones/records?

I was thinking of making the records be None by default. I will make another PR then.

@samuelcolvin
Copy link
Owner

ok.

@samuelcolvin samuelcolvin merged commit 574f95b into samuelcolvin:main Sep 30, 2022
@azmeuk azmeuk deleted the issue-14-dynamic-zone-update branch February 28, 2023 09:50
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