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.

Sign up for GitHub

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 TraceContextPropagator behavior when trace parent flags contain… #4893

Merged
merged 9 commits into from
Oct 6, 2023
Merged

Fix TraceContextPropagator behavior when trace parent flags contain… #4893

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7fe4d2d
Fix `TraceContextPropagator` behavior when trace parent flags contain…
Kielek Sep 27, 2023
f7ff5d1
Merge branch 'main' into fix-test_traceparent_trace_flags_illegal_cha…
Kielek Sep 27, 2023
71e691c
Merge branch 'main' into fix-test_traceparent_trace_flags_illegal_cha…
Kielek Sep 29, 2023
d946f3d
Merge branch 'main' into fix-test_traceparent_trace_flags_illegal_cha…
Kielek Oct 3, 2023
3227f02
Merge branch 'main' into fix-test_traceparent_trace_flags_illegal_cha…
Kielek Oct 4, 2023
75a1ace
Merge branch 'main' into fix-test_traceparent_trace_flags_illegal_cha…
Kielek Oct 6, 2023
d0df3db
PF feedback - changelog entry
Kielek Oct 6, 2023
726c499
Merge branch 'main' into fix-test_traceparent_trace_flags_illegal_cha…
utpilla Oct 6, 2023
e2cefb6
Merge branch 'main' into fix-test_traceparent_trace_flags_illegal_cha…
utpilla Oct 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/OpenTelemetry.Api/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@
`GetTracer` from leaking memory.
([#4906](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4906))

* Fix `TraceContextPropagator` by validating the first digit of the hex-encoded
`trace-flags` field of the `traceparent` header.
([#4893](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4893))

## 1.6.0

Released 2023-Sep-05
Expand Down
7 changes: 4 additions & 3 deletions src/OpenTelemetry.Api/Context/Propagation/TraceContextPropagator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,19 +195,20 @@ internal static bool TryExtractTraceparent(string traceparent, out ActivityTrace
return false;
}

byte options1;
byte optionsLowByte;
try
{
spanId = ActivitySpanId.CreateFromString(traceparent.AsSpan().Slice(VersionAndTraceIdLength, SpanIdLength));
options1 = HexCharToByte(traceparent[VersionAndTraceIdAndSpanIdLength + 1]);
_ = HexCharToByte(traceparent[VersionAndTraceIdAndSpanIdLength]); // to verify if there is no bad chars on options position
optionsLowByte = HexCharToByte(traceparent[VersionAndTraceIdAndSpanIdLength + 1]);
}
catch (ArgumentOutOfRangeException)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is outside the scope of this PR but we don't use the exception in any way. We could simply update HexCharToByte method to return false instead of throwing an unused exception.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Kielek Would you mind updating this follow-up PRs?

{
// it's ok to still parse tracestate
return false;
}

if ((options1 & 1) == 1)
if ((optionsLowByte & 1) == 1)
{
traceOptions |= ActivityTraceFlags.Recorded;
}
Expand Down
4 changes: 2 additions & 2 deletions test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/W3CTraceContextTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public class W3CTraceContextTests : IDisposable
*/
private const string W3cTraceContextEnvVarName = "OTEL_W3CTRACECONTEXT";
private static readonly Version AspNetCoreHostingVersion = typeof(Microsoft.AspNetCore.Hosting.Builder.IApplicationBuilderFactory).Assembly.GetName().Version;
private readonly HttpClient httpClient = new HttpClient();
private readonly HttpClient httpClient = new();
private readonly ITestOutputHelper output;

public W3CTraceContextTests(ITestOutputHelper output)
Expand Down Expand Up @@ -107,7 +107,7 @@ public void W3CTraceContextTestSuiteAsync(string value)
}
else
{
Assert.StartsWith("FAILED (failures=3)", lastLine);
Assert.StartsWith("FAILED (failures=2)", lastLine);
}
}

Expand Down
16 changes: 10 additions & 6 deletions test/OpenTelemetry.Tests/Trace/Propagation/TraceContextPropagatorTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,12 +103,16 @@ public void IsBlankIfNoHeader()
Assert.False(ctx.ActivityContext.IsValid());
}

[Fact]
public void IsBlankIfInvalid()
[Theory]
[InlineData($"00-xyz7651916cd43dd8448eb211c80319c-{SpanId}-01")]
[InlineData($"00-{TraceId}-xyz7c989f97918e1-01")]
[InlineData($"00-{TraceId}-{SpanId}-x1")]
[InlineData($"00-{TraceId}-{SpanId}-1x")]
public void IsBlankIfInvalid(string invalidTraceParent)
{
var headers = new Dictionary<string, string>
{
{ TraceParent, $"00-xyz7651916cd43dd8448eb211c80319c-{SpanId}-01" },
{ TraceParent, invalidTraceParent },
};

var f = new TraceContextPropagator();
Expand Down Expand Up @@ -191,8 +195,8 @@ public void DuplicateKeys()
// test_tracestate_duplicated_keys
Assert.Empty(CallTraceContextPropagator("foo=1,foo=1"));
Assert.Empty(CallTraceContextPropagator("foo=1,foo=2"));
Assert.Empty(CallTraceContextPropagator(new string[] { "foo=1", "foo=1" }));
Assert.Empty(CallTraceContextPropagator(new string[] { "foo=1", "foo=2" }));
Assert.Empty(CallTraceContextPropagator(new[] { "foo=1", "foo=1" }));
Assert.Empty(CallTraceContextPropagator(new[] { "foo=1", "foo=2" }));
}

[Fact]
Expand Down Expand Up @@ -318,7 +322,7 @@ private static string CallTraceContextPropagator(string[] tracestate)
{
var headers = new Dictionary<string, string[]>
{
{ TraceParent, new string[] { $"00-{TraceId}-{SpanId}-01" } },
{ TraceParent, new[] { $"00-{TraceId}-{SpanId}-01" } },
{ TraceState, tracestate },
};
var f = new TraceContextPropagator();
Expand Down