Open fredrikekre opened 6 months ago
The auth key should be redacted
This didn't happen because I use a node key without the tskey
prefix (https://github.com/artis3n/ansible-role-tailscale/blob/7ebbe49a0413b8912c02bd9a81aa54a02538f6f5/tasks/install.yml#L170C65-L170C76). Such keys are accepted by headscale. Perhaps the regex can look for --auth-key\s*=\s*.*\s
or something instead?
Thanks for this issue and it's linked PR! I want to think about how best to account for this. Tailscale's default behavior is to fail instead of silently add --reset
. Should this role change this default behavior? Should we provide a tailscale_reset
or similar parameter on the role that conditionally includes --reset
? Should we expect end users to do this themselves when passing in tailscale_args
?
I think one can argue for having different defaults for --reset
in an interactive context (i.e. when you interact with the CLI yourself) and in a non-interactive context like when using this role. In an interacitve context it is easier to forget some (non-default) arguments compared to when you put the arguments in code (like with this role). When using this role for configuring tailscale I would expect that the configuration I put in my code is what ends up on the remote host no matter what previus configuration I used when deploying the last time.
Should we provide a tailscale_reset or similar parameter on the role that conditionally includes --reset?
This is what I initially did, but if one subscribes to the idea that the code should always override any existing state it doesn't hurt to always include --reset
. I am happy to change my PR to make it configurable. I do think resetting by default make sense though, but at least there will be a way to opt out for anyone who doesn't like it. Should I update the PR to do this?
Should we expect end users to do this themselves when passing in tailscale_args?
This is what I do now as a workaround.
I think that's a persuasive reason to --reset
by default, but I want to think about it for a bit before merging. I have other breaking changes on my mind that would go into a new major version as well.
@McSim85 Moving your comment here, you are saying you are in favor of this role applying --reset
by default, or that doing so would introduce more unexpected issues for you?
https://github.com/artis3n/ansible-role-tailscale/pull/457#issuecomment-2035656789
I'm sorry for not getting back to you sooner. I will extend my previous comment:
Let me add my 2 cents. I prefer to run the role with
state: absent
->state: present
, if cli parameters has changed. That will bring less unexpected problems.
In most cases, Ansible inventory is automated. Inventory or role variables define server configuration. Ansible roles are expected to be idempotent. So, changing tailscale cli parameters is not the thing that happens often.
In case your environment expects to change cli params by admins manually, there is a way to pass --reset
flag by tailscale_args variable in your role:
- name: Servers
hosts: all
roles:
- role: artis3n.tailscale
vars:
tailscale_args: "--reset"
I might be wrong, but the --reset
param allows you to enforce the new tags (--advertise-tags=tag:XXX
instead of previous --advertise-tags=tag:YYY
) and this can assign the server unexpected permissions (if you use tags in ACLs).
Which should not happen by default.
To sum up, I would not add --reset
option by default into the role.
Describe the bug
tailscale up
fails after changing default arguments, such as e.g.--operator
.To Reproduce
tailscale_args: "--operator={{ ansible_user_id }}"
successfullyNote that the builtin redacting of the auth key failed here. I redacted it myself for this issue.
Expected behavior
Screenshots If applicable, add screenshots to help explain your problem.
Target (please complete the following information):
artis3n.tailscale
version: 4.4.4.verbose
to true): Ver: 1.62.0-tdf4d4ebd4-gd0454003c