This PR wires uptunnel.go to atailnet.Conn via the new/tailnet endpoint, with all the necessary controllers such that a VPN connection can be started, stopped and inspected via the CoderVPN protocol.

github-actionsbot assignedethanndickson

Nov 21, 2024

Copy link

MemberAuthor

ethanndickson commentedNov 21, 2024•
edited
Loading

This stack of pull requests is managed byGraphite. Learn more aboutstacking.

ethanndickson commented

Nov 21, 2024

View reviewed changes

vpn/vpn.proto

		stringapi_token=3;
		// Additional HTTP headers added to all requests
		messageHeader {
		stringname=1;

Copy link

MemberAuthor

ethanndicksonNov 21, 2024

Choose a reason for hiding this comment

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

Deliberately not a map to support repeated headers

spikecurtis reviewed

Nov 21, 2024

View reviewed changes

vpn/tunnel.go OutdatedShow resolvedHide resolved

tailnet/controllers.go OutdatedShow resolvedHide resolved

vpn/client.go OutdatedShow resolvedHide resolved

ethanndickson commented

Nov 22, 2024

View reviewed changes

vpn/client_test.go

		"github.com/coder/coder/v2/vpn"
		)

		funcTestClient_WorkspaceUpdates(t*testing.T) {

Copy link

MemberAuthor

ethanndicksonNov 22, 2024

Choose a reason for hiding this comment

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

This test might be a bit much, but I wanted to test the Client without spinning up a wholecoderd, and it turns out that takes a bit of code.

Copy link

Member

deansheatherNov 22, 2024

Choose a reason for hiding this comment

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

This is a lot. Is there any particular reason to not spin up coderd (other than wanting to be lean I guess) to avoid all this boilerplate?

vpn/client.goShow resolvedHide resolved

vpn/client.go


		typeConninterface {
		CurrentWorkspaceState()*proto.WorkspaceUpdate
		Close()error

Copy link

MemberAuthor

ethanndicksonNov 22, 2024

Choose a reason for hiding this comment

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

I suspect this interface will grow with whatever functions fromtailnet.Conn we end up needing.

ethanndickson requested review fromdeansheather andspikecurtis

November 22, 2024 05:40

ethanndickson marked this pull request as ready for review

November 22, 2024 05:40

ethanndickson requested a review fromjohnstcn as acode owner

November 22, 2024 05:40

ethanndickson changed the title~~chore: implement vpn client & dylib tunnel~~chore: implement CoderVPN client & tunnel

Nov 22, 2024

spikecurtis reviewed

Nov 22, 2024

View reviewed changes

vpn/tunnel_internal_test.go OutdatedShow resolvedHide resolved

vpn/client.go OutdatedShow resolvedHide resolved

vpn/tunnel.go OutdatedShow resolvedHide resolved

vpn/tunnel.go Outdated

		fori,agent:=rangeupdate.UpsertedAgents {
		out.UpsertedAgents[i]=&Agent{
		Id:agent.Id,
		Name:agent.Name,

Copy link

Contributor

spikecurtisNov 22, 2024

Choose a reason for hiding this comment

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

missing workspace_id, fqdn.

Also missing ip address and last handshake, but we can save those for another PR.

vpn/tunnel.go OutdatedShow resolvedHide resolved

vpn/client.go OutdatedShow resolvedHide resolved

deansheather reviewed

Nov 22, 2024

View reviewed changes

tailnet/controllers.go OutdatedShow resolvedHide resolved

vpn/tunnel.go OutdatedShow resolvedHide resolved

johnstcn reviewed

Nov 22, 2024

View reviewed changes

vpn/tunnel.go

Comment on lines +82 to +93

		// TODO: Wait for the reply to be sent before closing the speaker.
		// err := t.speaker.Close()
		// if err != nil {
		// t.logger.Error(t.ctx, "failed to close speaker", slog.Error(err))
		// }

Copy link

Member

johnstcnNov 22, 2024

Choose a reason for hiding this comment

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

I think our tunnel may need a mutex or sync.Once to handle being closed properly?

tailnet/conn.go

Comment on lines +256 to +260

		ifoptions.TUNDev==nil {
		dialer.NetstackDialTCP=func(ctx context.Context,dst netip.AddrPort) (net.Conn,error) {
		returnnetStack.DialContextTCP(ctx,dst)
		}
		netStack.ProcessLocalIPs=true

Copy link

Member

johnstcnNov 22, 2024

Choose a reason for hiding this comment

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

Can we consolidate the places we do this check onoptions.TUNDev?

Copy link

MemberAuthor

ethanndicksonNov 25, 2024

Choose a reason for hiding this comment

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

I'm not sure if changing some of the ordering here breaks anything, may need to revisit cleaning this up once we have tests that pass in a real TUN fd. Theconn.go changes I just copied from Spike's prototype branch.

Copy link

Contributor

spikecurtisNov 25, 2024

Choose a reason for hiding this comment

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

Unfortunately, ordering does matter here in general. It might be possible to do it better.

tailnet/controllers.go OutdatedShow resolvedHide resolved

ethanndickson commented

Nov 25, 2024

View reviewed changes

tailnet/controllers.go

		func (wworkspace)addAllDNSNames(namesmap[dnsname.FQDN][]netip.Addr,ownerstring)error {
		for_,a:=rangew.agents {
		// updateDNSNames updates the DNS names for all agents in the workspace.
		func (w*Workspace)updateDNSNames()error {

Copy link

MemberAuthor

ethanndicksonNov 25, 2024•
edited
Loading

Choose a reason for hiding this comment

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

Including all the FQDNs as part of an outgoing workspace update was kinda tricky, particularly because of thelen(agents) == 1 alias.

This solution just has this function update the hosts on each agent, and then store all the agents and workspaces as pointers. This lets us build our workspace update as we apply the diff to the state, and then before sending, we fill in all the correct FQDNs, which applies to both the state, and the outgoing update.

ethanndickson commented

Nov 25, 2024

View reviewed changes

tailnet/controllers.go

		func (t*tunnelUpdater)updateDNSNames()map[dnsname.FQDN][]netip.Addr {
		names:=make(map[dnsname.FQDN][]netip.Addr)
		for_,w:=ranget.workspaces {
		err:=w.addAllDNSNames(names,t.ownerUsername)

Copy link

MemberAuthor

ethanndicksonNov 25, 2024•
edited
Loading

Choose a reason for hiding this comment

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

I moved theownerUsername from an argument when updating a workspace to a private field on theWorkspace, as it made more sense for future-proofing w.r.t. shared workspaces.

ethanndickson requested review fromdeansheather andspikecurtis

November 25, 2024 05:50

deansheather approved these changes

Nov 25, 2024

View reviewed changes

vpn/tunnel.go

Comment on lines 156 to 177

		funcUseAsRouter()TunnelOption {
		returnfunc(t*Tunnel) {
		t.router=NewRouter(t)
		}
		}

		funcUseAsLogger()TunnelOption {
		returnfunc(t*Tunnel) {
		t.logger=t.logger.AppendSinks(t)
		}
		}

		funcUseAsDNSConfig()TunnelOption {
		returnfunc(t*Tunnel) {
		t.dnsConfigurator=NewDNSConfigurator(t)
		}
		}