Fix: Can't use Unix Sockets in Quick Tunnel mode #1367
Conversation
cmd/cloudflared/tunnel/cmd.go
Outdated
| shouldRunQuickTunnel := c.IsSet("url") || c.IsSet("unix-socket") || c.IsSet(ingress.HelloWorldFlag) | ||
| if !c.IsSet("proxy-dns") && c.String("quick-service") != "" && shouldRunQuickTunnel { | ||
| return RunQuickTunnel(sc) | ||
| return runQuickTunnel(sc) |
There was a problem hiding this comment.
I was hoping to avoid changes here as far as possible, but I'm happy to refactor this into a separate function if this is too hacky.
There was a problem hiding this comment.
@rhyswilliamsza
What is the point of this change ?
You have just aliased RunQuickTunnel to runQuickTunnel. Why ?
There was a problem hiding this comment.
Hi @lazysegtree !
Thanks for taking a look. I've unfortunately had to do this to allow injection of mock RunQuickTunnel's during testing (see cmd_test#61). Please let me know if there is a different way for me to be doing this!
There was a problem hiding this comment.
@rhyswilliamsza Got it. The test cases still seems pretty hacky (assigning runQuickTunnel to a new mock function, and defering the call to reset it. monkey patching). I would try to figure out if there is a cleaner way to mock RunQuickTunnel with the testify package.
A way that is not hacky, but neither does it introduces too many changes in source.
Still yours might be the only way that introduces minimum changes in the source.
There was a problem hiding this comment.
Yip, I agree but unfortunately couldn't find a better way to mock out using the existing testing framework... I'll take another look after work today.
There was a problem hiding this comment.
@rhyswilliamsza We could make TunnelCommand function a wrapper, that calls the actual function with a QuickTunnerRunner parameter. Then we could use a diff mock implementation in test.
Thats a bit more intrusive on the source code, but less hacky.
Tell me what you think about this.
Testcase run and diff
➜ ~/Workspace/Other_proj/rhyswilliamsza/cloudflared/cmd/cloudflared/tunnel git:(fix/unix-socket-quick-tunnel) ✗ [9:12:02] cfgo test -v -run "TestShouldRunQuickTunnel"
=== RUN TestShouldRunQuickTunnel
=== RUN TestShouldRunQuickTunnel/Quick_tunnel_with_URL_set
=== RUN TestShouldRunQuickTunnel/Quick_tunnel_with_unix-socket_set
=== RUN TestShouldRunQuickTunnel/Quick_tunnel_with_hello-world_flag
=== RUN TestShouldRunQuickTunnel/Quick_tunnel_with_proxy-dns_(invalid_combo)
=== RUN TestShouldRunQuickTunnel/No_quick-service_set
--- PASS: TestShouldRunQuickTunnel (0.00s)
--- PASS: TestShouldRunQuickTunnel/Quick_tunnel_with_URL_set (0.00s)
--- PASS: TestShouldRunQuickTunnel/Quick_tunnel_with_unix-socket_set (0.00s)
--- PASS: TestShouldRunQuickTunnel/Quick_tunnel_with_hello-world_flag (0.00s)
--- PASS: TestShouldRunQuickTunnel/Quick_tunnel_with_proxy-dns_(invalid_combo) (0.00s)
--- PASS: TestShouldRunQuickTunnel/No_quick-service_set (0.00s)
PASS
ok github.com/cloudflare/cloudflared/cmd/cloudflared/tunnel 1.251s
diff
➜ ~/Workspace/Other_proj/rhyswilliamsza/cloudflared/cmd/cloudflared/tunnel git:(fix/unix-socket-quick-tunnel) ✗ [9:16:05] git --no-pager diff
diff --git a/cmd/cloudflared/tunnel/cmd.go b/cmd/cloudflared/tunnel/cmd.go
index 24ec3817..21aff7b0 100644
--- a/cmd/cloudflared/tunnel/cmd.go
+++ b/cmd/cloudflared/tunnel/cmd.go
@@ -215,7 +215,6 @@ var (
"overwrite-dns",
"help",
}
- runQuickTunnel = RunQuickTunnel
)
func Flags() []cli.Flag {
@@ -287,7 +286,14 @@ See https://developers.cloudflare.com/cloudflare-one/connections/connect-apps/in
}
}
+// This is so that we can mock QuickTunnelRunner for TunnelCommand test cases
+type QuickTunnelRunner func(*subcommandContext) error
+
func TunnelCommand(c *cli.Context) error {
+ return TunnelCommandImpl(c, RunQuickTunnel)
+}
+
+func TunnelCommandImpl(c *cli.Context, quickTunnelRunner QuickTunnelRunner) error {
sc, err := newSubcommandContext(c)
if err != nil {
return err
@@ -316,7 +322,7 @@ func TunnelCommand(c *cli.Context) error {
// We don't support running proxy-dns and a quick tunnel at the same time as the same process
shouldRunQuickTunnel := c.IsSet("url") || c.IsSet("unix-socket") || c.IsSet(ingress.HelloWorldFlag)
if !c.IsSet("proxy-dns") && c.String("quick-service") != "" && shouldRunQuickTunnel {
- return runQuickTunnel(sc)
+ return quickTunnelRunner(sc)
}
// If user provides a config, check to see if they meant to use `tunnel run` instead
diff --git a/cmd/cloudflared/tunnel/cmd_test.go b/cmd/cloudflared/tunnel/cmd_test.go
index faf1de00..0ac039a0 100644
--- a/cmd/cloudflared/tunnel/cmd_test.go
+++ b/cmd/cloudflared/tunnel/cmd_test.go
@@ -55,10 +55,8 @@ func TestShouldRunQuickTunnel(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
// Mock RunQuickTunnel Function
- originalRunQuickTunnel := runQuickTunnel
- defer func() { runQuickTunnel = originalRunQuickTunnel }()
mockCalled := false
- runQuickTunnel = func(sc *subcommandContext) error {
+ runQuickTunnel := func(sc *subcommandContext) error {
mockCalled = true
return nil
}
@@ -69,7 +67,7 @@ func TestShouldRunQuickTunnel(t *testing.T) {
context := cli.NewContext(app, set, nil)
// Call TunnelCommand
- err := TunnelCommand(context)
+ err := TunnelCommandImpl(context, runQuickTunnel)
// Validate
if tt.expectError {
➜ ~/Workspace/Other_proj/rhyswilliamsza/cloudflared/cmd/cloudflared/tunnel git:(fix/unix-socket-quick-tunnel) ✗ [9:17:36](Maybe we can rename runQuickTunnel to mockQuickTunnelRunner and since TunnelCommandImpl is internal to the package, it could start with lowercase )
There was a problem hiding this comment.
Thanks, @lazysegtree! Sorry for the delay, has been a busy week.
I was hoping to change the source as little as possible, however I agree that it's better to do this in a more structured fashion. I like your approach better than the silly 'swapping in a function' method, even if it does require additional changes.
I have added you as a collaborator to the forked repo. Please feel free to stage the change directly to the rhyswilliamsza:fix/unix-socket-quick-tunnel branch, or let me know if you'd prefer I do so :)
There was a problem hiding this comment.
Cool. I have committed the changes. @rhyswilliamsza
| @@ -15,3 +18,76 @@ func TestHostnameFromURI(t *testing.T) { | |||
| assert.Equal(t, "", hostnameFromURI("trash")) | |||
| assert.Equal(t, "", hostnameFromURI("https://awesomesauce.com")) | |||
| } | |||
|
|
|||
| func TestShouldRunQuickTunnel(t *testing.T) { | |||
There was a problem hiding this comment.
I've scoped this specifically to quick tunnels, so it won't test falling through to other tunnel types.
| @@ -313,9 +314,9 @@ func TunnelCommand(c *cli.Context) error { | |||
| // Run a quick tunnel | |||
| // A unauthenticated named tunnel hosted on <random>.<quick-tunnels-service>.com | |||
| // We don't support running proxy-dns and a quick tunnel at the same time as the same process | |||
| shouldRunQuickTunnel := c.IsSet("url") || c.IsSet(ingress.HelloWorldFlag) | |||
| shouldRunQuickTunnel := c.IsSet("url") || c.IsSet("unix-socket") || c.IsSet(ingress.HelloWorldFlag) | |||
There was a problem hiding this comment.
I haven't done any side-effect analysis to check whether anyone could be relying on --unix-socket forcing a trapdoor to proxy-dns, but that seems unlikely so it should be okay.
There was a problem hiding this comment.
I tried going through the code.
In TunnelCommand function in cmd.go, if --name is not specified, only two non failure scenarios exists - quick tunnel or proxy dns.
And in case of proxy-dns, I did not found a possible use of --unix-socket anywhere, as much I could understand the code flow.
So, looks good.
From #1294:
This PR aims to address the above by adding
unix-socketas an accepted flag for spawning quick tunnels. I unfortunately couldn't find an elegant way to mock out theRunQuickTunnelcommand, so I had to add a small work around. Please let me know if there is a better standard for this.Thanks!