Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
My proposal to, well, add ipv6 servers to all zones in the pool.
Nothing here is carved in stone and I am looking forward for constructive feedback and discussions about it and possible alternatives.
Technical description
I parametrized the code that was used to distribute the IPv4 records across the zones. Then I wrote a loop around it to iterate over both Ipv4 and IPv6.
At the end I removed the now obsolete section that put IPv6 records in the 2. zones.
While the change view of Github does not acurately reflect it, only very few lines were actually changed - most just got indented.
I have successfully tested the changes on my pool development instance, albeit with much smaller amounts of servers than the live pool has.
Reasoning
The wish to enable IPv6 has been discussed at length at various times and spaces, most prominently in this forum thread.
Both IP versions should be handled without regard to the other one, since IPv4-only and IPv6-only clients exist. Using the same algorithm for both IP versions keeps the code simple and optimizes the distribution of servers in both adress spaces.
Change impacts
This change will increase the total amount of records contained in an ntppool zone file. Since those files are only generated and transferred sporadically, this won't have much of an impact.
The obvious impact and goal of this change is that pool zones that have so far only returned A records will now start returning AAAA records as well. This will change nothing for IPv4-only clients, and enable using all pool zones for IPv6-only clients. For dual stack clients, this change will increase the number of servers returned from the pool.
Known discussion points and my take on them
Since this change will increase the number of servers that are dealt out at the "default" unnumbered zones, this change will hopefully actually help to mitigate this. A better distribution algorithm is a good idea, but at least in my opinion an independent change from enabling IPv6. Waiting for a new distribution approach is, from my point of view, not necessary. Not having IPv6 has shown to have its own problems, and I think that overall this PR will solve more problems than it creates.
This is a curios topic. From my understanding, a properly configured client in a properly configured network should either be able to use DNS records requested by it, or discard them. So to introduce AAAA records should only negatively impact clients and networks that are arguably already broken.
On the other hand, the pool is a fixture in the world of internet services and serves millions of clients that don't even know about it, so changes with the potential to break things should always be considered carefully.
In this special case, it would be interesting to know more about the background behind this topic, and if it is more a theoretical possibility or an actual hard requirement. If backwards compatibility for vendor zones is non-negotiable, I have outlined a plan for that here
Closing thoughts
As stated above, I am open to constructive feedback and discussions about this approach. Many people, including me, are looking forward to a fully IPv6-enabled NTP Pool. While this PR is a quite simple and straightforward attempt, I am open to commit the time and incorporate changes or to implement a different approach should the discussion lead there.
To better showcase the actual code changes, here is a picture taken in VS Code:
resolves #231