Revamp integration test package #1298

soltzen · 2023-04-10T15:57:59Z

Related to https://github.com/getlantern/lantern-internal/issues/5972
Depends on getlantern/http-proxy#553

Reworks the existing (non-functional) integration testing package: it now runs a local Flashlight instance against a local http-proxy-lantern instance.

I've written a few tests already (for http, https and shadowsocks protocols). More cases to come.

Furthermore, I've made the logging for that package go to a file since Flashlight is notoriously verbose. I'm planning to make a TUI (text user interface) and have the tool be usable in CI as well, but this is a first stab only

This is a minor update, just to make the dependency point to main. No major changes.

hwh33

It seems like the two main outputs of this are:

Split out a new package of local-proxy implementations. This is now called proxyimpl.
Define a command for running integration tests using a local version of http-proxy-lantern for the remote proxy.

On point 1, I think the new interface boundary needs a bit of work. I left some comments to this effect.

On point 2, I think this is a positive change, but it adds a lot of overhead in terms of code and project complexity.

What if you wrote the tests as actual Go tests? So the integrationtest package would define facilities for setting up a local test proxy, config pairs, etc. Then you could define TestHTTP, TestHTTPS, TestShadowsocks, etc. You'd avoid all of the overhead and complexity of defining your own test runner. Most importantly, the user wouldn't need to learn anything new when running tests or creating new ones. Additionally, the tests would run as part of go test ./... from the top-level directory in flashlight, without the need for special handling of integrationtest and parsing of its results file.

I know you've put a lot of work into this. However, I'd like to see this land in the simplest and most usable form. I think defining our own test runner is unnecessary when we have a robust one available to us in the Go standard library.

To be clear, I think both (1) and (2) are hugely beneficial! Thanks for taking on this effort.

hwh33 · 2023-04-22T22:02:06Z

balancer/balancer.go

@@ -129,6 +130,8 @@ type Dialer interface {
 	Stop()

 	WriteStats(w io.Writer)
+
+	Implementation() proxyimpl.ProxyImpl


This feels like an implementation detail which should not be exposed via the public API. I also don't see this used anywhere?

hwh33 · 2023-04-22T22:22:42Z

chained/doc.go

+// chained package handles dialing to Lantern proxies. It contain code for
+// representing proxies, sending them CONNECT requests, wrapping different
+// net.Conn implementationsm etc.
+//
+// It mainly uses two packages for doing this:
+//   - balancer: for load balancing proxies
+//   - proxyimpl: for handling different protocol transports (e.g., QUIC,
+//     http/https, lampshade, obfs4, etc.)
+package chained


This comment should begin with "Package chained..."

There are a few grammatical errors and typos here.

I think the objects constructed by this package are proxies, as opposed to representing proxies. They are local proxies and they communicate through remote proxies. The original package doc is more clear on this point IMO.

Do we need to expose the dependency packages in the package doc? Those should be implementation details, right?

"It contains code for... sending [proxies] CONNECT requests." Again, this feels like an implementation detail, at least at this level. To be fair, I think the original package doc has the same issue.

It appears that the original package doc still exists in this branch. A package should only have one doc comment.

hwh33 · 2023-04-22T22:31:28Z

chained/proxy.go

-			dialer, err := CreateMPDialer(configDir, endpoint, group, uc)
+
+			// Make a new proxy for the multipath endpoint using the info from
+			// the **first server** in the group. Those attributes should be shared
+			// by all paths
+			var err error
+			var p *proxy
+			for _, s := range proxies {
+				p, err = newProxy(endpoint, endpoint+":0", "multipath", "multipath", s, uc)
+				if err != nil {
+					log.Errorf("Unable to configure multipath server to %v. Received error: %v", endpoint, err)
+					continue
+				}
+				// Break after the first one
+				break
+			}
+
+			// Make and assign a new MP dialer
+			p.impl, err = proxyimpl.NewMultipathProxyImpl(
+				configDir, endpoint,
+				group, uc,
+				extractParams, p.reportDialCore)


This seems totally unrelated to testing. Did this change sneak in from the multipath prefix testing stuff?

IMO it was better when this logic was in a helper function. This is a lot of logic which is not directly related to the task of "creating the dialers map". This adds to the mental overhead for the reader.

It's not clear to me that you've accurately reproduced the logic from the original helper function. It's pretty different from what you've got here. Granted, I didn't analyze it too hard because of point 1.

hwh33 · 2023-04-22T22:36:40Z

common/nopcloser.go

@@ -0,0 +1,5 @@
+package common
+
+type NopCloser struct{}


Every reference to this type is in proxyimpl. Could you move this into that package? The Close functions will still be exposed via the proxyimpl.ProxyImpl interface.

hwh33 · 2023-04-22T23:42:50Z

integrationtest/go.mod

@@ -0,0 +1,253 @@
+module github.com/getlantern/flashlight-integration-test


As a sub-module / sub-command of flashlight, this shouldn't need its own go.mod. Was there a specific reason you did this?

hwh33 · 2023-04-23T00:18:30Z

proxyimpl/tls_config.go

+	createKeyLogWriterOnce          sync.Once
+	InsecureSkipVerifyTLSMasqOrigin = false
+)
+


tlsConfigForProxy should be moved back into this file, not moved to https.go. That function is not specific to HTTPS proxies.

hwh33 · 2023-04-23T00:25:53Z

proxyimpl/multipath.go

 }

-func groupByMultipathEndpoint(proxies map[string]*config.ProxyConfig) map[string]map[string]*config.ProxyConfig {
+func GroupByMultipathEndpoint(proxies map[string]*config.ProxyConfig) map[string]map[string]*config.ProxyConfig {


It looks like this function should remain in the chained package - it serves no utility in proxyimpl.

hwh33 · 2023-04-23T00:32:21Z

proxyimpl/multipath.go

+	configDir, endpoint string,
+	ss map[string]*config.ProxyConfig,
+	uc common.UserConfig,
+	extractProxyConfigParamsFunc func(*config.ProxyConfig) (addr string, transport string, network string, err error),


This callback creates a messy cycle between chained and proxyimpl. It looks like this whole thing (NewMultipathProxyImpl) should just move to the chained package.

hwh33 · 2023-04-23T00:42:34Z

proxyimpl/shadowsocks.go

-			prefixFunc := func() ([]byte, error) { return gen(), nil }
-			cl.SetTCPSaltGenerator(client.NewPrefixSaltGenerator(prefixFunc))
+			cl.SetTCPSaltGenerator(
+				client.NewPrefixSaltGenerator(
+					func() ([]byte, error) {
+						return gen(), nil
+					},
+				))


Why did this change?

hwh33 · 2023-04-23T00:48:48Z

proxyimpl/proxyimpl.go

+	configDir, name, addr, transport string,
+	s *config.ProxyConfig,
+	uc common.UserConfig,
+	reportDialCore ReportDialCoreFn,


This callback creates a messy loop between chained and proxyimpl. proxyimpl wants to know how to dial, but chained needs to report the op and needs to then ask proxyimpl how to actually dial, hence the dialCore callback, nested within a callback.

To be fair, this nested callback thing existed before this PR. But splitting the packages out and retaining a loop of callbacks between the packages makes the problem much worse. Essentially, the (new) interface boundary needs work. I'm not exactly sure what the solution is.

In case that's confusing, the call-stack would be something like:

chained.CreateDialer -> calls proxyimpl.ProxyImpl.DialServer -> calls reportDialCoreFn callback, defined in chained -> calls reportDialCore callback callback, defined in proxyimpl

That's the loop I'm talking about. It's bouncing between the two packages for two full cycles. This creates a maintenance nightmare for the next engineer who wants to modify behavior anywhere in this stack: the two packages are interdependent.

I guess the dial doesn't actually happen until balancer.Dialer is used - not in chained.CreateDialer. But the point stands IMO.

soltzen added 5 commits April 10, 2023 17:50

[chained|proxyimpl] Move protocol implementations to proxyimpl

0296f67

[balancer] Add Implementation() to extract a ProxyImpl from a Dialer

bca53ac

[integrationtest] Revamp integrationtest package

dc80816

[docs:README] Add testing instructions

9d4bc47

[go.mod] Update shadowsocks and mod tidy

872b898

This is a minor update, just to make the dependency point to main. No major changes.

hwh33 reviewed Apr 23, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revamp integration test package #1298

Revamp integration test package #1298

soltzen commented Apr 10, 2023

hwh33 left a comment

hwh33 Apr 22, 2023

hwh33 Apr 22, 2023

hwh33 Apr 22, 2023

hwh33 Apr 22, 2023

hwh33 Apr 22, 2023

hwh33 Apr 23, 2023

hwh33 Apr 23, 2023

hwh33 Apr 23, 2023

hwh33 Apr 23, 2023

hwh33 Apr 23, 2023

hwh33 Apr 23, 2023

hwh33 Apr 23, 2023

		@@ -0,0 +1,253 @@
		module github.com/getlantern/flashlight-integration-test

Revamp integration test package #1298

Are you sure you want to change the base?

Revamp integration test package #1298

Conversation

soltzen commented Apr 10, 2023

hwh33 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

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