From b2ff68386821eef8e7d67bfa879cb63f0e283514 Mon Sep 17 00:00:00 2001 From: Paddy Date: Fri, 25 Aug 2017 02:08:35 -0700 Subject: [PATCH 1/3] dns: Add special handling for ns records. Cloud DNS requires every managed zone to have an NS record at all times. This means if people want to manage their own NS records, we need to add their new record and remove the old one in the same call. It also means we can't delete NS records, as we wouldn't know what to replace it with. So deleting of NS records short-circuits. For the case of `terraform destroy`, this prevents the error. It does mean if the user explicitly tries to remove their NS zone from their project, it silently does nothing, but that's unavoidable unless we want to A) restore a default value (and it looks like the default values change from zone to zone? And that is arguably just as unexpected?) or B) let the (arguably more reasonable) `terraform destroy` case be impossible. --- google/resource_dns_record_set.go | 26 +++++++++++++++++++ google/resource_dns_record_set_test.go | 35 ++++++++++++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/google/resource_dns_record_set.go b/google/resource_dns_record_set.go index 0f322bd862c..5879504068d 100644 --- a/google/resource_dns_record_set.go +++ b/google/resource_dns_record_set.go @@ -77,6 +77,25 @@ func resourceDnsRecordSetCreate(d *schema.ResourceData, meta interface{}) error }, } + if d.Get("type").(string) == "NS" { + log.Printf("{DEBUG] DNS record list request for %q", zone) + res, err := config.clientDns.ResourceRecordSets.List(project, zone).Do() + if err != nil { + return fmt.Errorf("Error retrieving record sets for %q: %s", zone, err) + } + var deletions []*dns.ResourceRecordSet + + for _, record := range res.Rrsets { + if record.Type != "NS" { + continue + } + deletions = append(deletions, record) + } + if len(deletions) > 0 { + chg.Deletions = deletions + } + } + log.Printf("[DEBUG] DNS Record create request: %#v", chg) chg, err = config.clientDns.Changes.Create(project, zone, chg).Do() if err != nil { @@ -135,6 +154,13 @@ func resourceDnsRecordSetRead(d *schema.ResourceData, meta interface{}) error { } func resourceDnsRecordSetDelete(d *schema.ResourceData, meta interface{}) error { + + // NS records must always have a value, so we short-circuit delete + // this allows terraform delete to work, but may have unexpected + // side-effects when deleting just that record set. + if d.Get("type").(string) == "NS" { + return nil + } config := meta.(*Config) project, err := getProject(d, config) diff --git a/google/resource_dns_record_set_test.go b/google/resource_dns_record_set_test.go index 35e1ac34761..691c24c9f68 100644 --- a/google/resource_dns_record_set_test.go +++ b/google/resource_dns_record_set_test.go @@ -84,6 +84,24 @@ func TestAccDnsRecordSet_changeType(t *testing.T) { }) } +func TestAccDnsRecordSet_ns(t *testing.T) { + zoneName := fmt.Sprintf("dnszone-test-ns-%s", acctest.RandString(10)) + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckDnsRecordSetDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: testAccDnsRecordSet_ns(zoneName, 300), + Check: resource.ComposeTestCheckFunc( + testAccCheckDnsRecordSetExists( + "google_dns_record_set.foobar", zoneName), + ), + }, + }, + }) +} + func testAccCheckDnsRecordSetDestroy(s *terraform.State) error { config := testAccProvider.Meta().(*Config) @@ -151,6 +169,23 @@ func testAccDnsRecordSet_basic(zoneName string, addr2 string, ttl int) string { `, zoneName, addr2, ttl) } +func testAccDnsRecordSet_ns(name string, ttl int) string { + return fmt.Sprintf(` + resource "google_dns_managed_zone" "parent-zone" { + name = "%s" + dns_name = "hashicorptest.com." + description = "Test Description" + } + resource "google_dns_record_set" "foobar" { + managed_zone = "${google_dns_managed_zone.parent-zone.name}" + name = "hashicorptest.com." + type = "NS" + rrdatas = ["ns.hashicorp.services.", "ns2.hashicorp.services."] + ttl = %d + } + `, name, ttl) +} + func testAccDnsRecordSet_bigChange(zoneName string, ttl int) string { return fmt.Sprintf(` resource "google_dns_managed_zone" "parent-zone" { From 0e2fa2e38f932dc72237412b87f58bc69dcfc7b8 Mon Sep 17 00:00:00 2001 From: Paddy Date: Tue, 24 Oct 2017 14:43:35 -0700 Subject: [PATCH 2/3] Address comments. Fix typo, add log line, and document what we're doing on the website. --- google/resource_dns_record_set.go | 3 ++- website/docs/r/dns_record_set.markdown | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/google/resource_dns_record_set.go b/google/resource_dns_record_set.go index 5879504068d..f20333a4e76 100644 --- a/google/resource_dns_record_set.go +++ b/google/resource_dns_record_set.go @@ -78,7 +78,7 @@ func resourceDnsRecordSetCreate(d *schema.ResourceData, meta interface{}) error } if d.Get("type").(string) == "NS" { - log.Printf("{DEBUG] DNS record list request for %q", zone) + log.Printf("[DEBUG] DNS record list request for %q", zone) res, err := config.clientDns.ResourceRecordSets.List(project, zone).Do() if err != nil { return fmt.Errorf("Error retrieving record sets for %q: %s", zone, err) @@ -159,6 +159,7 @@ func resourceDnsRecordSetDelete(d *schema.ResourceData, meta interface{}) error // this allows terraform delete to work, but may have unexpected // side-effects when deleting just that record set. if d.Get("type").(string) == "NS" { + log.Println("[DEBUG] NS records can't be deleted due to API restrictions, so they're being left in place. See https://www.terraform.io/docs/providers/google/r/dns_record_set.html for more information.") return nil } config := meta.(*Config) diff --git a/website/docs/r/dns_record_set.markdown b/website/docs/r/dns_record_set.markdown index 90d93591c17..6bb2a44cb22 100644 --- a/website/docs/r/dns_record_set.markdown +++ b/website/docs/r/dns_record_set.markdown @@ -10,6 +10,12 @@ description: |- Manages a set of DNS records within Google Cloud DNS. +~> **Note:** The Google Cloud DNS API requires NS records be present at all +times. To accommodate this, when creating NS records, the default records +Google automatically creates will be silently overwritten. Also, when +destroying NS records, Terraform will not actually remove NS records, but will +report that it did. + ## Example Usage ### Binding a DNS name to the ephemeral IP of a new instance: From 4a342ca8eec32039ad2003d92bc3802834ac910d Mon Sep 17 00:00:00 2001 From: Paddy Date: Tue, 7 Nov 2017 16:42:25 -0800 Subject: [PATCH 3/3] Add comment for create func. Add a comment explaining why we have such wonky update logic in the create func for NS record sets. --- google/resource_dns_record_set.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/google/resource_dns_record_set.go b/google/resource_dns_record_set.go index f20333a4e76..743aed57cb6 100644 --- a/google/resource_dns_record_set.go +++ b/google/resource_dns_record_set.go @@ -77,6 +77,14 @@ func resourceDnsRecordSetCreate(d *schema.ResourceData, meta interface{}) error }, } + // we need to replace NS record sets in the same call. That means + // we need to list all the current NS record sets attached to the + // zone and add them to the change as deletions. We can't just add + // new NS record sets, or we'll get an error about the NS record set + // already existing; see terraform-providers/terraform-provider-google#95. + // We also can't just remove the NS recordsets on creation, as at + // least one is required. So the solution is to "update in place" by + // putting the addition and the removal in the same API call. if d.Get("type").(string) == "NS" { log.Printf("[DEBUG] DNS record list request for %q", zone) res, err := config.clientDns.ResourceRecordSets.List(project, zone).Do()