Skip to content

Add YARP container support #8856

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

Merged
merged 19 commits into from
May 12, 2025
Merged

Conversation

benjaminpetit
Copy link
Member

Description

This PR add simple helper methods to use a YARP container in Aspire.

Fixes # (issue)

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?

@github-actions github-actions bot added the area-integrations Issues pertaining to Aspire Integrations packages label Apr 17, 2025
@danmoseley danmoseley requested a review from Copilot April 17, 2025 23:19
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds YARP container support to Aspire by introducing helper methods and resource definitions that simplify adding and configuring a YARP container.

  • Introduces an extension method (AddYarp) to add YARP resources with configuration and environment support.
  • Defines a YarpResource class and corresponding container image tags.
  • Updates sample usage in TestShop.AppHost to demonstrate the new extension method.

Reviewed Changes

Copilot reviewed 6 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/Aspire.Hosting.Yarp/YarpServiceExtensions.cs Adds extension methods to create and configure a YARP container resource, including environment and dynamic configuration handling.
src/Aspire.Hosting.Yarp/YarpResource.cs Introduces the YarpResource class, extending the container resource concept.
src/Aspire.Hosting.Yarp/YarpContainerImageTags.cs Defines constants for YARP container image attributes and configuration paths.
src/Aspire.Hosting.Yarp/README.md Minimal documentation for the YARP hosting library.
playground/TestShop/TestShop.AppHost/Program.cs Demonstrates usage of the new AddYarp extension method in a sample project.
Files not reviewed (7)
  • Aspire.sln: Language not supported
  • playground/TestShop/TestShop.AppHost/TestShop.AppHost.csproj: Language not supported
  • playground/TestShop/TestShop.AppHost/appsettings.Development.json: Language not supported
  • playground/TestShop/TestShop.AppHost/yarp.json: Language not supported
  • spelling.dic: Language not supported
  • src/Aspire.Hosting.Yarp/Aspire.Hosting.Yarp.csproj: Language not supported
  • tests/Shared/RepoTesting/Directory.Packages.Helix.props: Language not supported
Comments suppressed due to low confidence (1)

src/Aspire.Hosting.Yarp/YarpContainerImageTags.cs:12

  • [nitpick] The 'Tag' constant is defined but not used in the extension methods; consider either incorporating it into the configuration or removing it to avoid potential confusion.
public const string Tag = "2-preview";

@benjaminpetit benjaminpetit marked this pull request as ready for review April 30, 2025 16:13

namespace Aspire.Hosting.Yarp;

internal static class YarpContainerImageTags
Copy link
Member

Choose a reason for hiding this comment

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

Seems like the other ones all have tags for each of these.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure to understand?

@adityamandaleeka
Copy link
Member

We should have a functional test that actually has a YARP resource.

@mitchdenny
Copy link
Member

This is looking good as a first pass and I would like to get it in for 9.3. But I think we need to just have a quick look at how this API might evolve over time to make sure we aren't adding things that we'll regret later. Based on my read through of the API, this is what the usage looks like:

var builder = DistributedApplication.CreateBuilder(args);
var yarp = builder.AddYarp("myyarp")
                  .WithConfigFile("path/to/config/file");

This looks reasonable, but we should consider what would happen if someone does this:

var builder = DistributedApplication.CreateBuilder(args);
var yarp = builder.AddYarp("myyarp");

At the moment it will throw an exception, but I think it might be nicer if we can spin up the YARP proxy using a minimal config file, maybe with a simple hello world page or something like that (similar to what Nginx does).

In the future we might want to do things like this:

var builder = DistributedApplication.CreateBuilder(args);
var api = builder.AddProject<Proejcts.MyApi>("api");
var yarp = builder.AddYarp("myyarp")
                  .WithStaticFiles("path/to/static/files")
                  .WithProxiedPath(api.GetEndpoint("https"), "/api");

In this scenario we don't write a config file, instead it would be generated for us based on annotations that WithStaticFiles and WithProxiedPath add to the resource. For this reason I probably wouldn't put the ConfigFilePath property on the resource because it really depends on how you are configuring things.

We also need to think through how this flows all the way to production - not just for the inner loop.

@mitchdenny
Copy link
Member

You might want to look at adding a playground project that uses this resource to make it easy for folks to review/dogfood the resource as it gets built out.

@benjaminpetit
Copy link
Member Author

This is looking good as a first pass and I would like to get it in for 9.3. But I think we need to just have a quick look at how this API might evolve over time to make sure we aren't adding things that we'll regret later. Based on my read through of the API, this is what the usage looks like:

var builder = DistributedApplication.CreateBuilder(args);
var yarp = builder.AddYarp("myyarp")
                  .WithConfigFile("path/to/config/file");

This looks reasonable, but we should consider what would happen if someone does this:

var builder = DistributedApplication.CreateBuilder(args);
var yarp = builder.AddYarp("myyarp");

At the moment it will throw an exception, but I think it might be nicer if we can spin up the YARP proxy using a minimal config file, maybe with a simple hello world page or something like that (similar to what Nginx does).

In the future we might want to do things like this:

var builder = DistributedApplication.CreateBuilder(args);
var api = builder.AddProject<Proejcts.MyApi>("api");
var yarp = builder.AddYarp("myyarp")
                  .WithStaticFiles("path/to/static/files")
                  .WithProxiedPath(api.GetEndpoint("https"), "/api");

In this scenario we don't write a config file, instead it would be generated for us based on annotations that WithStaticFiles and WithProxiedPath add to the resource. For this reason I probably wouldn't put the ConfigFilePath property on the resource because it really depends on how you are configuring things.

We also need to think through how this flows all the way to production - not just for the inner loop.

Yeah maybe we could inject a default html page (the current container doesn't support static files, but that's something we can add) or simply redirect by default to the YARP container configuration doc. For now I think haveing a clear error message is enough.

For future use: I have a prototype branch, it was based on the k8s ingress api (I'll send you the internal demo I presented a while ago).

I believe having a file property for the config will always be useful (we could merge configs). I can make the property internal only for now if we decide to remove it later?

@davidfowl
Copy link
Member

I think this is commit but I'm not sure we can "ship" this without a deployment story.

Copy link
Member

@mitchdenny mitchdenny left a comment

Choose a reason for hiding this comment

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

Lets get this in for 9.3 as a preview (I updated the package to be preview) and we can iterate in 9.4. I think it is useful as it is but we can do more to make it really joyful to use.

/// <param name="builder">The YARP resource to configure.</param>
/// <param name="configFilePath">The path to the YARP config file.</param>
/// <returns>A reference to the <see cref="IResourceBuilder{T}"/>.</returns>
public static IResourceBuilder<YarpResource> WithConfigFile(this IResourceBuilder<YarpResource> builder, string configFilePath)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this was suggested elsewhere, but would it make sense to be able to define the config in C#? That way someone doesn't have to jump out to a JSON file. But instead something like:

builder.AddYarp("apigateway")
       .WithReference(basketService)
       .WithReference(catalogService)
       .WithConfig(context =>
       {
           context.AddRoute(basketService, "{**catch-all}");
       });

Copy link
Member

Choose a reason for hiding this comment

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

Yea but not as a gate for this initial PR

@joperezr joperezr added this to the 9.3 milestone May 12, 2025
@benjaminpetit benjaminpetit enabled auto-merge (squash) May 12, 2025 19:41
@benjaminpetit benjaminpetit merged commit 1e11c05 into dotnet:main May 12, 2025
171 of 172 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jun 12, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-integrations Issues pertaining to Aspire Integrations packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants