Removed invalid service invocation check when using HttpRequestMessage #1354
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…Message - it was improperly checking the destination host against the httpClient host when of course this wouldn't match up Signed-off-by: Whit Waldo <[email protected]>
|
@philliphoff Could you give this a quick look when you have a moment? Fix for an as-yet unreported bug discovered while working on another. |
WhitWaldo
changed the title
philliphoff
reviewed
Oct 11, 2024
| @@ -423,12 +423,7 @@ public override HttpRequestMessage CreateInvokeMethodRequest<TRequest>(HttpMetho | |||
| public override async Task<HttpResponseMessage> InvokeMethodWithResponseAsync(HttpRequestMessage request, CancellationToken cancellationToken = default) | |||
| { | |||
| ArgumentVerifier.ThrowIfNull(request, nameof(request)); | |||
|
|
|||
| if (!this.httpEndpoint.IsBaseOf(request.RequestUri)) | |||
There was a problem hiding this comment.
I think this is intentional. The HttpRequestMessage (from the DaprClient docs) "must be a conforming Dapr service invocation request". This implies that the base RequestUri would be the Dapr sidecar host/port (as created by the CreateInvokeMethodRequest() method).
There was a problem hiding this comment.
For the life of me, I cannot remember my scenario for why I thought this was out of date. I'm good closing this until it comes to mind again.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
While building out a repro project to investigate #1352, I was unable to validate that (awaiting further information), but did find a bug in the client when a service invocation involved an
HttpRequestMessage. It would incorrectly validate the host/port of the destination request with the local HTTP client endpoint for a match before proceeding - this would likely never work unless it were calling an endpoint on its own service.Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #1311
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: