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

feat: Add ability to echo request headers in trailing metadata #1501

Merged
merged 12 commits into from Apr 15, 2024

Conversation

parthea
Copy link
Contributor

@parthea parthea commented Apr 15, 2024

This includes fixes from #1500. Please review #1500 first Done

Fixes #1493

Using this showcase server, I was able to see the http request headers in the trailing metadata

Response

responses.trailing_metadata()[0]
_Metadatum(key='x-goog-api-version', value='v1_20240408')

@parthea parthea marked this pull request as ready for review April 15, 2024 16:20
@parthea parthea requested review from a team as code owners April 15, 2024 16:20
server/services/echo_service.go Outdated Show resolved Hide resolved
schema/google/showcase/v1beta1/echo.proto Outdated Show resolved Hide resolved
schema/google/showcase/v1beta1/echo.proto Outdated Show resolved Hide resolved
@parthea parthea changed the title feat: Add ability to echo request headers in response feat: Add ability to echo request headers in trailing metadata Apr 15, 2024
@parthea
Copy link
Contributor Author

parthea commented Apr 15, 2024

This works for gRPC. I haven't figured out how to get the information for REST

@parthea parthea requested a review from noahdietz April 15, 2024 18:35
@parthea
Copy link
Contributor Author

parthea commented Apr 15, 2024

@noahdietz Please could you take another look? Should we also support this for REST? Can that be added in a separate PR?

@noahdietz
Copy link
Collaborator

Should we also support this for REST?

Yes, definitely.

Can that be added in a separate PR?

Yes, definitely.

We may want the generated REST handlers, like this:

response, err := backend.EchoServer.Echo(ctx, request)
if err != nil {
backend.ReportGRPCError(w, err)
return
}
json, err := marshaler.Marshal(response)
if err != nil {
backend.Error(w, http.StatusInternalServerError, "error json-encoding response: %s", err.Error())
return
}
w.Write(json)

...to pull the same metadata from the context (as the gRPC handlers do) and write them as HTTP headers. I'd probably add a helper here for copying context metadata into HTTP response headers, then invoke that in the spot above (generator code here).

@vchudnov-g may be interested in this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please revert all of the changes in the server/genproto directory, the headers will just be replaced when things are regenerated by CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@parthea parthea requested a review from noahdietz April 15, 2024 19:59
@parthea parthea enabled auto-merge (squash) April 15, 2024 20:37
@parthea parthea merged commit 48f9b74 into main Apr 15, 2024
12 checks passed
@parthea parthea deleted the echo-request-headers branch April 15, 2024 20:40
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.

Add support to echo back the x-goog-api-version header so that AIP-4236 can be tested
3 participants