-
-
Notifications
You must be signed in to change notification settings - Fork 57
Allow updating multiple subdomains in single dyndns request #1112
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 updating multiple subdomains in single dyndns request #1112
Conversation
|
First, thanks for this great and significant contribution! Unfortuantely, while Thus, this PR either
Note that in the first case, the interface / URL syntax could remain untouched, but a 4xx response would have to be returned if FQDNs transcend the scope of one domain. In the second case, the migration would specifically have to be for the |
I see, I guess I could have noticed this while writing the tests, when multiple update requests are made to PowerDNS from a single change, e.g updating multiple domains (e.g. I could imagine that for some reason the second update fails and thus there would be difference between
Sounds reasonable, in case we need to introduce this restriction for now.
Yeah, that's definitely not something I would be able to do. Sounds like an interesting project though! Side note: Unfortunately I have problems getting the whole project to run locally, thus I wasn't really able to test the change fully (apart from the unit tests). |
|
@molikuner How would this work with a german full dual-stack internet socket?
How do you aggregate the combination of the IPv6-prefix + device identifier (either EUI64 or manually set) for each AAAA-record? Your proposal seems to be limited to one IPv6-address for all hosts. |
|
Hi @renne, If this PR is accepted, you should be able to configure the Fritz!Box using this: This should update EDIT: replaced domain names for clarity |
|
@peterthomassen I've looked a bit into restricting the update to one domain (but keep support for updating multiple subdomains). Would you prefer to "just" introduce a check somewhere to make sure only one domain is updated, or should I also "undo" the work of supporting multiple domains in the parsing / handling of the request parameters. This would include some changes inside the I would argue this decision depends on whether you would imagine this being useful in the future when the switch to Knot happens or you think it's too far away that too much will change until then. In the latter case it might make more sense to keep the code simple. |
|
I'd prefer the change to be minimal -- not because it might not be useful later, but because we have not meaningful way of testing the multi-domain aspects before doing the Knot migration. Thank you, and sorry for the extra work! |
6823195 to
efd95f3
Compare
No worries at all! I've just updated the PR to restrict updates to a single domain, still supporting updates of multiple subdomains as suggested. I've also adjusted the PR description accordingly. |
peterthomassen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks quite promising!
I've only reviewed the first commit, and will review the second once the first is stable. (I also haven't look at tests yet; will do in the next iteration.)
molikuner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peterthomassen Thanks for the review. I'll try to fix the issues in the coming days. Would be nice to already get some responses on some followup questions that I have added in the corresponding threads.
Thanks. Please comment here explicitly when done, so I know when to take a look.
See above! Let me know if anything else is unclear. |
efd95f3 to
62d989e
Compare
|
@peterthomassen I think I've addressed all your comments and updated the code accordingly. Please have another look 😄 |
62d989e to
721b4e6
Compare
721b4e6 to
c5eb211
Compare
|
What are the open steps needed for this change to be available on dedyn.io? |
|
I need time to finish the code review, and then do the deployment. I guess it'll be done in about 1 week max. |
Thank you very much for the information. |
c5eb211 to
2807a5f
Compare
|
I've rebased and polished the code (and in the process modernized some of the code we touched anyway). Please take a look at this fixup 2807a5f and let me know what you think. We can then merge this. Let's move your second commit to a different PR. I made a backup in the |
peterthomassen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, but @nils-wisiol's opinion appreciated
|
Thanks @peterthomassen! The fixup looks good 👍. I'll work on rebasing the other commit and opening a new PR once the current one is merged. |
nils-wisiol
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, thanks! Looking forward to have this deployed. Found a couple of nits.
Updates the requirements on [requests](https://github.com/psf/requests) to permit the latest version. - [Release notes](https://github.com/psf/requests/releases) - [Changelog](https://github.com/psf/requests/blob/main/HISTORY.md) - [Commits](psf/requests@v2.32.4...v2.32.5) --- updated-dependencies: - dependency-name: requests dependency-version: 2.32.5 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]>
Updates the requirements on [coverage](https://github.com/nedbat/coveragepy) to permit the latest version. - [Release notes](https://github.com/nedbat/coveragepy/releases) - [Changelog](https://github.com/nedbat/coveragepy/blob/master/CHANGES.rst) - [Commits](coveragepy/coveragepy@7.10.4...7.10.6) --- updated-dependencies: - dependency-name: coverage dependency-version: 7.10.6 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]>
Updates the requirements on [cryptography](https://github.com/pyca/cryptography) to permit the latest version. - [Changelog](https://github.com/pyca/cryptography/blob/main/CHANGELOG.rst) - [Commits](pyca/cryptography@45.0.6...45.0.7) --- updated-dependencies: - dependency-name: cryptography dependency-version: 45.0.7 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]>
Updates the requirements on [psycopg[binary]](https://github.com/psycopg/psycopg) to permit the latest version. - [Changelog](https://github.com/psycopg/psycopg/blob/master/docs/news.rst) - [Commits](psycopg/psycopg@3.2.9...3.2.10) --- updated-dependencies: - dependency-name: psycopg[binary] dependency-version: 3.2.10 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]>
Updates the requirements on [django-cors-headers](https://github.com/adamchainz/django-cors-headers) to permit the latest version. - [Changelog](https://github.com/adamchainz/django-cors-headers/blob/main/CHANGELOG.rst) - [Commits](adamchainz/django-cors-headers@4.7.0...4.8.0) --- updated-dependencies: - dependency-name: django-cors-headers dependency-version: 4.8.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]>
2807a5f to
7f753a9
Compare
|
The follow-up PR can be found at #1141. |
@molikuner Sorry to dig this up. I just stumbled upon this use-case and I am glad you put the effort into this PR :). Is the quotation still accurate? Because I can't seem to get it working. Suppose I have the following DNS state:
Now, suppose my router ( # line breaks are added for the sake of readability
curl -4 --user dyndns.mydomain.tld:mytoken \
"https://update.dedyn.io/?hostname=location.dyndns.mydomain.tld,host.location.dyndns.mydomain.tld
&ipv4=55.44.33.22
&location.dyndns.mydomain.tld.ipv6=fd08:2d7e:b40a:6d00:fd61:75a:fe5b:d565
&host.location.dyndns.mydomain.tld.ipv6=fdac:2812:b465::/48"Then I would expect the following state (changes in bold):
However, this is the state I actually get (i.e. AAAA records are removed entirely)
When using |
|
Hey @schnz, At the moment only the support for "Multi-Domain Updates" is implemented. The support for "Specific-Domain Overwrites" in the same request was moved into #1141 and isn't merged (yet). Depending on your needs, you might be able to already replicate your desired setup, e.g. if it's fine for you to not have any IPv6 address for |
What does this PR do?
This PR introduces the possibility to update multiple subdomains in a single dyndns request.
Multi-Domain Updates
The update endpoint now allows passing a comma separated list of domains in places of the
hostnameandhost_idrequest parameter, http basic auth username and theusernamerequest parameter. This allows updatingwww.example.comandhost.example.comat the same time using a single update request, e.g. if for whatever reason a CNAME record can't be used.Specific-Domain Overwrite
In addition, the update endpoint now supports parameters overwriting the IP-address for a specific domain. E.g. it would be possible to update
www.example.com,host.example.comandother.example.comwith the same IPv4 address, but overwrite the IPv6 address forother.example.comin a single request. This is achieved by supporting new request parameters in the form of{domain}.{parameter-name}, e.g. the parameterother.example.com.ipv6could be used to specify the IPv4 address ofother.example.com, while the other updated domains fallback to the value given inipv6.Example
A full request could look like this (assuming you own
example.com):This would do the following updates:
How is this implemented?
This change was implemented in two parts:
Supporting Multi-Domain Updates
This change adjusts the
DynDNS12UpdateViewto support parsing multiple subdomains from the request by splitting the domain name by comma. Afterwards it checks whether all given subdomains belong to a single domain. If that's not the case, a 400 bad request is returned to the caller. Internally the view had to now do more than one update in a single transaction.Supporting Specific-Domain Overwrites
This change again adjusts the
DynDNS12UpdateViewto search the request parameters for the new{domain}.{parameter-name}parameters and respect them in case they were provided. In case none of the new parameters was provided, a fallback to the "global"{parameter-name}parameters is used. Thus this change is backwards compatible and could technically be released also at a later point in time.References
** Though I would argue that this will solve 99% of the requests. E.g. people transitioning from dynv6.com, which was mentioned multiple times in the issue, would be able to fully replicate their update since all updated subdomains belong to a single domain in their API.
Further Considerations
This PR contains 2 commits to make the review easier. If required it could also be split into different PRs.