Add config for the header use by the general ingress #638

nanassito · 2024-01-05T21:37:20Z

💸 TL;DR

We now have a generic ingress for thrift traffic that routes the traffic based on a header. This means that looking at a service config we don't see what service the traffic is going to and that the application need to set the header manually unlike every other configuration bit.

So this PR adds a new configuration to set this header.

📜 Details

Design Doc

Jira

🧪 Testing Steps / Validation

✅ Checks

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

fishy · 2024-01-05T21:43:11Z

thriftbp/client_pool.go

@@ -429,6 +434,19 @@ func NewCustomClientPoolWithContext(
 	return newClientPool(ctx, cfg, genAddr, protoFactory, middlewares...)
 }

+const ThriftHostnameHeader = "thrift-hostname"


is this intentionally lower-case?

for THeaders we use Camel-Case (see https://github.com/reddit/baseplate.go/blob/master/transport/headers.go), it will be better to keep it consistent.

that's the value the ingress expects sadly.

@davinci26 can comment on whether this can be changed or not

So according to the HTTP RFC header casing doesnt matter. @fishy does the same apply for thrift?

Each header field consists of a case-insensitive field name followed
by a colon (":"), optional leading whitespace, the field value, and
optional trailing whitespace.

In any case envoy will lower case the headers when parsing the thrift payload so it doesn't care but it would be nice if we took a dependency on a thrift spec definition rather than an envoy implementation detail.

https://github.com/envoyproxy/envoy/blob/12210e53db6cea79b58ee027f047faa356e1e392/source/extensions/filters/network/thrift_proxy/header_transport_impl.cc#L163

well this is not http header, it's thrift header.

envoy lower case all headers was a bug and fixed in latest version of envoy: envoyproxy/envoy#20595, envoyproxy/envoy@53867ab

well this is not http header, it's thrift header.

Now I am reading the issue

then in that case I think we should maintain the original casing so we follow the thrift protocol rather than the envoy implementation detail

even if this breaks the code pattern here. This is a header defined externally to baseplate.go so it should be fine to have different casing

fishy · 2024-01-05T21:43:41Z

thriftbp/client_pool.go

@@ -505,6 +523,11 @@ func newClientPool(

 		slug: cfg.ServiceSlug,
 	}
+
+	if cfg.ThriftHostnameHeader != "" {


instead of conditionally add the middleware, it might be better to always add the middleware but the middleware itself conditionally add the header (don't add it if it's empty string)

davinci26

overall looks pretty sane to me. Is there a way we could add tests?

kylelemons · 2024-01-05T22:04:07Z

thriftbp/client_pool.go

+	// The hostname to add as a "thrift-hostname" header.
+	//
+	// Optional. If empty, not "thrift-hostname" header will be sent.
+	ThriftHostnameHeader string `yaml:"thriftHostnameHeader"`


Suggested change

// The hostname to add as a "thrift-hostname" header.

//

// Optional. If empty, not "thrift-hostname" header will be sent.

ThriftHostnameHeader string `yaml:"thriftHostnameHeader"`

// The hostname to add as a "Thrift-Hostname" header.

//

// Optional. If empty, no "Thrift-Hostname" header will be sent.

ThriftHostnameHeader string `yaml:"thriftHostnameHeader"`

well from the rest of the convo it looks like we'll keep the lower case in the end

but we still should s/not/no/ :)

nanassito · 2024-01-05T23:23:23Z

Ideally I'd like to add tests but I'm not sure how. I haven't found a way to check the headers yet.

fishy

I still think if it's not too late we should change the header to Camel-Case, but that's not a blocking issue.

As for testing, the way to test headers is to:

start a server from a loopback port
create a client pool on the loopback port, with the header config
send a request
you can check the header on the server endpoint handler via thrift.GetHeader(ctx)

thriftbp/client_pool.go

fishy · 2024-01-06T01:06:07Z

thriftbp/client_pool.go

+	// The hostname to add as a "thrift-hostname" header.
+	//
+	// Optional. If empty, not "thrift-hostname" header will be sent.
+	ThriftHostnameHeader string `yaml:"thriftHostnameHeader"`


but we still should s/not/no/ :)

fishy · 2024-01-08T18:42:59Z

thriftbp/client_pool_test.go

+func (thriftHostnameHandler) IsHealthy(ctx context.Context, _ *baseplatethrift.IsHealthyRequest) (r bool, err error) {
+	value, ok := thrift.GetHeader(ctx, thriftbp.ThriftHostnameHeader)
+	if !ok {
+		return false, errors.New("did not find the thrift header")


I would slightly prefer to add tb testing.TB into thriftHostnameHandler struct and use tb.Error() instead, but this also works.

Add config for the header use by the general ingress

fd3cb96

nanassito requested review from kylelemons and davinci26 January 5, 2024 21:37

fishy reviewed Jan 5, 2024

View reviewed changes

always add middleware but don't always add the header

6d1a763

davinci26 reviewed Jan 5, 2024

View reviewed changes

capitalize the header

5079a2c

kylelemons approved these changes Jan 5, 2024

View reviewed changes

use lower case

8d97cf0

nanassito marked this pull request as ready for review January 5, 2024 23:22

nanassito requested a review from a team as a code owner January 5, 2024 23:22

nanassito requested review from fishy and pacejackson and removed request for a team January 5, 2024 23:22

fishy reviewed Jan 6, 2024

View reviewed changes

Dorian Jaminais-Grellier added 2 commits January 8, 2024 08:58

address some of the feedback

1cb2425

add unit test

091d9c7

nanassito requested a review from fishy January 8, 2024 17:44

pacejackson approved these changes Jan 8, 2024

View reviewed changes

fishy approved these changes Jan 8, 2024

View reviewed changes

fishy reviewed Jan 8, 2024

View reviewed changes

nanassito merged commit f99bf0b into reddit:master Jan 8, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add config for the header use by the general ingress #638

Add config for the header use by the general ingress #638

nanassito commented Jan 5, 2024 •

edited

Loading

fishy Jan 5, 2024

nanassito Jan 5, 2024

davinci26 Jan 5, 2024

fishy Jan 5, 2024 •

edited

Loading

davinci26 Jan 5, 2024 •

edited

Loading

davinci26 Jan 5, 2024

davinci26 Jan 5, 2024

fishy Jan 5, 2024

nanassito Jan 5, 2024

davinci26 left a comment

kylelemons Jan 5, 2024

nanassito Jan 5, 2024

fishy Jan 6, 2024

nanassito commented Jan 5, 2024

fishy left a comment

fishy Jan 6, 2024

fishy Jan 8, 2024

Add config for the header use by the general ingress #638

Add config for the header use by the general ingress #638

Conversation

nanassito commented Jan 5, 2024 • edited Loading

💸 TL;DR

📜 Details

🧪 Testing Steps / Validation

✅ Checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fishy Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

davinci26 Jan 5, 2024 • 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

davinci26 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

nanassito commented Jan 5, 2024

fishy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nanassito commented Jan 5, 2024 •

edited

Loading

fishy Jan 5, 2024 •

edited

Loading

davinci26 Jan 5, 2024 •

edited

Loading