Relayer transfroms

[Tratcher]

#594 IHttpProxy is getting much more attention/usage than other parts of YARP. Long term we're interested in moving that component into ASP.NET Core. However, IHttpProxy also brings in the entire transforms infrastructure which it's not clear if people using IHttpProxy directly need, or if they could use a simplified model (e.g. callbacks).

Here's the proposed new transforms model for IHttpProxy:

    public record HttpTransforms
    {
        public static readonly HttpTransforms Empty = new HttpTransforms();

        /// <summary>
        /// Indicates if all request headers should be copied to the outgoing HttpRequestMessage before invoking
        /// OnRequest. Disable this if you want to manually control which headers are copied.
        /// Note this copy excludes certain protocol headers like HTTP/2 psuedo headers (":authority").
        /// </summary>
        public bool CopyRequestHeaders { get; init; } = true;

        /// <summary>
        /// Indicates if all response headers should be copied from the received HttpResponseMessage before invoking
        /// OnResponse. Disable this if you want to manually control which headers are copied.
        /// Note this copy excludes certain protocol headers like `Transfer-Encoding: chunked`.
        /// </summary>
        public bool CopyResponseHeaders { get; init; } = true;

        /// <summary>
        /// Indicates if all response trailers should be copied from the received HttpResponseMessage before invoking
        /// OnResponseTrailers. Disable this if you want to manually control which headers are copied.
        /// </summary>
        public bool CopyResponseTrailers { get; init; } = true;

        /// <summary>
        /// A callback that is invoked prior to sending the proxied request. All HttpRequestMessage fields are
        /// initialized except RequestUri, which will be initialized after the callback if no value is provided.
        /// The string parameter represents the destination URI prefix that should be used when constructing the RequestUri.
        /// The headers will be copied before the callback if CopyRequestHeaders is enabled.
        /// </summary>
        public Func<HttpContext, HttpRequestMessage, string, Task> OnRequest { get; init; }

        /// <summary>
        /// A callback that is invoked when the proxied response is received. The status code and reason phrase will be copied
        /// to the HttpContext.Response before the callback is invoked, but may still be modified there. The headers will be
        /// copied to HttpContext.Response.Headers before the callback if CopyResponseHeaders is enabled.
        /// </summary>
        public Func<HttpContext, HttpResponseMessage, Task> OnResponse { get; init; }

        /// <summary>
        /// A callback that is invoked after the response body to modify trailers, if supported. The trailers will be
        /// copied to the HttpContext.Response before the callback if CopyResponseTrailers is enabled.
        /// </summary>
        public Func<HttpContext, HttpResponseMessage, Task> OnResponseTrailers { get; init; }
    }

The idea is that everything should just work but you can get in and tweak things as needed. The higher layer uses these callbacks to hook in and run about the same structured transforms logic it was doing before. There's a little code duplication around header copying to to investigate, and some tests that need cleaning up.

[davidfowl]

The design change looks good. About the API, I think the type should be abstract and the methods should be virtual instead of delegates. Then maybe the base class can do the copying instead of exposing those properties.

public abstract class HttpTransforms
{
    public static readonly HttpTransforms Default = new DefaultHttpTransforms();

    /// <summary>
    /// A callback that is invoked prior to sending the proxied request. All HttpRequestMessage fields are
    /// initialized except RequestUri, which will be initialized after the callback if no value is provided.
    /// The string parameter represents the destination URI prefix that should be used when constructing the RequestUri.
    /// The headers will be copied before the callback if CopyRequestHeaders is enabled.
    /// </summary>
    public Task OnRequestAsync(HttpContext httpContext, HttpRequestMessage proxyRequest) => Default.OnRequestAsync(httpContext, proxyRequest);

    /// <summary>
    /// A callback that is invoked when the proxied response is received. The status code and reason phrase will be copied
    /// to the HttpContext.Response before the callback is invoked, but may still be modified there. The headers will be
    /// copied to HttpContext.Response.Headers before the callback if CopyResponseHeaders is enabled.
    /// </summary>
    public virtual Task OnResponseAsync(HttpContext httpContext, HttpResponseMessage proxyResponse) => Default.OnResponseAsync(httpContext, proxyResponse);

    /// <summary>
    /// A callback that is invoked after the response body to modify trailers, if supported. The trailers will be
    /// copied to the HttpContext.Response before the callback if CopyResponseTrailers is enabled.
    /// </summary>
    public virtual Task OnResponseTrailersAsync(HttpContext httpContext, HttpResponseMessage proxyResponse) => Default.OnResponseTrailersAsync(httpContext, proxyResponse);
}

[Tratcher]

@davidfowl Why abstract + Default instead of having the defaults directly in a virtual base class? As a way of communicating to people that they must derive from it to change anything? Would a protected constructor accomplish the same?

[davidfowl]

Should we make this protected static or is it simple enough to copy around

[Tratcher]

If you're going to customize TransformResponseAsync or TransformResponseTrailersAsync enough not to call the base implementation then you probably need to customize this code too. That's what happened for the structured transforms derivation.

[davidfowl]

This may as well be an array.

[Tratcher]

I expect this logic to get more interesting when we fix #583. I'll hold off changes until then.

[davidfowl]

Allocations in the hot path.

[Tratcher]

Yes, but it's trying to save you from even more allocations 😁. Let's revisit this on its own when we get to profiling.

[davidfowl]

@davidfowl Why abstract + Default instead of having the defaults directly in a virtual base class? As a way of communicating to people that they must derive from it to change anything? Would a protected constructor accomplish the same?

Yea you're right.

[Tratcher]

@davidfowl I think everybody else is out now, consider yourself the sole reviewer.

[davidfowl]

Do we still want to (eventually) provide an API that makes this easy to do without overriding the IHttpProxy? Something you can do in middleware?

[Tratcher]

Do we still want to (eventually) provide an API that makes this easy to do without overriding the IHttpProxy? Something you can do in middleware?

That's where #60 comes in.