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

Preserve comments in acl file #227

Closed
clstokes opened this issue Apr 11, 2023 · 5 comments · Fixed by #332
Closed

Preserve comments in acl file #227

clstokes opened this issue Apr 11, 2023 · 5 comments · Fixed by #332
Labels
enhancement New feature or request

Comments

@clstokes
Copy link
Collaborator

Is your feature request related to a problem? Please describe.
The tailscale_acl standardizes the acl input which removes all comments in the file.

Describe the solution you'd like
It would be helpful to preserve comments in the ACL file to help Tailscale admins better understand the ACL output in the Admin Console.

Example

This ACL resource:

resource "tailscale_acl" "sample_acl" {
  acl = <<EOF
{
	// Access control lists.
	"acls": [
		// default allow allow
		{"action": "accept", "src": ["*"], "dst": ["*:*"]},
	],
}
EOF
}

Becomes this in the admin console:

{
	"acls": [
		{
			"action": "accept",
			"src": [
				"*"
			],
			"dst": [
				"*:*"
			]
		}
	]
}
@clstokes clstokes added the enhancement New feature or request label Apr 11, 2023
@porkcharsui
Copy link

Using tailscale_acl within a CI workflow is akin to the Tailscale GitOps tool.

Perhaps the ask here is having the Terraform provider use the same parsing design as the GitOps tool, which uses github.com/tailscale/hujson not encoding/json. This ensures that all HuJSON comments are preserved.

@markwellis
Copy link
Contributor

perhaps a good middle ground would be to add a name attribute to acl rules? Which would also solve another problem I have:

I read the ACL policy via the api, then merge specific parts over the top to programmatically add rules. If there was a name attribute this would also allow me to programmatically remove rules via terraform too, which is not something I can't do at the moment.

@knyar
Copy link
Collaborator

knyar commented Dec 11, 2023

Based on a brief internal discussion, we think a reasonable way forward would be to add a new field to the tailscale_acl resource that will accept the ACL content as a HuJSON string.

To simplify ongoing maintenance related to keeping the ACL schema in sync between Tailscale management plane and the Terraform provider, the contents of this field will not be parsed or interpreted by the provider. It will be validated against the validate ACL API endpoint, and will be written verbatim as HuJSON, with all comments and markup preserved.

We're planning to call this field raw to indicate lack of local validation. Users of the Terraform provider will be expected to either set the acl field (JSON string, validated offline and re-encoded) or the raw field (HuJSON string, passed verbatim to the API), but not both.

Please comment on this issue if you have any feedback on this.

@markwellis
Copy link
Contributor

How would one work with HuJSON in terraform though?

jsondecode will either error on the comments, or remove them, which means this can't be used in a way that preserves the comments unless you only use it a string?

@alexeiser
Copy link

How would one work with HuJSON in terraform though?

jsondecode will either error on the comments, or remove them, which means this can't be used in a way that preserves the comments unless you only use it a string?

We would use a templated string - as mentioned in the earlier comment - it would still be validated on the tailscale api endpoint - but could potentially not be valid hujson.

knyar added a commit to tailscale/tailscale-client-go that referenced this issue Feb 12, 2024
Per [docs](https://github.com/tailscale/tailscale/blob/main/api.md):

> The HTTP status code will be '200' if the request was well formed and
> there were no server errors, even in the case of failing tests or an
> invalid ACL. Look at the response body to determine whether there was a
> problem within your ACL or tests:

Updates tailscale/terraform-provider-tailscale#227
knyar added a commit to tailscale/tailscale-client-go that referenced this issue Feb 12, 2024
knyar added a commit to tailscale/tailscale-client-go that referenced this issue Feb 12, 2024
knyar added a commit to tailscale/tailscale-client-go that referenced this issue Feb 12, 2024
knyar added a commit to tailscale/tailscale-client-go that referenced this issue Feb 14, 2024
Per [docs](https://github.com/tailscale/tailscale/blob/main/api.md):

> The HTTP status code will be '200' if the request was well formed and
> there were no server errors, even in the case of failing tests or an
> invalid ACL. Look at the response body to determine whether there was a
> problem within your ACL or tests:

Updates tailscale/terraform-provider-tailscale#227
knyar added a commit to tailscale/tailscale-client-go that referenced this issue Feb 14, 2024
knyar added a commit to tailscale/tailscale-client-go that referenced this issue Feb 14, 2024
knyar added a commit that referenced this issue Feb 14, 2024
The `acl` argument of the `tailscale_acl` resource can now be a HuJSON
string. Instead of unmarshalling it into an `ACL` message of the [API
client](https://github.com/tailscale/tailscale-client-go) just to have
the client serialize it into JSON again, policy content gets passed
to the Tailscale API verbatim.

This allows users to define their policy as HuJSON strings, with
comments being preserved. Since JSON is a subset of HuJSON, this is
backwards compatible, so I am not adding a separate field for this as
has been previously suggested in #227.

Validation is now performed by calling the [Validate and test policy
file](https://github.com/tailscale/tailscale/blob/main/api.md#validate-and-test-policy-file)
API, which will help catch any semantic errors in the policy at
`terraform plan` stage (for example, when a syntactically correct policy
contains configuration that is not supported by the Tailnet's current
[pricing plan](https://tailscale.com/pricing)).

Finally, this will also allow users to use new fields in the policy
without requiring a new release of the Terraform provider.

I've also added a new `hujson` field to the `tailscale_acl` data
resource that shows current policy as a HuJSON string.

Fixes #331
Fixes #227

Signed-off-by: Anton Tolchanov <[email protected]>
knyar added a commit that referenced this issue Feb 14, 2024
The `acl` argument of the `tailscale_acl` resource can now be a HuJSON
string. Instead of unmarshalling `acl` into an `ACL` struct of the [API
client](https://github.com/tailscale/tailscale-client-go) just to have
the client serialize it into JSON again, policy content gets passed
to the Tailscale API verbatim.

This allows users to define their policy as HuJSON strings, with
comments being preserved. Since JSON is a subset of HuJSON, this is
backwards compatible, so I am not adding a separate field for this as
has been previously suggested in #227.

Validation is now performed by calling the [Validate and test policy
file](https://github.com/tailscale/tailscale/blob/main/api.md#validate-and-test-policy-file)
API, which will help catch any semantic errors in the policy at
`terraform plan` stage (for example, when a syntactically correct policy
contains configuration that is not supported by the Tailnet's current
[pricing plan](https://tailscale.com/pricing)).

Finally, this will also allow users to use new fields in the policy
without requiring a new release of the Terraform provider.

I've also added a new `hujson` field to the `tailscale_acl` data
resource that shows current policy as a HuJSON string.

Fixes #331
Fixes #227

Signed-off-by: Anton Tolchanov <[email protected]>
knyar added a commit that referenced this issue Feb 14, 2024
The `acl` argument of the `tailscale_acl` resource can now be a HuJSON
string. Instead of unmarshalling `acl` into an `ACL` struct of the [API
client](https://github.com/tailscale/tailscale-client-go) just to have
the client serialize it into JSON again, policy content gets passed
to the Tailscale API verbatim.

This allows users to define their policy as HuJSON strings, with
comments being preserved. Since JSON is a subset of HuJSON, this is
backwards compatible, so I am not adding a separate field for this as
has been previously suggested in #227.

Validation is now performed by calling the [Validate and test policy
file](https://github.com/tailscale/tailscale/blob/main/api.md#validate-and-test-policy-file)
API, which will help catch any semantic errors in the policy at
`terraform plan` stage (for example, when a syntactically correct policy
contains configuration that is not supported by the Tailnet's current
[pricing plan](https://tailscale.com/pricing)).

Finally, this will also allow users to use new fields in the policy
without requiring a new release of the Terraform provider.

I've also added a new `hujson` field to the `tailscale_acl` data
resource that shows current policy as a HuJSON string.

Fixes #331
Fixes #227

Signed-off-by: Anton Tolchanov <[email protected]>
knyar added a commit that referenced this issue Feb 14, 2024
The `acl` argument of the `tailscale_acl` resource can now be a HuJSON
string. Instead of unmarshalling `acl` into an `ACL` struct of the [API
client](https://github.com/tailscale/tailscale-client-go) just to have
the client serialize it into JSON again, policy content gets passed
to the Tailscale API verbatim.

This allows users to define their policy as HuJSON strings, with
comments being preserved. Since JSON is a subset of HuJSON, this is
backwards compatible, so I am not adding a separate field for this as
has been previously suggested in #227.

Validation is now performed by calling the [Validate and test policy
file](https://github.com/tailscale/tailscale/blob/main/api.md#validate-and-test-policy-file)
API, which will help catch any semantic errors in the policy at
`terraform plan` stage (for example, when a syntactically correct policy
contains configuration that is not supported by the Tailnet's current
[pricing plan](https://tailscale.com/pricing)).

Finally, this will also allow users to use new fields in the policy
without requiring a new release of the Terraform provider.

I've also added a new `hujson` field to the `tailscale_acl` data
resource that shows current policy as a HuJSON string.

Fixes #331
Fixes #227

Signed-off-by: Anton Tolchanov <[email protected]>
knyar added a commit that referenced this issue Feb 15, 2024
The `acl` argument of the `tailscale_acl` resource can now be a HuJSON
string. Instead of unmarshalling `acl` into an `ACL` struct of the [API
client](https://github.com/tailscale/tailscale-client-go) just to have
the client serialize it into JSON again, policy content gets passed
to the Tailscale API verbatim.

This allows users to define their policy as HuJSON strings, with
comments being preserved. Since JSON is a subset of HuJSON, this is
backwards compatible, so I am not adding a separate field for this as
has been previously suggested in #227.

Validation is now performed by calling the [Validate and test policy
file](https://github.com/tailscale/tailscale/blob/main/api.md#validate-and-test-policy-file)
API, which will help catch any semantic errors in the policy at
`terraform plan` stage (for example, when a syntactically correct policy
contains configuration that is not supported by the Tailnet's current
[pricing plan](https://tailscale.com/pricing)).

Finally, this will also allow users to use new fields in the policy
without requiring a new release of the Terraform provider.

I've also added a new `hujson` field to the `tailscale_acl` data
resource that shows current policy as a HuJSON string.

Fixes #331
Fixes #227

Signed-off-by: Anton Tolchanov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants