diff --git a/iot-e2e-tests/common/src/test/java/tests/integration/com/microsoft/azure/sdk/iot/iothub/connection/ConnectionTests.java b/iot-e2e-tests/common/src/test/java/tests/integration/com/microsoft/azure/sdk/iot/iothub/connection/ConnectionTests.java index 11e69f6909..60ce45ccac 100644 --- a/iot-e2e-tests/common/src/test/java/tests/integration/com/microsoft/azure/sdk/iot/iothub/connection/ConnectionTests.java +++ b/iot-e2e-tests/common/src/test/java/tests/integration/com/microsoft/azure/sdk/iot/iothub/connection/ConnectionTests.java @@ -113,14 +113,26 @@ public class ConnectionTestInstance // same client twice and, worse, requeue the same identity into the shared pool twice. private TestIdentity identityToDispose; - // Set once setupEccDevice starts creating an identity, and never cleared. Identity classification has to - // outlive eccDeviceIdToDelete: teardown claims and clears that id, so if teardown runs between the device - // being registered and the identity being published, the late disposeIfTeardownAlreadyRan would see an - // identity with no ecc id and hand a self signed identity to the shared x509 pool. - private volatile boolean identityIsEcc; - - // Set once teardown has run. Guarded by lifecycleLock along with the two fields above. - private boolean disposed; + // Set once setupEccDevice starts creating an identity, and cleared only when a new attempt begins. + // Identity classification has to outlive eccDeviceIdToDelete: teardown claims and clears that id, so if + // teardown runs between the device being registered and the identity being published, the late + // disposeIfTeardownAlreadyRan would see an identity with no ecc id and hand a self signed identity to the + // shared x509 pool. + private boolean identityIsEcc; + + // Which setup attempt this instance is currently serving, and how far teardown has run. + // + // RerunFailedTestRule reuses this instance for every attempt at a test, so lifecycle state cannot be a plain + // "teardown has happened" flag. It was, and the result was that once the first attempt's @After had run, + // every later attempt disposed its own freshly acquired client the moment setup published it, and open() + // then failed with "Client was closed while attempting to open the connection". One flaky timeout became a + // guaranteed failure of every remaining attempt. + // + // Each setup takes a generation number, and teardown records the generation it covered. A setup only cleans + // up after itself if teardown has already run for its own generation, so an earlier attempt's teardown can + // no longer reach into a later attempt's. + private long setupGeneration; + private long disposedThrough; private final Object lifecycleLock = new Object(); public AuthenticationType authenticationType; @@ -166,26 +178,37 @@ private void applyProxySettings(ClientOptions.ClientOptionsBuilder optionsBuilde public void setup() throws Exception { + long generation = beginSetup(); + ClientOptions.ClientOptionsBuilder optionsBuilder = ClientOptions.builder(); applyProxySettings(optionsBuilder); + log.info("Acquiring test identity"); + if (clientType == ClientType.DEVICE_CLIENT) { - trackForCleanup(Tools.getTestDevice(iotHubConnectionString, this.protocol, this.authenticationType, false, optionsBuilder)); + trackForCleanup(generation, Tools.getTestDevice(iotHubConnectionString, this.protocol, this.authenticationType, false, optionsBuilder), false); } else if (clientType == ClientType.MODULE_CLIENT) { - trackForCleanup(Tools.getTestModule(iotHubConnectionString, this.protocol, this.authenticationType , false, optionsBuilder)); + trackForCleanup(generation, Tools.getTestModule(iotHubConnectionString, this.protocol, this.authenticationType , false, optionsBuilder), false); } - disposeIfTeardownAlreadyRan(); + log.info("Test identity acquired"); + + disposeIfTeardownAlreadyRan(generation); } public void setupEccDevice() throws Exception { + long generation = beginSetup(); + // Marked before anything is created, so that every identity this method goes on to publish is classified // as ECC even if teardown runs partway through. - this.identityIsEcc = true; + synchronized (lifecycleLock) + { + this.identityIsEcc = true; + } ClientOptions.ClientOptionsBuilder optionsBuilder = ClientOptions.builder(); applyProxySettings(optionsBuilder); @@ -200,12 +223,12 @@ public void setupEccDevice() throws Exception eccDevice.setThumbprint(certificateGenerator.getX509Thumbprint(), certificateGenerator.getX509Thumbprint()); Tools.addDeviceWithRetry(new RegistryClient(iotHubConnectionString), eccDevice); - trackEccDeviceForCleanup(eccDevice.getDeviceId()); + trackEccDeviceForCleanup(generation, eccDevice.getDeviceId()); String deviceConnectionString = Tools.getDeviceConnectionString(iotHubConnectionString, eccDevice); - trackForCleanup(new TestDeviceIdentity( + trackForCleanup(generation, new TestDeviceIdentity( new DeviceClient(deviceConnectionString, testInstance.protocol, optionsBuilder.build()), - eccDevice)); + eccDevice), true); } else if (clientType == ClientType.MODULE_CLIENT) { @@ -215,33 +238,74 @@ else if (clientType == ClientType.MODULE_CLIENT) eccModule.setThumbprint(certificateGenerator.getX509Thumbprint(), certificateGenerator.getX509Thumbprint()); Tools.addDeviceWithRetry(new RegistryClient(iotHubConnectionString), eccDevice); - trackEccDeviceForCleanup(eccDevice.getDeviceId()); + trackEccDeviceForCleanup(generation, eccDevice.getDeviceId()); Tools.addModuleWithRetry(new RegistryClient(iotHubConnectionString), eccModule); String moduleConnectionString = Tools.getDeviceConnectionString(iotHubConnectionString, eccDevice) + ";ModuleId=" + eccModule.getId(); - trackForCleanup(new TestModuleIdentity( + trackForCleanup(generation, new TestModuleIdentity( new ModuleClient(moduleConnectionString, testInstance.protocol, optionsBuilder.build()), eccDevice, - eccModule)); + eccModule), true); } - disposeIfTeardownAlreadyRan(); + disposeIfTeardownAlreadyRan(generation); + } + + /** + * Begin a new setup attempt and take its generation number. + * + *

Reclaims anything a previous attempt left behind first. Normally there is nothing: that attempt's + * {@code @After} already claimed and cleared what it owned. Anything still present is residue from a setup + * abandoned by the timeout, and disposing it here is the last chance to reclaim it.

+ * + * @return The generation number this setup attempt owns + */ + private long beginSetup() + { + dispose(); + + synchronized (lifecycleLock) + { + return ++this.setupGeneration; + } } /** * Hand an identity to teardown, and publish it for the test body to use. * + * @param generation The generation of the setup attempt that produced this identity * @param newIdentity The identity this test just acquired or created + * @param isEcc Whether this identity was created by setupEccDevice */ - private void trackForCleanup(TestIdentity newIdentity) + private void trackForCleanup(long generation, TestIdentity newIdentity, boolean isEcc) { - // Published for the test body. Never cleared, so a thread the JUnit timeout abandoned can keep reading it. - this.identity = newIdentity; - + boolean superseded; synchronized (lifecycleLock) { - this.identityToDispose = newIdentity; + superseded = generation != this.setupGeneration; + + if (!superseded) + { + this.identityToDispose = newIdentity; + + if (isEcc) + { + this.identityIsEcc = true; + } + + // Published for the test body under the same lock, so that what teardown owns and what the test + // body can see change together. Never cleared, so a thread the JUnit timeout abandoned can keep + // reading it. + this.identity = newIdentity; + } + } + + if (superseded) + { + // A setup the timeout abandoned finished after a later attempt had already started. Publishing now + // would give this instance an identity the running attempt is not using, and lose the one it is. + disposeSupersededIdentity(newIdentity, isEcc); } } @@ -249,18 +313,76 @@ private void trackForCleanup(TestIdentity newIdentity) * Hand a freshly registered ECC device to teardown, so it is removed from the registry even if the rest of * setupEccDevice never completes. * + * @param generation The generation of the setup attempt that registered this device * @param deviceId The device id that was just added to the registry */ - private void trackEccDeviceForCleanup(String deviceId) + private void trackEccDeviceForCleanup(long generation, String deviceId) { + boolean superseded; synchronized (lifecycleLock) { - this.eccDeviceIdToDelete = deviceId; + superseded = generation != this.setupGeneration; + + if (!superseded) + { + this.eccDeviceIdToDelete = deviceId; + } + } + + if (superseded) + { + removeEccDevice(deviceId); } } /** - * Dispose anything registered after teardown already ran. + * Close and discard an identity produced by a setup attempt that has already been superseded. + * + * @param supersededIdentity The identity to reclaim + * @param isEcc Whether it was created by setupEccDevice, and so must never be recycled + */ + private void disposeSupersededIdentity(TestIdentity supersededIdentity, boolean isEcc) + { + log.debug("Reclaiming identity {} from a superseded setup attempt", supersededIdentity.getDeviceId()); + + if (supersededIdentity.getClient() != null) + { + supersededIdentity.getClient().close(); + } + + if (isEcc) + { + // Only the client is reclaimed here. setupEccDevice always registers the device and calls + // trackEccDeviceForCleanup before it builds the identity, so the device is already owned by whichever + // path saw it first: trackEccDeviceForCleanup removed it directly if it was superseded at + // registration, and otherwise the dispose that superseded this attempt removed it. Deleting it again + // would just log a not found error over a cleanup that had already worked. + return; + } + + Tools.disposeTestIdentity(supersededIdentity, iotHubConnectionString); + } + + /** + * Remove an ECC device from the registry. These are never recycled: they are self signed with a certificate + * no other test knows about, so returning one to the shared x509 pool would fail a later test. + * + * @param deviceId The device to remove + */ + private void removeEccDevice(String deviceId) + { + try + { + Tools.getRegistyManager(iotHubConnectionString).removeDevice(deviceId); + } + catch (IOException | IotHubException e) + { + log.error("Failed to clean up ECC test device {}", deviceId, e); + } + } + + /** + * Dispose anything registered after teardown already ran for this setup's generation. * *

Every test in this class is bounded by a timeout, the two minute one that {@link IntegrationTest} * applies. JUnit runs the test body on a @@ -269,15 +391,21 @@ private void trackEccDeviceForCleanup(String deviceId) * acquiring its identity. Without this, the identity that setup goes on to produce would have no owner and * would leak, which is precisely the leak this class is trying to stop.

* + *

The generation matters. A rerun reuses this instance, so an unqualified "teardown has run" would still + * be set when the next attempt started, and that attempt would dispose its own client the moment it + * published it.

+ * *

This does not wait for setup, in either direction. Blocking teardown on a setup that is itself hung - * which is how these tests have actually timed out - would stall the rest of the run.

+ * + * @param generation The generation of the setup attempt that is finishing */ - private void disposeIfTeardownAlreadyRan() + private void disposeIfTeardownAlreadyRan(long generation) { boolean teardownAlreadyRan; synchronized (lifecycleLock) { - teardownAlreadyRan = this.disposed; + teardownAlreadyRan = this.disposedThrough >= generation; } if (teardownAlreadyRan) @@ -296,16 +424,21 @@ public void dispose() { TestIdentity identityToClean; String eccDeviceIdToClean; + boolean wasEcc; synchronized (lifecycleLock) { - this.disposed = true; + // Teardown covers every setup that has begun so far, and nothing later. A setup that starts after + // this takes a higher generation and is unaffected. + this.disposedThrough = this.setupGeneration; identityToClean = this.identityToDispose; eccDeviceIdToClean = this.eccDeviceIdToDelete; + wasEcc = this.identityIsEcc; this.identityToDispose = null; this.eccDeviceIdToDelete = null; + this.identityIsEcc = false; } if (identityToClean != null && identityToClean.getClient() != null) @@ -319,16 +452,9 @@ public void dispose() // the next test that takes an x509 identity from the shared pool, so delete it instead. This runs even // when the identity was never finished being built, because the device is in the registry from the // moment it is registered, whether or not the rest of the setup succeeded. - try - { - Tools.getRegistyManager(iotHubConnectionString).removeDevice(eccDeviceIdToClean); - } - catch (IOException | IotHubException e) - { - log.error("Failed to clean up ECC test device {}", eccDeviceIdToClean, e); - } + removeEccDevice(eccDeviceIdToClean); } - else if (identityToClean != null && this.identityIsEcc) + else if (identityToClean != null && wasEcc) { // An earlier dispose already claimed and deleted the device id, and this identity was published after // that. The device is gone from the registry, so there is nothing left to delete, but it must still @@ -450,7 +576,12 @@ public void CanOpenConnection() throws Exception InternalClient client = testInstance.identity.getClient(); logConnectionStatusChanges(client); + + // Bracketing the open so a timeout can be attributed. Silence after "Acquiring test identity" and before this + // line means setup was stuck getting an identity; silence after this line means the connect itself stalled. + log.info("Opening client"); client.open(true); + log.info("Client opened"); // deviceClient.open() is a no-op on HTTP, so a message needs to be sent to actually test opening the connection if (testInstance.protocol == HTTPS) @@ -482,7 +613,12 @@ public void CanOpenConnectionWithECCCertificates() throws Exception InternalClient client = testInstance.identity.getClient(); logConnectionStatusChanges(client); + + // Bracketing the open so a timeout can be attributed. Silence after "Acquiring test identity" and before this + // line means setup was stuck getting an identity; silence after this line means the connect itself stalled. + log.info("Opening client"); client.open(true); + log.info("Client opened"); // deviceClient.open() is a no-op on HTTP, so a message needs to be sent to actually test opening the connection if (testInstance.protocol == HTTPS) diff --git a/vsts/E2ETestsSetup/test-resources.bicep b/vsts/E2ETestsSetup/test-resources.bicep index 76b4bb0227..99a7043d11 100644 --- a/vsts/E2ETestsSetup/test-resources.bicep +++ b/vsts/E2ETestsSetup/test-resources.bicep @@ -21,9 +21,6 @@ param BlobServiceName string = 'default' @description('The name of the Container inside the BlobService.') param ContainerName string = 'fileupload' -@description('Flag to indicate if IoT hub should have security solution enabled.') -param EnableIotHubSecuritySolution bool = false - var hubKeysId = resourceId('Microsoft.Devices/IotHubs/Iothubkeys', HubName, 'iothubowner') var dpsKeysId = resourceId('Microsoft.Devices/ProvisioningServices/keys', DpsName, 'provisioningserviceowner') @@ -70,7 +67,7 @@ resource container 'Microsoft.Storage/storageAccounts/blobServices/containers@20 } } -resource iotHub 'Microsoft.Devices/IotHubs@2021-03-03-preview' = { +resource iotHub 'Microsoft.Devices/IotHubs@2023-06-30' = { name: HubName location: resourceGroup().location identity: { @@ -99,7 +96,7 @@ resource iotHub 'Microsoft.Devices/IotHubs@2021-03-03-preview' = { maxDeliveryCount: 100 } } - StorageEndpoints: { + storageEndpoints: { '$default': { sasTtlAsIso8601: 'PT1H' connectionString: 'DefaultEndpointsProtocol=https;AccountName=${storageAccount.name};AccountKey=${listkeys(storageAccount.id, '2019-06-01').keys[0].value}' diff --git a/vsts/E2ETestsSetup/test-resources.json b/vsts/E2ETestsSetup/test-resources.json index 976218a9dc..ceee2a88a3 100644 --- a/vsts/E2ETestsSetup/test-resources.json +++ b/vsts/E2ETestsSetup/test-resources.json @@ -4,18 +4,18 @@ "metadata": { "_generator": { "name": "bicep", - "version": "0.15.31.15270", - "templateHash": "3831266858947033411" + "version": "0.46.1.21595", + "templateHash": "3528572825137063880" } }, "parameters": { "StorageAccountName": { "type": "string", + "minLength": 3, + "maxLength": 24, "metadata": { "description": "The name of the storage account used by the IoT hub." - }, - "maxLength": 24, - "minLength": 3 + } }, "HubName": { "type": "string", @@ -118,7 +118,7 @@ }, { "type": "Microsoft.Devices/IotHubs", - "apiVersion": "2021-03-03-preview", + "apiVersion": "2023-06-30", "name": "[parameters('HubName')]", "location": "[resourceGroup().location]", "identity": { @@ -147,7 +147,7 @@ "maxDeliveryCount": 100 } }, - "StorageEndpoints": { + "storageEndpoints": { "$default": { "sasTtlAsIso8601": "PT1H", "connectionString": "[format('DefaultEndpointsProtocol=https;AccountName={0};AccountKey={1}', parameters('StorageAccountName'), listkeys(resourceId('Microsoft.Storage/storageAccounts', parameters('StorageAccountName')), '2019-06-01').keys[0].value)]",