Fire an event when allowing a bad request #39944
Conversation
|
Hi @Tratcher. If this is not a tell-mode PR, please make sure to follow the instructions laid out in the servicing process document. |
|
Hi @Tratcher. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge. To learn more about how to prepare a servicing PR click here. |
Co-authored-by: Stephen Halter <halter73@gmail.com>
| _requestRejectedException = ex; | ||
|
|
||
| const string badRequestEventName = "Microsoft.AspNetCore.Server.Kestrel.BadRequest"; | ||
| if (ServiceContext.DiagnosticSource?.IsEnabled(badRequestEventName) == true) | ||
| { | ||
| ServiceContext.DiagnosticSource.Write(badRequestEventName, this); | ||
| } | ||
|
|
||
| _requestRejectedException = null; |
There was a problem hiding this comment.
What happens if an exception is thrown from DiagnosticSource.Write? Do we care that _requestRejectedException isn't set back to null?
tldr add try/finally?
There was a problem hiding this comment.
I think that exception would cause the request to fail anyways, so _requestRejectedException wouldn't matter.
There was a problem hiding this comment.
There are different paths that a request goes down on failure when this property is sent. e.g. writing a 400 response
| // Normally this would have been rejected, but the developer opted into allowing the bad behavior. | ||
| public void ReportAllowedBadRequest(BadHttpRequestException ex) | ||
| { | ||
| Log.ConnectionBadRequest(ConnectionId, ex); |
There was a problem hiding this comment.
this is new logging that now occurs even without setting the appcontext switch?
There was a problem hiding this comment.
This method is only called when the appcontext switch is set. Compare to the SetBadRequestState method above that's called when the appcontext switch is not set. Both write the same log, but the new one avoids marking the request as failed/rejected.
Pilchie
added
Servicing-approved
amcasey
added
area-networking
Fire an event when allowing a bad request
This adds diagnostics for a prior fix so partners can determine when the client issues have been addressed.
Description
#39334 from Feb allowed a partner to opt into accepting a specific kind of bad request. They're working to address the issues with the remote client and aren't sure how long that will take. They've requested new event so they can add in their own logging to monitor the client's progress, detect if anyone else is encountering the issue, and decide if we need a long term fix added to 7.0.
Fixes #39756
Customer Impact
The customer is unable to tell if the prior mitigation is still required without disabling it and potentially breaking live customers.
Regression?
Risk
[Justify the selection above]
Verification
Packaging changes reviewed?