Add cname and UUID to tunnel #1176
Conversation
Clean up import/read logic
| attributes := strings.Split(d.Id(), "/") | ||
|
|
||
| if len(attributes) != 2 { | ||
| return fmt.Errorf("invalid id (\"%s\") specified, should be in format \"accountID/argoTunnelUUID\"", d.Id()) | ||
| } | ||
|
|
||
| accID, tunnelID := attributes[0], attributes[1] |
There was a problem hiding this comment.
this is Import specific behaviour, the Read doesn't need any of this. Read only syncs the remote API to the local state configuration.
| @@ -82,24 +111,8 @@ func resourceCloudflareArgoTunnelDelete(d *schema.ResourceData, meta interface{} | |||
| } | |||
|
|
|||
| func resourceCloudflareArgoTunnelImport(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) { | |||
| client := meta.(*cloudflare.API) | |||
There was a problem hiding this comment.
as mentioned at https://github.com/cloudflare/terraform-provider-cloudflare/pull/1176/files#r700663326, some of this functionality needs to stay as is as it's Import specific and Read doesn't use it.
|
we'll also need to update the website documentation to include these new attributes too (https://github.com/cloudflare/terraform-provider-cloudflare/blob/master/website/docs/r/argo_tunnel.html.markdown) |
| "uuid": { | ||
| Type: schema.TypeString, | ||
| Computed: true, | ||
| }, |
There was a problem hiding this comment.
we don't really need this; the resource ID is already this value.
|
thanks for the PR and i'm 👌 to ship the CNAME addition to help insulate any changes internally from being hardcoded on the customer end, but to your point that it's hard to use without these fields, we're not actually changing anything that dramatically so i'm not sure where the improvement is here. the resource "cloudflare_argo_tunnel" "my_tunnel" {
# .. snip
}
resource "cloudflare_record" "my_dns_record" {
# .. snip
value = "${cloudflare_argo_tunnel.my_tunnel.id}.argotunnel.com"
} |
|
@jacobbednarz thanks for the review! While I was testing it I pretty much proved your point that this achieves nothing. I got my wires crossed in how the import ID is represented as the union of account ID and tunnel ID, vs the actual ID just being the tunnel. I'll refactor this down into a much smaller addition that just exposes the CNAME for convenience. |
This reverts commit 4e1cdbd.
Updated the Read function to load the cname attribute as the easiest way to make sure all plans on existing resources will have the cname set.
Since tunnels are immutable, any attribute changes will force a new resource.
|
Couple options for doing this, I could either use a state migration to set the cname record on all resources, since it's deterministic from the ID, or do like I've done here and implement read to fetch and check. Terrraform plugin best practices are to read after a create/import anyways as an error check, so it's not wrong to do it this way, but maybe it's unnecessary considering it's an extra API call to check the state of an immutable resource. I could refactor this to use a state upgrade instead of the read, but I'm also starting to question if the whole thing is worth the convenience. Also in testing it seems that terraform doesn't realize that all the fields are immutable so I added the |
| @@ -97,6 +113,7 @@ func resourceCloudflareArgoTunnelImport(d *schema.ResourceData, meta interface{} | |||
| } | |||
|
|
|||
| d.Set("name", tunnel.Name) | |||
| d.Set("cname", fmt.Sprintf("%s.argotunnel.com", tunnel.ID)) | |||
There was a problem hiding this comment.
as this is happening in the Read and we're calling resourceCloudflareArgoTunnelRead at the end of this method, we don't really need this twice.
|
The change to expose
If you could elaborate, I'm happy to point you in the right direction. |
|
are you still interested in pursuing this? if so, we need to:
|
Supersedes #1176 Co-Authored-By: Andrew Burian <[email protected]>
Supersedes #1176 Co-Authored-By: Andrew Burian <[email protected]>
|
superseded by #1259 |
It's exceptionally hard to use named tunnels in conjunction with either DNS or load balancers without returning the UUID or the CNAME record of the named tunnel.
This PR exposes that CNAME and uuid directly as a computed attribute of the schema instead of having to try and extract it from the ID.
I also moved the import logic into read so that calls to read the tunnel will actually verify it exists, as well as set the CNAME attribute on existing resources that aren't already set.