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

added allow_overwrite option to resource_acl #303

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

markwellis
Copy link
Contributor

so resource_acl doesn't need to be imported first, as this is a manual task that breaks our workflow.

by default it is false, so will require import unless you read the docs and learn about the allow_overwite option

Fixes #229

Special notes for your reviewer:
It's my first time writing go, sorry if it's not best practice.

@knyar
Copy link
Collaborator

knyar commented Nov 15, 2023

Thank you for preparing a PR for this. Before adding more options, I'd like to understand what problem this solves and make sure this is something that other users will find helpful as well. Could you please share a bit more on why this is necessary?

resource_acl should support terraform import, so if you have a non-default policy, you should be able to import it before changing. For tailnets that never had their ACL changed from the default, this check should pass and allow setting the policy even without importing.

@markwellis
Copy link
Contributor Author

We have many terraform modules that modify the tailscale ACL

e.g.

  • a module for our aws org to create new aws accounts. This adds tags and groups to the tailscale ACL
  • a module to create a VPC. This adds hosts to the tailscale ACL
  • modules for RDS etc that add ACLs for access

All of these fail every time as we always forget to first import the ACL. It's annoying, and adding this as an option will streamline the process for us massively, whilst preserving the current behaviour of it not being overwritten by default.

It's also a little strange that this resource requires importing before it can be controlled with terraform, I can't think of a single other thing that works that way. I can see why you've done it, but for advanced users it's more of a hindrance than a help and having a way to opt out seems like a reasonable work around

Hope that clears it up, happy to provide any info :)

There is also #196 which relates to this

@markwellis
Copy link
Contributor Author

markwellis commented Nov 16, 2023

I just realised with import blocks added in tf 1.5 there is a work around for this already

data "tailscale_acl" "acl" {
}

locals {
  acl = jsondecode(data.tailscale_acl.acl.json)
}

import {
  to = tailscale_acl.acl
  id = "acl"
}
resource "tailscale_acl" "acl" {
  acl = jsonencode(merge(local.acl, {groups = merge(local.acl.groups, {"group:foo" = ["${plantimestamp()}@example.com"]})}))
}

I don't feel this is as nice as having allow_overwrite but it does work.

So with that said, there is a solution to the initial problem we were facing without needing to add more options

@knyar
Copy link
Collaborator

knyar commented Nov 16, 2023

We have many terraform modules that modify the tailscale ACL

Sorry, but I still don't quite understand how this works. Terraform is designed around the idea of each Terraform resource "owning" management of a particular cloud resource, keeping local state. If you have multiple resources in multiple modules pointing to the same tailnet's ACL, won't they keep overwriting each other's changes? This seems like a use case for an automation tool that's imperative in nature (making individual changes) rather than declarative (converging on desired state) like Terraform is.

@markwellis
Copy link
Contributor Author

markwellis commented Nov 16, 2023

Yes it is a little strange, but the way we do it is to manage parts of the ACL in places, by loading the existing ACL, and then updating certain groups etc. It is riddled with race conditions, but it works better for us than having one place that manages the ACL.

What would be best would be if the tailscale API allowed us to mange groups/acls/hosts etc individually, rather than as a single json document. But there's nothing I can do about that

E.g. what we do is add a host for a new VPC's network, a new group with the VPC name in, and an ACL that allows the group access to the host. Doing that when we create the VPC is neater for us than doing it in a single place. It means the ACL for accessing the VPC lives with the VPC terraform itself, rather than somewhere else. Even though this isn't how the tailscale API actually works

it would be awesome if we could do something like

resource "aws_vpc" "example" {
  ...
}

resource "tailscale_acl_host" "foo" {
  name = aws_vpc.example.tags.name
  ip_address = aws_vpc.example.cidr_block
}

resource "tailscale_acl_group" "foo"  {
  name = aws_vpc.example.tags.name
  members = [] #managed in the tailscale admin or scim in the future
}

resource "tailscale_acl_acl" "foo" {
  action = "accept"
  src = tailscale_acl_group.foo.name
  dst = tailscale_acl_host.foo.name
}

but we can't so instead we do something like

resource "aws_vpc" "example" {
  ...
}

data "tailscale_acl" "acl" {
}

locals {
  acl = jsondecode(data.tailscale_acl.acl.json)
}

import {
  to = tailscale_acl.acl
  id = "acl"
}
resource "tailscale_acl" "acl" {
  acl = jsonencode(
    merge(local.acl, {
      groups = merge(local.acl.groups, {"group:foo" = []}),
      hosts = merge(local.acl.hosts, {"hosts": {
        aws_vpc.example.tags.name  = aws_vpc.example.cidr_block
      }},
....
    })
  )
}

where we abuse merge to overwrite just parts of the json. it works, but it's not great, but is still better for us than managing the acl in one place

hope this clears up our abuseuse case

@markwellis
Copy link
Contributor Author

I'm going to close this as i can use the import block as a work around

@markwellis markwellis closed this Nov 24, 2023
@markwellis markwellis deleted the 229_acl_allow_overwrite branch December 7, 2023 13:38
@markwellis markwellis restored the 229_acl_allow_overwrite branch January 25, 2024 07:30
@markwellis
Copy link
Contributor Author

reopening as import blocks cannot be used in modules, see hashicorp/terraform#33474

Because of that issue I would still like to be able to set allow_overwrite on the tailscale_acl resource to not require importing it before terraform can manage the ACL

Copy link
Collaborator

@knyar knyar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your patience here. Just a couple comments from me.

tailscale/resource_acl.go Outdated Show resolved Hide resolved
tailscale/resource_acl.go Outdated Show resolved Hide resolved
so it doesn't need to be imported first, as this breaks is a manual task
that breaks our workflow.

Fixes tailscale#229
Copy link
Collaborator

@knyar knyar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@knyar knyar merged commit 5c36d39 into tailscale:main Feb 9, 2024
3 checks passed
@colans
Copy link

colans commented Sep 4, 2024

This shouldn't be necessary. See #426 for details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to set ACL
3 participants