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

Introduce trim-friendly APIs for controlling HTTPS Configuration support #47567

Closed
amcasey opened this issue Apr 4, 2023 · 3 comments · Fixed by #47454
Closed

Introduce trim-friendly APIs for controlling HTTPS Configuration support #47567

amcasey opened this issue Apr 4, 2023 · 3 comments · Fixed by #47454
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Comments

@amcasey
Copy link
Member

amcasey commented Apr 4, 2023

Background and Motivation

Not being able to trim HTTPS and QUIC support from apps that don't use them causes a very large size increase (~1MB on Windows and ~4MB on Linux). To be able to prove to the trim analyzer that they're not used, we need new code paths that statically do or do not reference the code we want to trim.

Context: #46681.

Proposed API

Please provide the specific public API signature diff that you are proposing. For example:

namespace Microsoft.AspNetCore.Hosting;

public static class WebHostBuilderKestrelExtensions
{
+    /// <summary>
+    /// In <see cref="UseKestrelSlim(IWebHostBuilder)"/> scenarios, it may be necessary to explicitly
+    /// opt in to certain HTTPS functionality.  For example, if <code>ASPNETCORE_URLS</code> includes
+    /// an <code>https://</code> address, <see cref="UseHttpsConfiguration"/> will enable configuration
+    /// of HTTPS on that endpoint.
+    ///
+    /// Has no effect in <see cref="UseKestrel(IWebHostBuilder)"/> scenarios.
+    /// </summary>
+    public static IWebHostBuilder UseHttpsConfiguration(this IWebHostBuilder hostBuilder)

+    /// <summary>
+    /// Specify Kestrel as the server to be used by the web host.
+    /// Includes less automatic functionality <see cref="UseKestrel(IWebHostBuilder)"/> to make trimming more effective
+    /// (e.g. for Native AOT scenarios).  If the host ends up depending on some of the absent functionality, a best-effort
+    /// attempt will be made to enable it on-demand.  Failing that, an instructive error will be reported.
+    /// </summary>
+    public static IWebHostBuilder UseKestrelCore(this IWebHostBuilder hostBuilder)
+    public static IWebHostBuilder UseKestrelCore(this IWebHostBuilder hostBuilder, Action<KestrelServerOptions> options)
+    public static IWebHostBuilder UseKestrelCore(this IWebHostBuilder hostBuilder, Action<WebHostBuilderContext, KestrelServerOptions> configureOptions)
}

Note that the corresponding method for opting in to quic support already exists.

namespace Microsoft.AspNetCore.Hosting;

public static class WebHostBuilderQuicExtensions
{
    public static IWebHostBuilder UseQuic(this IWebHostBuilder hostBuilder)
}

Usage Examples

var builder = WebApplication.CreateSlimBuilder(args); // Calls UseKestrelCore
builder.WebHost.UseHttpsConfiguration();
builder.WebHost.UseQuic();

Alternative Designs

Another approach would be to disable more things to make the behavior clearer.
For example, we could disable all HTTPS functionality and call this API UseHttps (or similar).
We decided against doing this since our goal was not to provide guard rails but to improve trimming.
Instead, we make needing to call UseHttpsConfiguration as rare as possible (by calling it ourselves, when possible).

Risks

The phrase "https configuration" has no intuitive meaning and the comments barely clarify.
This was by-design, since few users should need to call it and they should be prompted to do so by specialized error messages.
There is a risk that, not understanding its purpose, people will call it "just in case", hurting trimming perf.

The "core" in "UseKestrelCore" isn't related to either .NET Core or ASP.NET Core, which could cause some confusion.

@amcasey amcasey added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Apr 4, 2023
@adityamandaleeka adityamandaleeka added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Apr 4, 2023
@ghost
Copy link

ghost commented Apr 4, 2023

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@amcasey
Copy link
Member Author

amcasey commented Apr 4, 2023

PR: #47454

@amcasey amcasey self-assigned this Apr 4, 2023
@BrennanConroy
Copy link
Member

API Review notes:

  • UseHttpsConfiguration is on IWebHostBuilder which is odd since it's for kestrel
    • Name it UseKestrelHttpsConfiguration to make it clearer that it's for kestrel
    • If this was on KestrelOptions it would heavily complicate what user code has to do to get this behavior
    • We're in favor of a simple single line call to enable the functionality
  • Still aren't a fan of Slim suffix
    • Core has been used in other APIs like AddMvcCore, AddSignalRCore, AddRoutingCore, but those are more like the "core" services needed for those areas
    • This is for an opinionated trimmed Kestrel, not the "core" services to use Kestrel
    • Core was voted on by peers
  • Remove other overloads of UseKestrelCore (or whatever we name it)
    • Those APIs are for convenience and this API isn't exactly designed for that

API Approved!

namespace Microsoft.AspNetCore.Hosting;

public static class WebHostBuilderKestrelExtensions
{
+    public static IWebHostBuilder UseKestrelHttpsConfiguration(this IWebHostBuilder hostBuilder);

+    public static IWebHostBuilder UseKestrelCore(this IWebHostBuilder hostBuilder);
}

@BrennanConroy BrennanConroy added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Apr 13, 2023
@dotnet dotnet locked as resolved and limited conversation to collaborators May 14, 2023
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@adityamandaleeka @BrennanConroy @amcasey and others