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

pass headers to reflection client #351

Merged
merged 1 commit into from Jan 29, 2021

Conversation

dbarrosop
Copy link
Contributor

Fixes #347

So we just run into this problem. We have several services behind traefik and we distinguish between them by their service/package. However, because reflection runs on its own service/package namespace this doesn't work as there is no way of telling which service it's supposed to be querying to. To solve that we added a header to redirect to one service or another but evans doesn't pass the specified header to the reflection clients.

This PR takes the headers specified via the command line and passes them to the reflection clients, it doesn't introduce any new flag or modify any other behavior.

Not sure if this is how you wanted to solve this problem or if someone is working on this already but as I needed something quick I figured I'd fixed and open a PR to kick the discussion.

thanks

@codecov
Copy link

codecov bot commented Jan 26, 2021

Codecov Report

Merging #351 (ab5c40e) into master (62389c7) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #351      +/-   ##
==========================================
+ Coverage   80.29%   80.31%   +0.01%     
==========================================
  Files          57       57              
  Lines        2736     2738       +2     
==========================================
+ Hits         2197     2199       +2     
  Misses        309      309              
  Partials      230      230              

Copy link
Owner

@ktr0731 ktr0731 left a comment

Choose a reason for hiding this comment

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

Thanks for submitting the PR.
Most of the changes, and the policy of not changing the command interface, are completely in line with my assumptions!
I suggested a nitpick change, so could you take a look?

Comment on lines 40 to 44
md := metadata.New(nil)
for k, v := range headers {
md.Append(k, v...)
}
return metadata.NewOutgoingContext(context.Background(), md)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
md := metadata.New(nil)
for k, v := range headers {
md.Append(k, v...)
}
return metadata.NewOutgoingContext(context.Background(), md)
return metadata.NewOutgoingContext(context.Background(), metadata.MD(headers))

@dbarrosop
Copy link
Contributor Author

Thanks for the review, I addressed the comment and squashed the commits. Let me know if there is something else you need.

Copy link
Owner

@ktr0731 ktr0731 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to specify headers in gRPC reflection requests.
2 participants