awlsring / terraform-provider-headscale

A terraform provider for Headscale
https://registry.terraform.io/providers/awlsring/headscale/latest
Apache License 2.0
22 stars 2 forks source link

Preauth Key ACL Tags #2

Closed adamjacobmuller closed 1 year ago

adamjacobmuller commented 1 year ago

Hi,

Experimenting with this module and found two issues.

Happy to send a PR but wanted to ask about them first.

1 the regexp for validating preauth key ACL tags seems to be overly strict, it refuses something like tag:name-of-thing (no -s) or tag:name_of_thing or tag:name.of.thing

I think the correct fix for this is to figure out in headscale code how they validate the ACL tag and just mirror their logic (it's definitely more permissible than what is used here since I can easily create these with the headscale command)

2 the documentation for preauth key tags does not match the code for acl tags

Docs say just use tags (which is linguistically correct IMO and matches the headscale API and the headscale CLI) but the model uses acl_tags (and it does actually work fine).

If you have any comments on these (especially the direction on the 2nd one -- should fix docs or fix code) I'm happy to submit PRs for both.

awlsring commented 1 year ago

Hey! PRs are always welcome.

Issue 1: I like the idea of matching w/e regex headscale officially uses. I only observed the schema so my current regex is a guess. This validation is done here

Issue 2: I must have confused myself when writing the docs up. I believe I initially selected acl_tags as that is the name of the response field containing these in the api (example from the my /swagger below).

{
  "preAuthKeys": [
    {
      "user": "string",
      "id": "string",
      "key": "string",
      "reusable": true,
      "ephemeral": true,
      "used": true,
      "expiration": "2023-08-29T02:08:12.108Z",
      "createdAt": "2023-08-29T02:08:12.108Z",
      "aclTags": [
        "string"
      ]
    }
  ]
}

tags probably ended up in the docs due to that being the name for the device tags. And I wrote these all together. I agree tags makes more sense here, but to not introduce an api breaking change I'm biased toward just updating docs.

Feel welcome to submit changes to anything here. Otherwise, I'll probably pick these up at some point in the future.

awlsring commented 1 year ago

I took this up this morning, it looks like headscale doesn't do regex for this and just checks if it is prefixed with tag:, so I changed the regex to be more open.

This and the documentation will be in the new release, which I'm going to kick off now.