Fix edgeHub shutdown for renew certificate #6037

ancaantochi · 2022-01-25T00:14:01Z

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

I have read the contribution guidelines.
Title of the pull request is clear and informative.
Description of the pull request includes a concise summary of the enhancement or bug fix.

Testing Guidelines

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 for the PR title and description

edge-hub/core/src/Microsoft.Azure.Devices.Edge.Hub.Mqtt/MqttProtocolHead.cs

edge-hub/core/src/Microsoft.Azure.Devices.Edge.Hub.Amqp/AmqpProtocolHead.cs

...icrosoft.Azure.Devices.Edge.Hub.MqttBrokerAdapter/brokerConnection/MqttBrokerProtocolHead.cs

nimanch · 2022-02-02T23:52:03Z

edge-hub/core/src/Microsoft.Azure.Devices.Edge.Hub.Service/Program.cs

@@ -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));


Is ServerCertificateMaxRenewAfterInMs going to be an exposed environment variable? If so, should we document this?

darobs

Overall, great, a few minor things that would be good to change.

edge-hub/core/src/Microsoft.Azure.Devices.Edge.Hub.Service/Program.cs

varunpuranik · 2022-02-04T14:29:52Z

edge-hub/core/src/Microsoft.Azure.Devices.Edge.Hub.Service/Program.cs

@@ -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);


Nit: ServerCertificateMaxRenewAfterMs? We need to add Max, as the server cert can be renewed faster than that.

varunpuranik · 2022-02-04T14:33:06Z

test/Microsoft.Azure.Devices.Edge.Test/Module.cs

@@ -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()


What does this test do?

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 ***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

ancaantochi force-pushed the ancan/cert-renew branch from 1f745e0 to bc6f1b1 Compare February 1, 2022 18:25

ancaantochi changed the title ~~renew certificate test~~ Fix edgeHub shutdown for renew certificate Feb 1, 2022

ancaantochi added 2 commits February 2, 2022 14:07

fix

6b15264

Add E2E test

acb42c8

ancaantochi force-pushed the ancan/cert-renew branch from 3d2af6a to acb42c8 Compare February 2, 2022 22:08

ancaantochi marked this pull request as ready for review February 2, 2022 22:13

ancaantochi requested review from vipeller, varunpuranik and darobs February 2, 2022 22:15

darobs reviewed Feb 2, 2022

View reviewed changes

edge-hub/core/src/Microsoft.Azure.Devices.Edge.Hub.Mqtt/MqttProtocolHead.cs Show resolved Hide resolved

nimanch reviewed Feb 2, 2022

View reviewed changes

edge-hub/core/src/Microsoft.Azure.Devices.Edge.Hub.Amqp/AmqpProtocolHead.cs Show resolved Hide resolved

nimanch reviewed Feb 2, 2022

View reviewed changes

...icrosoft.Azure.Devices.Edge.Hub.MqttBrokerAdapter/brokerConnection/MqttBrokerProtocolHead.cs Show resolved Hide resolved

nimanch reviewed Feb 2, 2022

View reviewed changes

darobs suggested changes Feb 3, 2022

View reviewed changes

edge-hub/core/src/Microsoft.Azure.Devices.Edge.Hub.Service/Program.cs Outdated Show resolved Hide resolved

Address review comments

6f64bff

darobs approved these changes Feb 3, 2022

View reviewed changes

Merge branch 'master' into ancan/cert-renew

12bf510

varunpuranik reviewed Feb 4, 2022

View reviewed changes

varunpuranik approved these changes Feb 4, 2022

View reviewed changes

Merge branch 'master' into ancan/cert-renew

9f9034b

ancaantochi merged commit fcd4d00 into Azure:master Feb 7, 2022

ancaantochi added a commit to ancaantochi/iotedge that referenced this pull request Feb 7, 2022

Fix edgeHub shutdown for renew certificate (Azure#6037)

5069c43

* fix shutdown and E2E test

ancaantochi added a commit to ancaantochi/iotedge that referenced this pull request Feb 8, 2022

Fix edgeHub shutdown for renew certificate (Azure#6037)

ed663a9

* fix shutdown and E2E test

ancaantochi added a commit to ancaantochi/iotedge that referenced this pull request Feb 8, 2022

Fix edgeHub shutdown for renew certificate (Azure#6037)

89d87d9

* fix shutdown and E2E test

AnlauTriCloud mentioned this pull request Feb 9, 2022

Module client connections fail and do not reconnect #5856

Closed

damonbarry pushed a commit to damonbarry/iotedge that referenced this pull request Apr 15, 2022

Fix edgeHub shutdown for renew certificate (Azure#6037)

5239184

* fix shutdown and E2E test

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix edgeHub shutdown for renew certificate #6037

Fix edgeHub shutdown for renew certificate #6037

ancaantochi commented Jan 25, 2022 •

edited

Loading

nimanch Feb 2, 2022 •

edited

Loading

darobs left a comment •

edited

Loading

varunpuranik Feb 4, 2022

varunpuranik Feb 4, 2022

ancaantochi Feb 4, 2022

Fix edgeHub shutdown for renew certificate #6037

Fix edgeHub shutdown for renew certificate #6037

Conversation

ancaantochi commented Jan 25, 2022 • edited Loading

Azure IoT Edge PR checklist:

General Guidelines and Best Practices

Testing Guidelines

Draft PRs

nimanch Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

darobs left a comment • edited Loading

Choose a reason for hiding this comment

varunpuranik Feb 4, 2022

Choose a reason for hiding this comment

varunpuranik Feb 4, 2022

Choose a reason for hiding this comment

ancaantochi Feb 4, 2022

Choose a reason for hiding this comment

ancaantochi commented Jan 25, 2022 •

edited

Loading

nimanch Feb 2, 2022 •

edited

Loading

darobs left a comment •

edited

Loading