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

Forward metadata in proxy #167

Merged
merged 3 commits into from
Jun 24, 2024
Merged

Forward metadata in proxy #167

merged 3 commits into from
Jun 24, 2024

Conversation

cretz
Copy link
Member

@cretz cretz commented Jun 4, 2024

What changed?

Today headers set by client do not flow through the proxy. This will now forward them by default.

@cretz cretz requested review from a team as code owners June 4, 2024 22:01

// Copy incoming header to outgoing if not already present in outgoing. We
// have confirmed in gRPC that incoming is a copy so we can mutate it.
incoming, _ := metadata.FromIncomingContext(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Error check?

Copy link
Member Author

@cretz cretz Jun 5, 2024

Choose a reason for hiding this comment

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

This isn't an error, this is whether it was present, and we don't care really in our case. incoming is a type wrapper of map[string][]string and .Delete is a wrapper around delete. So a nil incoming is safe.


// Remove common headers and if there's nothing left, return early
incoming.Delete("user-agent")
incoming.Delete(":authority")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this supposed to have a colon?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is an HTTP/2 header

@cretz
Copy link
Member Author

cretz commented Jun 5, 2024

This should not be merged until after a Go SDK release is made with temporalio/sdk-go#1502. We did a type conversion in the Go SDK on options set here so we cannot release an API library version with this updated or SDK users will get compile errors.

@Quinn-With-Two-Ns
Copy link
Contributor

@cretz temporalio/sdk-go#1502 is included in v1.27.0

@cretz
Copy link
Member Author

cretz commented Jun 24, 2024

Thanks! I will go ahead and merge this after CI passes.

@cretz cretz merged commit 180af4b into temporalio:master Jun 24, 2024
3 checks passed
@cretz cretz deleted the proxy-metadata branch June 24, 2024 19:41
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.

3 participants