-
Notifications
You must be signed in to change notification settings - Fork 16
GRPC authority header seems to not have host component sometimes. #87
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
Conversation
Due to this, the host header was being set as :<port>, which isn't right. Fixing the bug to ignore such host header so that the span will be resolved based on the servicename that comes in the span.
|
snyk scan failing |
Codecov Report
@@ Coverage Diff @@
## main #87 +/- ##
============================================
+ Coverage 68.72% 68.80% +0.08%
- Complexity 814 816 +2
============================================
Files 83 83
Lines 3440 3440
Branches 367 367
============================================
+ Hits 2364 2367 +3
+ Misses 931 929 -2
+ Partials 145 144 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Fixed |
| private Optional<String> sanitizeHostValue(String host) { | ||
| if (host.contains(COLON) && !host.startsWith(COLON)) { | ||
| return COLON_SPLITTER.splitToList(host).get(0); | ||
| return Optional.ofNullable(COLON_SPLITTER.splitToList(host).get(0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here host is coming as null or as an empty string "", will that handle an empty string case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If host is empty, it shouldn't even match host.contains(COLON) right?
| if (!LOCALHOST.equalsIgnoreCase(host)) { | ||
| return Optional.of(host); | ||
| Optional<String> host = sanitizeHostValue(value); | ||
| if (host.isPresent() && !LOCALHOST.equalsIgnoreCase(host.get())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need conditon - StringUtils.isEmpty(host.get()) - for case where host is not null but an empty "" string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
equalsIgnoreCase() will take care of those.
Due to this, the host header was being set as
:port, which isn't right.Fixing the bug to ignore such host header so that the span will be resolved
based on the servicename that comes in the span.