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.

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

Query-Frontend: Fix response header handling #3363

Merged

Conversation

simonswine
Copy link
Contributor

@simonswine simonswine commented Jun 19, 2024

Fix frontend header handling

The query-frontend accidentally leaked content-type and content-encoding information from the underlying connectgrpc implementation, which is used to schedule queries on to the workers (queriers).

This will filter those out, so they won't be passed on through the frontend.

This could be observed by errors like this:

$ profilecli query series
level=info msg="query series from querier" url=http://localhost:8080 from=2024-06-18T15:46:19.932471+01:00 to=2024-06-18T16:46:19.932472+01:00 labelNames=[]
error: failed to query: unknown: invalid content-type: "application/grpc,application/proto"; expecting "application/grpc"
exit status 1

Which themselves were produced only because envoy (involved on our GC ingress), converted headers from this:

content-type: application/grpc
content-type: application/proto

to

content-type: application/grpc,application/proto

This then triggered a validation recently added to connect-go:

connectrpc/connect-go#679

@simonswine simonswine changed the title Fix frontend header handling Query-Frontend: Fix frontend header handling Jun 19, 2024
@simonswine simonswine changed the title Query-Frontend: Fix frontend header handling Query-Frontend: Fix response header handling Jun 19, 2024
The query-frontend accidentally leaked content-type and content-encoding
information from the underlying connectgrpc implementation, which is
used to schedule queries on to the workers (queriers).

This will filter those out, so they won't be passed on through the
frontend.

This could be observed by errors like this:

```
$ profilecli query series
level=info msg="query series from querier" url=http://localhost:8080 from=2024-06-18T15:46:19.932471+01:00 to=2024-06-18T16:46:19.932472+01:00 labelNames=[]
error: failed to query: unknown: invalid content-type: "application/grpc,application/proto"; expecting "application/grpc"
exit status 1
```

Which themselves were produced only because envoy (involved on our GC
ingress), converted headers from this:

```
content-type: application/grpc
content-type: application/proto
```

to

```
content-type: application/grpc,application/proto
```

This then triggered a validation recently added to connect-go:

connectrpc/connect-go#679
@simonswine simonswine force-pushed the 20240619_fix-header-handling-frontend branch from 7b53821 to 48dfda2 Compare June 19, 2024 11:11
@simonswine simonswine marked this pull request as ready for review June 19, 2024 11:23
@simonswine simonswine requested a review from a team as a code owner June 19, 2024 11:23
Copy link
Contributor

@aleks-p aleks-p left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for adding the test as well!

@simonswine simonswine merged commit e74f3b8 into grafana:main Jun 19, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants