Integrate tailscale into k3s #7352

manuelbuil · 2023-04-26T07:22:38Z

Proposed Changes

This PR adds the necessary code to integrate VPNs into k3s and incorporates tailscale as the first VPN option.
It also includes an ADR that explains the change

Types of Changes

New feature

Verification

1 - Install tailscale in the node
2 - Create a key in tailscale
3 - Deploy k3s with the flag:

vpn-auth: "name=tailscale,joinKey=$CREATED_KEY"

4 - Verify that:
a) Tailscale interface exists and has an IP
b) kubectl get endpoints is using the tailscale IP
c) kubectl get nodes -o wide shows the tailscale IP as the nodeIP

Testing

Linked Issues

#7353

User-Facing Change

Integration of tailscale VPN into k3s

Further Comments

pkg/agent/flannel/setup.go

pkg/netutil/vpn.go

pkg/cli/cmds/agent.go

pkg/netutil/vpn.go

cwayne18 · 2023-04-27T18:25:26Z

Can we get either an integration or an e2e test for this?

nikolaishields · 2023-04-28T21:37:47Z

From a community perspective it would be really nice to include the ability to setup a tailscale funnel at node deploy time to allow public access to the node at a targeted port or service address. This would come in particularly convenient in situations where public ingress is desired. A few example use-cases:

IDP (keycloak, vault, ory, dex) is deployed to the cluster by the k3s helm-manifest controller at cluster deploy. This would allow k3s users to have SSO access to their clusters immediately after bootstrap.
Traefik ingress is proxying public services, yet the cluster is behind CGNAT and ports cannot be exposed.
HTTP01 cert-generation via public exposure of the ACME endpoint

Perhaps a Kubernetes ingress integration could even be designed that instantiates a funnel as a CRD when ingress is created. The sky's the limit.
Special thanks to @manuelbuil for drafting this as it looks awesome!

codecov · 2023-05-01T13:53:16Z

Codecov Report

Patch coverage has no change and project coverage change: -28.27 ⚠️

Comparison is base (e5e1a67) 47.55% compared to head (023514f) 19.29%.

❗ Current head 023514f differs from pull request most recent head 869e030. Consider uploading reports for the commit 869e030 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #7352       +/-   ##
===========================================
- Coverage   47.55%   19.29%   -28.27%     
===========================================
  Files         141       81       -60     
  Lines       14347     5463     -8884     
===========================================
- Hits         6823     1054     -5769     
+ Misses       6442     4187     -2255     
+ Partials     1082      222      -860

Flag	Coverage Δ
inttests	`?`
unittests	`19.29% <0.00%> (-0.58%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/agent/flannel/setup.go	`0.00% <0.00%> (-41.14%)`	⬇️
pkg/cli/cmds/agent.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/cli/cmds/server.go	`0.00% <ø> (-100.00%)`	⬇️
pkg/daemons/config/types.go	`67.21% <ø> (-17.24%)`	⬇️

... and 137 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

manuelbuil · 2023-05-01T16:15:24Z

From a community perspective it would be really nice to include the ability to setup a tailscale funnel at node deploy time to allow public access to the node at a targeted port or service address. This would come in particularly convenient in situations where public ingress is desired. A few example use-cases:

IDP (keycloak, vault, ory, dex) is deployed to the cluster by the k3s helm-manifest controller at cluster deploy. This would allow k3s users to have SSO access to their clusters immediately after bootstrap.

Traefik ingress is proxying public services, yet the cluster is behind CGNAT and ports cannot be exposed.

HTTP01 cert-generation via public exposure of the ACME endpoint

Perhaps a Kubernetes ingress integration could even be designed that instantiates a funnel as a CRD when ingress is created. The sky's the limit. Special thanks to @manuelbuil for drafting this as it looks awesome!

Hey @nikolaishields, how are you? Thanks for your suggestion :). There are several cool features that tailscale provides, such as funneling, but I decided to keep them outside of the integration (at least for the time being). I want to keep it dead simple and check what is the community reaction, typical bugs, etc before taking the next iteration.

The CRD+ingress idea could be cool. I guess you mean having the ingress service as a nodePort service and offering that port with tailscale funnel. Kind of a "type: loadbalancer" service but without the loadbalancer, right? Wouldn't klipper-lb fulfill that use case too? Now I want/need to test it

manuelbuil · 2023-05-01T16:33:15Z

Can we get either an integration or an e2e test for this?

Yep, on it

nikolaishields · 2023-05-01T17:47:15Z

Hey @nikolaishields, how are you?

I'm doing well thanks for asking!

There are several cool features that tailscale provides, such as funneling, but I decided to keep them outside of the integration (at least for the time being). I want to keep it dead simple and check what is the community reaction, typical bugs, etc before taking the next iteration.

Makes perfect sense, if you ever want/need a hand on implementing any of these features let me know. Happy to help!

The CRD+ingress idea could be cool. I guess you mean having the ingress service as a nodePort service and offering that port with tailscale funnel. Kind of a "type: loadbalancer" service but without the loadbalancer, right? Wouldn't klipper-lb fulfill that use case too? Now I want/need to test it

It probably could and then a user could just run a funnel against the node port. In theory I suppose one could also just funnel a service directly via local dns lookup (e.g. tailscale serve <servicename>.default.cluster.local:<port number>).

manuelbuil · 2023-05-02T11:32:40Z

Can we get either an integration or an e2e test for this?

Actually, could we merge this PR and then work on the e2e test? It complicates things that the code is not yet part of upstream k3s

stale

brandond · 2023-05-02T21:56:00Z

I would defer to @cwayne18 on merging without tests. I think @dereknola might have some suggestions on how to add tests before the PR is merged? I have had some frustrations with that process as well.

tests/e2e/tailscale/tailscale_test.go

manuelbuil · 2023-05-04T16:27:25Z

I would defer to @cwayne18 on merging without tests. I think @dereknola might have some suggestions on how to add tests before the PR is merged? I have had some frustrations with that process as well.

I am adding them in the end

dereknola · 2023-06-08T16:49:20Z

pkg/util/command.go

+	var out bytes.Buffer
+
+	cmd := exec.Command(command, args...)
+	cmd.Stdout = &out
+	err := cmd.Run()
+	if err != nil {
+		return "", err
+	}


With this, you get more info if there is an error, the err outs from exec are almost always just exit 1/2, which isn't helpful.

Suggested change

var out bytes.Buffer

cmd := exec.Command(command, args...)

cmd.Stdout = &out

err := cmd.Run()

if err != nil {

return "", err

}

var out, errOut bytes.Buffer

cmd := exec.Command(command, args...)

cmd.Stdout = &out

cmd.Stderr = &errOut

err := cmd.Run()

if err != nil {

return errOut.String(), err

}

nice, thanks for that. The user of this function should then be aware that the interesting error is the string returned and not the error, right? I'll add a comment

dereknola · 2023-06-08T16:50:59Z

pkg/util/file.go

+		return "", nil
+	}
+
+	for {


I would suggest adding a failure state is after 5min or something, the path still does not exist.

yeah, good point, let me replace the infinite loop with something else using time. I copied/pasted this function from another place though, and it never happened before, but better safe than sorry

dereknola · 2023-06-08T16:52:28Z

pkg/util/file_windows.go

@@ -11,3 +14,21 @@ func SetFileModeForPath(name string, mode os.FileMode) error {
 func SetFileModeForFile(file *os.File, mode os.FileMode) error {
 	return nil
 }
+
+// ReadFile reads from a file
+func ReadFile(path string) (string, error) {


Same comment as in the linux version

I don't understand why this is OS-specific, there doesn't appear to be anything in either function that would differ across platforms.

We could remove it in another PR (just in case we need to revert) and see if things are still working

dereknola · 2023-06-08T16:54:02Z

tests/e2e/tailscale/Vagrantfile

+  node.vm.network "private_network", 
+    :ip => node_ip4,
+    :netmask => "255.255.255.0",
+    :libvirt__dhcp_enabled => false,
+    :libvirt__forward_mode => "none"


Do we need the special libvirt flags? Its not dualstack

Suggested change

node.vm.network "private_network",

:ip => node_ip4,

:netmask => "255.255.255.0",

:libvirt__dhcp_enabled => false,

:libvirt__forward_mode => "none"

vm.network "private_network", ip: node_ip, netmask: "255.255.255.0"

oh, I think we are using those flags in all the networking tests. We should remove it when not needed then. Thanks!

brandond · 2023-06-08T19:05:43Z

docs/adrs/integrat-vpns.md

@@ -0,0 +1,76 @@
+# Integrate vpn in k3s


nit on the filename - should it be integrate-vpns.md? I'm not sure why the word was truncated.

good catch, probably my mistake

brandond · 2023-06-08T19:18:59Z

pkg/cli/cmds/agent.go

@@ -151,6 +153,18 @@ var (
 		Usage:       "(agent/networking) Override default flannel cni config file",
 		Destination: &AgentConfig.FlannelCniConfFile,
 	}
+	VPNAuth = &cli.StringFlag{
+		Name:        "vpn-auth",
+		Usage:       "(agent/networking) (experimental) Credentials for the VPN provider. It must include the provider name and join key in the format name=tailscale,joinKey=<key>",


nit: be consistent with the format for vpn-auth-file

Suggested change

Usage: "(agent/networking) (experimental) Credentials for the VPN provider. It must include the provider name and join key in the format name=tailscale,joinKey=<key>",

Usage: "(agent/networking) (experimental) Credentials for the VPN provider. It must include the provider name and join key in the format name=<vpn-provider>,joinKey=<key>",

pkg/util/file.go

brandond · 2023-06-08T19:28:08Z

pkg/util/file_windows.go

@@ -11,3 +14,21 @@ func SetFileModeForPath(name string, mode os.FileMode) error {
 func SetFileModeForFile(file *os.File, mode os.FileMode) error {
 	return nil
 }
+
+// ReadFile reads from a file
+func ReadFile(path string) (string, error) {


I don't understand why this is OS-specific, there doesn't appear to be anything in either function that would differ across platforms.

Signed-off-by: Manuel Buil <mbuil@suse.com>

concerns were addressed

manuelbuil requested a review from a team as a code owner April 26, 2023 07:22

manuelbuil force-pushed the vpnintegrations-afterparental branch from cfc3a7b to 622c8fa Compare April 26, 2023 09:51

brandond reviewed Apr 26, 2023

View reviewed changes

pkg/agent/flannel/setup.go Outdated Show resolved Hide resolved

brandond reviewed Apr 26, 2023

View reviewed changes

pkg/agent/flannel/setup.go Outdated Show resolved Hide resolved

manuelbuil force-pushed the vpnintegrations-afterparental branch 2 times, most recently from d938d1e to b0668be Compare April 26, 2023 14:33

manuelbuil requested a review from brandond April 26, 2023 16:07

dereknola requested changes Apr 26, 2023

View reviewed changes

pkg/netutil/vpn.go Outdated Show resolved Hide resolved

manuelbuil force-pushed the vpnintegrations-afterparental branch from b0668be to 023514f Compare April 26, 2023 16:54

manuelbuil requested a review from matttrach April 26, 2023 19:42

dereknola approved these changes Apr 26, 2023

View reviewed changes

brandond previously requested changes Apr 27, 2023

View reviewed changes

pkg/cli/cmds/agent.go Outdated Show resolved Hide resolved

pkg/netutil/vpn.go Outdated Show resolved Hide resolved

pkg/netutil/vpn.go Outdated Show resolved Hide resolved

pkg/netutil/vpn.go Outdated Show resolved Hide resolved

manuelbuil force-pushed the vpnintegrations-afterparental branch from 023514f to 2a8954a Compare April 27, 2023 13:12

manuelbuil force-pushed the vpnintegrations-afterparental branch from 2a8954a to 60ce4b9 Compare May 1, 2023 13:52

manuelbuil force-pushed the vpnintegrations-afterparental branch from 60ce4b9 to c75499d Compare May 1, 2023 15:04

manuelbuil requested a review from brandond May 2, 2023 13:39

manuelbuil force-pushed the vpnintegrations-afterparental branch 2 times, most recently from 21ab427 to a6e0809 Compare May 4, 2023 14:55

dereknola reviewed May 4, 2023

View reviewed changes

tests/e2e/tailscale/tailscale_test.go Outdated Show resolved Hide resolved

manuelbuil force-pushed the vpnintegrations-afterparental branch from a6e0809 to 4d285a4 Compare May 5, 2023 08:58

manuelbuil force-pushed the vpnintegrations-afterparental branch from b429d71 to 3d8540c Compare June 1, 2023 07:02

manuelbuil requested a review from rbrtbnfgl June 1, 2023 15:26

manuelbuil force-pushed the vpnintegrations-afterparental branch from 3d8540c to 83c51d1 Compare June 6, 2023 06:30

rbrtbnfgl approved these changes Jun 6, 2023

View reviewed changes

manuelbuil force-pushed the vpnintegrations-afterparental branch 7 times, most recently from fab47fc to f171993 Compare June 8, 2023 16:41

dereknola requested changes Jun 8, 2023

View reviewed changes

brandond previously requested changes Jun 8, 2023

View reviewed changes

manuelbuil force-pushed the vpnintegrations-afterparental branch from f171993 to 6147c87 Compare June 9, 2023 07:20

manuelbuil requested review from brandond and dereknola June 9, 2023 07:20

VPN PoC

869e030

Signed-off-by: Manuel Buil <mbuil@suse.com>

manuelbuil force-pushed the vpnintegrations-afterparental branch from 6147c87 to 869e030 Compare June 9, 2023 10:39

briandowns approved these changes Jun 9, 2023

View reviewed changes

dereknola approved these changes Jun 9, 2023

View reviewed changes

manuelbuil merged commit 268c9a7 into k3s-io:master Jun 9, 2023

manuelbuil deleted the vpnintegrations-afterparental branch June 9, 2023 17:02

This was referenced Jun 9, 2023

[Release 1.26] VPN integration #7727

Merged

[Release 1.25] VPN integration #7728

Merged

[Release 1.24] VPN integration #7729

Merged

VestigeJ mentioned this pull request Jun 9, 2023

Integrate tailscale into k3s #7353

Closed

manuelbuil mentioned this pull request Jun 12, 2023

Remove useless libvirt config #7745

Merged

dennwc mentioned this pull request Jun 20, 2023

Support setting control server URL for Tailscale #7807

Merged

cguertin14 mentioned this pull request Jun 28, 2023

Minor: k3s update from v1.27.2+k3s1 to v1.27.3+k3s1 cguertin14/k3s-ansible-ha#22

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate tailscale into k3s #7352

Integrate tailscale into k3s #7352

manuelbuil commented Apr 26, 2023 •

edited

Loading

cwayne18 commented Apr 27, 2023

nikolaishields commented Apr 28, 2023 •

edited

Loading

codecov bot commented May 1, 2023 •

edited

Loading

manuelbuil commented May 1, 2023

manuelbuil commented May 1, 2023

nikolaishields commented May 1, 2023

manuelbuil commented May 2, 2023

brandond commented May 2, 2023

manuelbuil commented May 4, 2023

dereknola Jun 8, 2023

manuelbuil Jun 9, 2023

dereknola Jun 8, 2023

manuelbuil Jun 9, 2023

dereknola Jun 8, 2023

brandond Jun 8, 2023

manuelbuil Jun 9, 2023

dereknola Jun 8, 2023

manuelbuil Jun 9, 2023

brandond Jun 8, 2023

manuelbuil Jun 9, 2023

brandond Jun 8, 2023

manuelbuil Jun 9, 2023

brandond Jun 8, 2023

	Usage: "(agent/networking) (experimental) Credentials for the VPN provider. It must include the provider name and join key in the format name=tailscale,joinKey=<key>",
	Usage: "(agent/networking) (experimental) Credentials for the VPN provider. It must include the provider name and join key in the format name=<vpn-provider>,joinKey=<key>",

Integrate tailscale into k3s #7352

Integrate tailscale into k3s #7352

Conversation

manuelbuil commented Apr 26, 2023 • edited Loading

Proposed Changes

Types of Changes

Verification

Testing

Linked Issues

User-Facing Change

Further Comments

cwayne18 commented Apr 27, 2023

nikolaishields commented Apr 28, 2023 • edited Loading

codecov bot commented May 1, 2023 • edited Loading

Codecov Report

manuelbuil commented May 1, 2023

manuelbuil commented May 1, 2023

nikolaishields commented May 1, 2023

manuelbuil commented May 2, 2023

brandond commented May 2, 2023

manuelbuil commented May 4, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuelbuil commented Apr 26, 2023 •

edited

Loading

nikolaishields commented Apr 28, 2023 •

edited

Loading

codecov bot commented May 1, 2023 •

edited

Loading