-
Notifications
You must be signed in to change notification settings - Fork 462
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
Conversation
edge-hub/src/Microsoft.Azure.Devices.Edge.Hub.Service/Hosting.cs
Outdated
Show resolved
Hide resolved
edge-hub/src/Microsoft.Azure.Devices.Edge.Hub.Service/Startup.cs
Outdated
Show resolved
Hide resolved
edge-hub/test/Microsoft.Azure.Devices.Edge.Hub.Http.Test/DefaultHttpResponse.cs
Show resolved
Hide resolved
edge-modules/SimulatedTemperatureSensor/docker/windows/arm32v7/base/Dockerfile
Outdated
Show resolved
Hide resolved
...crosoft.Azure.Devices.Edge.Util/edged/version_2018_06_28/generatedCode/HttpWorkloadClient.cs
Outdated
Show resolved
Hide resolved
...crosoft.Azure.Devices.Edge.Util/edged/version_2019_01_30/generatedCode/HttpWorkloadClient.cs
Outdated
Show resolved
Hide resolved
...til/test/Microsoft.Azure.Devices.Edge.Util.Test/workloadtestserver/Controllers/Controller.cs
Show resolved
Hide resolved
...test/Microsoft.Azure.Devices.Edge.Util.Test/workloadtestserver/WorkloadTestImplementation.cs
Show resolved
Hide resolved
There was a problem hiding this 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!
There was a problem hiding this 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.
There was a problem hiding this 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
{ | ||
if (clientCertAuthEnabled) | ||
{ | ||
/* Certificate authorization can be enabled in edgeHub, however direct methods will hit this logic and not need certificates. |
There was a problem hiding this comment.
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.
9ec1e38
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reapprove
Same PR as here: #2896 The above PR wouldn't merge due to too many commits. Reopening a new one.
This PR had issues with github merging. Closing as we merged duplicate #2996 |
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:
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:
Relevant files:
CI
Contains changes similar to the checkin build.
Changes:
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:
Docker Images
Changes contain updating dotnet base image to 3.1.4 and refactoring our base image naming.
Changes:
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:
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