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

Bff yarp proxy improvements #1734

Merged
merged 3 commits into from
Feb 4, 2025

Conversation

Erwinvandervalk
Copy link
Contributor

@Erwinvandervalk Erwinvandervalk commented Jan 27, 2025

The wireup of YARP in the BFF is now simplified by relying more on the functionality
of the BFF. This has several advantages, aside from the simplification. For example,
tracing and service discovery are automatically wired up correctly.

Most of the for configuring the AccessTokenRequestTransform has been moved from the Map extension method and the DefaultMessageInvokerFactory into the extension method MapRemoteBffApiEndpoint.

Breaking changes:

Removed DefaultMessageInvokerFactory

The class DefaultMessageInvokerFactory and it's interface IHttpMessageInvokerFactory have been removed. This class was used to configure the message invoker and was a point for mocking. If you wish to inject a http handler for unit testing, you can inject a custom IForwarderHttpClientFactory. For example:

   // A Forwarder factory that forwards the messages to a message handler (which can be easily retrieved from a testhost)
    public class BackChannelHttpMessageInvokerFactory(HttpMessageHandler backChannel) 
        : IForwarderHttpClientFactory
    {
        public HttpMessageInvoker CreateClient(ForwarderHttpClientContext context) => 
            new HttpMessageInvoker(backChannel);
    }

   // Wire up the forwarder in your application's test host:
    services.AddSingleton<IForwarderHttpClientFactory>(
         new BackChannelHttpMessageInvokerFactory(_apiHost.Server.CreateHandler()));

Removed class DefaultHttpTransformerFactory and IHttpTransformerFactory.

This class was used to configure the default transforms. This has now been changed in favor of allowing you to inject a delegate that configures Yarp into the method MapRemoteBffApiEndpoint(). The default logic has been moved to DefaultTransformers.DirectProxyWithAccessToken(). If you wish to extend this logic, you can wrap this function in a different method and inject that into it.

            endpoints.MapRemoteBffApiEndpoint(
                "/local-path",
                _apiHost.Url(),
                context =>
                {
                    // do something custom: IE: copy request headers
                    context.CopyRequestHeaders = true;

                    // wire up the default transformer logic
                    DefaultTransformers.DirectProxyWithAccessToken("/local-path", context);
                })
                // Continue with normal BFF configuration, for example, allowing optional user access tokens
                .WithOptionalUserAccessToken();

If you used a custom implementation of IHttpTransformerFactory to change the default behavior of MapRemoteBffApiEndpoint(),
for example to add additional transforms, then you can now inject a custom delegate into the di container:

services.AddSingleton<BffYarpTransformBuilder>(CustomDefaultYarpTransforms);
//...
// This is an example of how to add a response header to ALL invocations of MapRemoteBffApiEndpoint()
private void CustomDefaultBffTransformBuilder(string localpath, TransformBuilderContext context)
{
    context.AddResponseHeader("added-by-custom-default-transform", "some-value");
    DefaultBffYarpTransformerBuilders.DirectProxyWithAccessToken(localpath, context);
}

Removed method RemoteApiEndpoint.Map(localpath, apiAddress).

The Map method was no longer needed as most of the logic had been moved to either the MapRemoteBffApiEndpoint and the DefaultTransformers. The map method also wasn't very explicit about what it did and a number of test scenario's tried to verify if it wasn't called wrongly. You are now expected to call the method MapRemoteBffApiEndpoint. This method now has a nullable parameter that allows you to inject your own transformers.

AccessTokenRetrievalContext properties are now typed

The LocalPath and ApiAddress properties are now typed. They used to be strings. If you rely on these, for example for implementing
a custom IAccessTokenRetriever, then you should adjust their usage accordingly.

    /// <summary>
    /// The locally requested path.
    /// </summary>
    public required PathString LocalPath { get; set; }

    /// <summary>
    /// The remote address of the API.
    /// </summary>
    public required Uri ApiAddress { get; set; }

Tasks:

  • Add exploration test for YARP
  • Simplify wire-up of yarp
  • Added additional unit tests
  • remove double //
  • restore the AccessTokenRetrievalContext properties

fixes #1692
fixes #1693

@Erwinvandervalk Erwinvandervalk added impact/breaking The fix or change will be a breaking one area/bff Related to all BFF labels Jan 27, 2025
@Erwinvandervalk Erwinvandervalk added this to the bff-3.0.0 milestone Jan 27, 2025
@Erwinvandervalk Erwinvandervalk force-pushed the bff-yarp-proxy branch 2 times, most recently from c35e9a9 to 1c9cff5 Compare January 28, 2025 10:22
@Erwinvandervalk Erwinvandervalk marked this pull request as ready for review January 28, 2025 10:22
@leastprivilege
Copy link
Member

Thanks Erwin! This looks very good!

Could you also add a migration guide from the old extensibility points to the YARP ones?

@Erwinvandervalk Erwinvandervalk force-pushed the bff-yarp-proxy branch 2 times, most recently from 2babc49 to 4d66640 Compare January 31, 2025 07:48
Copy link
Member

@leastprivilege leastprivilege left a comment

Choose a reason for hiding this comment

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

Didn't do a full code review (someone else must do that) - but changes look good to me.

@Erwinvandervalk Erwinvandervalk force-pushed the bff-yarp-proxy branch 3 times, most recently from 2ad0cba to 21ec988 Compare February 4, 2025 08:33
@Erwinvandervalk Erwinvandervalk force-pushed the bff-yarp-proxy branch 2 times, most recently from 1e0b0aa to 61e01fc Compare February 4, 2025 11:00
@Erwinvandervalk Erwinvandervalk merged commit 9039293 into DuendeSoftware:main Feb 4, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/bff Related to all BFF impact/breaking The fix or change will be a breaking one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BFF's SocketsHttpHandler configuration out of sync with Yarp's Integrate Yarp's direct forwarding feature
3 participants