[RUN-4692] Migrate off deprecated Azure Track 1 SDK to azure-resourcemanager/azure-identity/azure-storage-blob - #84
Open
fdevans wants to merge 2 commits into
Open
[RUN-4692] Migrate off deprecated Azure Track 1 SDK to azure-resourcemanager/azure-identity/azure-storage-blob#84fdevans wants to merge 2 commits into
fdevans wants to merge 2 commits into
Conversation
…re-identity/azure-storage-blob com.microsoft.azure:azure and com.microsoft.azure:azure-storage lost Microsoft support on 31-Mar-2023; the recent okhttp version bump only papered over a CVE in that unsupported dependency tree without removing it, and none of the existing tests exercise real SDK HTTP plumbing. - Auth: ApplicationTokenCredentials -> azure-identity's ClientSecretCredential/ClientCertificateCredential; also fixes a latent bug where the PFX cert path was never actually read from disk. - Compute: AzureManager/AzureNode ported to azure-resourcemanager-compute's ComputeManager API (scoped dependency instead of the umbrella azure-resourcemanager artifact, cutting the flattened plugin lib set from 72 to 51 jars). VM image/size catalogs refreshed to Track 2's current lists (documented as a breaking change in README). - Storage: new shared AzureBlobStorageClientFactory replaces 4x-duplicated client-building boilerplate; ported to azure-storage-blob with atomic upload+metadata. - Removed the now-unnecessary OkHttp/Retrofit1/RxJava1/nimbus CVE constraint blocks in build.gradle, since Track 2's default Netty client drops that vulnerable dependency lineage entirely. - Tests: fixed compile breaks from the SDK type swap, added AzureNodeSpec and AzureBlobStorageClientFactorySpec, and corrected the "invalid key" storage test after confirming empirically that Track 2 throws IllegalArgumentException (not InvalidKeyException) and that the old test's key value was actually valid Base64 under the new SDK.
Contributor
There was a problem hiding this comment.
Pull request overview
Migrates the Rundeck Azure plugin from Azure SDK Track 1 (com.microsoft.azure:*) to supported Track 2 SDKs (azure-resourcemanager-compute, azure-identity, azure-storage-blob), updating compute/auth/storage integrations and refreshing the VM image/size catalogs while aligning tests and documentation with the new SDK behavior.
Changes:
- Replaces Track 1 compute/auth usage with
ComputeManager+ClientSecretCredential/ClientCertificateCredential. - Ports storage integration to
azure-storage-bloband introduces a sharedAzureBlobStorageClientFactoryto centralize client construction. - Updates and adds Spock specs to cover the new SDK types and observed exception behavior.
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/groovy/com/rundeck/plugins/azure/plugin/AzureVmListPluginSpec.groovy | Updates mocks to Track 2 ComputeManager. |
| src/test/groovy/com/rundeck/plugins/azure/plugin/AzureResourceModelSourceSpec.groovy | Updates mocks to Track 2 ComputeManager across scenarios. |
| src/test/groovy/com/rundeck/plugins/azure/plugin/AzureFileStoragePluginSpec.groovy | Updates invalid-key test to Track 2 key parsing/exception behavior. |
| src/test/groovy/com/rundeck/plugins/azure/azure/AzureNodeSpec.groovy | Adds coverage for tag flattening and useAzureTags behavior under Track 2 VM types. |
| src/test/groovy/com/rundeck/plugins/azure/azure/AzureBlobStorageClientFactorySpec.groovy | Adds basic coverage for factory-built container client properties. |
| src/main/groovy/com/rundeck/plugins/azure/util/AzurePluginUtil.groovy | Adds findRegionByLabelOrName helper to preserve Track 1 region lookup behavior. |
| src/main/groovy/com/rundeck/plugins/azure/plugin/files/endpoints/AzureEndpoint.groovy | Ports endpoint handler storage operations from Track 1 blob types to Track 2 blob clients. |
| src/main/groovy/com/rundeck/plugins/azure/plugin/AzureVmListPlugin.groovy | Switches region lookup to AzurePluginUtil.findRegionByLabelOrName with Track 2 Region. |
| src/main/groovy/com/rundeck/plugins/azure/plugin/AzureVMCreatePlugin.groovy | Switches region lookup to AzurePluginUtil.findRegionByLabelOrName with Track 2 Region. |
| src/main/groovy/com/rundeck/plugins/azure/plugin/AzureStorageListStepPlugin.groovy | Ports blob listing to Track 2 hierarchy listing and blob property access. |
| src/main/groovy/com/rundeck/plugins/azure/plugin/AzureStorageDeleteStepPlugin.groovy | Ports blob deletion to Track 2 blob clients. |
| src/main/groovy/com/rundeck/plugins/azure/plugin/AzureFileStoragePlugin.groovy | Ports execution log storage to Track 2 blob uploads/downloads and shared factory usage. |
| src/main/groovy/com/rundeck/plugins/azure/azure/AzureVMSizeType.groovy | Refreshes/adjusts size catalog and adapts to Track 2 VirtualMachineSizeTypes.fromString. |
| src/main/groovy/com/rundeck/plugins/azure/azure/AzureVmImageType.groovy | Refreshes known image catalog and adapts to Track 2 known-image enums. |
| src/main/groovy/com/rundeck/plugins/azure/azure/AzureVm.groovy | Updates Region import to Track 2. |
| src/main/groovy/com/rundeck/plugins/azure/azure/AzureNode.groovy | Updates VM/size imports to Track 2 compute models. |
| src/main/groovy/com/rundeck/plugins/azure/azure/AzureManagerBuilder.groovy | Updates Region import to Track 2. |
| src/main/groovy/com/rundeck/plugins/azure/azure/AzureManager.groovy | Replaces Track 1 auth/compute manager usage with Track 2 identity + ComputeManager. |
| src/main/groovy/com/rundeck/plugins/azure/azure/AzureImage.groovy | Updates ImageReference import to Track 2 compute models. |
| src/main/groovy/com/rundeck/plugins/azure/azure/AzureBlobStorageClientFactory.groovy | Adds shared factory for building Track 2 BlobContainerClient from a connection string. |
| README.md | Documents Track 1 → Track 2 migration and behavioral changes (images/sizes, PFX path handling). |
| gradle/libs.versions.toml | Replaces Track 1 dependency coordinates/versions with Track 2 artifacts. |
| build.gradle | Updates dependencies to Track 2 libraries and removes Track 1-related constraint blocks. |
Comments suppressed due to low confidence (2)
src/main/groovy/com/rundeck/plugins/azure/plugin/files/endpoints/AzureEndpoint.groovy:99
- The
FileOutputStreamused for download isn't closed before opening a read stream and deleting the temp file, risking incomplete writes and file-handle leaks. Use try-with-resources to ensure the download stream is closed.
tempFile = File.createTempFile("azure-transfer", "tmp", null);
blob.downloadStream(new FileOutputStream(tempFile))
src/main/groovy/com/rundeck/plugins/azure/plugin/AzureStorageDeleteStepPlugin.groovy:76
- This rethrows without the original cause/details, which makes it hard to see what was invalid about the configured blob path. Preserve the cause and include the blob path in the message.
try{
blob = container.getBlobClient(path);
}catch(IllegalArgumentException e){
throw new IllegalArgumentException("Error getting the blob");
}
Comment on lines
+50
to
54
| ComputeManager connect(){ | ||
| TokenCredential credential | ||
| AzureProfile profile = new AzureProfile(this.tenantId, this.subscriptionId, AzureEnvironment.AZURE) | ||
|
|
||
| if(this.key!=null){ |
| } | ||
|
|
||
| final String publicIPAddressLeafDNS1 = SdkContext.randomResourceName("pip1", 24) | ||
| final String publicIPAddressLeafDNS1 = new ResourceManagerUtils.InternalRuntimeContext().randomResourceName("pip1", 24) |
Comment on lines
+84
to
86
| BlobClient blob = container.getBlobClient(fileName); | ||
| blob.upload(new FileInputStream(tempFile), tempFile.length()); | ||
|
|
Comment on lines
71
to
75
| try{ | ||
| container = serviceClient.getContainerReference(containerName) | ||
| }catch(URISyntaxException| StorageException e){ | ||
| container = AzureBlobStorageClientFactory.buildContainerClient(storageName, accessKey, containerName, "http") | ||
| }catch(IllegalArgumentException e){ | ||
| throw new IllegalArgumentException("Error getting the container Name"); | ||
| } |
Comment on lines
65
to
69
| try{ | ||
| container = serviceClient.getContainerReference(containerName) | ||
| }catch(URISyntaxException| StorageException e){ | ||
| container = AzureBlobStorageClientFactory.buildContainerClient(storageName, accessKey, containerName, "http") | ||
| }catch(IllegalArgumentException e){ | ||
| throw new IllegalArgumentException("Error getting the container Name"); | ||
| } |
Comment on lines
+18
to
+21
| return new BlobContainerClientBuilder() | ||
| .connectionString(connectionString) | ||
| .containerName(containerName) | ||
| .buildClient() |
- AzureManager.connect(): fix implicit null return when only key-based auth is used (two separate if-blocks with no explicit return meant the method returned the second if's value, null, even though the azure field was set correctly). Also avoid leaving the PFX cert file handle open by reading it fully into memory first. - Replace ResourceManagerUtils.InternalRuntimeContext usage (an internal Azure SDK API not covered by any compatibility guarantee) with a small local random-name generator for the public IP DNS label. - AzureEndpoint: close the upload/download file streams before deleting the temp file instead of leaking the file descriptor. - AzureStorageListStepPlugin/AzureStorageDeleteStepPlugin: preserve the original exception as cause and include the container/account/path in the rethrown message instead of a generic one. - AzureBlobStorageClientFactory: lowercase the container name, matching the safety check AzureFileStoragePlugin already had, since Azure Blob container names must be lowercase.
fdevans
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
com.microsoft.azure:azure(Track 1 fluent mgmt SDK) andcom.microsoft.azure:azure-storage(Microsoft support ended 31-Mar-2023) with the current Azure SDK for Java:azure-resourcemanager-compute+azure-identityfor compute/auth,azure-storage-blobfor storage.azure-keyvault-core(unused in source) is dropped.ApplicationTokenCredentials→azure-identity'sClientSecretCredential/ClientCertificateCredential. Also fixes a latent bug where the PFX certificate "path" field was never actually read from disk (it treated the path string's characters as the cert bytes).AzureManager/AzureNodeported toazure-resourcemanager-compute'sComputeManagerAPI — deliberately scoped to the-computeartifact instead of the umbrellaazure-resourcemanagerpackage, cutting the flattened plugin lib set from 72 to 51 jars by dropping ~15 unrelated service modules (cosmos, redis, sql, servicebus, etc.). VM image/size catalogs refreshed to Track 2's current lists (documented as a breaking change in the README for anyone with saved jobs using retired image names).AzureBlobStorageClientFactoryreplaces 4x-duplicated client-building boilerplate across the storage plugins; ported toazure-storage-blobwith atomic upload+metadata (avoids a window where a blob exists without its Rundeck metadata).build.gradle, since Track 2's default Netty-based HTTP client drops that entire vulnerable dependency lineage.AzureNodeSpecandAzureBlobStorageClientFactorySpec, and corrected the "invalid key" storage test — empirically confirmed Track 2 throwsIllegalArgumentException(notInvalidKeyException) for a malformed key, and that the old test's key value was actually valid Base64 under the new SDK (so it wouldn't have failed synchronously anymore).Test plan
./gradlew compileGroovy/compileTestGroovy— clean compile./gradlew test— full suite (25 tests) passes./gradlew dependencies --configuration pluginLibs— confirmed nocom.microsoft.azure:*/com.microsoft.rest:*artifacts remain in the resolved graph./gradlew clean copyToLib— inspected flattenedbuild/output/lib/for version conflicts (none found)./gradlew jar— full plugin build succeeds