aspnetcore icon indicating copy to clipboard operation
aspnetcore copied to clipboard

Improve logging and error handling for IIS ASP.NET Core module shadow copy

Open absurdlylongusername opened this issue 4 months ago • 5 comments

Improve logging and error handling for IIS ASP.NET Core module shadow copy

  • [x] You've read the Contributor Guide and Code of Conduct.
  • [x] You've included unit or integration tests for your change, where applicable.
  • [x] You've included inline docs for your change, where applicable.
  • [x] 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) Improve shadow copy logging and error handling

Description

Goal of this PR: if shadow copy fails due to lack of permissions to the shadow copy directory

  1. The application will return a useful 500 error instead of an empty
  2. Useful logs will be printed to debug log output
  3. Useful information will be logged in event viewer

Laundry list of changes

  • In CreateApplication, log exception message for all types of exception and not just ConfigurationLoadException
  • When exception is thrown in CreateApplication, return useful HTTP 500 with exception message instead of an empty one
  • I moved the code that checks if shadow copy is enabled up from HandleShadowCopy to TryCreateApplication
  • TryCreateApplication will now return failed HRESULT if shadow copy fails
  • HandleShadowCopy now takes an ErrorContext that will provide error information when it fails
  • Added error handling in Environment::CopyToDirectory for when it fails; it also now throws exception
  • Added two tests in ShadowCopyTests that test shadow copy failure behaviour when there's no permissions to the shadow copy directory, and when there's no write permissions
  • Tiny refactor in IISDeployer.cs to use TryGetValue
  • Some refactoring in ShadowCopyTests of variable renaming and adding Arrange Act Assert comments for clarity (can undo this if it's not wanted)

Fixes #62148

Bare in mind I am well aware this is not production ready. However, I am not a C++ enjoyer (yet), so I am opening this PR with a reasonable working solution to get it reviewed instead of trying to perfect it myself.

I have left some TODOs in here which are mostly questions for the people who are going to review it. Once they are addressed I will remove them.

The code displays a new 500 error about shadow copying: what's the protocol on sub error codes for these? Is reusing existing ones okay or should a new one be made? I presume existing documentation elsewhere would also need to be updated?

absurdlylongusername avatar Jul 30 '25 06:07 absurdlylongusername

@dotnet-policy-service agree

absurdlylongusername avatar Jul 30 '25 07:07 absurdlylongusername

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime. To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

@dotnet-policy-service agree

absurdlylongusername avatar Aug 12 '25 06:08 absurdlylongusername

@halter73 @BrennanConroy @JamesNK Hi all, gentle reminder that this PR is open for review

absurdlylongusername avatar Aug 14 '25 06:08 absurdlylongusername

@halter73 @BrennanConroy @JamesNK Hi all, another reminder; hoping I can get a review by the end of the year

absurdlylongusername avatar Nov 25 '25 12:11 absurdlylongusername