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

refactor caddy-tailscale logic #39

Merged
merged 1 commit into from
May 8, 2024
Merged

refactor caddy-tailscale logic #39

merged 1 commit into from
May 8, 2024

Conversation

willnorris
Copy link
Member

This is admittedly a pretty sizeable refactor, but is almost exclusively renames, code movement, and documentation. There should be no logic changes. There is technically one breaking change in renaming TSApp.Servers to TSApp.Nodes, but only for folks using the JSON config directly (caddyfile doesn't use the name).

The refactoring in this change includes:

  • Move TailscaleAuth logic into auth.go
  • Move all TSApp logic into app.go (including caddyfile parsing)
  • Rename "server" to "node" throughout. This aligns better with Tailscale terminology, and is reflective of the fact that nodes can also just be used as proxy transports, in which case they are not acting as servers at all.
  • Generally prefer referring to a node's "name" than "host". While this name is still used as the default hostname for the node, I would expect that to change with a future iteration of Set Tailscale hostname through environment variable. #18.
  • add godocs throughout

@willnorris
Copy link
Member Author

/cc @clly and @mholt though don't worry if this is more than you've got time to look at. Even if the changes are straightforward, it's still a lot.

Copy link
Contributor

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Neat! I didn't read it too thoroughly yet but with a quick scan I don't see anything that caught my attention other than a typo :) (I don't know much/anything about TS-domain stuff so I was just looking at the Caddy stuff)

transport.go Outdated Show resolved Hide resolved
@willnorris
Copy link
Member Author

awesome, thanks for giving it a once-over!

Copy link
Contributor

@clly clly left a comment

Choose a reason for hiding this comment

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

Looks like a really clean refactor!

transport.go Outdated
@@ -21,7 +33,8 @@ func (t *TailscaleCaddyTransport) UnmarshalCaddyfile(d *caddyfile.Dispenser) err
func (t *TailscaleCaddyTransport) Provision(ctx caddy.Context) error {
t.logger = ctx.Logger()

s, err := getServer(ctx, "caddy-tsnet-client:80")
// TODO(will): allow users to specify a node name used to lookup that node's config in TSApp.
s, err := getNode(ctx, "caddy-tsnet-client:80")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be just caddy-tsnet-client?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, good catch! I remember seeing that, and forgot to fix it

This includes:
 - Move TailscaleAuth logic into auth.go
 - Move all TSApp logic into app.go (including caddyfile parsing)
 - Rename "server" to "node" throughout. This aligns better with
   Tailscale terminology, and is reflective of the fact that nodes can
   also just be used as proxy transports, in which case they are not
   acting as servers at all.
 - Generally prefer referring to a node's "name" than "host". While this
   name is still used as the default hostname for the node, I would
   expect that to change with a future iteration of #18.
 - add godocs throughout

Signed-off-by: Will Norris <will@tailscale.com>
@willnorris willnorris merged commit 27fad94 into main May 8, 2024
1 check passed
@willnorris willnorris deleted the will/node branch May 8, 2024 03:05
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

3 participants