Skip to content

Added initial support for app service as a compute environment #9090

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 18 commits into from
May 5, 2025
Merged

Added initial support for app service as a compute environment #9090

merged 18 commits into from
May 5, 2025

Conversation

@davidfowl
Copy link
Member

@davidfowl davidfowl commented May 3, 2025 •
edited
Loading

Description

This will allow us to start experimenting with cross compute endpoint references (which doesn't work today).

  • The mirrors what we have with azure container apps pretty closely with some limitations.
    • Only support for projects initially (will add containers that can be built next)
    • On support for public http endpoints (we don't do anything with private networking)
    • Single app service plan, which means you can't scale compute independently
    • Secret parameters stored in environment variables are stored in plain (we need to do something about this before preview)
      • The App Service environment might need a default key vault (that can be overridden) for secret parameters.
    • We default to B1 Basic SKU for the app service plan
  • Added tests

Contributes to #5675

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
  • Did you add public API?
    • Yes
  • 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?
    • Not yet (not shipping yet)

@davidfowl davidfowl requested review from eerhardt and mitchdenny May 3, 2025 07:14
@github-actions github-actions bot added the area-integrations Issues pertaining to Aspire Integrations packages label May 3, 2025
@davidfowl
Added initial support for app service as a compute environment
f601838 
This will allow us to start experimenting with cross compute endpoint references (which doesn't work today).
- The mirrors what we have with azure container apps pretty closely with some limitations.
   - Only support for projects in this pass
   - On support for public http endpoints (we don't do anything with private networking)
   - Single app service plan, which means you can't scale compute independently
- Added tests
@davidfowl
Rename AzureContainerAppExecutableExtensions to AzureAppServiceComput…
75754d0 
…eResourceExtensions and remove unused AzureProvisioningResourceExtensions file
@davidfowl davidfowl force-pushed the davidfowl/app-service branch from 8e8dbd6 to 75754d0 Compare May 3, 2025 07:22
davidfowl
davidfowl commented May 3, 2025
View reviewed changes
@davidfowl davidfowl added the azure Issues associated specifically with scenarios tied to using Azure label May 3, 2025
@davidfowl davidfowl mentioned this pull request May 3, 2025
Closed
1 task
davidfowl
davidfowl commented May 3, 2025
View reviewed changes
src/Aspire.Hosting.Azure.AppService/AzureAppServiceWebsiteContext.cs
}
}

private (object, SecretType) ProcessValue(object value, SecretType secretType = SecretType.None, object? parent = null)
Copy link
Member Author

Choose a reason for hiding this comment

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

This logic can be largely reused between aca and app service. Endpoint resolution would need to be extracted somehow.

davidfowl
davidfowl commented May 4, 2025
View reviewed changes
src/Aspire.Hosting.Azure.AppService/AzureAppServiceWebsiteContext.cs
Scheme: endpoint.UriScheme,
Host: HostName,
Port: endpoint.UriScheme == "https" ? 443 : 80,
TargetPort: null, // App Service manages internal port mapping
Copy link
Member Author

@davidfowl davidfowl May 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We should be able to support overriding this. Right now this PR only supports projects and projects with default http/https endpoints.

@mitchdenny
Copy link
Member

This looks good overall. I noticed that it blows up if you try to run it through AZD (wasn't expecting it to work really - but the error was a bit odd):

image

@mitchdenny
Copy link
Member

In terms of cross compute endpoint references it's going to be very interesting to see how we achieve this. Consider a scenario where you have two different compute environments with a circular reference between them.

@davidfowl
Copy link
Member Author

In terms of cross compute endpoint references it's going to be very interesting to see how we achieve this. Consider a scenario where you have two different compute environments with a circular reference between them.

We attempt to tolerate cycles for endpoints for pragmatic reasons (https://gist.github.com/davidfowl/b408af870d4b5b54a28bf18bffa127e1#special-case-endpoints), but we assume that they can be resolved without deploying website itself.

@davidfowl
Copy link
Member Author

This looks good overall. I noticed that it blows up if you try to run it through AZD (wasn't expecting it to work really - but the error was a bit odd):

It should work with azd, it looks like you already have an environment somehow? Update azd and try making a new environment.

eerhardt
eerhardt reviewed May 5, 2025
View reviewed changes
playground/AzureAppService/.aspire/settings.json
@@ -0,0 +1,3 @@
{
"appHostPath": "../AzureAppService.AppHost/AzureAppService.AppHost.csproj"
Copy link
Member

Choose a reason for hiding this comment

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

Should we add .aspire to a .gitignore?

I don't understand why this file is even required.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, it should be checked in

eerhardt
eerhardt reviewed May 5, 2025
View reviewed changes
eerhardt
eerhardt reviewed May 5, 2025
View reviewed changes
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentExtensions.cs
var prefix = infra.AspireResource.Name;
var resource = infra.AspireResource;

// This tells azd to avoid creating infrastructure
Copy link
Member

Choose a reason for hiding this comment

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

Really? That's the contract? If there is a parameter named userPrincipalId?

Copy link
Member Author

@davidfowl davidfowl May 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yep @vhvb1989. I think this is because we also support yaml mode where the app uses none of these types and azd still works.

eerhardt
eerhardt reviewed May 5, 2025
View reviewed changes
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentExtensions.cs
{
containerRegistry = new ContainerRegistryService(Infrastructure.NormalizeBicepIdentifier($"{prefix}_acr"))
{
Sku = new() { Name = ContainerRegistrySkuName.Basic },
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should consolidate this across ACA and here, so there is only 1 place to change the defaults of an ACR.

Copy link
Member Author

Choose a reason for hiding this comment

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

This, endpoint grouping, and env and arg processing.

eerhardt
eerhardt reviewed May 5, 2025
View reviewed changes
eerhardt
eerhardt reviewed May 5, 2025
View reviewed changes
src/Aspire.Hosting.Azure.AppService/Aspire.Hosting.Azure.AppService.csproj Outdated
<PropertyGroup>
<TargetFramework>$(DefaultTargetFramework)</TargetFramework>
<IsPackable>true</IsPackable>
<PackageTags>aspire integration hosting azure</PackageTags>
Copy link
Member

Choose a reason for hiding this comment

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

Do we want any package tags specific for AppService?

eerhardt
eerhardt reviewed May 5, 2025
View reviewed changes
src/Aspire.Hosting.Azure.AppService/Aspire.Hosting.Azure.AppService.csproj
<IsPackable>true</IsPackable>
<PackageTags>aspire integration hosting azure</PackageTags>
<Description>Azure app service resource types for .NET Aspire.</Description>
<PackageIconFullPath>$(SharedDir)Azure_256x.png</PackageIconFullPath>
Copy link
Member

Choose a reason for hiding this comment

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

Does app service have its own icon?

eerhardt
eerhardt reviewed May 5, 2025
View reviewed changes
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentExtensions.cs
builder.AddAzureProvisioning();
builder.Services.Configure<AzureProvisioningOptions>(options => options.SupportsTargetedRoleAssignments = true);

if (builder.ExecutionContext.IsPublishMode)
Copy link
Member

Choose a reason for hiding this comment

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

Is this check necessary? The infrastructure class checks in its hook already.

Copy link
Member Author

Choose a reason for hiding this comment

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

its the same in aca

eerhardt
eerhardt reviewed May 5, 2025
View reviewed changes
src/Aspire.Hosting.Azure.AppService/AzureAppServiceComputeResourceExtensions.cs Outdated
/// for Azure App Service and applies the provided configuration action to the App Service WebSite resource.
/// <example>
/// <code>
/// builder.AddNpmApp("name", "image").PublishAsAzureAppServiceWebsite((infrastructure, app) =>
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't work yet, right? Should this be AddProject?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep!

eerhardt
eerhardt reviewed May 5, 2025
View reviewed changes
src/Aspire.Hosting.Azure.AppService/AzureAppServiceWebsiteContext.cs
var acrClientIdParameter = environmentContext.Environment.ContainerRegistryClientId.AsProvisioningParameter(infra);
var containerImage = AllocateParameter(ResourceExpression.GetContainerImageExpression(Resource));

var webSite = new WebSite("webapp")
Copy link
Member

Choose a reason for hiding this comment

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

(nit) why the hard-coded "webapp" instead of being based on the Resource name?

Copy link
Member Author

@davidfowl davidfowl May 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Does it matter? It's in a module (it's the variable name 😄 )

Copy link
Member

@eerhardt eerhardt May 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It doesn't. Just inconsistent with ACA.

eerhardt
eerhardt reviewed May 5, 2025
View reviewed changes
src/Aspire.Hosting.Azure.AppService/AzureAppServiceWebsiteContext.cs

foreach (var arg in Args)
{
var (val, secretType) = ProcessValue(arg);
Copy link
Member

Choose a reason for hiding this comment

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

Does this code need to inspect secretType?

Copy link
Member Author

Choose a reason for hiding this comment

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

See the PR description. There's no secret store in app service (and secrets also aren't supported in args in ACA). We need to throw here an in app service if secrets are used in args.

We might also need a strategy for App service secrets in general (like an associated keyvault).

eerhardt
eerhardt approved these changes May 5, 2025
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Looks good. I just had a few comments.

@davidfowl davidfowl merged commit 3f8fcc1 into main May 5, 2025
170 checks passed
@davidfowl davidfowl deleted the davidfowl/app-service branch May 5, 2025 17:33
@davidfowl davidfowl mentioned this pull request May 13, 2025
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Jun 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mitchdenny mitchdenny mitchdenny left review comments

@eerhardt eerhardt eerhardt approved these changes

Assignees

No one assigned

Labels

area-integrations Issues pertaining to Aspire Integrations packages azure Issues associated specifically with scenarios tied to using Azure

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@davidfowl @mitchdenny @eerhardt