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

cmd/containerboot: add EXPERIMENTAL_TS_CONFIGFILE_PATH env var to allow passing tailscaled config in a file #10759

Merged
merged 3 commits into from
Jan 8, 2024

Conversation

irbekrm
Copy link
Contributor

@irbekrm irbekrm commented Jan 5, 2024

Currently when a containerboot container starts:

  • if TS_AUTH_ONCE env var is set to true it runs tailscaled followed by tailscale up + auth key + config opts on first container start and tailscaled + tailscale set + config opts on subsequent starts
  • if TS_AUTH_ONCE env var is set to false (default) runs tailscaled followed by tailscale up + config opts on any container start

The config opts are passed as env vars. This has a number of issues- env vars are not a great configuration option as we need to add extra logic in tailscale to validate them and having the config scattered accross multiple commands makes it harder to implement declarative configuration.
Right now we need to add more configuration options (see #10740) so it's a good time to add support for a better config mechanism.

This PR adds support for optionally providing configuration via a config file passed to tailscaled.

Users can set EXPERIMENTAL_TS_CONFIGFILE_PATH to a path to tailscaled config file, see https://pkg.go.dev/tailscale.com@v1.56.1/ipn#ConfigVAlpha.
If this is set containerboot only runs tailscaled --config <path-to-the-configfile> ..<other config opts> (and not tailscale up or tailscale set). Users must pass auth key via the tailscaled config.

Notes

So that we can distinguish between the value being set to
false explicitly bs being unset.

Updates #10708

Signed-off-by: Irbe Krumina <irbe@tailscale.com>
If EXPERIMENTAL_TS_CONFIGFILE_PATH env var is set,
only run tailscaled with the provided config file.
Do not run 'tailscale up' or 'tailscale set'.

Signed-off-by: Irbe Krumina <irbe@tailscale.com>
cmd/containerboot/main.go Outdated Show resolved Hide resolved
@@ -253,7 +251,7 @@ func main() {
return nil
}

if !cfg.AuthOnce {
if !runTailscaleSet(cfg) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit weird that running tailscale up is gated by a function called run tailscale set. Perhaps it would be better to leave this as !cfg.AuthOnce, since authTailscale exits early if we use a config file anyway?

Copy link
Contributor Author

@irbekrm irbekrm Jan 8, 2024

Choose a reason for hiding this comment

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

This PR adds a bunch more combinations of 'is/is not cfg.AuthOnce and ' to the extent that I think it becomes difficult to follow which bits of the three installation modes a certain conditional gates.

I am trying to distinguish between the three install modes (run tailscaled + tailscale auth, run tailscaled + tailscale auth and subsequent tailscale set, run tailscaled only).
I've updated the helper functions to have hopefully better names, happy to rename them differently if you have other suggestions 0bf017c#diff-700830ac79a41da42cd3a82065f0fee3003a4390eead4db1de4bf30557a06bd6L999-R1019

@@ -309,7 +314,7 @@ authLoop:
}
}

if cfg.InKubernetes && cfg.KubeSecret != "" && cfg.KubernetesCanPatch && cfg.AuthOnce {
if cfg.InKubernetes && cfg.KubeSecret != "" && cfg.KubernetesCanPatch && runTailscaleSet(cfg) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, weird to see runTailscaleSet gating something other than tailscaleSet. Perhaps we could use a better function name, or gate this by AuthOnce and runTailscaledOnly like you do on line 172?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as in #10759 (comment)

Signed-off-by: Irbe Krumina <irbe@tailscale.com>
@irbekrm
Copy link
Contributor Author

irbekrm commented Jan 8, 2024

I'm going to merge this, since it's approved and so that I can rebase the other PR. Happy to address any further readability improvements in followups though!

@irbekrm irbekrm merged commit 1336992 into main Jan 8, 2024
46 checks passed
@irbekrm irbekrm deleted the irbekrm/containerbootconf branch January 8, 2024 16:14
Asutorufa pushed a commit to Asutorufa/tailscale that referenced this pull request Jan 19, 2024
…ow passing tailscaled config in a file (tailscale#10759)

* cmd/containerboot: optionally configure tailscaled with a configfile.

If EXPERIMENTAL_TS_CONFIGFILE_PATH env var is set,
only run tailscaled with the provided config file.
Do not run 'tailscale up' or 'tailscale set'.

* cmd/containerboot: store containerboot accept_dns val in bool pointer

So that we can distinguish between the value being set to
false explicitly bs being unset.

Signed-off-by: Irbe Krumina <irbe@tailscale.com>
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.

None yet

2 participants