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.

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

Removed invalid service invocation check when using HttpRequestMessage #1354

Closed
wants to merge 3 commits into from

Conversation

WhitWaldo
Copy link
Contributor

@WhitWaldo WhitWaldo commented Oct 6, 2024

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:

  • Code compiles correctly
  • [N/A] Created/updated tests - No existing tests to update
  • [N/A] Extended the documentation

…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]>
@WhitWaldo WhitWaldo requested review from a team as code owners October 6, 2024 10:44
@WhitWaldo WhitWaldo self-assigned this Oct 6, 2024
@WhitWaldo
Copy link
Contributor Author

@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 WhitWaldo changed the title Removed invalid service invocation check Removed invalid service invocation check when using HttpRequestMessage Oct 6, 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))
Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@WhitWaldo WhitWaldo closed this Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non-Dapr endpoints invocation supported?
2 participants