Add support for PreAuthKey tags/automatic tagging #767
Conversation
|
just to note that there's a small test break I'll need to push a change to depending on which order this and #763 get merged (if either is accepted |
|
fixed broken test from merge |
|
np will have a crack tomorrow - just noticed my local linting is broken too so will address both |
Thanks ! Maybe you'll want to wait for an owner review first 😄 |
Sorry this comment was for another PR. No need for approval to add tests they are always welcome 😄 |
|
haha all good, fwiw just about to push a fix for the linting issues but not the unit test as the point where tags is applied is in Let me know if an equivalent integration test is desired - just a bit of extra effort to stand up a new test case in there 🥲 |
tsujamin
force-pushed
the
preauthkey-tags
branch
from
05d8794 to
e8fc5b4
Compare
It appears to be causing confusion for users on Discord when copying/pasting from the example here, if Headscale crashes on launch then the container will be removed and logs can't be viewed with `docker logs`.
tsujamin
force-pushed
the
preauthkey-tags
branch
from
81f7783 to
91e3039
Compare
| aclTags := pak.toProto().AclTags | ||
| if len(aclTags) > 0 { | ||
| // This conditional preserves the existing behaviour, although SaaS would reset the tags on auth-key login | ||
| err = h.SetTags(machine, aclTags) |
There was a problem hiding this comment.
If this overrides the tags, then technically if the list is empty, we should still set it? as in, we should set it to no tags?
There was a problem hiding this comment.
So this handles some divergent behaviour between SaaS and headscale. I believe it's possible in headscale for a machine to re-auth with a pre-auth key and keep its previously set tags, whereas upstream would reset the tags in my testing.
This logic tries to keep the best of both worlds, when a machine is re-authenticated with a pak:
- if the pak has no associated tags, the machine keeps its previous tags (the current, non SaaS behaviour)
- if the pak has tags, these tags replace any previously set on the machine (which would more closely align with a user's intent)
Removing the conditional would mean that, when a machine is re-authenticated with a pak, it would lose any previous tags. This is what SaaS does but I didn't want to force a change in headscales behaviour
tl;dr I feel this is messy but its somewhat behaviour preserving
There was a problem hiding this comment.
ok, make sense, we can live with that for now, and if we decide to change it, we will have to mark it as breaking.
|
@kradalby thanks for the review - have merged in your recommendations |
tsujamin
force-pushed
the
preauthkey-tags
branch
from
e6c203c to
8a1c0e0
Compare
|
Seems like there is a lint error, and could you add changelog entry for this one as well? |
|
fixed, sorry my vscode linter re-added that space |
These changes add the ability to automatically tag nodes when activated with a preauthkey as is available in SaaS
CreatePreAuthKey and like APIs were expanded to optionally accept a list of tags and the CLI arguments were updated. The database schema was also updated with a new table to store tags associated with preauth keys.
Keys associated to a preauthkey are applied to a new or existing Machine's ForcedKeys when registered/refreshed