feat:[NEXT-287] Direct aws, azure & gcp to the asset state service#2346
feat:[NEXT-287] Direct aws, azure & gcp to the asset state service#2346kevin-paladin wants to merge 2 commits into
Conversation
WalkthroughThe changes in this pull request primarily focus on modifying the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
commons/pac-batch-commons/src/main/java/com/tmobile/pacman/commons/dto/AssetStateStartEvent.java (1)
1-20: Consider implementing builder patternGiven the number of parameters and validation requirements, consider implementing the builder pattern for better object construction and validation.
Would you like me to provide an example implementation of the builder pattern for this class?
commons/pac-batch-commons/src/main/java/com/tmobile/pacman/commons/aws/sqs/SQSManager.java (2)
45-47: Consider architectural improvements for message handling.The class exhibits several architectural concerns:
- Inconsistent serialization strategies between
JobDoneMessage(JSON) andAssetStateStartEvent(command line)- Multiple responsibilities in message handling
- Hardcoded environmental variable names
Consider these improvements:
- Create a common message interface with a consistent serialization strategy
- Extract message-specific logic into separate handler classes
- Move environment variable names to a configuration class
Example interface:
public interface QueueMessage { String serialize(); }Example configuration:
public final class SQSConfig { public static final String BASE_ACCOUNT_ENV = "BASE_AWS_ACCOUNT"; public static final String ROLE_NAME_ENV = "PALADINCLOUD_RO"; public static final String REGION_ENV = "REGION"; }Also applies to: 51-57
45-47: Enhance error handling and logging.The error handling and logging strategy could be improved:
- Add correlation IDs for request tracing
- Use structured logging for better observability
- Consider throwing custom exceptions instead of returning null
Example improvements:
public class SQSException extends RuntimeException { public SQSException(String message, Throwable cause) { super(message, cause); } } private String sendMessage(String messageBody, String url) { String correlationId = UUID.randomUUID().toString(); LOGGER.debug("Sending message to SQS. correlationId={}, queueUrl={}", correlationId, url); try { AmazonSQS sqs = generateSQSClient(); if (sqs == null) { throw new SQSException("Failed to initialize SQS client", null); } SendMessageResult result = sqs.sendMessage(/* ... */); LOGGER.info("Message sent successfully. correlationId={}, messageId={}", correlationId, result.getMessageId()); return result.getMessageId(); } catch (Exception e) { LOGGER.error("Failed to send message. correlationId={}, error={}", correlationId, e.getMessage(), e); throw new SQSException("Failed to send message", e); } }Also applies to: 67-77
jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/Main.java (2)
115-117: Refactor to use aSetfor data source comparisonUsing a
Setfor comparing data sources enhances scalability and readability, especially if more data sources are added in the future.Consider applying this diff to refactor the comparison:
+ Set<String> assetDataSources = new HashSet<>(Arrays.asList("azure", "aws", "gcp")); + boolean isAsset = assetDataSources.contains(ds.toLowerCase()); - boolean isAsset = ds.equalsIgnoreCase("azure") - || ds.equalsIgnoreCase("aws") - || ds.equalsIgnoreCase("gcp");
125-125: Use appropriate logging levels for message IDsCurrently,
LOGGER.debugis used to log the SQS message IDs. Depending on the importance of tracking these IDs in production, consider usingLOGGER.infoto ensure they appear in standard logs.Also applies to: 130-130
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
commons/pac-batch-commons/src/main/java/com/tmobile/pacman/commons/aws/sqs/SQSManager.java(2 hunks)commons/pac-batch-commons/src/main/java/com/tmobile/pacman/commons/dto/AssetStateStartEvent.java(1 hunks)jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/Main.java(2 hunks)
🔇 Additional comments (4)
commons/pac-batch-commons/src/main/java/com/tmobile/pacman/commons/dto/AssetStateStartEvent.java (1)
1-20: Verify the command line format compatibility
Ensure that the command line format matches the expected format in the asset state service.
commons/pac-batch-commons/src/main/java/com/tmobile/pacman/commons/aws/sqs/SQSManager.java (1)
26-26: LGTM!
The import statement for AssetStateStartEvent is appropriately placed with other DTO imports.
jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/Main.java (2)
124-124: Verify usage of sendMessage vs. sendSQSMessage in SQSManager
Different methods are used to send messages: sendMessage for assetStateStartEvent and sendSQSMessage for jobDoneMessage. Please verify that both methods exist in SQSManager and are intended for their respective message types to ensure consistency and correctness.
Also applies to: 129-129
127-127: Confirm passing null as the last parameter to JobDoneMessage constructor
Passing null as the last parameter in new JobDoneMessage(ds + "-Shipper-Job", tenantId, ds, null) may not be appropriate if the constructor expects a non-null value. Please verify whether null is acceptable or if a valid value should be provided.
| package com.tmobile.pacman.commons.dto; | ||
|
|
||
| public class AssetStateStartEvent { | ||
| private final String tenantId; | ||
| private final String dataSource; | ||
| private final String[] assetTypes; | ||
| private final boolean isFromPolicyEngine; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add class-level documentation and strengthen immutability
The class lacks documentation explaining its purpose and usage. Additionally, the assetTypes array field could be modified externally, breaking immutability.
Add documentation and defensive copying:
package com.tmobile.pacman.commons.dto;
+/**
+ * Data Transfer Object representing an asset state start event.
+ * Used to notify the asset state service when asset processing begins
+ * for AWS, Azure, and GCP platforms.
+ */
public class AssetStateStartEvent {
private final String tenantId;
private final String dataSource;
- private final String[] assetTypes;
+ private final String[] assetTypes; // Consider using List<String> or making defensive copies
private final boolean isFromPolicyEngine;Committable suggestion skipped: line range outside the PR's diff.
| public AssetStateStartEvent(String tenantId, String dataSource, String[] assetTypes, boolean isFromPolicyEngine) { | ||
| this.tenantId = tenantId; | ||
| this.dataSource = dataSource; | ||
| this.assetTypes = assetTypes; | ||
| this.isFromPolicyEngine = isFromPolicyEngine; | ||
| } |
There was a problem hiding this comment.
Add parameter validation and defensive copying
The constructor should validate input parameters and make a defensive copy of the array to prevent external modifications.
Apply this improvement:
public AssetStateStartEvent(String tenantId, String dataSource, String[] assetTypes, boolean isFromPolicyEngine) {
+ if (tenantId == null || tenantId.trim().isEmpty()) {
+ throw new IllegalArgumentException("tenantId cannot be null or empty");
+ }
+ if (dataSource == null || dataSource.trim().isEmpty()) {
+ throw new IllegalArgumentException("dataSource cannot be null or empty");
+ }
+ if (assetTypes == null || assetTypes.length == 0) {
+ throw new IllegalArgumentException("assetTypes cannot be null or empty");
+ }
this.tenantId = tenantId;
this.dataSource = dataSource;
- this.assetTypes = assetTypes;
+ this.assetTypes = assetTypes.clone(); // Defensive copy
this.isFromPolicyEngine = isFromPolicyEngine;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public AssetStateStartEvent(String tenantId, String dataSource, String[] assetTypes, boolean isFromPolicyEngine) { | |
| this.tenantId = tenantId; | |
| this.dataSource = dataSource; | |
| this.assetTypes = assetTypes; | |
| this.isFromPolicyEngine = isFromPolicyEngine; | |
| } | |
| public AssetStateStartEvent(String tenantId, String dataSource, String[] assetTypes, boolean isFromPolicyEngine) { | |
| if (tenantId == null || tenantId.trim().isEmpty()) { | |
| throw new IllegalArgumentException("tenantId cannot be null or empty"); | |
| } | |
| if (dataSource == null || dataSource.trim().isEmpty()) { | |
| throw new IllegalArgumentException("dataSource cannot be null or empty"); | |
| } | |
| if (assetTypes == null || assetTypes.length == 0) { | |
| throw new IllegalArgumentException("assetTypes cannot be null or empty"); | |
| } | |
| this.tenantId = tenantId; | |
| this.dataSource = dataSource; | |
| this.assetTypes = assetTypes.clone(); // Defensive copy | |
| this.isFromPolicyEngine = isFromPolicyEngine; | |
| } |
| public String toCommandLine() { | ||
| return String.format("--tenant_id=%s --data_source=%s --asset_types=%s --is_from_policy_engine=%s", | ||
| tenantId, dataSource, String.join(",", assetTypes), isFromPolicyEngine); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input escaping and getter methods
- The
toCommandLinemethod should escape special characters to prevent command injection. - Consider adding getter methods as this is a DTO.
Add getters and improve command line safety:
+ public String getTenantId() {
+ return tenantId;
+ }
+
+ public String getDataSource() {
+ return dataSource;
+ }
+
+ public String[] getAssetTypes() {
+ return assetTypes.clone(); // Return defensive copy
+ }
+
+ public boolean isFromPolicyEngine() {
+ return isFromPolicyEngine;
+ }
+
public String toCommandLine() {
+ // Escape special characters in values
+ String escapedTenantId = escapeCommandLineArg(tenantId);
+ String escapedDataSource = escapeCommandLineArg(dataSource);
+ String escapedAssetTypes = escapeCommandLineArg(String.join(",", assetTypes));
return String.format("--tenant_id=%s --data_source=%s --asset_types=%s --is_from_policy_engine=%s",
- tenantId, dataSource, String.join(",", assetTypes), isFromPolicyEngine);
+ escapedTenantId, escapedDataSource, escapedAssetTypes, isFromPolicyEngine);
}
+
+ private String escapeCommandLineArg(String value) {
+ return value.replace("\"", "\\\"").replace("'", "\\'");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public String toCommandLine() { | |
| return String.format("--tenant_id=%s --data_source=%s --asset_types=%s --is_from_policy_engine=%s", | |
| tenantId, dataSource, String.join(",", assetTypes), isFromPolicyEngine); | |
| } | |
| public String getTenantId() { | |
| return tenantId; | |
| } | |
| public String getDataSource() { | |
| return dataSource; | |
| } | |
| public String[] getAssetTypes() { | |
| return assetTypes.clone(); // Return defensive copy | |
| } | |
| public boolean isFromPolicyEngine() { | |
| return isFromPolicyEngine; | |
| } | |
| public String toCommandLine() { | |
| // Escape special characters in values | |
| String escapedTenantId = escapeCommandLineArg(tenantId); | |
| String escapedDataSource = escapeCommandLineArg(dataSource); | |
| String escapedAssetTypes = escapeCommandLineArg(String.join(",", assetTypes)); | |
| return String.format("--tenant_id=%s --data_source=%s --asset_types=%s --is_from_policy_engine=%s", | |
| escapedTenantId, escapedDataSource, escapedAssetTypes, isFromPolicyEngine); | |
| } | |
| private String escapeCommandLineArg(String value) { | |
| return value.replace("\"", "\\\"").replace("'", "\\'"); | |
| } |
| public String sendMessage(AssetStateStartEvent message, String url) { | ||
| return sendMessage(message.toCommandLine(), url); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation, documentation, and improve method naming.
The new method needs several improvements:
- Add null parameter validation
- Add Javadoc documentation
- Consider a more specific method name like
sendAssetStateEvent - Consider throwing an exception instead of returning null on failure
Here's the suggested implementation:
+ /**
+ * Sends an asset state start event to the specified SQS queue.
+ *
+ * @param message The asset state event to send. Must not be null.
+ * @param url The SQS queue URL. Must not be null.
+ * @return The message ID if successful, null otherwise.
+ * @throws IllegalArgumentException if message or url is null
+ */
- public String sendMessage(AssetStateStartEvent message, String url) {
- return sendMessage(message.toCommandLine(), url);
+ public String sendAssetStateEvent(AssetStateStartEvent message, String url) {
+ if (message == null || url == null) {
+ throw new IllegalArgumentException("Message and URL must not be null");
+ }
+ return sendMessage(message.toCommandLine(), url);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public String sendMessage(AssetStateStartEvent message, String url) { | |
| return sendMessage(message.toCommandLine(), url); | |
| } | |
| /** | |
| * Sends an asset state start event to the specified SQS queue. | |
| * | |
| * @param message The asset state event to send. Must not be null. | |
| * @param url The SQS queue URL. Must not be null. | |
| * @return The message ID if successful, null otherwise. | |
| * @throws IllegalArgumentException if message or url is null | |
| */ | |
| public String sendAssetStateEvent(AssetStateStartEvent message, String url) { | |
| if (message == null || url == null) { | |
| throw new IllegalArgumentException("Message and URL must not be null"); | |
| } | |
| return sendMessage(message.toCommandLine(), url); | |
| } |
| AssetStateStartEvent assetStateStartEvent = | ||
| new AssetStateStartEvent(tenantId, ds, assetTypes.toArray(new String[0]), false); | ||
| String sqsMessageID = | ||
| sqsManager.sendMessage(assetStateStartEvent, System.getenv("ASSET_STATE_QUEUE_URL")); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle missing environment variables to prevent runtime exceptions
System.getenv("ASSET_STATE_QUEUE_URL") and System.getenv("SHIPPER_SQS_QUEUE_URL") may return null if the environment variables are not set. This could lead to a NullPointerException when sending messages. Please ensure that these environment variables are validated before use.
Consider adding null checks and handling missing environment variables:
+ String assetStateQueueUrl = System.getenv("ASSET_STATE_QUEUE_URL");
+ if (assetStateQueueUrl == null) {
+ LOGGER.error("Environment variable ASSET_STATE_QUEUE_URL is not set");
+ // Handle the error, e.g., throw an exception or use a default value
+ }
String sqsMessageID =
sqsManager.sendMessage(assetStateStartEvent, assetStateQueueUrl);
...
+ String shipperSqsQueueUrl = System.getenv("SHIPPER_SQS_QUEUE_URL");
+ if (shipperSqsQueueUrl == null) {
+ LOGGER.error("Environment variable SHIPPER_SQS_QUEUE_URL is not set");
+ // Handle the error accordingly
+ }
String sqsMessageID =
sqsManager.sendSQSMessage(jobDoneMessage, shipperSqsQueueUrl);Also applies to: 129-129
| List<String> assetTypes = new ArrayList<>(ConfigManager.getTypesWithDisplayName(ds).keySet()); | ||
| Collections.sort(assetTypes); | ||
| AssetStateStartEvent assetStateStartEvent = | ||
| new AssetStateStartEvent(tenantId, ds, assetTypes.toArray(new String[0]), false); |
There was a problem hiding this comment.
Add null check for ConfigManager.getTypesWithDisplayName(ds) to prevent NullPointerException
If ConfigManager.getTypesWithDisplayName(ds) returns null, calling keySet() will result in a NullPointerException. It's important to add a null check to ensure the application handles this scenario gracefully.
Consider applying this diff to add the null check:
+ Map<String, String> typesWithDisplayName = ConfigManager.getTypesWithDisplayName(ds);
+ if (typesWithDisplayName != null) {
List<String> assetTypes = new ArrayList<>(typesWithDisplayName.keySet());
Collections.sort(assetTypes);
AssetStateStartEvent assetStateStartEvent =
new AssetStateStartEvent(tenantId, ds, assetTypes.toArray(new String[0]), false);
String sqsMessageID =
sqsManager.sendMessage(assetStateStartEvent, System.getenv("ASSET_STATE_QUEUE_URL"));
LOGGER.debug("AssetState Start SQS message ID: {}", sqsMessageID);
+ } else {
+ LOGGER.error("Failed to retrieve asset types; typesWithDisplayName is null");
+ // Handle the error appropriately, e.g., throw an exception or set a default value
+ }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
commons/pac-batch-commons/src/main/java/com/tmobile/pacman/commons/dto/JobDoneMessage.java (3)
22-27: Consider consolidating tenant ID fieldsHaving two fields (
paladinCloudTenantIdandtenantId) for the same value increases maintenance overhead and could lead to confusion. Consider:
- Using a single field with a clear name
- Adding a deprecated annotation if backward compatibility is needed
- Planning for future consolidation
- // NOTE: 'paladinCloudTenantId' is currently sent by the legacy shipper while 'tenantId' is sent - // by the delta engine; it's not clear if different recipients will use what's available so - // both are sent here. - private String paladinCloudTenantId; - @JsonProperty("tenant_id") - private String tenantId; + @JsonProperty(value = "tenant_id", access = JsonProperty.Access.WRITE_ONLY) + private String tenantId; + + @Deprecated + public String getPaladinCloudTenantId() { + return tenantId; + }
30-32: Maintain consistent JSON property naming conventionFor consistency with
tenant_name, consider adding@JsonPropertyannotation toassetTypesusing snake_case.@JsonProperty("tenant_name") private String tenantName; - private String[] assetTypes; + @JsonProperty("asset_types") + private String[] assetTypes;
67-68: Add JavaDoc for new getter methodsConsider adding JavaDoc comments to maintain consistency with other methods in the class.
+ /** + * Gets the tenant name. + * @return the tenant name + */ public String getTenantName() { return tenantName; } + + /** + * Gets the asset types. + * @return array of asset types + */ public String[] getAssetTypes() { return assetTypes; }jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/Main.java (1)
114-116: Optimize cloud provider checks and use constantsThe current implementation uses multiple
equalsIgnoreCasecalls and magic strings. Consider using a Set for better performance and maintainability.+ private static final Set<String> ASSET_PROVIDERS = Set.of("azure", "aws", "gcp"); + - boolean isAsset = ds.equalsIgnoreCase("azure") - || ds.equalsIgnoreCase("aws") - || ds.equalsIgnoreCase("gcp"); + boolean isAsset = ASSET_PROVIDERS.contains(ds.toLowerCase());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
commons/pac-batch-commons/src/main/java/com/tmobile/pacman/commons/dto/JobDoneMessage.java(2 hunks)jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/Main.java(3 hunks)
🔇 Additional comments (2)
jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/Main.java (2)
118-124:
Add null checks for ConfigManager and environment variables
The code needs to handle potential null values from ConfigManager and environment variables.
This is a duplicate of previous review comments. The same concerns about null checks for ConfigManager.getTypesWithDisplayName(ds) and validation of environment variables still apply. Please refer to the previous review comments for the suggested fixes.
111-130: Verify integration with asset state service
The implementation routes AWS, Azure, and GCP assets through the asset state service as intended. Let's verify the integration points.
✅ Verification successful
Integration with asset state service appears correctly implemented
The verification shows:
ASSET_STATE_QUEUE_URLis only referenced in the data shipper, maintaining clean separationJobDoneMessageimplementation supports both asset and non-asset flows with appropriate constructors- Other services (Qualys, Tenable, Aqua) use
JobDoneMessagebut route toENRICHER_SQS_QUEUE_URL, showing proper separation of concerns - Asset state routing is exclusive to cloud provider assets (AWS, Azure, GCP)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify asset state service integration points
# Check for asset state service configuration
echo "Checking for asset state service configuration..."
rg -l "ASSET_STATE_QUEUE_URL"
# Check for asset type definitions
echo "Checking asset type definitions for cloud providers..."
ast-grep --pattern 'getTypesWithDisplayName($provider)'
# Check for other potential consumers of JobDoneMessage
echo "Checking for JobDoneMessage consumers..."
rg -A 5 "JobDoneMessage"
Length of output: 11624
| public JobDoneMessage (String jobName, String paladinCloudTenantId, String source, String enricherSource, String tenantName, String[] assetTypes) { | ||
| this.jobName = jobName; | ||
| this.paladinCloudTenantId = paladinCloudTenantId; | ||
| this.tenantId = paladinCloudTenantId; | ||
| this.tenantName = tenantName; | ||
| this.source = source; | ||
| this.enricherSource = enricherSource; | ||
| this.assetTypes = assetTypes; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement defensive copying for array parameter
The assetTypes array should be defensively copied to prevent external modification of the internal state.
public JobDoneMessage (String jobName, String paladinCloudTenantId, String source, String enricherSource, String tenantName, String[] assetTypes) {
this.jobName = jobName;
this.paladinCloudTenantId = paladinCloudTenantId;
this.tenantId = paladinCloudTenantId;
this.tenantName = tenantName;
this.source = source;
this.enricherSource = enricherSource;
- this.assetTypes = assetTypes;
+ this.assetTypes = assetTypes != null ? assetTypes.clone() : null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public JobDoneMessage (String jobName, String paladinCloudTenantId, String source, String enricherSource, String tenantName, String[] assetTypes) { | |
| this.jobName = jobName; | |
| this.paladinCloudTenantId = paladinCloudTenantId; | |
| this.tenantId = paladinCloudTenantId; | |
| this.tenantName = tenantName; | |
| this.source = source; | |
| this.enricherSource = enricherSource; | |
| this.assetTypes = assetTypes; | |
| } | |
| public JobDoneMessage (String jobName, String paladinCloudTenantId, String source, String enricherSource, String tenantName, String[] assetTypes) { | |
| this.jobName = jobName; | |
| this.paladinCloudTenantId = paladinCloudTenantId; | |
| this.tenantId = paladinCloudTenantId; | |
| this.tenantName = tenantName; | |
| this.source = source; | |
| this.enricherSource = enricherSource; | |
| this.assetTypes = assetTypes != null ? assetTypes.clone() : null; | |
| } |
|
2 participants
Description
Send the asset state service start event when finished processing assets. Currently, these are aws, azure & gcp.
NOTE: The asset state service in the ce-extensions repository must be deployed and validated prior to deploying this change.
Type of change
How Has This Been Tested?
Manually validated sending event.
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
JobDoneMessagestructure and usage.