Skip to content
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

Prefer using jsonencode for ACLs instead of heredocs #36

Closed
pokgak opened this issue Aug 30, 2021 · 3 comments · Fixed by #38
Closed

Prefer using jsonencode for ACLs instead of heredocs #36

pokgak opened this issue Aug 30, 2021 · 3 comments · Fixed by #38
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers

Comments

@pokgak
Copy link

pokgak commented Aug 30, 2021

Is your feature request related to a problem? Please describe.
Currently, the documentation for this provider uses heredocs (acl = <<EOF) for defining the ACLs. The official Terraform documentation said to prefer using the jsonencode function instead of heredoc for generating JSON.

Describe the solution you'd like
Change documentation to use jsonencode instead of heredoc.

Example (not tested):

resource "tailscale_acl" "sample_acl" {
  acl = jsonencode({
    "acls" = list(
      {
        "action" = "accept",
        "users"  = ["*"],
        "ports"  = ["*:*"],
      }
    ),
  })
}

Additional context
Not sure how this will work when support for hujson is added later though, it might make it more complicated than the proposed way in #17.

@pokgak pokgak added the enhancement New feature or request label Aug 30, 2021
@davidsbond
Copy link
Collaborator

Hey @pokgak, thanks for raising this.

I think what you've proposed for the documentation change is sensible. So I can raise a PR to update that.

Per your comment on #17, I currently have a branch I'm working on that gets the hujson work done without any significant changes.

Originally, the tailscale API required callers to specify the Content-Type depending on if you used hujson or regular json. This no longer appears to be the case so what I am currently working on just always assumes the ACL is in hujson, as the Tailscale hujson repo works equally for both JSON and hujson data.

What this means is, the ACL stored locally with the terraform files can have all the fancy features like comments, but what gets pushed to Tailscale itself won't have them (as they are lost when we encode/decode the ACL into an actual type).

I'm not sure how big an issue that is, as the ACL stored locally can still have all the comments. It would depend if users want the Tailscale dashboard to be the source of truth for the ACL or what they have locally with their terraform files. I think the latter is more likely what people would want. But am open to input.

For the use of jsonencode, it would mean you can't have your comments if you'd like to do your ACL the hujson way. But it will still work just fine. You could use the HCL comments instead. WDYT?

@davidsbond davidsbond added documentation Improvements or additions to documentation good first issue Good for newcomers and removed enhancement New feature or request labels Aug 30, 2021
@davidsbond
Copy link
Collaborator

Also, I did a quick test, and this is how you should do it:

resource "tailscale_acl" "homelab_acl" {
  acl = jsonencode({
    acls : [
      {
        action = "accept",
        users  = ["*"],
        ports  = ["*:*"],
    }],
  })
}

davidsbond added a commit that referenced this issue Aug 30, 2021
This commit modifies the example/documentation for ACLs to use the `jsonencode` function
instead of heredocs.

Closes #36

Signed-off-by: David Bond <[email protected]>
davidsbond added a commit that referenced this issue Aug 30, 2021
This commit modifies the example/documentation for ACLs to use the `jsonencode` function
instead of heredocs.

Closes #36

Signed-off-by: David Bond <[email protected]>
@pokgak
Copy link
Author

pokgak commented Aug 30, 2021

For the use of jsonencode, it would mean you can't have your comments if you'd like to do your ACL the hujson way. But it will still work just fine. You could use the HCL comments instead. WDYT?

Works for me. I prefer just referring to the Terraform files as the source of truth so HCL comments should be enough. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants