fix : added fallback support for host headers #279
Conversation
Codecov Report
@@ Coverage Diff @@
## main #279 +/- ##
============================================
+ Coverage 79.06% 79.07% +0.01%
- Complexity 1230 1231 +1
============================================
Files 110 110
Lines 4857 4861 +4
Branches 439 440 +1
============================================
+ Hits 3840 3844 +4
Misses 813 813
Partials 204 204
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
| if (grpcAuthority.isPresent()) { | ||
| return getSanitizedHostValue(grpcAuthority.get()); | ||
| } else { | ||
| return getGrpcRequestMetadataHost(event); |
There was a problem hiding this comment.
Look for the method - getGrpcXForwardedFor, this method should be similar to that. It should internally check that isRpcSystemGrpc, if yes, it should check for the rpc.request.metadata.host attribute.
There was a problem hiding this comment.
Second, we also have to sanitize the host value read from other attributes - getSanitizedHostValue (getGrpcRequestMetadataHost())
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| } | ||
|
|
||
| private Optional<String> getGrpcAuthority(Event event) { | ||
| private Optional<String> getGrpcHostHeader(Event event) { |
There was a problem hiding this comment.
@kotharironak should I add test over here also?
There was a problem hiding this comment.
Yes, add a test with an event not having authority but required tags like rpc.sytem and rec.request.metadata.host, and hostHeader fall back to metadata.host value.
There was a problem hiding this comment.
Refer the test file ApiBoundaryTypeAttributeEnricherTest, test cases testEnrichEventWithGrpcNoAuthority and similar.
There was a problem hiding this comment.
There is a test testEnrichEventWithGrpcAuthority in the same file, can you also add the below attribute to it.
addAttributeToEvent(
innerEntrySpan,
RPC_REQUEST_METADATA_HOST.getValue(),
AttributeValue.newBuilder().setValue("testHost2").build());
|
@sarthak77 Can you add PR description and message? |
sarthak77
changed the title
sarthak77
changed the title
| return Optional.empty(); | ||
| } | ||
|
|
||
| if (attributeValueMap.get(RPC_REQUEST_METADATA_HOST.getValue()) != null) { |
There was a problem hiding this comment.
Let's move RPC_REQUEST_METATA_HOST.getValue() as static string var at top.
private static final String RPC_REQUEST_METADATA_HOST_ATTR = RPC_REQUEST_METADATA_HOST.getValue();
There was a problem hiding this comment.
Repace if block with as below to avoid multiple lookups in map.
return Optional.ofNullable(attributeValueMap.get(RPC_REQUEST_METADATA_HOST_ATTR))
.map(v -> v.getValue());
|
|
||
| @Test | ||
| public void testGetGrpcRequestMetadataHost() { | ||
| Event event = mock(Event.class); |
There was a problem hiding this comment.
Also add one more test, if rpc system is not set to Grpc, the method returns to Optional.Empty()
This comment has been minimized.
This comment has been minimized.
kotharironak
changed the title
This comment has been minimized.
This comment has been minimized.
| @Test | ||
| public void testEnrichEventWithGrpcNoAuthorityButRequestMetadataHost() { | ||
| mockProtocol(innerEntrySpan, Protocol.PROTOCOL_GRPC); | ||
| org.hypertrace.core.datamodel.eventfields.grpc.Grpc grpc = |
There was a problem hiding this comment.
Can you remove lines from 307 to 315 that are not required?
Below code,
org.hypertrace.core.datamodel.eventfields.grpc.Grpc grpc =
mock(org.hypertrace.core.datamodel.eventfields.grpc.Grpc.class);
when(innerEntrySpan.getGrpc()).thenReturn(grpc);
Request request = mock(Request.class);
when(grpc.getRequest()).thenReturn(request);
RequestMetadata metadata = mock(RequestMetadata.class);
when(request.getRequestMetadata()).thenReturn(metadata);
when(metadata.getAuthority()).thenReturn("localhost:443");
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2 participants
Earlier host header was being checked through grpc authority. So it was suggested to check metadata host if authority was not present as a fallback. Added support for this.