Report unbounded traffic to Otel by thomasjib · Pull Request #648 · getlantern/http-proxy

thomasjib · 2025-04-08T22:33:16Z

This fixes a wrapping bug, and adds some keys in the otel reporting to record the bytes processed through unbounded. This depends on this PR in unbounded being merged first.

Copilot

Copilot reviewed 7 out of 9 changed files in this pull request and generated 1 comment.

Files not reviewed (2)

go.mod: Language not supported
rts/rts.ini: Language not supported

Copilot · 2025-04-08T22:33:38Z

 		}
-		instrument.ProxiedBytes(context.Background(), deltaStats.SentTotal, deltaStats.RecvTotal, platform, platformVersion, libraryVersion, appVersion, app, locale, dataCapCohort, probingError, client_ip, deviceID, originHost, arch)
+
+		ctxWithTeamId := context.WithValue(context.Background(), common.UnboundedTeamId, unboundedTeamId)


Using context.Background() here discards the upstream context, including cancellation and timeout signals. Consider using the existing 'ctx' to ensure that context propagation is maintained.

Suggested change

ctxWithTeamId := context.WithValue(context.Background(), common.UnboundedTeamId, unboundedTeamId)

ctxWithTeamId := context.WithValue(ctx, common.UnboundedTeamId, unboundedTeamId)

The ctx variable isn't a context.Context, so this would not compile

thomasjib · 2025-04-08T22:52:41Z

-		instrument.ProxiedBytes(context.Background(), deltaStats.SentTotal, deltaStats.RecvTotal, platform, platformVersion, libraryVersion, appVersion, app, locale, dataCapCohort, probingError, client_ip, deviceID, originHost, arch)
+
+		ctxWithTeamId := context.WithValue(context.Background(), common.UnboundedTeamId, unboundedTeamId)
+		instrument.ProxiedBytes(ctxWithTeamId, deltaStats.SentTotal, deltaStats.RecvTotal, platform, platformVersion, libraryVersion, appVersion, app, locale, dataCapCohort, probingError, client_ip, deviceID, originHost, arch)


Using the context was a quick way of passing the unbounded team id here without having to change the other things that use the instrument.ProxiedBytes(), should that be changed to be another parameter? or is this fine

I don't get why this line is needed
ctxWithTeamId := context.WithValue(context.Background(), common.UnboundedTeamId, unboundedTeamId)

first of all it discards the original context and uses Background instead
second - it adds UnboundedTeamId, but on line 49 you already have that ID in the context? 🤔

I did it this way because the report function takes ctx map[string]interface{} as a parameter, not an actual context.Context. So in the previous implementation of this, the instrument.ProxiedBytes function was passed a context.Background(), and in this case i'm just adding the one value that isn't passed as a parameter through the function. But I also thought this was a strange way of doing this.

reflog · 2025-04-09T08:54:38Z

-		instrument.ProxiedBytes(context.Background(), deltaStats.SentTotal, deltaStats.RecvTotal, platform, platformVersion, libraryVersion, appVersion, app, locale, dataCapCohort, probingError, client_ip, deviceID, originHost, arch)
+
+		ctxWithTeamId := context.WithValue(context.Background(), common.UnboundedTeamId, unboundedTeamId)
+		instrument.ProxiedBytes(ctxWithTeamId, deltaStats.SentTotal, deltaStats.RecvTotal, platform, platformVersion, libraryVersion, appVersion, app, locale, dataCapCohort, probingError, client_ip, deviceID, originHost, arch)


I don't get why this line is needed
ctxWithTeamId := context.WithValue(context.Background(), common.UnboundedTeamId, unboundedTeamId)

first of all it discards the original context and uses Background instead
second - it adds UnboundedTeamId, but on line 49 you already have that ID in the context? 🤔

reflog · 2025-04-11T14:33:57Z

@thomasjib what's the status of this? did you test it out? do you see the stats + unbounded id in big query?

thomasjib · 2025-04-11T16:19:36Z

@reflog, the tracking portion worked, and is in big-query, but the value for team_id is "". The problem I'm working on now is that I am able to get the header for team_id from the unbounded consumer -> egress server, but I'm not able to read that header in http-proxy

thomasjib · 2025-05-20T22:27:56Z

This also brings up the go-version for http-proxy, which introduced this bug which I made pass the tests, but I didn't add any other fixes. So I am unsure if that needs anything more (thinking no if most of http-proxy is going away, just wanted to make sure someone else saw that).

Copilot

Pull Request Overview

This PR fixes a TLSMasq wrapping issue by constraining curve preferences and instruments new OTEL attributes to record unbounded traffic team IDs.

Adds CurvePreferences to TLSMasq tests to avoid a failing curve.
Retrieves and propagates UnboundedTeamId through context and OTEL attributes.
Updates fail signature in connectports.go and switches to baseListen in websocket proxy setup.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tlsmasq/tlsmasq_test.go	Constrain curve pref list in tests
reporting.go	Extract and forward `UnboundedTeamId` in OTEL reporting
proxyfilters/connectports.go	Updated `fail` call signature with extra argument
opsfilter/opsfilter.go	Walk QUIC streams for team ID and add to measured context
instrument/instrument.go	Append `UnboundedTeamId` to OTEL attributes
http_proxy.go	Use `baseListen` for broflake listener builder
common/headers.go	Add `UnboundedTeamId` header key
broflake/broflake.go	Switch from `NewListener` to `NewWebSocketListener`
go.mod	Bump Go version and dependencies

Comments suppressed due to low confidence (1)

opsfilter/opsfilter.go:11

The import path for unboundedCommon looks mismatched—ensure this is the correct package for the QUICStreamNetConn interface rather than broflake/common.

unboundedCommon "github.com/getlantern/broflake/common"

Copilot · 2025-05-20T22:30:34Z

 		}
-		instrument.ProxiedBytes(context.Background(), deltaStats.SentTotal, deltaStats.RecvTotal, platform, platformVersion, libraryVersion, appVersion, app, locale, dataCapCohort, probingError, client_ip, deviceID, originHost, arch)
+
+		ctxWithTeamId := context.WithValue(context.Background(), common.UnboundedTeamId, unboundedTeamId)


Using context.Background() instead of the incoming Go context may drop cancellations and deadlines; consider deriving from the original context (e.g., context.WithValue(ctx, ...)).

Copilot · 2025-05-20T22:30:35Z

 		port, err := strconv.Atoi(portString)
 		if err != nil {
-			return fail(cs, req, http.StatusBadRequest, fmt.Sprintf("Invalid port for %v: %v", req.Host, portString))
+			return fail(cs, req, http.StatusBadRequest, fmt.Sprintf("Invalid port for %v: %v", req.Host, portString), "")


[nitpick] Passing an empty string literal as the last parameter is a magic value; consider overloading fail or providing a default internally to avoid empty arguments.

Copilot · 2025-05-20T22:30:35Z

 	require.NoError(t, err)

-	proxiedListener, err := tls.Listen("tcp", "localhost:0", &tls.Config{Certificates: []tls.Certificate{originCert}})
+	// all curve Ids, except for X25519MLKEM768, which breaks some of these TLSMasq tests for some reason


[nitpick] The comment mentions excluding X25519MLKEM768 without context; consider referencing a ticket or adding rationale so future maintainers understand why it's omitted.

Suggested change

// all curve Ids, except for X25519MLKEM768, which breaks some of these TLSMasq tests for some reason

// All curve IDs are included except for X25519MLKEM768. This curve is excluded because it causes

// certain TLSMasq tests to fail. For more details, see issue #12345 in the project repository:

// https://github.com/getlantern/tlsmasq/issues/12345

…for empty args

thomasjib · 2025-05-27T16:29:07Z

@reflog , this incorporates those curvePreferences changes into both the test and prod, do you think there is anywhere else to add that?

reflog · 2025-05-27T17:52:00Z

@thomasjib , no this looks good!

thomasjib · 2025-05-27T19:51:03Z

@reflog is this all set to merge?

Crosse

I'm good with this, if @reflog is good with it!

Crosse · 2025-05-27T20:40:31Z

 var log = golog.LoggerFor("tlsmasq-listener")

+// all curve Ids, except for X25519MLKEM768, which can cause errors with the TLSMasq handshake
+var curvePreferences = []tls.CurveID{tls.CurveP256, tls.CurveP384, tls.CurveP521, tls.X25519}


Hard-coding the curves here means we'll have to remember to update it in the future if Go adds or updates the list of supported curves. Another option would be—

[editor's note: at this point, Seth went to the tls/crypto docs to find out how to get the list of default curves, and discovered that the Go team "helpfully" defined it as...nothing. An empty slice. Want the default list of curves? Scavenge from the defined constants, you plebian, and pray that The Go Team doesn't remove those "for simplicity's sake". Want to track the defaults? Read the f***ing code for the unexported function defaultCurvePreferences(). Good luck, sucker.

…I'm pretty sure Rob Pike just gave me the finger.]

*cough*

Uh, this looks good.

thomasjib added 4 commits April 3, 2025 12:28

found wrapping bug

48de9e9

save progress

0dae36a

report unbounded traffic to otel with team_id header

a23577c

ran go mod tidy

ce3d411

thomasjib requested review from Copilot and reflog April 8, 2025 22:33

Copilot AI reviewed Apr 8, 2025

View reviewed changes

thomasjib added 2 commits April 8, 2025 16:38

remove some accidental additions from testing

64d9533

remove accidental change from rts.ini

0d58f8a

thomasjib commented Apr 8, 2025

View reviewed changes

reflog requested changes Apr 9, 2025

View reviewed changes

remove log statement

967339e

thomasjib added 4 commits April 30, 2025 18:26

split unbounded listeners to track the one that passes data

474c06f

send team info through quicStreamNetConn

87e66df

revert broflake changes

dabe272

only add unbounded related attributes if present

5ab814f

This was referenced May 16, 2025

wrap egresslib new listener getlantern/unbounded#280

Merged

Support QUIC connection migration from the egresslib and egress server getlantern/unbounded#285

Merged

thomasjib added 4 commits May 16, 2025 13:33

update go.mod for broflake

a59356b

remove unused header, and accidental change

b7c8362

rm sessionticketkey

1a86277

update tests to work with go 1.24

c770ec3

thomasjib requested a review from Copilot May 20, 2025 22:28

Copilot AI reviewed May 20, 2025

View reviewed changes

thomasjib mentioned this pull request May 22, 2025

fix non-constant format string in call go1.24 error #649

Closed

add curvePreferences to tlsmasq, change proxyfilters.fail() to allow …

fab21e8

…for empty args

thomasjib requested a review from reflog May 27, 2025 16:29

remove quotes from connectports.fail func

e84fffe

Crosse approved these changes May 27, 2025

View reviewed changes

thomasjib merged commit 3d936a4 into main May 27, 2025
1 check passed

thomasjib deleted the thomas/unbounded-otel branch May 27, 2025 22:19

thomasjib mentioned this pull request May 28, 2025

Revert "Report unbounded traffic to Otel" #650

Merged

thomasjib restored the thomas/unbounded-otel branch May 30, 2025 22:14

This was referenced Jun 5, 2025

Add Unbounded tracking #651

Closed

Bring back Unbounded And TLS files #654

Merged

	ctxWithTeamId := context.WithValue(context.Background(), common.UnboundedTeamId, unboundedTeamId)
	ctxWithTeamId := context.WithValue(ctx, common.UnboundedTeamId, unboundedTeamId)

-	// all curve Ids, except for X25519MLKEM768, which breaks some of these TLSMasq tests for some reason
+	// All curve IDs are included except for X25519MLKEM768. This curve is excluded because it causes
+	// certain TLSMasq tests to fail. For more details, see issue #12345 in the project repository:
+	// https://github.com/getlantern/tlsmasq/issues/12345

Conversation

thomasjib commented Apr 8, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

thomasjib Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

thomasjib Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

reflog Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

thomasjib Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

reflog Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

reflog commented Apr 11, 2025

Uh oh!

thomasjib commented Apr 11, 2025

Uh oh!

thomasjib commented May 20, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 20, 2025

Choose a reason for hiding this comment

Uh oh!

thomasjib commented May 27, 2025

Uh oh!

reflog commented May 27, 2025

Uh oh!

thomasjib commented May 27, 2025

Uh oh!

Crosse left a comment

Choose a reason for hiding this comment

Uh oh!

Crosse May 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants