Skip to content
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

Make custom ApplicationName work for hosting and user secrets #52305

Closed
wants to merge 4 commits into from

Conversation

gfoidl
Copy link
Member

@gfoidl gfoidl commented Nov 22, 2023

Make custom ApplicationName work for hosting and user secrets

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Summary of the changes (Less than 80 chars)

Allow configuring a custom ApplicationName w/o crash of the host

Description

For startup assemblies and user secrets the assembly is loaded by IHostEnvironment.ApplicationName.
If that app name isn't the default one, which is constructed from the entry assembly, then those assemblies may not exist and startup crashes.

This change probes if that assembly actually can be loaded, if so use it, otherwise fallback to the entry assembly.

Fixes #52152

Note: in my local setup I'm unable to build the functional tests, they hang. But I'm quite sure this has to do with my local setup, thus I'll let CI decide if it's good or not.

@gfoidl gfoidl requested review from a team and halter73 as code owners November 22, 2023 15:16
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Nov 22, 2023
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 22, 2023
@ghost
Copy link

ghost commented Nov 22, 2023

Thanks for your PR, @gfoidl. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@gfoidl gfoidl added area-hosting Includes Hosting community-contribution Indicates that the PR has been added by a community member and removed community-contribution Indicates that the PR has been added by a community member area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions labels Nov 22, 2023
}
catch
{
return Assembly.GetEntryAssembly();
Copy link
Member Author

Choose a reason for hiding this comment

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

In unit tests this can be testhost, but that's no problem as the user secrets are reigstered as optional, thus if the attribute for user sectes doesn't exist, it's just ignored.

Besides that is there any other reliable way to probe for existance of an assembly?

@ilmax
Copy link
Contributor

ilmax commented Nov 22, 2023

Hey @gfoidl are you also planning to backport this to net8.0?
We use custom application names and it breaks with net8.0, also integration tests were never able to run due to this because AFAIK the application name was used to load the assembly

@davidfowl
Copy link
Member

We should avoid first chance exceptions in the default code path. I haven't been following this issue closely but why do we need this change?

@ilmax
Copy link
Contributor

ilmax commented Nov 22, 2023

I agree, unfortunately we already use a setting called ApplicationName for other reason and currently (on net6.0) we don't have any issue. AFAIK that config value also binds to the IHostEnvironment.ApplicationName (at least to my understanding) and that's used to bootstrap the application.

While trying to update to net8.0 this part broke with assembly not found error, to overcome the issue I had to use a different configuration name (e.g. ApplicationName -> AppName) in order to successfully start the web app.

I can do that of course but I have quite few services to update and it's working fine in net6.0 so I was wondering if we can keep it this way also with net8.0

@gfoidl
Copy link
Member Author

gfoidl commented Nov 22, 2023

@davidfowl just try a default app (from the template) and change the app name, like

WebApplicationBuilder builder = WebApplication.CreateBuilder(new WebApplicationOptions
{
    Args            = args,
    ApplicationName = "MySuperApp has a name that doesn't match the name of the entry assembly"
});

then host startup will crash.

As the app name can be set, and it's documented that it can be changed, it's very strange that it won't work with that anymore.

Unfortunately I don't have any better solution than probing at my hands. Other solution would have more impact, like additional configuration to explicitly set the startup assembly when the app name is change, which would deserve a nice exception message, etc. (and I don't have very much time ATM to evaluate more ways, but I'm very open for suggestions)

BTW: that probing with the try-catch is done once at startup, so not on a hot-path.

@davidfowl
Copy link
Member

The name has to be an assembly name. Are you saying it stops the app from starting in .NET 8 but didn't in .NET 6?

@captainsafia
Copy link
Member

The name has to be an assembly name. Are you saying it stops the app from starting in .NET 8 but didn't in .NET 6?

Yes, we introduced this break when we added the SlimBuilder APIs in .NET 8. We started using the ApplicationName to do assembly lookups in a way that we weren't before.

Also, I don't think we say anything in the documentation or through runtime exceptions that tells people their ApplicationName has to be an assembly name. It's just assumed...

@gfoidl
Copy link
Member Author

gfoidl commented Nov 22, 2023

The name has to be an assembly name

Ah, new information for me 😉

So I see two solutions:

  • either make it working with any name (as IMO ApplicationName implies)
  • or document that it must be an actual assembly name

I'm fine either way, as there are workarounds available, though I lean a little bit towards the first bullet, as otherwise ApplicationName should actually be HostingStartupAssemblyName or that like.


As background why I'm using a different ApplicationName:
There is one server-project that runs on multiple servers. Via configuration individual features are turned on/off, depending on the actual server -- so no plain scaling, rather specialization depending on config.
To distinguish the services (e.g. for observability) I use the ApplicationName as it's handy and exposed on IHostEnvironment, so no custom type needed.

@gfoidl
Copy link
Member Author

gfoidl commented Nov 22, 2023

The fix in this PR isn't complete, there's Microsoft.AspNetCore.Mvc.ApplicationParts.ApplicationPartManager.GetApplicationPartAssemblies missing (as CI unveils).

I'm waiting for a decision on whether it's a documentation issue or the one I'm trying to fix here.

@captainsafia
Copy link
Member

either make it working with any name (as IMO ApplicationName implies)

I'm in favor of this approach. We don't do anything to document or enforce the requirement that ApplicationName must be an assembly name and the code should behave defensively anyways....


try
{
_ = Assembly.Load(new AssemblyName(environment.ApplicationName));
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'm not really happy with that solution.

What if we extend IWebHostEnvironment with a property for the HostingStartupAssemblyName?
Then these checks just need to be done once, the property-value can be used whereever needed.

Also the logic for determing the correct assembly name is only in one place.

@davidfowl
Copy link
Member

davidfowl commented Nov 22, 2023

Also, I don't think we say anything in the documentation or through runtime exceptions that tells people their ApplicationName has to be an assembly name. It's just assumed...

Yes, its assumed and it has to be an assembly name, it breaks more than just the host it also breaks other parts of the framework. MVC also blows up.

I'm not sure we should "fix" this. At least not in this way. Fixing this properly means introducing another property than isn't used in any of these code paths.

I'm in favor of that because it would actually work everywhere.

@gfoidl
Copy link
Member Author

gfoidl commented Nov 22, 2023

@davidfowl so something like outlined in #52305 (comment)

@gfoidl
Copy link
Member Author

gfoidl commented Nov 23, 2023

ApplicationName is defined in Microsoft.Extensions.Hosting.IHostEnvironment (which lives in runtime-repo).
There the docs also don't mention anything about the requirementes of that name.
So we can make that property to allow arbitrary "application names".

Then introduce a new propert on IHostEnvironment like

public interface Microsoft.Extensions.Hosting.IHostEnvironment
{
    string EnvironmentName { get; set; }
    string ApplicationName { get; set; }
+   string HostingStartupAssemblyName { get; set; }
    string ContentRootPath { get; set; }
    IFileProvider ContentRootFileProvider { get; set; }
}

which clearly indicates what name it should be.
All the usages of ApplicationName within runtime / aspnetcore must be re-visited and changed to use the new property.
I grepped around a bit and this should be doable.


A different approach is to keep ApplicationName as is (now) and document that it must be the name of an assembly (also include that in error messages). Then introduce a new property on IHostEnvironment for the arbitrary app name.
But what should the name of that property be w/o causing confusion? As ApplicationName alludes the name of the app, not an assembly.


So far I've written about IHostEnvironment, because ApplicationName is defined there, and the host in runtime also has the same problem (e.g. for user secrets loading). Thus I think that change should be done over there or should that new property be added to IWebHostEnvironment instead (where it IMO doesn't belong)?

@davidfowl
Copy link
Member

davidfowl commented Nov 23, 2023

There the docs also don't mention anything about the requirementes of that name.
So we can make that property to allow arbitrary "application names".

I know we keep saying that but we’ve been using it this way since the beginning and to fix it properly we need to add a new property.

I agree this change should be done in the runtime.

@gfoidl
Copy link
Member Author

gfoidl commented Nov 23, 2023

Fine. I'll create an API proposal over there (but not before next week).

@gfoidl gfoidl closed this Nov 23, 2023
@gfoidl gfoidl deleted the app-name branch November 23, 2023 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-hosting Includes Hosting 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.

WebApplication.CreateBuilder with non-default ApplicationName doesn't load user secrets
4 participants