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

[BUG] tailscale up fails when changing non-default flags #455

Open
fredrikekre opened this issue Mar 26, 2024 · 6 comments · May be fixed by #457
Open

[BUG] tailscale up fails when changing non-default flags #455

fredrikekre opened this issue Mar 26, 2024 · 6 comments · May be fixed by #457
Labels
enhancement New feature or request question Further information is requested

Comments

@fredrikekre
Copy link
Contributor

Describe the bug
tailscale up fails after changing default arguments, such as e.g. --operator.

To Reproduce

  1. Bring up tailscale with tailscale_args: "--operator={{ ansible_user_id }}" successfully
  2. Remove the operator argument
  3. Trying to execute the play again now fails with
    fatal: [hostname]: FAILED! => {
      "changed": false,
      "msg": [
        "Error: changing settings via 'tailscale up' requires mentioning all",
        "non-default flags. To proceed, either re-run your command with --reset or",
        "use the command below to explicitly mention the current value of",
        "all non-default settings:",
        "",
        "tailscale up --auth-key=<<REDACTED>> --timeout=2m0s --operator=ubuntu"
      ]
    }
    

Note that the builtin redacting of the auth key failed here. I redacted it myself for this issue.

Expected behavior

  1. Would be nice if a change in arguments like this could be handled automatically.
  2. The auth key should be redacted

Screenshots
If applicable, add screenshots to help explain your problem.

Target (please complete the following information):

  • OS: Ubuntu 22
  • Ansible version: 2.16.5
  • artis3n.tailscale version: 4.4.4.
  • Tailscale version (set verbose to true): Ver: 1.62.0-tdf4d4ebd4-gd0454003c
@fredrikekre fredrikekre added the bug:needs-reproduction A reported bug that needs to be confirmed and reproduced. label Mar 26, 2024
@fredrikekre
Copy link
Contributor Author

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?

fredrikekre added a commit to fredrikekre/ansible-role-tailscale that referenced this issue Mar 26, 2024
This patch adds `--reset` to the default `tailscale up` arguments which
reset all non-default arguments to their defaults. This behavior matches
what is documented in the README:

> Flags are not persisted between runs; you must specify all flags each
  time.

Fixes artis3n#455.
@fredrikekre fredrikekre linked a pull request Mar 26, 2024 that will close this issue
@artis3n
Copy link
Owner

artis3n commented Mar 26, 2024

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?

@fredrikekre
Copy link
Contributor Author

fredrikekre commented Mar 28, 2024

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.

@artis3n artis3n added the question Further information is requested label Mar 30, 2024
@artis3n
Copy link
Owner

artis3n commented Mar 30, 2024

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.

@artis3n
Copy link
Owner

artis3n commented Apr 3, 2024

@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?

#457 (comment)

@McSim85
Copy link
Contributor

McSim85 commented Apr 23, 2024

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.

@artis3n artis3n added enhancement New feature or request and removed bug:needs-reproduction A reported bug that needs to be confirmed and reproduced. labels May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants