Skip to content

Adding basepath to the config #6963

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

Draft
wants to merge 27 commits into
base: main
Choose a base branch
from
Draft

Adding basepath to the config #6963

wants to merge 27 commits into from

Conversation

ewassef
Copy link

@ewassef ewassef commented Dec 18, 2024

This will allow the aspire app to handle running behind a reverse proxy such as a k8s ingress on a subpath #4159
#4528
#5134

Description

Added a new dashboard base path option, integrated it into the Dashboard Option, validated and replaced all areas that are creating the links

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
    • [x ] 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?
Microsoft Reviewers: Open in CodeFlow

This will allow the aspire app to handle running behind a reverse proxy such as a k8s ingress on a subpath
dotnet#4159
dotnet#4528
dotnet#5134
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Dec 18, 2024
Copy link
Member

@adamint adamint left a comment

Choose a reason for hiding this comment

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

Please add test coverage in ValidateDashboardOptions.

@adamint
Copy link
Member

adamint commented Dec 19, 2024

@JamesNK for review

@ewassef
Copy link
Author

ewassef commented Dec 23, 2024

Please add test coverage in ValidateDashboardOptions.

Tests added

rerunning the PR checks
@davidfowl
Copy link
Member

The options changes look fine, but this is a one liner in middleware https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.builder.usepathbaseextensions.usepathbase?view=aspnetcore-9.0.

You can undo all of the other changes you made.

@ewassef
Copy link
Author

ewassef commented Dec 26, 2024

The options changes look fine, but this is a one liner in middleware https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.builder.usepathbaseextensions.usepathbase?view=aspnetcore-9.0.

You can undo all of the other changes you made.

@davidfowl , I'm afraid I do not follow. I am using that method in the middleware, but everything else is to handle the options and also the way we are generating the link in the blazor app. The links seem to be string interpolation from the base before

@adamint
Copy link
Member

adamint commented Dec 26, 2024

The options changes look fine, but this is a one liner in middleware https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.builder.usepathbaseextensions.usepathbase?view=aspnetcore-9.0.
You can undo all of the other changes you made.

@davidfowl , I'm afraid I do not follow. I am using that method in the middleware, but everything else is to handle the options and also the way we are generating the link in the blazor app. The links seem to be string interpolation from the base before

I see what fowler means now. We don't need to store path base inside DashboardUrls. You do however need to provide a <base> tag in blazor since the base path is changed.

the way we are generating the link

The path base is provided in the dashboard options already. You can use that when writing out the dashboard url.

@davidfowl
Copy link
Member

davidfowl commented Dec 26, 2024

There should be a single call to the UsePathBase middleware in the code base. You can undo all of the other changes that were made to prepend the path base into various places in the application.

@adamint
Copy link
Member

adamint commented Dec 27, 2024

There should be a single call to the UsePathBase middleware in the code base. You can undo all of the other changes that were made to prepend the path base into various places in the application.

We need to change the blazor client-side path base as well.

@davidfowl
Copy link
Member

davidfowl commented Dec 28, 2024

Why? Also UsePathBase should be the first middleware (order matters). We should be able to remove the prefix in UseErrorHandler as well.

@JamesNK
Copy link
Member

JamesNK commented Dec 28, 2024

Is blazor loading something from a rooted path? Eg /blazor.

Can it be changed to a relative path? ./blazor

@JamesNK
Copy link
Member

JamesNK commented Dec 28, 2024

https://learn.microsoft.com/en-nz/aspnet/core/blazor/host-and-deploy/?view=aspnetcore-8.0&tabs=visual-studio#app-base-path

I don’t know the right thing to do (haven’t read docs) but there are a lot of docs on this issue. Find out what is best with blazor server in this situation.

@davidfowl
Copy link
Member

/azp run

@davidfowl
Copy link
Member

@JamesNK @adamint have you tried this out?

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ewassef
Copy link
Author

ewassef commented Feb 2, 2025

Im still unclear on the ask. I started with just the one liner, but it doesnt work as expected. we need to handle the url creation helpers in all the places that ive modified. The pipelines keep cycling and im not sure whats not actually working from this code?

@davidfowl
Copy link
Member

UsePathBase should do most of the heavy lifting. I will take a stab at this and see why that simple one line change doesn’t work.

@ewassef
Copy link
Author

ewassef commented Feb 4, 2025

Thanks @davidfowl . we really just want to install this as a subpath in a k8s ingress :)
To save you some time, you can take the first commit from the 5 that are there and run your tests

@ewassef
Copy link
Author

ewassef commented Mar 26, 2025

Any progress?

@WeihanLi
Copy link
Contributor

Is it possible to use the forwarded header for it, ASP.NET supports the X-Forwarded-Prefix since .NET 8

Maybe configure the ForwardedHeadersOptions is more simple to implement

https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.builder.forwardedheadersoptions.forwardedheaders?view=aspnetcore-9.0#microsoft-aspnetcore-builder-forwardedheadersoptions-forwardedheaders

dotnet/aspnetcore#23263

@DamianEdwards DamianEdwards removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Apr 1, 2025
@DamianEdwards DamianEdwards marked this pull request as draft April 3, 2025 01:07
Copy link
Member

@JamesNK JamesNK left a comment

Choose a reason for hiding this comment

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

I haven't tested the branch, just looked at code. My knowledge of the things in this PR (running an ASP.NET Core with a base path, running behind a proxy, forwarding headers) is low. An expert should take another look.

I mentioned coming URLs with OrdinalIgnoreCase a number of times. FYI we have StringComparers/StringComparisons with known comparison types. One already might be there for URLs. If not then you could add one.

Comment on lines +9 to +10
/// Adds a rference to the dashboard resource so that the required environment variables to allow service discovery to the Aspire dashboard
/// are added to this resource. Also configures a reference relationship to the dashboard resource.
Copy link
Member

Choose a reason for hiding this comment

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

Needs proof reading

Comment on lines +20 to +25
var dashboardUris = builder.ApplicationBuilder.Configuration["ASPNETCORE_URLS"]?.Split(';')
?.Select(u => Uri.TryCreate(u, UriKind.Absolute, out var uri) is { } ? uri : null)
?.Where(u => u is not null)
?.Cast<Uri>()
?.GroupBy(u => u.Scheme)
?? [];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var dashboardUris = builder.ApplicationBuilder.Configuration["ASPNETCORE_URLS"]?.Split(';')
?.Select(u => Uri.TryCreate(u, UriKind.Absolute, out var uri) is { } ? uri : null)
?.Where(u => u is not null)
?.Cast<Uri>()
?.GroupBy(u => u.Scheme)
?? [];
var dashboardUris = builder.ApplicationBuilder.Configuration["ASPNETCORE_URLS"]?.Split(';')
.Select(u => Uri.TryCreate(u, UriKind.Absolute, out var uri) is { } ? uri : null)
.OfType<Uri>()
.GroupBy(u => u.Scheme)
?? [];

if (https is not null || http is not null)
{
var endpoint = https ?? http!;
context.Urls.Add(new() { Url = $"{endpoint.Url}{proxyPath}", DisplayText = "Proxied Dashboard" });
Copy link
Member

Choose a reason for hiding this comment

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

A string concat of URL + path could mean there is no forward slash, or multiple forward slashes in the new URL. Use a better way to combine them?

@@ -26,7 +27,8 @@
"ASPIRE_DASHBOARD_OTLP_HTTP_ENDPOINT_URL": "http://localhost:16032",
"ASPIRE_RESOURCE_SERVICE_ENDPOINT_URL": "http://localhost:17031",
"ASPIRE_SHOW_DASHBOARD_RESOURCES": "true",
"ASPIRE_ALLOW_UNSECURED_TRANSPORT": "true"
"ASPIRE_ALLOW_UNSECURED_TRANSPORT": "true",
"Dashboard__ReverseProxy__ForwardHeaders": "true"
Copy link
Member

Choose a reason for hiding this comment

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

This is probably answered in later code, but how does this env var to the host make its way to the dashboard?

@@ -39,3 +42,22 @@
<script src="_content/Microsoft.FluentUI.AspNetCore.Components/Microsoft.FluentUI.AspNetCore.Components.lib.module.js" type="module"></script>
</body>
</html>

@code {
private string pathBase = "/";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private string pathBase = "/";
private string _pathBase = "/";

@@ -3,7 +3,7 @@

using Aspire.Dashboard.Configuration;
using Aspire.Dashboard.Model;
using Aspire.Dashboard.Utils;
using Aspire.Dashboard.Utils;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
using Aspire.Dashboard.Utils;
using Aspire.Dashboard.Utils;

{
// In development we want to ensure that requests outside of the path base return a 404 to mimic what would
// happen when the dashboard is being served from behind a reverse-proxy at a configured path. Note we still let
// ~/login requests through as that's what VS launches the browser to. In that case we redirect instead.
Copy link
Member

Choose a reason for hiding this comment

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

Need intergration tests of this:

Base path + good path = 200
Base path + bad path = 404
Base path + /login = 302

Comment on lines +19 to +27
if (baseRelativePath == targetPagePath)
{
return true;
}

var queryIndex = baseRelativePath.IndexOf('?');
var testPath = queryIndex >= 0 ? baseRelativePath[..queryIndex] : baseRelativePath;
var isMatch = testPath.TrimEnd('/') == targetPagePath;
return isMatch;
Copy link
Member

Choose a reason for hiding this comment

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

Path comparisons here should ignore case and culture, e.g. OrdinalIgnoreCase

if (!string.IsNullOrEmpty(dashboardPathBase))
{
// Ensure OTLP URLs are rewritten to include the path base.
if (Uri.TryCreate(dashboardOtlpGrpcUrl, UriKind.Absolute, out var otlpGrpcUri) && !otlpGrpcUri.AbsolutePath.StartsWith(dashboardPathBase))
Copy link
Member

Choose a reason for hiding this comment

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

OrdinalIgnoreCase?

uriBuilder.Path = dashboardPathBase + uriBuilder.Path.TrimStart('/');
dashboardOtlpGrpcUrl = uriBuilder.Uri.ToString();
}
if (Uri.TryCreate(dashboardOtlpHttpUrl, UriKind.Absolute, out var otlpHttpUri) && !otlpHttpUri.AbsolutePath.StartsWith(dashboardPathBase))
Copy link
Member

Choose a reason for hiding this comment

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

OrdinalIgnoreCase?

?.Select(u => Uri.TryCreate(u, UriKind.Absolute, out var uri) is { } ? uri : null)
?.Where(u => u is not null)
?.Cast<Uri>()
?.GroupBy(u => u.Scheme)
Copy link
Member

@JamesNK JamesNK Apr 4, 2025

Choose a reason for hiding this comment

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

I think scheme is always lower case. Maybe ToLowerInvariant() in the group by in case it isn't and someone does:

ASPNETCORE_URLS=https://localhost:80;HTTPS://LOCALHOST:8080

@JamesNK
Copy link
Member

JamesNK commented Apr 4, 2025

This is going to require a lot of new tests. For example, does all the existing authentication functionality work properly when running with a base path.

@DamianEdwards
Copy link
Member

This is going to require a lot of new tests. For example, does all the existing authentication functionality work properly when running with a base path.

@JamesNK yep for sure. This is now functionally complete more or less and the next step is to figure out a testing strategy.

@danmoseley
Copy link
Member

@ewassef do you plan to continue with this PR?

@ewassef
Copy link
Author

ewassef commented May 1, 2025

@ewassef do you plan to continue with this PR?

Are you serious? All the maintainers did their best to discourage contributions, said they would fix it and then let it rot. Most of the people i know have forked this and run it themselves.

I would Love to not have to do that, but the style police is stopping everything. So I'm waiting for them to fulfill their commitment ( see thread )

@DamianEdwards
Copy link
Member

@ewassef apologies for confusion. My intent is to complete this PR, it's just a priority and sequencing question right now. No asks on you right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dashboard community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants