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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-integrations Issues pertaining to Aspire Integrations packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants