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

Use canonical header keys to extract needed header attributes #177

Closed
wants to merge 1 commit into from

Conversation

ohad83
Copy link

@ohad83 ohad83 commented Jul 3, 2024

When extracting headers from http.Header, it's better to use Values instead of accessing the map directly, since it canonicalizes the key before checking it. Many proxies and interceptors canonicalize header keys before forwarding them, so headers could have been omitted without this fix.

Signed-off-by: Ohad Abarbanel <ohada@tannin.io>
@ohad83 ohad83 changed the title fix: Use canonical header keys to extract needed header attributes Use canonical header keys to extract needed header attributes Jul 3, 2024
@jhump
Copy link
Member

jhump commented Jul 3, 2024

@ohad83, are you encountering an actual issue? If so, could you elaborate?

Use of this method should not be necessary because we canonicalize all of the keys first, specifically so we can do direct map queries (to reduce performance overhead):
https://github.com/connectrpc/otelconnect-go/blob/v0.7.0/option.go#L194
https://github.com/connectrpc/otelconnect-go/blob/v0.7.0/option.go#L204

@ohad83
Copy link
Author

ohad83 commented Jul 3, 2024

Hmm, I see that now. I was having a problem with differences between environments where proxies change headers, so I figured that would be the case. I'll try digging into it some more to make sure canonicalization isn't the problem.
Thanks for the quick response!

@ohad83 ohad83 closed this Aug 5, 2024
@ohad83 ohad83 deleted the fix/canonical-headers branch August 5, 2024 14:47
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