-
Notifications
You must be signed in to change notification settings - Fork 459
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
Fix edgeHub shutdown for renew certificate #6037
Conversation
1f745e0
to
bc6f1b1
Compare
3d2af6a
to
acb42c8
Compare
edge-hub/core/src/Microsoft.Azure.Devices.Edge.Hub.Mqtt/MqttProtocolHead.cs
Show resolved
Hide resolved
edge-hub/core/src/Microsoft.Azure.Devices.Edge.Hub.Amqp/AmqpProtocolHead.cs
Show resolved
Hide resolved
...icrosoft.Azure.Devices.Edge.Hub.MqttBrokerAdapter/brokerConnection/MqttBrokerProtocolHead.cs
Show resolved
Hide resolved
@@ -122,9 +122,10 @@ static async Task<int> MainAsync(IConfigurationRoot configuration) | |||
TimeSpan shutdownWaitPeriod = TimeSpan.FromSeconds(configuration.GetValue("ShutdownWaitPeriod", DefaultShutdownWaitPeriod)); | |||
(CancellationTokenSource cts, ManualResetEventSlim completed, Option<object> handler) = ShutdownHandler.Init(shutdownWaitPeriod, logger); | |||
|
|||
TimeSpan maxRenewAfter = TimeSpan.FromMilliseconds(configuration.GetValue("ServerCertificateMaxRenewAfterInMs", int.MaxValue)); |
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.
Is ServerCertificateMaxRenewAfterInMs
going to be an exposed environment variable? If so, should we document this?
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.
Overall, great, a few minor things that would be good to change.
edge-hub/core/src/Microsoft.Azure.Devices.Edge.Hub.Service/Program.cs
Outdated
Show resolved
Hide resolved
@@ -122,9 +122,12 @@ static async Task<int> MainAsync(IConfigurationRoot configuration) | |||
TimeSpan shutdownWaitPeriod = TimeSpan.FromSeconds(configuration.GetValue("ShutdownWaitPeriod", DefaultShutdownWaitPeriod)); | |||
(CancellationTokenSource cts, ManualResetEventSlim completed, Option<object> handler) = ShutdownHandler.Init(shutdownWaitPeriod, logger); | |||
|
|||
double renewAfter = configuration.GetValue("ServerCertificateRenewAfterInMs", int.MaxValue); |
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.
Nit: ServerCertificateMaxRenewAfterMs? We need to add Max, as the server cert can be renewed faster than that.
@@ -17,6 +17,44 @@ public class Module : SasManualProvisioningFixture | |||
const string SensorName = "tempSensor"; | |||
const string DefaultSensorImage = "mcr.microsoft.com/azureiotedge-simulated-temperature-sensor:1.0"; | |||
|
|||
[Test] | |||
[Category("CentOsSafe")] | |||
public async Task CertRenew() |
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.
What does this test do?
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.
it sets the renew timestamp at 1 minutes and then it checks that edgeHub was restarted by edgeAgent by checking reported properties restartCount to be 1
* fix shutdown and E2E test
* fix shutdown and E2E test
* fix shutdown and E2E test
* fix shutdown and E2E test ***Please replace this line with your PR description and read PR checklist below*** ## Azure IoT Edge PR checklist: This checklist is used to make sure that common guidelines for a pull request are followed. ### General Guidelines and Best Practices - [x] I have read the [contribution guidelines](https://github.com/azure/iotedge#contributing). - [x] Title of the pull request is clear and informative. - [x] Description of the pull request includes a concise summary of the enhancement or bug fix. ### Testing Guidelines - [x] Pull request includes test coverage for the included changes. - Description of the pull request includes - [x] concise summary of tests added/modified - [ ] local testing done. ### Draft PRs - Open the PR in `Draft` mode if it is: - Work in progress or not intended to be merged. - Encountering multiple pipeline failures and working on fixes. _Note: We use the kodiakhq bot to merge PRs once the necessary checks and approvals are in place. When it merges a PR, kodiakhq converts the PR title to the commit title, PR description to the commit description, and squashes all the commits in the PR to a single commit. The net effect is that entire PR becomes a single commit. Please follow the best practices mentioned [here](https://chris.beams.io/posts/git-commit/#:~:text=The%20seven%20rules%20of%20a%20great%20Git%20commit,what%20and%20why%20vs.%20how%20For%20example%3A%20) for the PR title and description_
* fix shutdown and E2E test ***Please replace this line with your PR description and read PR checklist below*** ## Azure IoT Edge PR checklist: This checklist is used to make sure that common guidelines for a pull request are followed. ### General Guidelines and Best Practices - [x] I have read the [contribution guidelines](https://github.com/azure/iotedge#contributing). - [x] Title of the pull request is clear and informative. - [x] Description of the pull request includes a concise summary of the enhancement or bug fix. ### Testing Guidelines - [x] Pull request includes test coverage for the included changes. - Description of the pull request includes - [ ] concise summary of tests added/modified - [ ] local testing done. ### Draft PRs - Open the PR in `Draft` mode if it is: - Work in progress or not intended to be merged. - Encountering multiple pipeline failures and working on fixes. _Note: We use the kodiakhq bot to merge PRs once the necessary checks and approvals are in place. When it merges a PR, kodiakhq converts the PR title to the commit title, PR description to the commit description, and squashes all the commits in the PR to a single commit. The net effect is that entire PR becomes a single commit. Please follow the best practices mentioned [here](https://chris.beams.io/posts/git-commit/#:~:text=The%20seven%20rules%20of%20a%20great%20Git%20commit,what%20and%20why%20vs.%20how%20For%20example%3A%20) for the PR title and description_
* fix shutdown and E2E test
Before server certificate expires, edgeHub tries to shutdown itself so new server certificate is requested when it gets started again. Changes are to fix the shutdown of MqttProtocolHead which doesn't complete and keep edgeHub running after the shutdown was triggered.
Removed CloseAsync from Dispose because it called Wait and CloseAsync was already called explicitly, it caused issue with MQTT when protocolheads were in using statement and inside CloseAsync was also called, resulted in calling CloseAsync.
Added E2E test and tested manually.
Azure IoT Edge PR checklist:
This checklist is used to make sure that common guidelines for a pull request are followed.
General Guidelines and Best Practices
Testing Guidelines
Draft PRs
Draft
mode if it is:Note: We use the kodiakhq bot to merge PRs once the necessary checks and approvals are in place. When it merges a PR, kodiakhq converts the PR title to the commit title, PR description to the commit description, and squashes all the commits in the PR to a single commit. The net effect is that entire PR becomes a single commit. Please follow the best practices mentioned here for the PR title and description