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

Jump to bottom

Set the baggage even when the trace id is not successfully extracted #55341

Merged
merged 6 commits into from
Jun 28, 2024
Merged

Set the baggage even when the trace id is not successfully extracted #55341

merged 6 commits into from
Jun 28, 2024

Conversation

joegoldman2
Copy link
Contributor

@joegoldman2 joegoldman2 commented Apr 24, 2024 •
edited
Loading

Fixes #50158.

Better to review it without whitespace diff: https://github.com/dotnet/aspnetcore/pull/55341/files?diff=unified&w=1.

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

@joegoldman2 joegoldman2 requested a review from halter73 as a code owner April 24, 2024 15:56
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Apr 24, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 24, 2024
amcasey
amcasey reviewed Apr 29, 2024
View reviewed changes
Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

Some basic questions.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label May 6, 2024
@joegoldman2 joegoldman2 requested a review from amcasey May 7, 2024 05:50
@joegoldman2
Copy link
Contributor Author

@cijothomas, could you please take a moment to share your thoughts on this PR? Thank you.

@cijothomas
Copy link

open-telemetry/opentelemetry-dotnet#5667
dotnet/runtime#45496

Linking to couple of related issues.

@joegoldman2 thanks for adding me! Agree with the statement that baggage should be extracted irrespective of tracecontext. I'll let owners of this repo review/share feedback.

cijothomas
cijothomas approved these changes Jun 6, 2024
View reviewed changes
Copy link

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

LGTM

amcasey
amcasey approved these changes Jun 24, 2024
View reviewed changes
Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

The best I can say is that I'm not aware of a problem this could cause.

src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs
@@ -430,28 +430,33 @@ private void RecordRequestStartMetrics(HttpContext httpContext)
}
}

// The trace id was successfully extracted, so we can set the trace state
// https://www.w3.org/TR/trace-context/#tracestate-header
if (!string.IsNullOrEmpty(requestId))
Copy link
Member

Choose a reason for hiding this comment

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

I suspect that checking requestId has probably become redundant, but I'm fine keeping it in the spirit of making a conservative change.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed that it is probably redundant, and also agreed that keeping the check anyways is the more conservative change :)

@amcasey
Copy link
Member

amcasey commented Jun 24, 2024

cc @JamesNK @noahfalk in case there are security/privacy/perf considerations I'm not aware of. Thanks!

@noahfalk
Copy link
Member

cc @JamesNK @noahfalk in case there are security/privacy/perf considerations I'm not aware of. Thanks!

No issues were apparent to me at least. Using baggage from an untrusted source is a potential security threat similar to any other untrusted on-the-wire input, but nothing in this change appeared to make that attack meaningfully easier to execute. At worst previously the attacker would have needed to add a randomly generated request id to their malicious message and now they don't need to do that.

@amcasey @noahfalk
Make comment more explicit.
27f7938 
Co-authored-by: Noah Falk <noahfalk@users.noreply.github.com>
@amcasey amcasey enabled auto-merge (squash) June 26, 2024 00:22
@amcasey
Copy link
Member

amcasey commented Jun 26, 2024

Thanks, @noahfalk! And thanks for your patience, @joegoldman2!

@build-analysis build-analysis bot mentioned this pull request Jun 26, 2024
Open
3 tasks
@amcasey
Copy link
Member

amcasey commented Jun 26, 2024

Failures in CI seem unrelated.

Agreed, but it's not one of the usual suspects. @captainsafia for SchemaTransformerTests.SchemaTransformer_RunsInRegisteredOrder.

@captainsafia
Copy link
Member

Agreed, but it's not one of the usual suspects. @captainsafia for SchemaTransformerTests.SchemaTransformer_RunsInRegisteredOrder.

I just merged a fix for this in 4c8b5fe. We may have to use /azp run to get the CI to build against the latest commits in main.

@amcasey
Copy link
Member

amcasey commented Jun 26, 2024

/azp run

@dotnet-policy-service dotnet-policy-service bot removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jun 26, 2024
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@amcasey
Copy link
Member

amcasey commented Jun 26, 2024

Those failures are the usual suspects. 😆

@amcasey
Copy link
Member

amcasey commented Jun 27, 2024

Can't say I've ever seen that signalr failure before. @BrennanConroy?

hubConnection using messagepack over WebSockets transport can return result to server

Error: expect(received).toEqual(expected) // deep equality

Expected: "10"
Received: "Connection 'd1ywZfinqXJgavmV85jGEA' does not exist."
    at Object.toEqual (/Users/runner/work/1/s/src/SignalR/clients/ts/FunctionalTests/ts/HubConnectionTests.ts:537:42)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

@amcasey
Copy link
Member

amcasey commented Jun 27, 2024

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@amcasey
Copy link
Member

amcasey commented Jun 27, 2024

Latest CI failure is #53294

@amcasey
Copy link
Member

amcasey commented Jun 27, 2024

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@amcasey
Copy link
Member

amcasey commented Jun 28, 2024

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@amcasey amcasey merged commit ecdc518 into dotnet:main Jun 28, 2024
26 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the 9.0-preview7 milestone Jun 28, 2024
@joegoldman2 joegoldman2 deleted the fix/50158 branch June 28, 2024 05:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noahfalk noahfalk noahfalk left review comments

@cijothomas cijothomas cijothomas approved these changes

@amcasey amcasey amcasey approved these changes

@halter73 halter73 Awaiting requested review from halter73 halter73 is a code owner

Assignees

@adityamandaleeka adityamandaleeka

Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
9.0-preview7
Development

Successfully merging this pull request may close these issues.

Baggage not extracted without trace id from the request
6 participants
@joegoldman2 @cijothomas @amcasey @noahfalk @captainsafia @adityamandaleeka