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

Make IHttpProxy public #455

Merged
merged 8 commits into from
Oct 10, 2020
Merged

Make IHttpProxy public #455

merged 8 commits into from
Oct 10, 2020

Conversation

Tratcher
Copy link
Member

@Tratcher Tratcher commented Oct 6, 2020

Fixes #408 Multiple partners have asked to directly access the IHttpProxy to proxy requests since they handle most of the other aspects themselves (discovery, routing, health checks, etc.). Making it public is easy, but there are some things we want to clean up on the signature first. The main change is removing ProxyTelemetryContext from IHttpProxy.ProxyAsync which has a cascading effect down to StreamCopier.

@MihaZupan The metrics removed from StreamCopier are a good example of something we may want to replace with new diagnostics.

An additional change I hadn't anticipated was needing to make many of the transform classes public, only the base classes were public before.

@Tratcher Tratcher self-assigned this Oct 6, 2020
@Tratcher
Copy link
Member Author

Tratcher commented Oct 7, 2020

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@Tratcher
Copy link
Member Author

Tratcher commented Oct 7, 2020

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@Tratcher
Copy link
Member Author

Tratcher commented Oct 7, 2020

(Note, not ready for review, but marking as ready to debug a CI issue).

@Tratcher Tratcher marked this pull request as ready for review October 7, 2020 16:16
@Tratcher Tratcher closed this Oct 7, 2020
@Tratcher Tratcher reopened this Oct 7, 2020
@alexperovich
Copy link
Member

/azp help

@azure-pipelines
Copy link

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

@alexperovich
Copy link
Member

/azp where

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 455 in repo microsoft/reverse-proxy

@alexperovich
Copy link
Member

/azp list

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 455 in repo microsoft/reverse-proxy

@Tratcher
Copy link
Member Author

Tratcher commented Oct 7, 2020

/azp where

@azure-pipelines
Copy link

No Azure DevOps organizations are setup to build repository microsoft/reverse-proxy using the Azure Pipelines App.

@Tratcher
Copy link
Member Author

Tratcher commented Oct 7, 2020

/azp list

@azure-pipelines
Copy link

No pipelines found for this repository.

@Tratcher
Copy link
Member Author

Tratcher commented Oct 7, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Tratcher Tratcher marked this pull request as draft October 8, 2020 23:11
@Tratcher
Copy link
Member Author

Tratcher commented Oct 8, 2020

(Actually ready for review this time 😁 )


### Error handling

IHttpProxy catches exceptions and timeouts from the HTTP client, logs them, and converts them to 5xx status codes or aborts the response. The error details, if any, can be accessed from the [IProxyErrorFeature](xref:Microsoft.ReverseProxy.Service.Proxy.IProxyErrorFeature) as shown above.
Copy link
Member

Choose a reason for hiding this comment

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

What if I want to manually retry a failed request?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the response hasn't already started then that can be done by calling the ProxyAsync again. The normal error handling code sets the status code but does not otherwise start the response. You may also be able to use Polly?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If I want to retry a failed request, I do need to buffer the request body and rewind it for retry, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, though requests with bodies are not generally considered retriable as you can't tell if the server processed it. E.g. you wouldn't want to double submit a form.


The http client may be customized, but the above example is recommended for common proxy scenarios.

Always use HttpMessageInvoker rather than HttpClient, HttpClient buffers responses by default. Buffering breaks streaming scenarios and increases memory usage and latency.
Copy link
Member

Choose a reason for hiding this comment

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

We should also point out that response buffering can make handling partial responses easier since the status code and headers of the proxied response can still be modified at that time. This is necessary if you want the proxy to retry a request in this case.

Copy link
Member Author

@Tratcher Tratcher Oct 9, 2020

Choose a reason for hiding this comment

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

We decided in triage to block the use of HttpClient for now, so no need to include any endorsements for it. If we want to support response buffering that can also be done as an explicit feature of HttpProxy or as a DelegatingHandler in a way that does not break streaming scenarios.

@Tratcher Tratcher merged commit 2061e7c into master Oct 10, 2020
@Tratcher Tratcher deleted the tratcher/publicproxy branch October 10, 2020 01:10
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.

Proxy component can be used stand-alone for advanced scenarios
4 participants