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

fix special grpc metadata handling #697

Merged
merged 5 commits into from
Feb 2, 2023
Merged

fix special grpc metadata handling #697

merged 5 commits into from
Feb 2, 2023

Conversation

ktalg
Copy link
Collaborator

@ktalg ktalg commented Jan 30, 2023

fix #700

There is a bug in the current handling of special grpc metadata (user-agent,host):
The MD itself is modified in the Dial method so that other threads cannot read the original MD while establishing a connection

Modifications to the MD are now moved outside of the Dial and need to be called separately when the request is sent

@codecov
Copy link

codecov bot commented Jan 30, 2023

Codecov Report

Base: 89.0% // Head: 89.0% // Increases project coverage by +0.0% 🎉

Coverage data is based on head (e016ae0) compared to base (7de742f).
Patch coverage: 100.0% of modified lines in pull request are covered.

Additional details and impacted files
@@          Coverage Diff           @@
##           master    #697   +/-   ##
======================================
  Coverage    89.0%   89.0%           
======================================
  Files          30      30           
  Lines        4448    4454    +6     
======================================
+ Hits         3958    3964    +6     
  Misses        324     324           
  Partials      166     166           
Impacted Files Coverage Δ
fgrpc/grpcrunner.go 90.7% <100.0%> (+0.2%) ⬆️
fgrpc/pingsrv.go 89.9% <100.0%> (+0.2%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ldemailly
Copy link
Member

Hi @ktalg how does the issue show up? (grpc run - missing headers in some? do you have a human visible repro?)

fgrpc/grpcrunner.go Outdated Show resolved Hide resolved
fgrpc/grpcrunner_test.go Outdated Show resolved Hide resolved
@ldemailly
Copy link
Member

I'm testing a variant with setting things only once

Copy link
Member

@ldemailly ldemailly left a comment

Choose a reason for hiding this comment

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

(though I'm kinda approving my own changes too so KT ptal)

@ktalg
Copy link
Collaborator Author

ktalg commented Feb 2, 2023

@ldemailly and how about improving the name of extractDialOptions, because it does two things

@ldemailly
Copy link
Member

@ldemailly and how about improving the name of extractDialOptions, because it does two things

yes that's a good point. will change it. code wise though, thoughts/ok?

@ldemailly
Copy link
Member

oh you did already, cool

@ktalg
Copy link
Collaborator Author

ktalg commented Feb 2, 2023

@ldemailly and how about improving the name of extractDialOptions, because it does two things

yes that's a good point. will change it. code wise though, thoughts/ok?

I think our changes are good enough, there's nothing more to suggest!

But I seem to have violated 'DRY' in some places, and I think I should avoid these if there is a big refactoring in the future

@ktalg ktalg merged commit a681ef1 into fortio:master Feb 2, 2023
@ldemailly
Copy link
Member

thx we can further cleanup in #681 (repeat in tests is somewhat fine btw)

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.

Bug about handling grpc authority metadata
2 participants