Add headerbp integration #669

pacejackson · 2025-01-09T19:58:32Z

💸 TL;DR

Add integrations to automatically propagate baseplate headers received from other services and forbids sending new baseplate headers in client calls.

✅ Checks

CI tests (if present) are passing
Adheres to code style for repo
Contributor License Agreement (CLA) completed if not a Reddit employee

this will be used to write middleware to automatically propagate baseplate headers received by a v0 service, but not allow sending new ones

pacejackson · 2025-01-10T03:15:27Z

httpbp/client_middlewares.go

+
+	// only add the middleware to forward baseplate headers if the client is for internal calls
+	if config.InternalOnly {
+		defaults = append(defaults, ClientHeaderBPMiddleware(config.Slug))
+	}


Since we don't have a distinct "internal vs non-internal" client type, I was thinking it would probably be good to have some sort of flag that you have to manually set to have HTTP clients automatically propagate headers?

fishy

the test passed for go 1.22 but failed for go 1.23, I think that just means the test is flaky? I don't think you used anything that will cause a difference between go 1.22 and 1.23.

fishy · 2025-01-10T21:09:51Z

internal/headerbp/headerbp.go

+	if len(key) < len(headerPrefixLower) {
+		return false
+	}
+	prefix := key[:len(headerPrefixLower)]
+	return prefix == headerPrefixLower || prefix == headerPrefixCanonicalHTTP


nit: just use:

Suggested change

if len(key) < len(headerPrefixLower) {

return false

}

prefix := key[:len(headerPrefixLower)]

return prefix == headerPrefixLower || prefix == headerPrefixCanonicalHTTP

return strings.HasPrefix(key, headerPrefixLower) || strings.HasPrefix(key, headerPrefixCanonicalHTTP)

?

this way you no longer rely on headerPrefixCanonicalHTTP and headerPrefixLower being the same length.

the strings.HasPrefix approach is significantly slower (relatively speaking)

func BenchmarkIsBaseplateHeader(b *testing.B) { // the question of using string.HasPrefix has come up a couple of times, benchmark the two approaches to see if there // is a significant enough differenc between the two. isBaseplateHeader_startswith := func(key string) bool { return strings.HasPrefix(key, HeaderPrefixLower) || strings.HasPrefix(key, HeaderPrefixCanonicalHTTP) } b.Run("startswith", func(b *testing.B) { for i := 0; i < b.N; i++ { isBaseplateHeader_startswith("X-Bp-Header") } }) b.Run("==", func(b *testing.B) { for i := 0; i < b.N; i++ { IsBaseplateHeader("X-Bp-Header") } }) }

BenchmarkIsBaseplateHeader
BenchmarkIsBaseplateHeader/startswith
BenchmarkIsBaseplateHeader/startswith-16 299018905 3.799 ns/op
BenchmarkIsBaseplateHeader/==
BenchmarkIsBaseplateHeader/==-16 1000000000 0.2518 ns/op
PASS

this way you no longer rely on headerPrefixCanonicalHTTP and headerPrefixLower being the same length.

They must always be the same since they are just the same string with different cases

fishy · 2025-01-10T21:14:32Z

internal/headerbp/metrics.go

+
+var (
+	clientHeadersRejectedTotal = promauto.With(prometheusbpint.GlobalRegistry).NewCounterVec(prometheus.CounterOpts{
+		Name: "baseplate_client_headers_rejected_total",


nit: _rejected_headers_total, the last word before total should be the unit in plural form ("headers")

fishy · 2025-01-10T21:14:50Z

internal/headerbp/metrics.go

+	})
+
+	clientHeadersSentTotal = promauto.With(prometheusbpint.GlobalRegistry).NewHistogramVec(prometheus.HistogramOpts{
+		Name:    "baseplate_client_headers_sent_total",


same comment here, _sent_headers_total.

fishy · 2025-01-10T21:15:13Z

internal/headerbp/metrics.go

+	})
+
+	clientHeadersSentSize = promauto.With(prometheusbpint.GlobalRegistry).NewHistogramVec(prometheus.HistogramOpts{
+		Name:    "baseplate_client_headers_sent_size_total",


_sent_size_bytes, _total is only for counters (this is a histogram), and there the unit is bytes.

fishy · 2025-01-10T21:15:26Z

internal/headerbp/metrics.go

+	})
+
+	serverHeadersReceivedTotal = promauto.With(prometheusbpint.GlobalRegistry).NewCounterVec(prometheus.CounterOpts{
+		Name: "baseplate_service_headers_received_total",


_received_headers_total.

fishy · 2025-01-10T21:15:36Z

internal/headerbp/metrics.go

+	})
+
+	serverHeadersReceivedSize = promauto.With(prometheusbpint.GlobalRegistry).NewHistogramVec(prometheus.HistogramOpts{
+		Name:    "baseplate_server_headers_received_size_total",


_received_size_bytes.

fishy · 2025-01-10T21:27:13Z

thriftbp/server_middlewares.go

+				} else if _, ok := thrift.GetHeader(ctx, headerbp.IsUntrustedRequestHeaderCanonicalHTTP); ok {
+					untrusted = true
+				}
+				if untrusted {


I wonder if we should add a x-bp- version of the deadline propagation header, so that we can still propagate deadline even in a untrusted chain of calls?

I wonder if we should add a x-bp- version of the deadline propagation header, so that we can still propagate deadline even in a untrusted chain of calls?

I'm not sure I understand, this specifically would discard that header if the request was marked as untrusted? 😅

fishy · 2025-01-10T21:27:59Z

thriftbp/server_middlewares.go

+				}
+
+				headers := headerbp.NewIncomingHeaders(
+					headerbp.WithThriftService("", name),


nit: for readability:

Suggested change

headerbp.WithThriftService("", name),

headerbp.WithThriftService("" /* service */, name),

(and I assume we use "" for service because we don't really have a notion of service name in thrift servers?)

…ware

pacejackson · 2025-01-17T22:14:39Z

the test passed for go 1.22 but failed for go 1.23, I think that just means the test is flaky? I don't think you used anything that will cause a difference between go 1.22 and 1.23.

It does seem like the test is flaky, but I think it's flaky because it can't call the test service I'm setting up. Is there something I'm missing there?

fishy · 2025-01-17T22:19:01Z

httpbp/server_test.go

+	t.Cleanup(func() {
+		originServer.Close()
+	})
+	go originServer.Serve()


you can probably add a small sleep after this go to give it time to actually start the server, not sure if it's worth it though 🤷 looks like the failed test is just very unlucky.

…time to come up

konradreiche · 2025-01-21T18:16:05Z

httpbp/client_middlewares.go

+// ClientHeaderBPMiddleware is a middleware that forwards baseplate headers from the context to the outgoing request.
+//
+// If it detects any new baseplate headers set on the request, it will reject the request and return an error.
+func ClientHeaderBPMiddleware(client string) ClientMiddleware {


Suggested change

func ClientHeaderBPMiddleware(client string) ClientMiddleware {

func ClientHeaderBaseplateMiddleware(client string) ClientMiddleware {

🔕 maybe best spelled out since we have Baseplate more often in function or method names than BP.

headerbp is the name of the actual library so I figured it was better to keep it consistent with that?

konradreiche · 2025-01-21T18:18:34Z

httpbp/client_middlewares.go

+			for k := range req.Header {
+				if err := headerbp.CheckClientHeader(k,
+					headerbp.WithHTTPClient("", client, ""),
+				); err != nil {
+					return nil, err
+				}


Suggested change

for k := range req.Header {

if err := headerbp.CheckClientHeader(k,

headerbp.WithHTTPClient("", client, ""),

); err != nil {

return nil, err

}

for key := range req.Header {

if err := headerbp.CheckClientHeader(

key,

headerbp.WithHTTPClient("", client, ""),

); err != nil {

return nil, err

}

🔕 I find this slightly more readable 🙂

konradreiche · 2025-01-21T18:20:33Z

httpbp/middlewares.go

+}
+
+func GetUntrustedBaseplateHeaders(ctx context.Context) (map[string]string, bool) {
+	h, ok := ctx.Value(untrustedHeadersKey{}).(map[string]string)


I'm a big fan of always checking if the assertion was successful, though I got the impression that if we tightly control it, it's okay to not check to make the API more user-friendly 🤔

I don't think that really does anything here though

konradreiche · 2025-01-21T18:25:43Z

internalv2compat/compat.go

+}
+
+func SetV2BaseplateHeadersLookup(setter func(context.Context, map[string]string) context.Context) {
+	headerbp.SetV2Context = setter


One thing I noticed with our internal2compat primarily sets on structs local to this package but then makes it accessible through a separate getter. Would it make sense to define this locally here too and then call V2Context() to get the function outside of this package?

I see, that makes sense, updated.

konradreiche

Only requesting changes for the open internal2compat question/discussion since it seems like it won't clear my review queue unless I approve or request changes 😅

konradreiche

pacejackson force-pushed the headerbp branch from 8c6d7c0 to 5b80cc1 Compare January 9, 2025 21:46

add internal headerbp library

1bc1961

this will be used to write middleware to automatically propagate baseplate headers received by a v0 service, but not allow sending new ones

pacejackson force-pushed the headerbp branch from 5b80cc1 to 954f000 Compare January 9, 2025 21:49

add headerbp middleware to httpbp

c52cdf1

pacejackson changed the title ~~Headerbp~~ Add headerbp integration Jan 10, 2025

pacejackson force-pushed the headerbp branch from 954f000 to f99c13f Compare January 10, 2025 03:10

add headerbp middleware to thriftbp

496bc9b

pacejackson force-pushed the headerbp branch from f99c13f to 496bc9b Compare January 10, 2025 03:11

pacejackson marked this pull request as ready for review January 10, 2025 03:12

pacejackson requested a review from a team as a code owner January 10, 2025 03:12

pacejackson requested review from fishy, kylelemons and konradreiche and removed request for a team January 10, 2025 03:12

pacejackson commented Jan 10, 2025

View reviewed changes

pacejackson added 2 commits January 9, 2025 21:24

Add helpers for V0 and V2 interoperability

75e5b96

do interop at set rather than lookup

7bdf051

fishy reviewed Jan 10, 2025

View reviewed changes

pr feedback and keep track of untrusted headers in http server middle…

6342673

…ware

pacejackson requested a review from fishy January 17, 2025 22:01

fishy approved these changes Jan 17, 2025

View reviewed changes

fishy reviewed Jan 17, 2025

View reviewed changes

add sleeps after starting test services to give them a little bit of …

a752b34

…time to come up

konradreiche reviewed Jan 21, 2025

View reviewed changes

konradreiche requested changes Jan 21, 2025

View reviewed changes

cr feedback

94c324c

pacejackson requested a review from konradreiche January 21, 2025 19:32

pacejackson added 2 commits January 21, 2025 13:34

fix linter error

49741ba

expose HeadersToContext for use by v2 interop

4200301

konradreiche approved these changes Jan 21, 2025

View reviewed changes

pacejackson merged commit 39862e6 into reddit:master Jan 21, 2025
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add headerbp integration #669

Add headerbp integration #669

pacejackson commented Jan 9, 2025 •

edited

Loading

pacejackson Jan 10, 2025

fishy left a comment

fishy Jan 10, 2025

pacejackson Jan 17, 2025 •

edited

Loading

pacejackson Jan 17, 2025

fishy Jan 10, 2025

fishy Jan 10, 2025

fishy Jan 10, 2025

fishy Jan 10, 2025

fishy Jan 10, 2025

fishy Jan 10, 2025

pacejackson Jan 15, 2025

fishy Jan 10, 2025

pacejackson commented Jan 17, 2025

fishy Jan 17, 2025

konradreiche Jan 21, 2025

pacejackson Jan 21, 2025

konradreiche Jan 21, 2025

konradreiche Jan 21, 2025

pacejackson Jan 21, 2025

konradreiche Jan 21, 2025

pacejackson Jan 21, 2025

konradreiche left a comment

konradreiche left a comment

	headerbp.WithThriftService("", name),
	headerbp.WithThriftService("" /* service */, name),

	func ClientHeaderBPMiddleware(client string) ClientMiddleware {
	func ClientHeaderBaseplateMiddleware(client string) ClientMiddleware {

Add headerbp integration #669

Add headerbp integration #669

Conversation

pacejackson commented Jan 9, 2025 • edited Loading

💸 TL;DR

✅ Checks

Choose a reason for hiding this comment

fishy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pacejackson Jan 17, 2025 • edited Loading

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

pacejackson commented Jan 17, 2025

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

konradreiche left a comment

Choose a reason for hiding this comment

konradreiche left a comment

Choose a reason for hiding this comment

pacejackson commented Jan 9, 2025 •

edited

Loading

pacejackson Jan 17, 2025 •

edited

Loading