Add a test confirming that X509Certificate is trimmed #48295

amcasey · 2023-05-18T01:25:48Z

Add a test confirming that X509Certificate is trimmed

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.

Description

Add a test confirming that X509Certificate is trimmed when using CreateSlimBuilder.

amcasey · 2023-05-18T01:32:46Z

I hit a couple of surprises while authoring this:

I get a warning about JSON serialization. I think this indicates that the feature switch for using reflection by default is enabled, which is not the case in "real" native AOT scenarios.

src\Http\Http.Extensions\src\JsonOptions.cs(39): Trim analysis warning IL2026: Microsoft.AspNetCore.Http.Json.JsonOptions.CreateDefaultTypeResolver(): Using member 'System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver.DefaultJsonTypeInfoResolver()' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. JSON serialization and deserialization might require types that cannot be statically a nalyzed. Use the overload that takes a JsonTypeInfo or JsonSerializerContext, or make sure all of the required types are preserved.
src\Http\Http.Extensions\src\JsonOptions.cs(39): AOT analysis warning IL3050: Microsoft.AspNetCore.Http.Json.JsonOptions.CreateDefaultTypeResolver(): Using member 'System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver.DefaultJsonTypeInfoResolver()' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. JSON serialization and deserialization might require types that cannot be statically analyzed and might need runtime code generation. Use System.Text.Json source generation for native AOT applications.

I was originally planning to sketch out a more complete app, but I got errors when I tried to use MapGet

artifacts\bin\trimmingTests\projects\Microsoft.AspNetCore.NativeAotTests\SlimBuilderDoesNotDependOnX509Test\SlimBuilderDoesNotDependOnX509Test.cs(12,1): error IL3050: Using member 'Microsoft.AspNetCore.Builder.EndpointRouteBuilderExtensions.MapGet(IEndpointRouteBuilder, String, Delegate)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. This API may perform reflection on the supplied delegate and its parameters. These types may require generated code and aren't compatible with native AOT applications.
artifacts\bin\trimmingTests\projects\Microsoft.AspNetCore.NativeAotTests\SlimBuilderDoesNotDependOnX509Test\SlimBuilderDoesNotDependOnX509Test.cs(12,1): error IL2026: Using member 'Microsoft.AspNetCore.Builder.EndpointRouteBuilderExtensions.MapGet(IEndpointRouteBuilder, String, Delegate)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. This API may perform reflection on the supplied delegate and its parameters. These types may be trimmed if not directly referenced.
artifacts\bin\trimmingTests\projects\Microsoft.AspNetCore.NativeAotTests\SlimBuilderDoesNotDependOnX509Test\SlimBuilderDoesNotDependOnX509Test.cs(12,1): error IL3050: Using member 'Microsoft.AspNetCore.Builder.EndpointRouteBuilderExtensions.MapGet(IEndpointRouteBuilder, String, Delegate)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. This API may perform reflection on the supplied delegate and its parameters. These types may require generated code and aren't compatible with native AOT applications.
artifacts\bin\trimmingTests\projects\Microsoft.AspNetCore.NativeAotTests\SlimBuilderDoesNotDependOnX509Test\SlimBuilderDoesNotDependOnX509Test.cs(12,1): error IL2026: Using member 'Microsoft.AspNetCore.Builder.EndpointRouteBuilderExtensions.MapGet(IEndpointRouteBuilder, String, Delegate)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. This API may perform reflection on the supplied delegate and its parameters. These types may be trimmed if not directly referenced.

@eerhardt or @captainsafia might have thoughts on how the test environment differs from publishing the AOT template.

Edit: @mitchdenny figured it out - at least for individual test cases. See f0e720e.

...efaultBuilder/test/Microsoft.AspNetCore.NativeAotTests/SlimBuilderDoesNotDependOnX509Test.cs

src/DefaultBuilder/test/Microsoft.AspNetCore.TrimmingTests/UseHttpsDoesDependOnX509Test.cs

JamesNK

LGTM. I have yet to write one of these, so I don't know all the ins and outs. Wait for @eerhardt to double-check its correctness.

amcasey · 2023-05-18T16:20:47Z

The test failure is clearly me, but it's not obvious to me what to do about it. I'm guessing it has something to do with the way we're depending on aspnetcore via source, rather than binaries.

CSC : error CS0006: Metadata file '/mnt/vss/_work/1/s/artifacts/obj/Microsoft.AspNetCore.DataProtection.Extensions/Release/net8.0/ref/Microsoft.AspNetCore.DataProtection.Extensions.dll' could not be found [/mnt/vss/_work/1/s/artifacts/bin/trimmingTests/projects/Microsoft.AspNetCore.TrimmingTests/SlimBuilderDoesNotDependOnX509Test/project.csproj]

Edit: The CI command fails locally, so it should be debuggable.

eerhardt · 2023-05-18T19:14:31Z

The test failure is clearly me, but it's not obvious to me what to do about it. I'm guessing it has something to do with the way we're depending on aspnetcore via source, rather than binaries.

So this is a wart in the experience that I haven't had a chance to address yet.

We need these tests to be built after the ASP.NET libraries are built. We do this with:

aspnetcore/eng/testing/linker/trimmingTests.props

Lines 16 to 17 in c5a5b3b

    
           <!-- Enforce build order. Ensure the Shared Fx is built before generating and publishing the test apps. --> 
        
           <RequiresDelayedBuild>true</RequiresDelayedBuild>

However, the RequiresDelayedBuild has an MSBuild bug in it that doesn't automatically update the generated file that depends on this property:

aspnetcore/eng/RequiresDelayedBuildProjects.props

Lines 1 to 21 in c5a5b3b

    
           <!-- 
        
             This file is automatically generated. Run `./eng/scripts/GenerateProjectList.ps1` to update. 
        
             This file contains a list of projects that must be restored etc. after App.Ref and App.Runtime are fully built. 
        
             This file is generated using <RequiresDelayedBuild/> properties. Content may overlap ProjectReferences.csproj 
        
             but that is not required (projects that are not project reference providers are also supported). 
        
           --> 
        
           <Project> 
        
             <ItemGroup> 
        
               <RequiresDelayedBuild Include="$(RepoRoot)src\DataProtection\DataProtection\test\Microsoft.AspNetCore.DataProtection.TrimmingTests\Microsoft.AspNetCore.DataProtection.TrimmingTests.proj" /> 
        
               <RequiresDelayedBuild Include="$(RepoRoot)src\DefaultBuilder\test\Microsoft.AspNetCore.NativeAotTests\Microsoft.AspNetCore.NativeAotTests.proj" /> 
        
               <RequiresDelayedBuild Include="$(RepoRoot)src\Grpc\JsonTranscoding\perf\Microsoft.AspNetCore.Grpc.Microbenchmarks\Microsoft.AspNetCore.Grpc.Microbenchmarks.csproj" /> 
        
               <RequiresDelayedBuild Include="$(RepoRoot)src\Grpc\JsonTranscoding\src\Microsoft.AspNetCore.Grpc.JsonTranscoding\Microsoft.AspNetCore.Grpc.JsonTranscoding.csproj" /> 
        
               <RequiresDelayedBuild Include="$(RepoRoot)src\Grpc\JsonTranscoding\src\Microsoft.AspNetCore.Grpc.Swagger\Microsoft.AspNetCore.Grpc.Swagger.csproj" /> 
        
               <RequiresDelayedBuild Include="$(RepoRoot)src\Grpc\JsonTranscoding\test\Microsoft.AspNetCore.Grpc.JsonTranscoding.IntegrationTests\Microsoft.AspNetCore.Grpc.JsonTranscoding.IntegrationTests.csproj" /> 
        
               <RequiresDelayedBuild Include="$(RepoRoot)src\Grpc\JsonTranscoding\test\Microsoft.AspNetCore.Grpc.JsonTranscoding.Tests\Microsoft.AspNetCore.Grpc.JsonTranscoding.Tests.csproj" /> 
        
               <RequiresDelayedBuild Include="$(RepoRoot)src\Grpc\JsonTranscoding\test\Microsoft.AspNetCore.Grpc.Swagger.Tests\Microsoft.AspNetCore.Grpc.Swagger.Tests.csproj" /> 
        
               <RequiresDelayedBuild Include="$(RepoRoot)src\Grpc\JsonTranscoding\test\testassets\IntegrationTestsWebsite\IntegrationTestsWebsite.csproj" /> 
        
               <RequiresDelayedBuild Include="$(RepoRoot)src\Grpc\JsonTranscoding\test\testassets\Sandbox\Sandbox.csproj" /> 
        
               <RequiresDelayedBuild Include="$(RepoRoot)src\ProjectTemplates\test\Templates.Blazor.Tests\Templates.Blazor.Tests.csproj" />

Since RequiresDelayedBuildProjects.props doesn't get updated, your new test project doesn't get built "delayed" and the build breaks.

So to fix this for just your PR, you can add a <RequiresDelayedBuild Include=" line to that file for your new test project.

...faultBuilder/test/Microsoft.AspNetCore.TrimmingTests/Microsoft.AspNetCore.TrimmingTests.proj

...DefaultBuilder/test/Microsoft.AspNetCore.TrimmingTests/SlimBuilderDoesNotDependOnX509Test.cs

src/DefaultBuilder/test/Microsoft.AspNetCore.TrimmingTests/X509Utilities.cs

amcasey · 2023-05-18T21:04:31Z

So to fix this for just your PR, you can add a <RequiresDelayedBuild Include=" line to that file for your new test project.

Ah, just like the line for the NativeAotTests project I copied. 😆

Fixed it on my box, hopefully in CI too.

eerhardt

Looks great! Thanks for the good work.

mitchdenny · 2023-05-19T07:17:39Z

@eerhardt I notice this failed on macOS. Is there some way to get more detailed output in these cases?

eerhardt · 2023-05-19T13:49:23Z

Log says:

The Command /Users/runner/work/1/s/artifacts/bin/trimmingTests/projects/Microsoft.AspNetCore.TrimmingTests/SlimBuilderDoesNotDependOnX509Test/bin/Release/net8.0/osx-x64/publish/project returned a non-success exit code: 255.

Why that would happen... I'm not sure.

eerhardt · 2023-05-19T13:50:36Z

Re-running to see if it passes on retry. (disabled auto-merge in case it does pass)

eerhardt · 2023-05-19T14:19:28Z

Log says:

The Command /Users/runner/work/1/s/artifacts/bin/trimmingTests/projects/Microsoft.AspNetCore.TrimmingTests/SlimBuilderDoesNotDependOnX509Test/bin/Release/net8.0/osx-x64/publish/project returned a non-success exit code: 255.

Why that would happen... I'm not sure.

According to https://www.redhat.com/sysadmin/exit-codes-demystified, returning an exit code greater than 256 will subtract 256. So exit code 255 means the app returned -1. There must be something on macOS that is still preserving X509Certificate.

amcasey · 2023-05-19T16:08:43Z

That's odd. I think the mac leg was passing (somehow) before I added the delay-build entry. I just assumed these tests weren't running on mac.

amcasey · 2023-05-19T16:09:49Z

It passed on re-run, so I don't think it can have been about preserving the dependency?

eerhardt · 2023-05-19T19:34:45Z

Maybe let's run it again to see if it passes. If it does, we can merge it and monitor if it fails in CI again. We can always disable it on macOS pretty quickly if it is flakey.

amcasey · 2023-05-19T21:08:19Z

Oh, I didn't want to create noise, but I have a draft PR here running it repeatedly. I believe it's failing. 🙃

Edit: It looks like windows passed but mac and linux failed. I can't tell if order is important.
Edit 2: AzDO shows failure output for the windows run.

amcasey · 2023-05-19T23:15:45Z

The tests pass when I revert back to my wacky way of looking up the types (i.e. not using Type.GetType).

...when using `CreateSlimBuilder`.

...since we're testing trimming behavior and add a test in the other direction (i.e. `UseHttps` does cause `X509Certificate` to be preserved).

amcasey · 2023-05-22T21:29:44Z

Force push is a trivial rebase on latest main. The test now fails locally as well (Eric found https://github.com/dotnet/aspnetcore/pull/48293/files#diff-946dd406190b49f4dd925f5a414d3f689d966b4c8044344ebb76f89952be5f9bR79). Fix pending.

amcasey · 2023-05-22T23:31:21Z

This CI pass is legit (i.e. zero errors). 😄

amcasey · 2023-05-23T00:13:58Z

I'm going to call this good and look into the build status issue separately.

eerhardt · 2023-05-23T21:46:18Z

src/DefaultBuilder/test/Microsoft.AspNetCore.TrimmingTests/X509Utilities.cs

+            // We're checking for members, rather than just the presence of the type,
+            // because Debugger Display types may reference it without actually
+            // causing a meaningful binary size increase.
+            return certificateType is not null && GetMembers(certificateType).Any();


Another approach we could take here is to disable the feature switch System.Diagnostics.Debugger.IsSupported (DisabledFeatureSwitches in the .proj file) and ensure this type is fully trimmed. This will get us closer to what AOT does, because that feature switch is disabled by default for PublishAot.

https://github.com/dotnet/runtime/blob/ce689d9189d549e249f4bb5e3d9aeef322464bfb/src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets#L256-L257

I thought about that but worried it would hide problems. I could add a separate test for that flag, if you think it's worthwhile.

but worried it would hide problems

What kind of problems?

This mode would more closely align with the Native AOT behavior. So it seems the more preferrable to me. The fact that we are rooting types for Debugging published apps is less desirable to verify (IMO).

amcasey requested review from JamesNK and eerhardt May 18, 2023 01:25

amcasey requested review from Tratcher and halter73 as code owners May 18, 2023 01:25

dotnet-issue-labeler bot added the needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically label May 18, 2023

ghost added the area-hosting Includes Hosting label May 18, 2023

JamesNK reviewed May 18, 2023

View reviewed changes

...efaultBuilder/test/Microsoft.AspNetCore.NativeAotTests/SlimBuilderDoesNotDependOnX509Test.cs Outdated Show resolved Hide resolved

JamesNK reviewed May 18, 2023

View reviewed changes

src/DefaultBuilder/test/Microsoft.AspNetCore.TrimmingTests/UseHttpsDoesDependOnX509Test.cs Outdated Show resolved Hide resolved

JamesNK approved these changes May 18, 2023

View reviewed changes

eerhardt reviewed May 18, 2023

View reviewed changes

amcasey requested review from a team and wtgodbe as code owners May 18, 2023 21:07

eerhardt approved these changes May 19, 2023

View reviewed changes

amcasey enabled auto-merge (squash) May 19, 2023 03:44

eerhardt disabled auto-merge May 19, 2023 13:49

amcasey mentioned this pull request May 19, 2023

Add some notes on testing trimming #48333

Merged

mkArtakMSFT removed the needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically label May 22, 2023

amcasey added 9 commits May 22, 2023 12:10

Add a test confirming that X509Certificate is trimmed

65d593d

...when using `CreateSlimBuilder`.

Disable IsReflectionEnabledByDefault to eliminate warning

5fc85a2

Switch to trimming tests

7b3f2ce

...since we're testing trimming behavior and add a test in the other direction (i.e. `UseHttps` does cause `X509Certificate` to be preserved).

Hack around the need for a certificate

279f596

Mitigate missing dev cert in a way that doesn't invalidate the test

338644e

Delay-build trimming test projects

7fd977c

Add copyright headers

9b1cf95

Use negative exit codes in error scenarios

7d68cae

Simplify type lookup

20c245a

amcasey force-pushed the x509Test branch from d118f30 to 20c245a Compare May 22, 2023 21:28

Allow X509Certificate to be preserved if it has no members

707b683

amcasey merged commit 5026817 into dotnet:main May 23, 2023

amcasey deleted the x509Test branch May 23, 2023 00:14

ghost added this to the 8.0-preview5 milestone May 23, 2023

eerhardt reviewed May 23, 2023

View reviewed changes

amcasey mentioned this pull request May 24, 2023

Disable debugger support in trimming tests #48399

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a test confirming that X509Certificate is trimmed #48295

Add a test confirming that X509Certificate is trimmed #48295

amcasey commented May 18, 2023

amcasey commented May 18, 2023 •

edited

Loading

JamesNK left a comment •

edited

Loading

amcasey commented May 18, 2023 •

edited

Loading

eerhardt commented May 18, 2023

amcasey commented May 18, 2023

eerhardt left a comment

mitchdenny commented May 19, 2023

eerhardt commented May 19, 2023

eerhardt commented May 19, 2023

eerhardt commented May 19, 2023

amcasey commented May 19, 2023

amcasey commented May 19, 2023

eerhardt commented May 19, 2023

amcasey commented May 19, 2023 •

edited

Loading

amcasey commented May 19, 2023

amcasey commented May 22, 2023

amcasey commented May 22, 2023

amcasey commented May 23, 2023

eerhardt May 23, 2023

amcasey May 23, 2023

eerhardt May 23, 2023

Add a test confirming that X509Certificate is trimmed #48295

Add a test confirming that X509Certificate is trimmed #48295

Conversation

amcasey commented May 18, 2023

Add a test confirming that X509Certificate is trimmed

Description

amcasey commented May 18, 2023 • edited Loading

JamesNK left a comment • edited Loading

Choose a reason for hiding this comment

amcasey commented May 18, 2023 • edited Loading

eerhardt commented May 18, 2023

amcasey commented May 18, 2023

eerhardt left a comment

Choose a reason for hiding this comment

mitchdenny commented May 19, 2023

eerhardt commented May 19, 2023

eerhardt commented May 19, 2023

eerhardt commented May 19, 2023

amcasey commented May 19, 2023

amcasey commented May 19, 2023

eerhardt commented May 19, 2023

amcasey commented May 19, 2023 • edited Loading

amcasey commented May 19, 2023

amcasey commented May 22, 2023

amcasey commented May 22, 2023

amcasey commented May 23, 2023

eerhardt May 23, 2023

Choose a reason for hiding this comment

amcasey May 23, 2023

Choose a reason for hiding this comment

eerhardt May 23, 2023

Choose a reason for hiding this comment

amcasey commented May 18, 2023 •

edited

Loading

JamesNK left a comment •

edited

Loading

amcasey commented May 18, 2023 •

edited

Loading

amcasey commented May 19, 2023 •

edited

Loading