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

action.yml: allow specifying a state directory #133

Merged
merged 1 commit into from
Jul 10, 2024
Merged

Conversation

knyar
Copy link
Contributor

@knyar knyar commented Jul 9, 2024

Also, document usage of the action with Tailnet Lock.

Fixes #132

Also, document usage of the action with Tailnet Lock.

Fixes #132

Signed-off-by: Anton Tolchanov <[email protected]>
Copy link
Member

@Erisa Erisa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with various commits in https://github.com/Erisa/tailscale-action-test

@knyar knyar requested a review from willnorris July 9, 2024 11:17
statedir:
description: 'Optional state directory to use (if unset, memory state is used)'
required: false
default: ''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe set the default mem: value here, so you don't need the if/else below?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--state=mem: is a different flag to --statedir= so it still wouldn't work without a check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doh! I totally overlooked that. Disregard then 😛

Copy link
Member

@willnorris willnorris Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, I know what I was thinking about... you can just set --state to a file and then --statedir will be derived from that. That's what I normally do in testing. But having statedir be the config option here makes more sense.

Copy link
Member

@Erisa Erisa Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I haven't tested, I think that these may be different?
https://tailscale.com/kb/1278/tailscaled#flags-to-tailscaled

To control where state (including preferences and keys) is stored, use one of:

--statedir= for a directory on disk where to store config, keys, Taildrop files, and other state.
--state= to either a /path/to/file, kube: to use Kubernetes secrets, arn:aws:ssm:... to store state in AWS SSM, or mem: to not store state and reigster as an ephemeral node. By default, if not provided, the state is stored in /tailscaled.state.

--state is specifically for the tailscaled.state file, but --statedir gives a whole directory for config/keys which is what is needed for Taillnet Lock to work (it needs somewhere to store keys).

and "one of" implies they're mutually exclusive, so using --statedir you shouldnt also use --state

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I properly read what you mean now with

and then --statedir will be derived from that.

This isn't documented but if it works and is supposed to work I'm on board!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like the KB is missing a little bit of the documentation we have in the CLI...

% tailscaled --help

...
  -statedir string
        path to directory for storage of config state, TLS certs, temporary incoming Taildrop files, etc. 
        If empty, it's derived from --state when possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make use of that in a way that doesn't have a check in code I think you would have to change the option to be a state file rather than a directory.

e.g. if you set state: /tmp/tailscale/tailscaled.sock then you can safely set that and have the default as mem:

But if you expose the actions option as statedir: /tmp/tailscale/ with a default of mem: then you still need to think "is this set to mem:" before you go setting the --state file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I had originally just forgotten there are two different flags. I think this PR is good to merge as-is.

@willnorris willnorris merged commit b2b96d3 into main Jul 10, 2024
1 check passed
@willnorris willnorris deleted the knyar/statedir branch July 10, 2024 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tailnet lock enabled networks need persistent tailscaled state
3 participants