Skip to content

Enable trimming for data protection #41118

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

Merged
merged 2 commits into from
Apr 19, 2022

Conversation

pranavkm
Copy link
Contributor

@pranavkm pranavkm commented Apr 8, 2022

No description provided.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 8, 2022
@pranavkm
Copy link
Contributor Author

pranavkm commented Apr 9, 2022

DataProtection supports newing up types based on config strings e.g. https://github.com/dotnet/aspnetcore/blob/main/src/DataProtection/DataProtection/src/RegistryPolicyResolver.cs#L57-L60. This results in RequiresUnreferencedCode spreading virally in these APIs.

Additionally, there are two call sites where the DataProtection APIs are invoked from interface implementations. The trimmer requires that interfaces are annotated the same way implementations are. In this case we are unable to modify the interface (they are IHostingStartup.RunAsync, IOptionsSetup.Configure), so I used the xml file to suppress the warning from this repo.

@pranavkm pranavkm marked this pull request as ready for review April 10, 2022 13:07
@pranavkm pranavkm requested review from a team, dougbu, wtgodbe and Pilchie as code owners April 10, 2022 13:07
@pranavkm
Copy link
Contributor Author

/cc @JamesNK

@ShreyasJejurkar
Copy link
Contributor

Why bot is adding community contributions to your PRs?

@mkArtakMSFT mkArtakMSFT added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Apr 11, 2022
@mkArtakMSFT
Copy link
Member

Thanks @pranavkm.
@adityamandaleeka can you please get this reviewed? Thanks!

@mkArtakMSFT
Copy link
Member

@HaoK can you please review this? Thanks!

@HaoK
Copy link
Member

HaoK commented Apr 11, 2022

@JamesNK are you handling all of the trimming PRs?

@JamesNK
Copy link
Member

JamesNK commented Apr 12, 2022

I will help. But I don't know the code and the common scenarios its used.

To start with, thoughts on #41118 (comment)? Right now most of the API surface is generating trimmer warnings.


internal static class TrimmerWarning
{
public const string Message = "Data Protection may attempt to load types in a way that cannot be statically analyzed. Ensure all required types are preserved.";
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to improve this error message. "Ensure all required types are preserved." is what they need to do, but it doesn't give any indication about how they do it.

The simplest way to provide more info is to link to a doc. I took a look at https://docs.microsoft.com/en-us/dotnet/core/deploying/trimming/trim-self-contained and there isn't any content about what to do here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could add trimming specific guidance to the DataProtection docs. In theory, the average user wouldn't run in to trimming specific issues with this library - it's only when they start configuring providers via xml configs or the registry that it runs the risk of being trimmed away.

Copy link
Member

Choose a reason for hiding this comment

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

I agree.

This doesn't need to block this PR. Trimming docs issue: #41229

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer area-dataprotection Includes: DataProtection 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.

6 participants