dns: Add special handling for ns records. #359
Conversation
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
Outdated
| @@ -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) | |||
| // 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" { |
There was a problem hiding this comment.
Let's put a [DEBUG] log here; Terraform is doing something "unexpected", or otherwise lying to people, so let's tell them that we aren't doing X because of Y, and link to some documentation. Either Google's or preferably some kind of NOTE on https://www.terraform.io/docs/providers/google/r/dns_record_set.html that explains the what and why here.
|
@paddycarver, what's the status on this? |
Fix typo, add log line, and document what we're doing on the website.
|
Sorry for the delay on this, believe I've addressed all the comments. |
There was a problem hiding this comment.
Looks good, just one quick thing (feel free to merge once you've addressed it, I don't need to re-review)
$ make testacc TEST=./google TESTARGS='-run=TestAccDnsRecordSet_ns'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccDnsRecordSet_ns -timeout 120m
=== RUN TestAccDnsRecordSet_ns
--- PASS: TestAccDnsRecordSet_ns (12.23s)
PASS
ok github.com/terraform-providers/terraform-provider-google/google 12.375s
google/resource_dns_record_set.go
Outdated
| @@ -77,6 +77,25 @@ func resourceDnsRecordSetCreate(d *schema.ResourceData, meta interface{}) error | |||
| }, | |||
| } | |||
|
|
|||
| if d.Get("type").(string) == "NS" { | |||
There was a problem hiding this comment.
Can you add a comment explaining why this section needs to happen? (even just a link to the docs is probably fine)
Add a comment explaining why we have such wonky update logic in the create func for NS record sets.
|
@danawillow just updated with the comment. Good to merge? |
|
Yup |
…s_ns dns: Add special handling for ns records.
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks! |
ghost
locked and limited conversation to collaborators
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 explicitlytries 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 destroycase be impossible.Fixes #95.