refactor: clean up outline-connectivity #146

fortuna · 2023-12-26T21:05:23Z

This PR cleans up the connectivity test, so it's easier to use different resolver implementations later.

It makes the test easier to use by explicitly returning the connectivity error and differentiating from invalid tests, so you don't need to use errors.As.

I'm renaming the binary to make it decoupled from Outline, since it's a generic tool.

I also fixed the report collection which was broken if the reporter was not specified.

/cc @jyyi1 @daniellacosse

amircybersec

I realized that I tested the correct PR (fortuna-connectivity branch) but I mistakenly entered my comments on PR #145 previously. I have moved my comments over the to correct PR and removed them from #145.

I fully tested all examples and code and review the changes. I believe we can go ahead with the merge to main. I have a few comments on the code that we can address in subsequent PRs.

Also I believe we need to also refactor outline-connectivity-app/shared-backend/main.go since it has connectivity testing chops. I can do that after we get this PR merged.

amircybersec · 2023-12-28T05:41:53Z

x/examples/outline-cli/outline_packet_proxy.go

I understand this PR is mostly a refactor and the current code is working, but it seems like we need TCP connectivity test here (NewTCPResolver) in addition to UDP (NewUDPResolver) for the CLI implementation. If that makes sense I can open a separate PR for it after we merge this into main.

This is a test for the Packet connectivity. The Stream connectivity is covered in the tests above.

I was referring mainly to Outline CLI code and did not see a test for Stream connectivity in Outline CLI implementation. I was wondering why the CLI is not testing for Stream connectivity and only Packet connectivity...

Ah, I thought it was the other file.
The CLI doesn't test stream connectivity because there's nothing to be done. It tests packet connectivity on network changes to decide whether to proxy packets or intercept DNS to do DNS over TCP.

amircybersec · 2023-12-28T05:50:30Z

x/examples/test-connectivity/main.go

 			switch proto {
 			case "tcp":
 				streamDialer, err := config.NewStreamDialer(*transportFlag)
 				if err != nil {
 					log.Fatalf("Failed to create StreamDialer: %v", err)
 				}
-				resolver := &transport.StreamDialerEndpoint{Dialer: streamDialer, Address: resolverAddress}
-				testDuration, testErr = connectivity.TestResolverStreamConnectivity(context.Background(), resolver, *domainFlag)
+				resolver = connectivity.NewTCPResolver(streamDialer, resolverAddress)
 			case "udp":
 				packetDialer, err := config.NewPacketDialer(*transportFlag)
 				if err != nil {
 					log.Fatalf("Failed to create PacketDialer: %v", err)


On GUI apps, log.Fatalf leads to apps crashing on this error, however, it does not matter for test_connectivity example here.

One solution I am using in pending mobile app PR 137 for GUI app (outline-connectivity-app/shared_backed/main.go) is to catch this error similar to other types of connectivity error (i.e protocol not supported by transport).

Since no connection is made, I set the duration to zero seconds. This way the go backend can communicate the issue to the front-end through ConnectivityTestResult in response to ConnectivityTestRequest that contains the config parameters. This error gets collected to report collector similar to other connectivity errors.

I think it makes sense to refactor the core logic of running a test such that the CLI, GUI app test operations are more consistent. I like the way the mobile app is implementing the logic with ConnectivityTestRequest and ConnectivityTestResult. Perhaps we can adopt that design in other places.

Lastly the GUI app code also needs refactor to use dns.NewUDPResolver and dns.NewTCPesolver. I can do that through a separate PR.

Suggested change

log.Fatalf("Failed to create PacketDialer: %v", err)

if err != nil {

testDuration = time.Duration(0)

testErr = err

} else {

resolver := &transport.PacketDialerEndpoint{Dialer: packetDialer, Address: resolverAddress}

testDuration, testErr = connectivity.TestResolverPacketConnectivity(context.Background(), resolver, resolverAddress)

}

The GUI app is vastly more complex. It needs to communicate across language boundaries. I don't think it makes sense to make the CLI app more complex to match the GUI. The GUI pattern is not what someone would follow in Go code.

I'm not updating the GUI app to use the new code because it has its own go.mod file, with separate versioning dependencies, so it can be updated separately. (it should also be renamed).

The goal here is to unblock your work as soon as possible, without waiting for the DNS library.

fortuna

Thanks for the review!

fortuna · 2023-12-28T15:22:43Z

x/examples/outline-cli/outline_packet_proxy.go

This is a test for the Packet connectivity. The Stream connectivity is covered in the tests above.

fortuna · 2023-12-28T15:26:21Z

x/examples/test-connectivity/main.go

 			switch proto {
 			case "tcp":
 				streamDialer, err := config.NewStreamDialer(*transportFlag)
 				if err != nil {
 					log.Fatalf("Failed to create StreamDialer: %v", err)
 				}
-				resolver := &transport.StreamDialerEndpoint{Dialer: streamDialer, Address: resolverAddress}
-				testDuration, testErr = connectivity.TestResolverStreamConnectivity(context.Background(), resolver, *domainFlag)
+				resolver = connectivity.NewTCPResolver(streamDialer, resolverAddress)
 			case "udp":
 				packetDialer, err := config.NewPacketDialer(*transportFlag)
 				if err != nil {
 					log.Fatalf("Failed to create PacketDialer: %v", err)


The GUI app is vastly more complex. It needs to communicate across language boundaries. I don't think it makes sense to make the CLI app more complex to match the GUI. The GUI pattern is not what someone would follow in Go code.

I'm not updating the GUI app to use the new code because it has its own go.mod file, with separate versioning dependencies, so it can be updated separately. (it should also be renamed).

The goal here is to unblock your work as soon as possible, without waiting for the DNS library.

fortuna requested a review from amircybersec December 26, 2023 21:05

fortuna marked this pull request as draft December 26, 2023 21:05

fortuna removed the request for review from amircybersec December 26, 2023 21:05

fortuna added 2 commits December 26, 2023 17:03

Clean up

bdcca46

Fix outline-cli

5361291

fortuna force-pushed the fortuna-connectivity branch from 3156f70 to 5361291 Compare December 26, 2023 22:04

fortuna changed the title ~~Rename outline-connectivity~~ refactor: Clean up outline-connectivity Dec 26, 2023

fortuna requested a review from amircybersec December 26, 2023 22:07

fortuna marked this pull request as ready for review December 26, 2023 22:07

Add transport comment.

f4d461d

fortuna mentioned this pull request Dec 26, 2023

chore: update examples to use new dns package #145

Merged

Cleaner deadline

476ecfe

fortuna changed the title ~~refactor: Clean up outline-connectivity~~ refactor: clean up outline-connectivity Dec 27, 2023

amircybersec approved these changes Dec 28, 2023

View reviewed changes

fortuna commented Dec 28, 2023

View reviewed changes

fortuna merged commit e0ac1f0 into main Dec 28, 2023
6 checks passed

fortuna deleted the fortuna-connectivity branch December 28, 2023 15:27

fortuna added a commit that referenced this pull request Jan 9, 2024

refactor: clean up outline-connectivity (#146)

41a1b20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: clean up outline-connectivity #146

refactor: clean up outline-connectivity #146

fortuna commented Dec 26, 2023 •

edited

Loading

amircybersec left a comment

amircybersec Dec 28, 2023

fortuna Dec 28, 2023

amircybersec Dec 28, 2023

fortuna Dec 28, 2023

amircybersec Dec 28, 2023

fortuna Dec 28, 2023

fortuna left a comment

fortuna Dec 28, 2023

fortuna Dec 28, 2023

-					log.Fatalf("Failed to create PacketDialer: %v", err)
+			if err != nil {
+				testDuration = time.Duration(0)
+				testErr = err
+			} else {
+				resolver := &transport.PacketDialerEndpoint{Dialer: packetDialer, Address: resolverAddress}
+				testDuration, testErr = connectivity.TestResolverPacketConnectivity(context.Background(), resolver, resolverAddress)
+			}

refactor: clean up outline-connectivity #146

refactor: clean up outline-connectivity #146

Conversation

fortuna commented Dec 26, 2023 • edited Loading

amircybersec 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

fortuna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fortuna commented Dec 26, 2023 •

edited

Loading