Issue

1401

https://github.com/CDCgov/trusted-intermediary/issues/1401

github-actions[bot] commented 3 weeks ago

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

**🎫 Ticket compliance analysis 🔶** **[1399](https://github.com/CDCgov/trusted-intermediary/issues/1399) - Partially compliant** Fully compliant requirements: - Alert on high memory usage for CDC TI. Not compliant requirements: - Alert specifically on high CPU usage. - Alerts for SFTP and database not addressed.

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review

Possible Misconfiguration
The memory threshold values are hardcoded. Consider making them configurable to adapt to different environments. Resource Duplication
There are two similar alert resources defined. Check if they can be combined or if the differentiation is necessary.

github-actions[bot] commented 3 weeks ago

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Maintainability	Use a variable for memory thresholds to enhance maintainability ___ Consider using a variable for the memory threshold values in the `ti_memory_alert` resource to maintain consistency and ease of configuration changes. [operations/template/alert.tf [276]](https://github.com/CDCgov/trusted-intermediary/pull/1546/files#diff-ea7b0de5fd253c48860400c7536561ab82d76414012b803eeb3a2cb75d8a439aR276-R276) ```diff -threshold = local.higher_environment_level ? 4000000000 : 2000000000 +threshold = var.memory_threshold ``` Suggestion importance[1-10]: 7 Why: Using a variable for memory thresholds in the 'ti_memory_alert' resource would improve maintainability and ease future configuration changes. This is a practical suggestion that enhances the code's flexibility.	7
Best practice	Implement validation for the `environment` variable to prevent misconfigurations ___ Add validation for the `environment` variable used in the alert names to prevent configuration errors. [operations/template/alert.tf [263]](https://github.com/CDCgov/trusted-intermediary/pull/1546/files#diff-ea7b0de5fd253c48860400c7536561ab82d76414012b803eeb3a2cb75d8a439aR263-R263) ```diff -name = "cdcti-${var.environment}-memory-alert" +variable "environment" { + description = "The deployment environment" + type = string + validation { + condition = contains(["dev", "prod", "staging"], var.environment) + error_message = "The environment must be dev, prod, or staging." + } +} ``` Suggestion importance[1-10]: 6 Why: Adding validation for the `environment` variable is a good practice to ensure the configuration is correct and to prevent potential runtime errors. This suggestion enhances the robustness of the configuration.	6
Best practice	Verify and update the `ignore_changes` list to cover all necessary tags ___ Ensure that the `ignore_changes` lifecycle policy in both memory alert resources covers all necessary tags to prevent unintended updates. [operations/template/alert.tf [244-257]](https://github.com/CDCgov/trusted-intermediary/pull/1546/files#diff-ea7b0de5fd253c48860400c7536561ab82d76414012b803eeb3a2cb75d8a439aR244-R257) ```diff ignore_changes = [ tags["business_steward"], ... - tags["zone"] + tags["zone"], + tags["new_necessary_tag"] ] ``` Suggestion importance[1-10]: 5 Why: Ensuring that the `ignore_changes` lifecycle policy covers all necessary tags is important to prevent unintended updates. However, the suggestion lacks specific details on what new tags might be necessary, making it less actionable.	5
Enhancement	Use a variable for `alert_sensitivity` to allow flexible configuration ___ Consider adding more granularity to the `alert_sensitivity` setting in the `dynamic_criteria` block to better tailor alert responses. [operations/template/alert.tf [235]](https://github.com/CDCgov/trusted-intermediary/pull/1546/files#diff-ea7b0de5fd253c48860400c7536561ab82d76414012b803eeb3a2cb75d8a439aR235-R235) ```diff -alert_sensitivity = "Medium" +alert_sensitivity = var.alert_sensitivity ``` Suggestion importance[1-10]: 6 Why: Using a variable for `alert_sensitivity` in the `dynamic_criteria` block provides flexibility in configuring alert sensitivity levels, which can be beneficial for different deployment scenarios. This suggestion adds customization potential to the alert settings.	6