Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Jan 8, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fd3cb96
Add config for the header use by the general ingress
Jan 5, 2024
6d1a763
always add middleware but don't always add the header
Jan 5, 2024
5079a2c
capitalize the header
Jan 5, 2024
8d97cf0
use lower case
Jan 5, 2024
1cb2425
address some of the feedback
Jan 8, 2024
091d9c7
add unit test
Jan 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions thriftbp/client_pool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,11 @@ type ClientPoolConfig struct {
//
// Optional. If this is empty, no "User-Agent" header will be sent.
ClientName string `yaml:"clientName"`

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

// Validate checks ClientPoolConfig for any missing or erroneous values.
Expand Down Expand Up @@ -429,6 +434,24 @@ func NewCustomClientPoolWithContext(
return newClientPool(ctx, cfg, genAddr, protoFactory, middlewares...)
}

const ThriftHostnameHeader = "thrift-hostname"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's the value the ingress expects sadly.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@fishy fishy Jan 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@davinci26 davinci26 Jan 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Now I am reading the issue

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// thriftHostnameHeaderMiddleware adds a `thrift-hostname` header if one was
// specified in the configuration.
// This middleware is always added but will only add the header is necessary.
func thriftHostnameHeaderMiddleware(hostname string) thrift.ClientMiddleware {
return func(next thrift.TClient) thrift.TClient {
return thrift.WrappedTClient{
Wrapped: func(ctx context.Context, method string, args, result thrift.TStruct) (thrift.ResponseMeta, error) {
if hostname != "" {
ctx = AddClientHeader(ctx, ThriftHostnameHeader, hostname)
}
return next.Call(ctx, method, args, result)
},
}
}
}

func newClientPool(
ctx context.Context,
cfg ClientPoolConfig,
Expand Down Expand Up @@ -505,6 +528,8 @@ func newClientPool(

slug: cfg.ServiceSlug,
}
middlewares = append(middlewares, thriftHostnameHeaderMiddleware(cfg.ThriftHostnameHeader))

// finish setting up the clientPool by wrapping the inner "Call" with the
// given middleware.
//
Expand Down
46 changes: 46 additions & 0 deletions thriftbp/client_pool_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,11 @@ import (

"github.com/apache/thrift/lib/go/thrift"

"github.com/reddit/baseplate.go"
"github.com/reddit/baseplate.go/ecinterface"
baseplatethrift "github.com/reddit/baseplate.go/internal/gen-go/reddit/baseplate"
"github.com/reddit/baseplate.go/thriftbp"
"github.com/reddit/baseplate.go/thriftbp/thrifttest"
)

const (
Expand Down Expand Up @@ -190,6 +193,49 @@ func TestBehaviorWithNetworkIssues(t *testing.T) {
}
}

type thriftHostnameHandler struct {
server baseplate.Server
}

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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
if value != "my-thrift-header" {
return false, errors.New("unexpected value for the thrift header")
}
return true, nil
}

func TestThriftHostnameHeader(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

store := newSecretsStore(t)
defer store.Close()

handler := thriftHostnameHandler{}
server, err := thrifttest.NewBaseplateServer(thrifttest.ServerConfig{
Processor: baseplatethrift.NewBaseplateServiceV2Processor(&handler),
SecretStore: store,
ClientConfig: thriftbp.ClientPoolConfig{
ThriftHostnameHeader: "my-thrift-header",
},
})
if err != nil {
t.Fatal(err)
}
handler.server = server
server.Start(ctx)

client := baseplatethrift.NewBaseplateServiceV2Client(server.ClientPool.TClient())
_, err = client.IsHealthy(ctx, &baseplatethrift.IsHealthyRequest{})
if err != nil {
t.Fatal(err)
}
}

func TestInitialConnectionsFallback(t *testing.T) {
ln, err := net.Listen("tcp", addr)
if err != nil {
Expand Down