tailscale: use V2 client for dns split nameservers resource #419
Conversation
oxtoacart
force-pushed
the
percy/dns-split-nameservers-client-v2
branch
from
8749b41
to
cc0b766
Compare
| @@ -24,6 +24,7 @@ func resourceDNSSplitNameservers() *schema.Resource { | |||
| Type: schema.TypeString, | |||
| Description: "Domain to configure split DNS for. Requests for this domain will be resolved using the provided nameservers.", | |||
| Required: true, | |||
| ForceNew: true, | |||
There was a problem hiding this comment.
Drive-by: I noticed that this provider uses UpdateSplitDNS(), which is a patch operation. This means that the resource is not setting all split DNS settings for the tailnet, but just those for a specific domain. As originally implemented, if the domain changes, the settings for the previous domain will be left alone. That seems confusing.
Using ForceNew here ensures that the settings for the previous domain are removed, so that the state of the system accurately reflects what's configured in Terraform.
There was a problem hiding this comment.
I'm going to wait for @mpminardi to chime in before merging, since this is a behavior change. From one perspective, it might be fixing buggy behavior, but possibly people could be relying on that buggy behavior ...
There was a problem hiding this comment.
Whoops, good catch! Agree that this is buggy / unintended behaviour here that we should fix. Feel like this should be a safe change since we are essentially leaving a dangling resource outside of Terraform's control / lifecycle otherwise.
| } | ||
|
|
||
| d.SetId(domain) | ||
| return resourceSplitDNSNameserversRead(ctx, d, m) | ||
| } | ||
|
|
||
| func resourceSplitDNSNameserversUpdate(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { | ||
| if !d.HasChange("nameservers") { |
There was a problem hiding this comment.
Because of this, we no longer need to check for changes to nameservers.
There was a problem hiding this comment.
Hmm I'm not deeply familiar with the underlying API but I don't quite follow; "nameservers" doesn't have ForceNew: true, so it seems like this field could be updated without recreating the resource?
There was a problem hiding this comment.
There are only two fields, domain and nameservers. Now that domain is ForceNew, any change to domain means that we'll create a new resource. If we're in resourceSplitDNSNameserversCreateOrUpdate and domain didn't change, that by definition means that nameservers must have changed, so there's no need to check for that explicitly.
oxtoacart
force-pushed
the
percy/dns-split-nameservers-client-v2
branch
3 times, most recently
from
6a938ac
to
d114b1a
Compare
| } | ||
|
|
||
| d.SetId(domain) | ||
| return resourceSplitDNSNameserversRead(ctx, d, m) | ||
| } | ||
|
|
||
| func resourceSplitDNSNameserversUpdate(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { | ||
| if !d.HasChange("nameservers") { |
There was a problem hiding this comment.
Hmm I'm not deeply familiar with the underlying API but I don't quite follow; "nameservers" doesn't have ForceNew: true, so it seems like this field could be updated without recreating the resource?
oxtoacart
force-pushed
the
percy/dns-split-nameservers-client-v2
branch
from
d114b1a
to
0d86672
Compare
There was a problem hiding this comment.
LGTM but agreed it might be prudent to wait for @mpminardi's input too.
| if _, err := client.UpdateSplitDNS(ctx, req); err != nil { | ||
| return diagnosticsError(err, "Failed to set dns split nameservers") | ||
| if _, err := client.DNS().UpdateSplitDNS(ctx, req); err != nil { | ||
| return diagnosticsError(err, "Failed to set create dns split nameservers") |
There was a problem hiding this comment.
Was this error line intentionally changed?
There was a problem hiding this comment.
I've recently taken to differentiating between create and update flows. Of course, in this case, they're the same code. Will change back.
There was a problem hiding this comment.
Oh, the message had both create and set, oops :)
Updates tailscale/corp#21867 Signed-off-by: Percy Wegmann <[email protected]>
oxtoacart
force-pushed
the
percy/dns-split-nameservers-client-v2
branch
from
0d86672
to
22c1b45
Compare
Updates tailscale/corp#21867
Depends on tailscale/tailscale-client-go#104