Skip to content

FEATURE: Create zones during preview (Disable with --populate-on-preview=false) #3337

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

Merged
merged 2 commits into from
Jan 14, 2025

Conversation

das7pad
Copy link
Collaborator

@das7pad das7pad commented Jan 9, 2025

This PR is changing the order for creating zones to happen before any data is gathered for the zone (i.e. driver.GetNameservers / driver.GetZoneRecords calls).

Any corrections for creating zones will be displayed/executed in a separate loop before the main loop for gathering corrections kicks off. This zone-check loop is silent unless any provider has corrections.

Zones will get created on preview already to show a complete picture right away (opt-out via --populate-on-preview=false).

With these changes, a single dnscontrol push invocation is sufficient for bootstrapping a zone with HETZNER (and most other providers) 🎉

Demo with HETZNER and two zones

Before

$ dnscontrol preview
CONCURRENTLY gathering 2 zone(s)
SERIALLY gathering 0 zone(s)
Waiting for concurrent gathering(s) to complete...DONE
******************** Domain: testing-2025-01-08-11.dev
#1: Ensuring zone "testing-2025-01-08-11.dev" exists in "HETZNER"
INFO#1: Domain "testing-2025-01-08-11.dev" provider HETZNER Error: "testing-2025-01-08-11.dev" is not a zone in this HETZNER account
INFO#1: DetermineNS: zone "testing-2025-01-08-11.dev"; Error: error while getting Nameservers for zone="testing-2025-01-08-11.dev" with provider="HETZNER": "testing-2025-01-08-11.dev" is not a zone in this HETZNER account
******************** Domain: testing-2025-01-08-12.dev
#1: Ensuring zone "testing-2025-01-08-12.dev" exists in "HETZNER"
INFO#1: Domain "testing-2025-01-08-12.dev" provider HETZNER Error: "testing-2025-01-08-12.dev" is not a zone in this HETZNER account
INFO#1: DetermineNS: zone "testing-2025-01-08-12.dev"; Error: error while getting Nameservers for zone="testing-2025-01-08-12.dev" with provider="HETZNER": "testing-2025-01-08-12.dev" is not a zone in this HETZNER account
Done. 0 corrections.


// repeat
$ dnscontrol preview
// same output as about

A lot of errors and nothing is happening.

$ dnscontrol push
CONCURRENTLY gathering 2 zone(s)
SERIALLY gathering 0 zone(s)
Waiting for concurrent gathering(s) to complete...DONE
******************** Domain: testing-2025-01-08-9.dev
#1: Ensuring zone "testing-2025-01-08-9.dev" exists in "HETZNER"
SUCCESS!
INFO#1: Domain "testing-2025-01-08-9.dev" provider HETZNER Error: "testing-2025-01-08-9.dev" is not a zone in this HETZNER account
INFO#1: DetermineNS: zone "testing-2025-01-08-9.dev"; Error: error while getting Nameservers for zone="testing-2025-01-08-9.dev" with provider="HETZNER": "testing-2025-01-08-9.dev" is not a zone in this HETZNER account
******************** Domain: testing-2025-01-08-10.dev
#1: Ensuring zone "testing-2025-01-08-10.dev" exists in "HETZNER"
SUCCESS!
INFO#1: Domain "testing-2025-01-08-10.dev" provider HETZNER Error: "testing-2025-01-08-10.dev" is not a zone in this HETZNER account
INFO#1: DetermineNS: zone "testing-2025-01-08-10.dev"; Error: error while getting Nameservers for zone="testing-2025-01-08-10.dev" with provider="HETZNER": "testing-2025-01-08-10.dev" is not a zone in this HETZNER account
Done. 0 corrections.


// repeat
$ dnscontrol push
CONCURRENTLY gathering 2 zone(s)
SERIALLY gathering 0 zone(s)
Waiting for concurrent gathering(s) to complete...DONE
******************** Domain: testing-2025-01-08-9.dev
4 corrections (HETZNER)
#1: Batch creation of records:
	+ CREATE NS testing-2025-01-08-9.dev helium.ns.hetzner.de. ttl=300
	+ CREATE NS testing-2025-01-08-9.dev hydrogen.ns.hetzner.com. ttl=300
	+ CREATE NS testing-2025-01-08-9.dev oxygen.ns.hetzner.com. ttl=300
	+ CREATE A testing-2025-01-08-9.dev 127.0.0.1 ttl=300
SUCCESS!
******************** Domain: testing-2025-01-08-10.dev
4 corrections (HETZNER)
#1: Batch creation of records:
	+ CREATE NS testing-2025-01-08-10.dev helium.ns.hetzner.de. ttl=300
	+ CREATE NS testing-2025-01-08-10.dev hydrogen.ns.hetzner.com. ttl=300
	+ CREATE NS testing-2025-01-08-10.dev oxygen.ns.hetzner.com. ttl=300
	+ CREATE A testing-2025-01-08-10.dev 127.0.0.6 ttl=300
SUCCESS!
Done. 8 corrections.


// repeat
$ dnscontrol push
CONCURRENTLY gathering 2 zone(s)
SERIALLY gathering 0 zone(s)
Waiting for concurrent gathering(s) to complete...DONE
******************** Domain: testing-2025-01-08-9.dev
******************** Domain: testing-2025-01-08-10.dev
Done. 0 corrections.

A lot of errors, the zone gets created in the 1st pass. The 2nd pass actually creates resources in the zone.

After

$ dnscontrol preview
******************** Domain: testing-2025-01-08-5.dev
1 correction (HETZNER)
#1: Ensuring zone "testing-2025-01-08-5.dev" exists in "HETZNER"
SUCCESS!
******************** Domain: testing-2025-01-08-6.dev
1 correction (HETZNER)
#1: Ensuring zone "testing-2025-01-08-6.dev" exists in "HETZNER"
SUCCESS!
CONCURRENTLY gathering 2 zone(s)
SERIALLY gathering 0 zone(s)
Waiting for concurrent gathering(s) to complete...DONE
******************** Domain: testing-2025-01-08-5.dev
4 corrections (HETZNER)
#1: Batch creation of records:
	+ CREATE NS testing-2025-01-08-5.dev helium.ns.hetzner.de. ttl=300
	+ CREATE NS testing-2025-01-08-5.dev hydrogen.ns.hetzner.com. ttl=300
	+ CREATE NS testing-2025-01-08-5.dev oxygen.ns.hetzner.com. ttl=300
	+ CREATE A testing-2025-01-08-5.dev 127.0.0.1 ttl=300
******************** Domain: testing-2025-01-08-6.dev
4 corrections (HETZNER)
#1: Batch creation of records:
	+ CREATE NS testing-2025-01-08-6.dev helium.ns.hetzner.de. ttl=300
	+ CREATE NS testing-2025-01-08-6.dev hydrogen.ns.hetzner.com. ttl=300
	+ CREATE NS testing-2025-01-08-6.dev oxygen.ns.hetzner.com. ttl=300
	+ CREATE A testing-2025-01-08-6.dev 127.0.0.6 ttl=300
Done. 10 corrections.


// repeat
$ dnscontrol preview
CONCURRENTLY gathering 2 zone(s)
SERIALLY gathering 0 zone(s)
Waiting for concurrent gathering(s) to complete...DONE
******************** Domain: testing-2025-01-08-5.dev
4 corrections (HETZNER)
#1: Batch creation of records:
	+ CREATE NS testing-2025-01-08-5.dev helium.ns.hetzner.de. ttl=300
	+ CREATE NS testing-2025-01-08-5.dev hydrogen.ns.hetzner.com. ttl=300
	+ CREATE NS testing-2025-01-08-5.dev oxygen.ns.hetzner.com. ttl=300
	+ CREATE A testing-2025-01-08-5.dev 127.0.0.1 ttl=300
******************** Domain: testing-2025-01-08-6.dev
4 corrections (HETZNER)
#1: Batch creation of records:
	+ CREATE NS testing-2025-01-08-6.dev helium.ns.hetzner.de. ttl=300
	+ CREATE NS testing-2025-01-08-6.dev hydrogen.ns.hetzner.com. ttl=300
	+ CREATE NS testing-2025-01-08-6.dev oxygen.ns.hetzner.com. ttl=300
	+ CREATE A testing-2025-01-08-6.dev 127.0.0.6 ttl=300
Done. 8 corrections.

No errors! The first pass creates the zone. The 2nd pass is just the preview, no noise from zone checking.

$ dnscontrol push
******************** Domain: testing-2025-01-08-7.dev
1 correction (HETZNER)
#1: Ensuring zone "testing-2025-01-08-7.dev" exists in "HETZNER"
SUCCESS!
******************** Domain: testing-2025-01-08-8.dev
1 correction (HETZNER)
#1: Ensuring zone "testing-2025-01-08-8.dev" exists in "HETZNER"
SUCCESS!
CONCURRENTLY gathering 2 zone(s)
SERIALLY gathering 0 zone(s)
Waiting for concurrent gathering(s) to complete...DONE
******************** Domain: testing-2025-01-08-7.dev
4 corrections (HETZNER)
#1: Batch creation of records:
	+ CREATE NS testing-2025-01-08-7.dev helium.ns.hetzner.de. ttl=300
	+ CREATE NS testing-2025-01-08-7.dev hydrogen.ns.hetzner.com. ttl=300
	+ CREATE NS testing-2025-01-08-7.dev oxygen.ns.hetzner.com. ttl=300
	+ CREATE A testing-2025-01-08-7.dev 127.0.0.1 ttl=300
SUCCESS!
******************** Domain: testing-2025-01-08-8.dev
4 corrections (HETZNER)
#1: Batch creation of records:
	+ CREATE NS testing-2025-01-08-8.dev helium.ns.hetzner.de. ttl=300
	+ CREATE NS testing-2025-01-08-8.dev hydrogen.ns.hetzner.com. ttl=300
	+ CREATE NS testing-2025-01-08-8.dev oxygen.ns.hetzner.com. ttl=300
	+ CREATE A testing-2025-01-08-8.dev 127.0.0.6 ttl=300
SUCCESS!
Done. 10 corrections.


// repeat
$ dnscontrol push
CONCURRENTLY gathering 2 zone(s)
SERIALLY gathering 0 zone(s)
Waiting for concurrent gathering(s) to complete...DONE
******************** Domain: testing-2025-01-08-7.dev
******************** Domain: testing-2025-01-08-8.dev
Done. 0 corrections.

A single pass is sufficient for creating the zone, figuring out nameservers and zone records. 🎉

Closes #3007

@das7pad
Copy link
Collaborator Author

das7pad commented Jan 9, 2025

Initially I was planning on invoking driver.EnsureZoneExists from the (concurrent) gathering loop. Printing concurrently is not very nice and also requires a lot of extra parameters to be passed through. So I went with another loop, similar to the current phase 1 and 2.

Side note: The race-condition as fixed in #3328 is no longer an issue; the improved cache handling when creating zones is still worth landing.

Copy link
Contributor

@tlimoncelli tlimoncelli left a comment

Choose a reason for hiding this comment

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

Code: looks good
Testing: I'll get around to that later this week or early next week.

Design alternative suggestion: Maybe Zone caching should be done globally instead? I see each provider has a mutex-protected cache. Expecting every provider to get this right seems like a risk. Any time we make all providers do the same kind of work, it's better to move that code to the main DNSControl system.

That is... Maybe a better interface would be to treat ZoneNames like we do other things with GetFOO() and GetFOOCorrections().

What would this look like?

Providers would provide 2 functions:

  • GetZoneNames() which returns a []string. No mutexes, etc. just do the work and return a list of strings.
  • GetZoneNamesCorrections(). We send it a []string of zones to be created; it returns []Corrections that create those zones.

The main DNSControl code would call those functions, store the data, handling all mutexes, etc. as needed.

Thoughts?

@das7pad
Copy link
Collaborator Author

das7pad commented Jan 9, 2025

Thanks for the feedback, Tom!

Multiple drivers need details from the zone listing for internal usage. A few examples off the top of my head:

  • very common: zone name to provider ID mapping
  • default TTL for records, per zone
  • nameservers, per zone

Drivers need access to that information from various external and internal methods, so they need a place to "cache" this structured information to avoid duplicate provider API calls.

Idea: Go-Generics are GA now, we could provide a simple caching primitive that drivers can use:

func NewZoneCache[Zone any](fetchAll func() (map[string]Zone, error)) ZoneCache[Zone]
type ZoneCache [Zone any] struct {
  mu sync.Mutex
  cache map[string]Zone
  fetchAll func() (map[string]Zone, error)
}
func (z *ZoneCache) GetZone(name string) (Zone, error)
func (z *ZoneCache) GetZoneNames() ([]string, error)
func (z *ZoneCache) AddZone(name string, zone Zone)

type myZone struct {
  ID string
  TTL int
}
type myDriver struct {
  zoneCache ZoneCache[myZone]
}
func (d *myDriver) fetchZones (map[string]myZone, error)
func NewDriver() {
  d := myDriver{}
  d.zoneCache = NewZoneCache(d.fetchZones)
}

With that said, I like your suggestion for refactoring the ZoneCreator interface and adopting the Corrections model for change management of zones.

@tlimoncelli
Copy link
Contributor

Good point! Providers do need more than just a []string of zone names. I hadn't thought of that.

I like your idea of using generics! Let's give that a try.

Some thoughts (feel free to ignore any of these)

  • I'm trying to standardize the filenames in the source code. If this is an opportunity to move zone-related code to dnscontrol/providers/*/listzones.go ... let's take the opportunity.

  • I'd be glad to help test any of these: BIND, cloudflare, gandi, gcloud, azure, route53, namedotcom, gandi_v5. My employer, Stack Overflow, uses Azure, Google DNS, and Cloudflare heavily. Would be glad to test them. I might even be convinced to convert some of them if you do a few first to show me how.

Thanks!

@das7pad
Copy link
Collaborator Author

das7pad commented Jan 11, 2025

I've added HasZone(name string) (bool, error) to the interface. This greatly simplifies the not-found case in EnsureZoneExists -- the alternative is having this in each provider: _,err := driver.zoneCache.GetZone(name); err==nil || err!=ErrZoneNotFound 🙈 .

I've tested the changes together with the other PRs for Cloudflare and Hetzner: I ran the integration tests for both (CF: worker/redirect tests skipped). I've manually tested pushing with a single new zone first and then an existing plus a new zone. All working as expected 🎉

@tlimoncelli
Copy link
Contributor

Hi @das7pad

Please rebase and I'll try to merge this before we get more conflicts (oops!)

How does this relate to the other PRs you've submitted recently? sorry but I've gotten a little lost and I'm not sure what order to merge things.

Any corrections for creating zones will be displayed/executed in a
separate loop before the main loop for gathering corrections kicks off.
This zone-check loop is silent unless any provider has corrections.

Zones will get created on preview already to show a complete picture
right away (opt-out via --populate-on-preview=false).
@das7pad
Copy link
Collaborator Author

das7pad commented Jan 14, 2025

Please rebase and I'll try to merge this before we get more conflicts (oops!)

No worries. I've rebased the changes and squashed the copy change for the flag.

How does this relate to the other PRs you've submitted recently? sorry but I've gotten a little lost and I'm not sure what order to merge things.

This PR ensures that zones get created as the first step of processing zones. Compatible drivers [1] can then be used with dnscontrol preview to create zones and use these newly created zones for producing corrections and for dnscontrol push to applying them in a single invocation of dnscontrol (see "> Demo with HETZNER and two zones" in this PRs description for a demo).

Here, a "compatible driver" [1] is a provider without a zone cache, or with a zone cache implementation that either populates the cache with any newly creates zones; or (properly) resets the cache after creating zones.
The other driver PRs make all the drivers compatible in this regard (and/or save some API calls). They are independent from this change, in the sense that they are not breaking the driver before merging this PR; and when merging this PR first and not merging the driver PRs, dnscontrol preview/dnscontrol push behaves as before and a 2nd run in required to get corrections.

The ZoneCache idea using generics to allow each driver to use a custom zone struct with a shared cache implementation is perhaps something for future work? We could also land the new package (das7pad@3533d31) and this PR, then use the already opened driver PRs to adopt ZoneCache (i.e. das7pad@c7789cc or das7pad@85e7965).

Does that help?

@tlimoncelli
Copy link
Contributor

tlimoncelli commented Jan 14, 2025

Yes! Very helpful!

Sounds like I can merge these in any order (which is brilliant of you!). However the optimal order is this PR and the ZoneCache PR (I don't see a PR related to https://github.com/das7pad/dnscontrol/tree/zone-cache... has it been created?). Then the other PRs can be rewritten to use ZoneCache and be merged independently.

Did I get that right? (I hope so!)

Tom

@tlimoncelli tlimoncelli changed the title BUG: Create zones ahead of gathering data FEATURE: Create zones during preview (Disable with --populate-on-preview=false) Jan 14, 2025
@tlimoncelli tlimoncelli merged commit 556926a into StackExchange:main Jan 14, 2025
20 checks passed
@das7pad
Copy link
Collaborator Author

das7pad commented Jan 15, 2025

Awesome 🎉 I've opened #3365 with the ZoneCache implementation (excluding the driver changes).

This was referenced Jan 15, 2025
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.

Preview of records if zone doesn't exist yet
2 participants