Support tailscale configuration via Caddy configuration #30

clly · 2024-02-12T04:03:26Z

As described in #24, this PR creates a new Caddy module registering a global configuration
option and a module fulfilling the required interface. This is an initial implementation using a
global caddy.UsagePool to share the configuration between the application parsed from the
caddyfile and the standalone listener functions. This will also fallback to the environment if the
configuration wasn't parsed from the Caddyfile.

In the future this can support ephemeral servers by marking the servers as ephemeral based
on the Caddy config and then stopping them in the Stop function.

mholt · 2024-03-06T21:03:24Z

I haven't had a chance to scrutinize it, but at a glance this looks like a good start 👍

clly · 2024-04-02T01:49:40Z

Hey @willnorris @mholt, just curious if you had, had a chance to look at this PR yet.

If preferred, I could start stacking new PR's against this one and incorporate feedback into them as necessary.

willnorris · 2024-04-02T23:05:28Z

I did start reviewing this back when you initially sent it, but never got to the point that I sent the comments 😞

I recall that some of my initial thoughts were whether we needed to use a UsagePool for app (since I think it's always a singleton, right?). I'm also a little worried about the complexity of configuring options at two different levels... the default values and then the per-server values. Even though it would be a little more verbose to always require config at the server level, I wonder if that would be easier to reason about what exactly is happening?

(I'm sure I had some other thoughts as well, but these are the ones that come to mind)

And thank you by the way for doing this... I absolutely love this approach, and am honestly really excited to have this in, even if my lack of attention says otherwise!

mholt · 2024-04-03T03:50:07Z

Oof, sorry, it's still in my notifications inbox and I promise I'll get around to it but I've been hustling for a new Caddy release -- and I'm traveling this week 😬

I'll try to get to this soon! 😃 I am also excited about it. Leaving it unread in my inbox so I don't forget.

clly · 2024-04-06T13:39:32Z

@willnorris from what I remember I thought the UsagePool would be helpful for caddy reloads (although there's a lot of assumptions I'm making in that process).

Assuming caddy does partial tear down and setup where a server doesn't get torn down and re-setup on reload, we wouldn't want to tear down a listener if it's used in multiple HTTP servers. The UsagePool would handle all of that for us because it handles the reference tracking and only calls Destruct on the TSServer when the last reference is deleted.

It's not implemented as part of this PR in an effort to keep it small, but I think we'd wrap the Close function in the listeners we currently return to Caddy with a call to Delete the reference from the UsagePool. On the last Delete call, the UsagePool should call Destruct on TSServer and shut it down.

Even though it would be a little more verbose to always require config at the server level, I wonder if that would be easier to reason about what exactly is happening?

Potentially. Personally, I prefer more flexible configuration, less typing in configuration, but I'm not against making it all explicit. The environment variables are still supported in this PR which already has the two tier configuration functionality. My suggestion would be to remove the environment variable support and bump to the next major version to indicate the breaking change.

How would you like to proceed?

mholt · 2024-04-15T18:01:42Z

module.go

@@ -136,6 +132,26 @@ func getServer(_, addr string) (*tsnetServerDestructor, error) {
 	return s.(*tsnetServerDestructor), nil
 }

+func getAuthKey(host string) string {
+	hostAuthKey, loaded := app.LoadOrStore(host, "")


Any reason you wouldn't just pass the auth key in here? You have 2 LordOrStore operations which my intuition tells me breaks the atomicity...

Since the global key is semi-static I should be able to fetch it outside this function and use it if the host specific key doesn't exist.

My assumption was the Caddy App setup happens semi-serially and that these would all be already loaded which would mean we should still get back the same values? Or am I misunderstanding the atomicity issue

Oh, I see, so the storedAuthKey and hostAuthKey are different values? In that case, it's probably fine. Just make sure to call Delete() in your module's Cleanup() method so that it can free up unused memory.

clly · 2024-04-27T02:45:27Z

I recall that some of my initial thoughts were whether we needed to use a UsagePool for app (since I think it's always a singleton, right?). I'm also a little worried about the complexity of configuring options at two different levels... the default values and then the per-server values. Even though it would be a little more verbose to always require config at the server level, I wonder if that would be easier to reason about what exactly is happening?

@willnorris looking at this some more you're right. The apps themselves have no state, just the actual tailscale servers so this should switch to a singleton of some kind. I'm not a fan but it's probably for the best.

Register a parseGlobal function to parse the caddy configuration file. The parse function creates a new TSApp which will hold our configuration and create the CaddyModule. Signed-off-by: Connor Kelly <connor.r.kelly@gmail.com>

Signed-off-by: Connor Kelly <connor.r.kelly@gmail.com>

Initially see if the provisioned server or global key exist in the app usage pool. If not then fallback to the already existing environment variables Signed-off-by: Connor Kelly <connor.r.kelly@gmail.com>

Signed-off-by: Connor Kelly <connor.r.kelly@gmail.com>

Everytime we get a listener we increment the refernce on a caddy UsagePool. With this change, wrap the listener in a custom type that will decrement the UsagePool when we close the listener. When all listeners are closed we will call Destruct on the tsnet Server which will Close the server and should cleanup potential references in the tailscale control plane Signed-off-by: Connor Kelly <connor.r.kelly@gmail.com>

clly · 2024-05-01T02:41:54Z

@mholt @willnorris I've switched from a UsagePool to just a global which made the references much simpler.

I've also wrapped the Listener's with a small type to ensure we call Delete on the servers UsagePool so that we automatically Close the tsnet server when all Listeners are closed.

I'm happy with the state of this PR and think it's a good jumping off point for more changes in the future. Let me know if you have strong opinions and I'm happy to make them. @willnorris I know that you thought that more verbose but explicit config would be better and if it would make you more comfortable with this change I'm happy to take care of it.

Thanks!

clly · 2024-05-01T12:33:04Z

module.go

+}
+
+func (t *tsnetServerListener) Close() error {
+	fmt.Println("Delete", t.hostname)


Drop this and close the actual listener as well

willnorris

Hey, thanks so much for hanging in there for so long! I've got a few minor comments below, but I've already made them in my local client, so I'll just push them to your branch, then squash and merge. I agree, this is a great new foundation that we can keep building on.

willnorris · 2024-05-02T21:07:19Z

app.go

+
+func (t *TSApp) Validate() error {
+	if t.DefaultAuthKey == "" {
+		return errors.New("auth_key must be set")


I don't think we want to require that a default auth key is set. A user could instead choose to specify only per-server key. That was actually how I initially tested this, and was surprised by the error.

Good point!

willnorris · 2024-05-02T21:15:57Z

app.go

+}
+
+type TSServer struct {
+	AuthKey string `json:"auth_key,omitempty" caddy:"namespace=auth_key"`


I've honestly never used these caddy struct tags before. I guess they're used in doc generation? This seems to be the only one that doesn't have "tailscale." in the namespace. Is that intentional?

It was, I think this is the translation for marshaling the caddy file format or going to/from JSON. It was modeled on the dynamicdns caddy app.

willnorris · 2024-05-02T21:34:25Z

module.go

+		return err
+	}
+
+	_, err := servers.Delete(t.hostname)


This definitely confused me at first. It looks like it will destruct the server on the first listener to close. I didn't realize this is really just a counter decrement on the usage pool. Maybe add a comment here that this decrements the usage pool for the server, and will close it only when it reaches zero?

willnorris · 2024-05-02T22:39:43Z

app.go

+		}
+	}
+
+	app = t


the global app value shouldn't be overwritten as part of Provisioning, since the new apps are provisioned and started before the old one is stopped. So this should probably be inside of Start(). We should probaby also make app an atomic.Pointer since we'll have multiple TSApps potentially trying to write to it.

Good point. I didn't realize these apps were started and stopped at the same time.

willnorris · 2024-05-02T22:40:53Z

app.go

+}
+
+func (t *TSApp) Stop() error {
+	app = nil


If we make app an atomic.Pointer, then this should probably be app.CompareAndSwap(t, nil) so that it only gets set to nil if it is still pointing to the current t.

If the apps are started and stopped at the same time would we do this at all?

Could we have a race condition where stop on an old app is called after start on a new one?

I had the same thought. For updating configs this isn't really necessary because the new TSApp's Start will overwrite the value before Stop is called. But if the new config removes the Tailscale configuration, we still want to clean this up. The use of atomic and CompareAndSwap handle race conditions.

Correct me if I'm wrong but atomic will solve concurrent access but not an ordering error where the calls are not guaranteed to always be in the same order?

Yes, you're absolutely right that atomic is focused on handling concurrent access. It's the CompareAndSwap that helps us here. On Start, we always update the value of our singleton to the new value. But on Stop, we only set it to nil if the value is still the old value. So depending on the order they run...

old.Stop() // this will update tsapp to nil new.Start() // this will update tsapp to new new.Start() // this will update tsapp to new old.Stop() // this won't do anything because tsapp no longer points to old, which is what CompareAndSwap checks for

In the case of the new caddy config not having a tailscale config at all, then only old.Stop() gets called, which will set tsapp to nil and free up the memory.

Got it. Thanks for the explanation!

willnorris · 2024-05-02T22:56:44Z

app.go

+}
+
+func (t *TSApp) Provision(ctx caddy.Context) error {
+	for _, svr := range t.Servers {


I don't think this is doing anything. Ranging over the t.Servers map is returning copies of the servers, so the changes aren't being stored back. You would need to assign the updated svr value back to the t.Servers map. But you don't really need this at all, since you check the default values in the getAuthKey and getEphemeral funcs.

So I think you can just remove the for loop entirely.

I had this to try to make the usage pool for the app easier before I threw it out and never came back to update this here

Thanks!

Signed-off-by: Will Norris <will@tailscale.com>

willnorris · 2024-05-02T23:21:43Z

Thanks for your review help @mholt !

mholt · 2024-05-03T15:21:06Z

I feel like I dropped the ball on this one, been very busy with the 2.8 release and such. But you both did a great job!

unixfox · 2024-05-05T19:48:50Z

I'm late to the party but I guess now we can more easily add the functionality requested in #22 and #18?

Giving the ability to use a hosted login server for tailscale --login-server=<url>: https://tailscale.com/kb/1080/cli#login

willnorris · 2024-05-06T18:52:09Z

I'm late to the party but I guess now we can more easily add the functionality requested in #22 and #18?

Yes, that was exactly my thought as well! We now have an obvious place to set these things. We won't add support for any new Tailscale-specific environment variables, since you can just use caddy's built-in support for env vars.

unixfox · 2024-05-06T19:17:57Z

since you can just use caddy's built-in support for env vars.

Interesting. How does it works? I didn't really understood how to do that.

I can help to improve the documentation if you can direct me into the right direction :)

mholt · 2024-05-06T19:33:16Z

Like placeholders? {env.*} where * is the name of the env var.

clly force-pushed the clly/caddy-config branch from 5745e44 to ddc9b3d Compare February 12, 2024 04:06

clly mentioned this pull request Feb 25, 2024

Proposal: Configuration block directives for loading tailscale configuration #24

Closed

mholt reviewed Apr 15, 2024

View reviewed changes

clly added 8 commits April 30, 2024 21:38

Create initial caddy-tailscale app config

ded0324

Register a parseGlobal function to parse the caddy configuration file. The parse function creates a new TSApp which will hold our configuration and create the CaddyModule. Signed-off-by: Connor Kelly <connor.r.kelly@gmail.com>

Create a config for parsing a tailscale server

00def93

Signed-off-by: Connor Kelly <connor.r.kelly@gmail.com>

Provision variables into caddy usage pool

31abd2b

Signed-off-by: Connor Kelly <connor.r.kelly@gmail.com>

Registry the module into caddy

733f7ea

Signed-off-by: Connor Kelly <connor.r.kelly@gmail.com>

Fetch authkey in two phases

4bb0252

Initially see if the provisioned server or global key exist in the app usage pool. If not then fallback to the already existing environment variables Signed-off-by: Connor Kelly <connor.r.kelly@gmail.com>

Support host specific keys

17f85e5

Signed-off-by: Connor Kelly <connor.r.kelly@gmail.com>

Switch from UsagePool to global

ffef646

Signed-off-by: Connor Kelly <connor.r.kelly@gmail.com>

clly force-pushed the clly/caddy-config branch from b35ddf8 to 9fc8b14 Compare May 1, 2024 02:38

clly commented May 1, 2024

View reviewed changes

Remove print and close Listener

f6a9281

willnorris approved these changes May 2, 2024

View reviewed changes

small fixes from code review

d74f857

Signed-off-by: Will Norris <will@tailscale.com>

willnorris merged commit 25b816a into tailscale:main May 2, 2024
1 check failed

willnorris mentioned this pull request Jun 3, 2024

Reload fails: "tsnet: listener already open for tailscale, :80" #6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support tailscale configuration via Caddy configuration #30

Support tailscale configuration via Caddy configuration #30

clly commented Feb 12, 2024

mholt commented Mar 6, 2024 •

edited

Loading

clly commented Apr 2, 2024

willnorris commented Apr 2, 2024

mholt commented Apr 3, 2024 •

edited

Loading

clly commented Apr 6, 2024

mholt Apr 15, 2024

clly Apr 17, 2024

mholt Apr 17, 2024

clly commented Apr 27, 2024

clly commented May 1, 2024

clly May 1, 2024

willnorris left a comment

willnorris May 2, 2024

clly May 3, 2024

willnorris May 2, 2024

clly May 3, 2024

willnorris May 2, 2024

willnorris May 2, 2024

clly May 3, 2024

willnorris May 2, 2024

clly May 3, 2024

willnorris May 3, 2024

clly May 3, 2024

willnorris May 3, 2024 •

edited

Loading

clly May 3, 2024

willnorris May 2, 2024

clly May 3, 2024

willnorris commented May 2, 2024

mholt commented May 3, 2024

unixfox commented May 5, 2024 •

edited

Loading

willnorris commented May 6, 2024

unixfox commented May 6, 2024

mholt commented May 6, 2024

+              		}
+              	}
+              	app = t

Support tailscale configuration via Caddy configuration #30

Support tailscale configuration via Caddy configuration #30

Conversation

clly commented Feb 12, 2024

mholt commented Mar 6, 2024 • edited Loading

clly commented Apr 2, 2024

willnorris commented Apr 2, 2024

mholt commented Apr 3, 2024 • edited Loading

clly commented Apr 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clly commented Apr 27, 2024

clly commented May 1, 2024

Choose a reason for hiding this comment

willnorris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willnorris May 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willnorris commented May 2, 2024

mholt commented May 3, 2024

unixfox commented May 5, 2024 • edited Loading

willnorris commented May 6, 2024

unixfox commented May 6, 2024

mholt commented May 6, 2024

mholt commented Mar 6, 2024 •

edited

Loading

mholt commented Apr 3, 2024 •

edited

Loading

willnorris May 3, 2024 •

edited

Loading

unixfox commented May 5, 2024 •

edited

Loading