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

fix: tailscale state issue #2380

Merged
merged 4 commits into from
Aug 21, 2023
Merged

Conversation

konne
Copy link
Contributor

@konne konne commented Aug 2, 2023

Description

If you with the current setup try to fetch a SSL cert with tailscale cert you get an error message around TailscaleVarRoot.
If you don't apply that state, it just perfectly works. It still even use the same directory.
Because of that issue also tailscale server https:8443 / HTTP://127.0.0.1:80 is also not working

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Verification Process

try to fetch a cert with tailscale cert before and after that change.
You need to be in a directory that is writeable so for example /tmp/
Same apply to the tailscale serve

Release Notes

Contributing checklist

  • My code follows the code style of this project.
  • I have read the CONTRIBUTING and LICENSE document.
  • I fully agree to distribute my changes under Apache 2.0 license.

@jens-maus
Copy link
Owner

jens-maus commented Aug 2, 2023

Sorry, but just out of your description I can't follow why removing the state argument would solve any problem. In fact putting the state variable to /etc/config was done on purpose so that after a reboot the state of tailscale is preserved. Furthermore, why does removing the state argument allow to use the cert command while with state pointing to /etc/config does not? So please elaborate a bit more and perhaps with as much technical details as possible.

@konne
Copy link
Contributor Author

konne commented Aug 21, 2023

@jens-maus thanks for checkin and feedback.

  1. yes you are fully correct that remove does not work because the normal statedir /var/lib/tailscale is in the tmpfs area
  2. adapted the PR and now it works just perfect on my side.

The main issue is if you just point to the state, tailscale has an issue with the statedir, maybe it points somewhere that is only readonly or don't have it.
Now with tailscale just having a proper statedir, it stores the state and also the generated certs,... in that dir.

If you want to test it for your self, just run:

tailscale serve https / http://127.0.0.1:80

and after that you have even perfect access via a nice valid SSL cert to your CCU from your tailnet.
This need further improvement before I can make a PR.

@jens-maus
Copy link
Owner

Thanks for your revised PR. However, I do have some more questions before I can consider accepting it.

  1. According to the docs (https://github.com/tailscale/tailscale/blob/main/cmd/tailscaled/tailscaled.go#L168-L169) the -statdir parameter should point to an actualy directory and not to an actual state file. However, you PR introduces -statedir with a value that seems to point to a file.
  2. In addition, the docs (https://github.com/tailscale/tailscale/blob/main/cmd/tailscaled/tailscaled.go#L168-L169) also state that if -statedir is used the default state file name is <STATEDIR>/tailscaled.state.

So IMHO it seems to look like that using -statedir /etc/config should be enough for the state, certs, etc. to correctly be created in the default /etc/config directory path where all other persistent config files are usually stored.

Or would it be even better/wiser to use -statedir /etc/config/tailscale to make sure that all necessary files are created in that particular subfolder and not directly in /etc/config? In that case I would propose to use

TAILSCALED_ARGS="-state /etc/config/${DAEMON}.state -statedir /etc/config/tailscale -socket /var/run/tailscale/${DAEMON}.sock"

That would make sure that the location of the state file is still the same like before, but the -statedir points to the /etc/config/tailscale directory path for additional files (SSL certs, drop files, etc...). If this is the case one would have to – however – hceck that /etc/config/tailscale will be automatically generated and that we don't need to create it in advance upon tailscaled startup.

@konne
Copy link
Contributor Author

konne commented Aug 21, 2023

@jens-maus I will change it in 5min to /etc/config/tailscaled/

As you can see in the statedir are multiple files and I think they should not pollute the /etc/config directory
image

The second thing is the service automatically creates the statedir if it does not exists with the following access rights:

image

@jens-maus
Copy link
Owner

@jens-maus I will change it in 5min to /etc/config/tailscaled/

Thx. In addition, I still think it would be good to also keep -state /etc/config/${DAEMON}.state to keep the *.state file outside of the /etc/config/tailscaled directory path so that existing installations keep on working since the previous state file naming / location was directory in /etc/config.

@jens-maus jens-maus added the 🐛 bug-report Something isn't working label Aug 21, 2023
@jens-maus jens-maus added this to the next release milestone Aug 21, 2023
@konne
Copy link
Contributor Author

konne commented Aug 21, 2023

@jens-maus I will try that topic out and give you feedback. From "cleanless", I would more prefer to move the state file into that folder, because if you don't define the --state flag all files are always together in a folder.
We could add a "migration" script that just moves the state file into the folder, but this just creates more stuff in the init script, so this is also not nice. And I also understand that it make sense to be backward compatible.

I will share my outcome if that works with both later.

@jens-maus
Copy link
Owner

@jens-maus I will try that topic out and give you feedback. From "cleanless", I would more prefer to move the state file into that folder, because if you don't define the --state flag all files are always together in a folder. We could add a "migration" script that just moves the state file into the folder, but this just creates more stuff in the init script, so this is also not nice. And I also understand that it make sense to be backward compatible.

Backward compatibility is IMHO more important here than "cleanliness". Especially since we are talking about just a single state file that will also be located in the /etc/config top-level directory path. So please apply my suggestion accordingly and refrain from adding any migration method to the init script, etc.

@jens-maus
Copy link
Owner

Thx, this was quick and I will merge this PR now. Thanks for your contribution!

@jens-maus jens-maus merged commit 3a08607 into jens-maus:master Aug 21, 2023
4 checks passed
@konne
Copy link
Contributor Author

konne commented Aug 21, 2023

@jens-maus I double checked it and it works fine.

@konne konne deleted the fix-tailscale-state branch August 21, 2023 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug-report Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants