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

Update codebase to dotnet 3.1 #2896

Closed
wants to merge 121 commits into from

Conversation

and-rewsmith
Copy link
Contributor

@and-rewsmith and-rewsmith commented Apr 28, 2020

Overview

This PR upgrades the entire iotedge codebase to dotnet 3.1.4. It is so large because the dotnet version needs to be changed for the product and all build/test pipelines at once. Because of the size, I have provided this write-up in order to summarize the changes and make it easier to digest. I recommend reading through this doc in it's entirety, following links where appropriate, before looking at the "files changed" tab.

Pipelines

Build Images

Contains changes converting from manual install to install template and consolidation of linux arm64 build with other platforms.

Changes:

  • Use install template
  • We previously built linux arm64 using dotnet 3 and other linux platforms on dotnet 2. Because of this, they needed to be separate jobs. We can consolidate them now. The only issue is that functions-sample cannot be built on arm64. I have added a conditional check to image-linux.yaml to get around this.

Relevant files:

Build Images Release

This build currently exists in GUI format in Dev Ops and there is no yaml checked in. I have duplicated this GUI build and made the relevant changes in order to get it building with dotnet 3. This means that the release should go smoothly without any more changes needed.

Ideally everything should be checked in in yaml format though, so I have made a yaml, but need to get access to the codesigning resource. Since the yaml is one of the few changes not needed in this already-large PR, I am going to include it in a separate PR.

Checkin

Contains changes to the yaml and prereq installation scripts for both linux and windows.

Changes:

  • Switched to a template for installing dotnet 3 here and in other pipelines so that when the dotnet version changes (i.e. 3.1.4 -> 3.1.5), we only have to edit in one place.
  • We had a windows install prerequisites script Install-Prerequisites.ps1 that installed dotnet, python, and a windows x86 cli. None of these were needed so I removed the script and all references to it.
  • We had a linux install prerequisites script InstallPrereqs.sh that installed dotnet and libsnappy. I tried removing this, but libsnappy is required for both checkin and CI, so I kept the references to the script and removed the dotnet installation component from it.

Relevant files:

CI

Contains changes similar to the checkin build.

Changes:

  • Switched to a template for installing dotnet 3
  • For linux, swap direct call to install libsnappy with the InstallPrereqs.sh script. No analogous change for windows as no prerequisite installs needed.

Relvant Files:

New E2E

The only change necessary here was to install dotnet 3 on the test agent and update the bin directory to point to netcoreapp3.1.

The state of our current New E2E right now is failing everything for all platforms, making it hard to test if dotnet 3 will have any adverse effects. I am helping Dylan to investigate why this is happening and get a fix.

Product changes

Project Files

We used to specify the target framework in every csproj file. I have changed this to have a reusable props file for projects targeting netcoreapp3.1.

Aspnetcore

Contains changes to csproj files, hosting logic, and some tests.

Changes:

  • The update to aspnetcore makes apps depend on a reference to the aspnetcore framework which is assumed to exist on the device. This obviously affects our csproj files, but has larger implications for our plans to move towards relying on self contained executables. Using these self contained executables directly will eliminate the need to have dotnet installed on each device, which means we can use smaller base images. There is also new tooling in dotnet 3 to publish a trimmed self contained executable which would allow us to further reduce the size of our images. Unfortunately we cannot employ either of these two optimizations with aspnet's framework reference. More context here.
  • Aspnetcore recommends GenericHost over WebHost in aspnetcore. This is because GenericHost can be used better for non-HTTP workloads. We converted to GenericHost for edge-agent's edgelet test server, the edge-util's edgelet test server, TestResultCoordinator, and the TestAnalyzer. EdgeHub needs a larger refactor in order to use this GenericHost. In terms of the benefits of switching, if we can find a way to do what we currently do with mqtt and amqp inside generic host we might see some improvements. I haven't looked into this at all as I think it is outside the scope of this PR.
  • Migrated from UseMvc() to UseRouting() with endpoint routing for all aspnet hosts, including edge-hub. With this new routing, aspnetcore has an issue on dotnet 3 discovering controllers in other projects, even with a project reference. We use this workaround to fix the issue.
  • Aspnetcore uses System.Json instead of Newtonsoft.Json. Thus, we needed to tell our aspnet hosts to use newtonsoft instead. In the future, we should explore if converting to System.Json gives any performance benefits. For now, that is outside the scope of this PR.

Docker Images

Changes contain updating dotnet base image to 3.1.4 and refactoring our base image naming.

Changes:

  • Updated base images to 3.1.4 for nanoserver and alpine
  • In dotnet 3, aspnet is a framework and the dotnet runtime assumes that it exists on the device. Therefore, we need to change base images for hub/agent to mcr.microsoft.com/dotnet/core/aspnet.
  • Due to the aforementioned aspnet dependency, I refactored our custom base images. We used to have azureiotedge-agent-base, azureiotedge-hub-base, azureiotedge-module-base, azureiotedge-module-base-rocksdb, and azureiotedge-module-base-network-controller. All these different base images made it difficult to update. I reduced this complexity at the cost of slightly larger images for some modules that don't need all the utility provided. The new breakdown is: azureiotedge-agent-base, azureiotedge-hub-base, azureiotedge-module-base, azureiotedge-module-base-full. The module base image, azureiotedge-module-base-full, contains rocksdb aspnet, and traffic shaping tooling, and elevated permissions in order to interface with Docker.
  • Due to the direction docker on windows is taking, we have removed windows arm32 from our build / test pipelines. I would like to remove our windows arm32 Dockerfiles if possible to remain consistent with our supported tiers. IMO it is dangerous to have something here remaining untested that can and will be broken in the future.

Relevant files:

Adapter Pattern Migration

The aspnetcore update deprecated connection adapters which we currently use in edgeHub to help set up the https connections. Specifically, the connection adapter will get the connecting client's cert chain and save it in the connection context. This cert chain can then be accessed from the http context for CA validation with the service identity.

Aspnetcore recommends replacing adapters with middleware. Unfortunately, there is no way to replicate what was being done previously so I had to do some investigation. Much of the adapter code is boilerplate, which also exists in the aspnetcore internal https middleware. Aspnetcore exposes this middleware with a call to listenOptions.UseHttps().

Now, with this aspnetcore middleware, we can hand it a certificate validation callback which exposes the client's cert chain. The challenge though is that we need to be able to keep the client cert chain scoped to the connection so that we don't have to manage it's lifecycle ourselves. We tried to do this with custom middleware and a variety of callback setups, but found only one viable solution after examining the internals of aspnetcore middleware.

We can use this chained callback pattern to keep the client's certificate chain scoped to the connection. There is more context in this issue I opened in the aspnetcore repo.

Changes:

Miscellaneous Dotnet 3 Changes

Contains changes to certificate management, the workload clients (only ones that we currently use), and a few minor test fixes.

Changes:

  • Our existing code saves a reference to a connecting client's cert chain without deep copying it. This was fine in dotnet 2 but won't work in dotnet 3. We can fix this by deep copying the certs in amqp, mqtt, and https scenarios.
  • I used nswag to regenerate the workload client code (6/28/18, 1/30/19) with the dotnet 3 option. I did this in an attempt to resolve an issue that was resolved with a separate fix. It is likely that the original workload client worked, so we can revert this change if we think it is best. There are no issues now with the new version though.
  • A few minor test changes were necessary. The LoggingCommandFactoryTest needed a fix as described here Cannot verify calls to ILogger in .NET Core 3.0 Preview 8 (Generic Type Matcher doesn't work with Verify) devlooped/moq#918. Other tests had changes to their aspnet logic as described in the preceding aspnet section.

Build / Test Verification

Checkin Builds - Too many to link. Click last commit test indicator for current status of all checkins
Build Images
Build Images Release
Edgelet Packages
CI
E2E
Longhaul
Stress
New E2E - Currently failing all platforms in master
Connectivity - Still needed

builds/misc/images.yaml Outdated Show resolved Hide resolved
dylanbronson
dylanbronson previously approved these changes May 20, 2020
Copy link
Contributor

@dylanbronson dylanbronson left a comment

Choose a reason for hiding this comment

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

LGTM as long as all our tests are passing with it. Nice work!

dylanbronson
dylanbronson previously approved these changes May 21, 2020
philipktlin
philipktlin previously approved these changes May 21, 2020
Copy link
Contributor

@philipktlin philipktlin left a comment

Choose a reason for hiding this comment

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

LGTM, please run CI and old E2E test before commit. thanks for your great effort to migrate to .Net Core 3.1.

vipeller
vipeller previously approved these changes May 21, 2020
Copy link
Contributor

@vipeller vipeller left a comment

Choose a reason for hiding this comment

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

I've checked only the aspnet realted parts

ancaantochi
ancaantochi previously approved these changes May 21, 2020
{
if (clientCertAuthEnabled)
{
/* Certificate authorization can be enabled in edgeHub, however direct methods will hit this logic and not need certificates.
Copy link
Contributor

Choose a reason for hiding this comment

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

The way I understand it is that direct method logic will hit this but still work without certificates because the RemoteCertificateValidationCallback is set and always returns true, so I think the comment should be where return true is and explaining that.

Copy link
Contributor

@philipktlin philipktlin left a comment

Choose a reason for hiding this comment

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

Reapprove

kodiakhq bot pushed a commit that referenced this pull request May 22, 2020
Same PR as here:
#2896

The above PR wouldn't merge due to too many commits. Reopening a new one.
@and-rewsmith
Copy link
Contributor Author

This PR had issues with github merging. Closing as we merged duplicate #2996

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants