egov-malware-detection: add Azure Blob Storage support - #1376
Open
LataNaik wants to merge 1 commit into
Open
Conversation
The service only supported MinIO/S3 (io.minio.MinioClient) and explicitly rejected AzureBlobStorage as a file source, unlike egov-filestore which already supports both backends. This left Azure-based deployments unable to run malware scanning at all. Introduces a StorageClient abstraction (mirroring egov-filestore's CloudFilesManager pattern) with MinioStorageClient and AzureBlobStorageClient implementations, selected via isS3Enabled/isAzureStorageEnabled - FileRetrievalService and QuarantineService now depend on the interface instead of MinioClient directly. Also fixes MinioClientConfig's bean condition, which previously defaulted to enabled (matchIfMissing=true) regardless of the Azure flag, so both clients could never be selected correctly at once. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📝 WalkthroughWalkthroughThe malware detection service adds Azure Blob Storage support, introduces a shared ChangesStorage backend abstraction
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant FileRetrievalService
participant StorageClient
participant AzureBlobStorageClient
participant AzureBlobStorage
FileRetrievalService->>StorageClient: request file metadata or content
StorageClient->>AzureBlobStorageClient: delegate storage operation
AzureBlobStorageClient->>AzureBlobStorage: access Azure blob
AzureBlobStorage-->>FileRetrievalService: return metadata or stream
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core-services/egov-malware-detection/src/main/java/org/egov/malware/service/FileRetrievalService.java (1)
67-71: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winResource leak: Unclosed InputStream on exception.
The
InputStreamis manually closed afterIOUtils.toByteArray(). If an exception occurs while reading the stream (e.g., a network error), the.close()method is bypassed, which leaks the underlying network connection to the storage backend. Please use a try-with-resources block to ensure the stream is always safely closed.
core-services/egov-malware-detection/src/main/java/org/egov/malware/service/FileRetrievalService.java#L67-L71: Wrap thestorageClient.getObjectcall in a try-with-resources block.core-services/egov-malware-detection/src/main/java/org/egov/malware/service/FileRetrievalService.java#L191-L194: Wrap thestorageClient.getObjectcall in a try-with-resources block.💻 Proposed fixes
For
fetchFile(Lines 67-71):- InputStream inputStream = storageClient.getObject(bucketName, objectPath); - - // Read content into byte array for scanning - byte[] content = IOUtils.toByteArray(inputStream); - inputStream.close(); + byte[] content; + try (InputStream inputStream = storageClient.getObject(bucketName, objectPath)) { + // Read content into byte array for scanning + content = IOUtils.toByteArray(inputStream); + }For
getFileContent(Lines 191-194):- InputStream inputStream = storageClient.getObject(config.getMinioBucketName(), filePath); - byte[] content = IOUtils.toByteArray(inputStream); - inputStream.close(); - return content; + try (InputStream inputStream = storageClient.getObject(config.getMinioBucketName(), filePath)) { + return IOUtils.toByteArray(inputStream); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core-services/egov-malware-detection/src/main/java/org/egov/malware/service/FileRetrievalService.java` around lines 67 - 71, Prevent stream leaks in both fetchFile and getFileContent by wrapping each storageClient.getObject result in a try-with-resources block, including the IOUtils.toByteArray processing inside the block, and remove the manual close calls. Apply this at FileRetrievalService.java lines 67-71 and 191-194.
🧹 Nitpick comments (3)
core-services/egov-malware-detection/pom.xml (1)
93-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftPlan a migration to the modern Azure SDK
This module still depends on the legacycom.microsoft.azure:azure-storageSDK. If it can diverge fromegov-filestore, migrating tocom.azure:azure-storage-blobwould improve long-term support and maintainability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core-services/egov-malware-detection/pom.xml` around lines 93 - 99, Plan and implement migration of the Azure Blob Storage dependency in the module’s pom configuration from legacy com.microsoft.azure:azure-storage to modern com.azure:azure-storage-blob, updating the version property and all dependent code or configuration as needed. Verify behavior remains aligned with egov-filestore, and remove the legacy dependency once migration is complete.core-services/egov-malware-detection/src/main/java/org/egov/malware/storage/AzureBlobStorageClient.java (1)
46-55: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove redundant container creation check to improve upload performance.
Calling
container.createIfNotExists(...)on everyputObjectinvocation introduces unnecessary network overhead (I/O latency) for each upload. TheStorageClientcontract already manages bucket creation explicitly viamakeBucket()(which callers likeQuarantineServiceuse beforehand). Removing this check improves performance and aligns the Azure implementation with the MinIO client, which assumes the bucket already exists.⚡ Proposed performance optimization
`@Override` public void putObject(String bucket, String objectPath, InputStream content, long contentLength, String contentType) throws Exception { CloudBlobContainer container = getContainer(bucket); - container.createIfNotExists(BlobContainerPublicAccessType.OFF, new BlobRequestOptions(), new OperationContext()); CloudBlockBlob blob = container.getBlockBlobReference(objectPath); if (contentType != null) { blob.getProperties().setContentType(contentType); } blob.upload(content, contentLength); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core-services/egov-malware-detection/src/main/java/org/egov/malware/storage/AzureBlobStorageClient.java` around lines 46 - 55, Remove the container.createIfNotExists call from AzureBlobStorageClient.putObject, leaving bucket/container provisioning to makeBucket() and preserving the existing blob reference, content type, and upload flow.core-services/egov-malware-detection/src/main/java/org/egov/malware/service/QuarantineService.java (1)
112-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider renaming
minioBucketNamefor clarity.Since the service is now storage-agnostic and supports Azure Blob Storage, relying on
config.getMinioBucketName()(which maps to${fixed.bucketname}) is slightly misleading. Consider renaming this configuration property field inMalwareDetectionConfig(and its usages) to something likegetStorageBucketName()orgetFixedBucketName()as an optional cleanup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core-services/egov-malware-detection/src/main/java/org/egov/malware/service/QuarantineService.java` around lines 112 - 114, Rename the storage bucket configuration accessor from getMinioBucketName to a storage-agnostic name such as getStorageBucketName, updating its field definition in MalwareDetectionConfig and every usage including deleteOriginalFile. Preserve the existing ${fixed.bucketname} configuration mapping and bucket-selection behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@core-services/egov-malware-detection/src/main/java/org/egov/malware/service/FileRetrievalService.java`:
- Around line 67-71: Prevent stream leaks in both fetchFile and getFileContent
by wrapping each storageClient.getObject result in a try-with-resources block,
including the IOUtils.toByteArray processing inside the block, and remove the
manual close calls. Apply this at FileRetrievalService.java lines 67-71 and
191-194.
---
Nitpick comments:
In `@core-services/egov-malware-detection/pom.xml`:
- Around line 93-99: Plan and implement migration of the Azure Blob Storage
dependency in the module’s pom configuration from legacy
com.microsoft.azure:azure-storage to modern com.azure:azure-storage-blob,
updating the version property and all dependent code or configuration as needed.
Verify behavior remains aligned with egov-filestore, and remove the legacy
dependency once migration is complete.
In
`@core-services/egov-malware-detection/src/main/java/org/egov/malware/service/QuarantineService.java`:
- Around line 112-114: Rename the storage bucket configuration accessor from
getMinioBucketName to a storage-agnostic name such as getStorageBucketName,
updating its field definition in MalwareDetectionConfig and every usage
including deleteOriginalFile. Preserve the existing ${fixed.bucketname}
configuration mapping and bucket-selection behavior.
In
`@core-services/egov-malware-detection/src/main/java/org/egov/malware/storage/AzureBlobStorageClient.java`:
- Around line 46-55: Remove the container.createIfNotExists call from
AzureBlobStorageClient.putObject, leaving bucket/container provisioning to
makeBucket() and preserving the existing blob reference, content type, and
upload flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 611be715-f4d9-46ee-b4d5-1bd0cc5bc1b8
📒 Files selected for processing (11)
core-services/egov-malware-detection/pom.xmlcore-services/egov-malware-detection/src/main/java/org/egov/malware/config/AzureBlobConfig.javacore-services/egov-malware-detection/src/main/java/org/egov/malware/config/MalwareDetectionConfig.javacore-services/egov-malware-detection/src/main/java/org/egov/malware/config/MinioClientConfig.javacore-services/egov-malware-detection/src/main/java/org/egov/malware/service/FileRetrievalService.javacore-services/egov-malware-detection/src/main/java/org/egov/malware/service/QuarantineService.javacore-services/egov-malware-detection/src/main/java/org/egov/malware/storage/AzureBlobStorageClient.javacore-services/egov-malware-detection/src/main/java/org/egov/malware/storage/MinioStorageClient.javacore-services/egov-malware-detection/src/main/java/org/egov/malware/storage/StorageClient.javacore-services/egov-malware-detection/src/main/java/org/egov/malware/storage/StorageObjectMetadata.javacore-services/egov-malware-detection/src/main/resources/application.properties
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
2 participants
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
egov-malware-detectioncurrently only supports MinIO/S3 (io.minio.MinioClient) and explicitly rejectsAzureBlobStorageas a file source (FileRetrievalService.validateFileSource), unlikeegov-filestorewhich already supports both backends. This leaves Azure-based DIGIT deployments unable to run malware scanning at all - the app fails at scan time (or, ifMINIO_URLis malformed, at Spring context startup).StorageClientabstraction (mirroringegov-filestore'sCloudFilesManagerpattern) with two implementations selected via config flags:MinioStorageClient- existing MinIO/S3 behavior, unchangedAzureBlobStorageClient- new, usingcom.microsoft.azure:azure-storage(same SDK/version asegov-filestore), driven byisAzureStorageEnabled+azure.accountName/azure.accountKey(same property names as filestore, so the same secret can be reused)FileRetrievalServiceandQuarantineServicenow depend onStorageClientinstead ofMinioClientdirectly.MinioClientConfig: its@ConditionalOnProperty(value = "isS3Enabled", havingValue = "true", matchIfMissing = true)meant the Minio bean was built even whenisS3Enabledwas unset, with no way to actually disable it in favor of another backend. It's now@ConditionalOnExpression("${isS3Enabled:true} and !${isAzureStorageEnabled:false}"), ensuring exactly one storage client is ever active.Config additions
Container names reuse the existing
fixed.bucketname/quarantine.bucketnameproperties for both backends.Test plan
mvn compilepasses (verified locally)isAzureStorageEnabled=trueagainst a real Azure Storage account and confirm scan requests withfileSource=AzureBlobStorageare retrieved, scanned, and quarantined correctlyisS3Enabled=true,isAzureStorageEnabledunset)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes