Add Traffic Manager Endpoint module supporting all routing methods#189
Add Traffic Manager Endpoint module supporting all routing methods#189SazniMohamed merged 3 commits intowso2:mainfrom
Conversation
WalkthroughAdds public Terraform variables and a provider/version constraint, and defines six conditionally-created Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
modules/azurerm/Traffic-Manager-External-Endpoint/traffic_manager_priority_external_endpoint.tf(1 hunks)modules/azurerm/Traffic-Manager-External-Endpoint/variables.tf(1 hunks)modules/azurerm/Traffic-Manager-External-Endpoint/versions.tf(1 hunks)
🔇 Additional comments (3)
modules/azurerm/Traffic-Manager-External-Endpoint/versions.tf (1)
21-29: Terraform and provider version constraints look good.The constraints are well-configured and reasonable for this module.
modules/azurerm/Traffic-Manager-External-Endpoint/traffic_manager_priority_external_endpoint.tf (2)
1-35: Scope mismatch: filename and resource naming vs. implementation.The filename
traffic_manager_priority_external_endpoint.tfand resource nameperformace_based_external_endpointsuggest this handles Priority routing, but the implementation only creates resources whenvar.routing_method == "Performance"(line 22). Verify whether:
- This module is intended to support all routing methods (Priority, Performance, Weighted, Geographic, Multivalue, Subnet)
- Other routing methods are handled in separate files within this module
- The filename and PR description accurately reflect the current scope
If all routing methods are intended, expand the implementation to include them. If only Performance routing is supported, rename the file to clarify scope.
27-33: The custom_header field mapping is correct.The dynamic block properly maps
custom_header.value.header_nameandcustom_header.value.header_valueto thenameandvaluefields, which are the exact field names expected by theazurerm_traffic_manager_external_endpointprovider'scustom_headerblock. No changes needed.
modules/azurerm/Traffic-Manager-External-Endpoint/traffic_manager_priority_external_endpoint.tf
Outdated
Show resolved
Hide resolved
77d79d8 to
f0054ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
modules/azurerm/Traffic-Manager-External-Endpoint/variables.tf (2)
36-39: Routing_method variable still lacks validation (unresolved from past review).The variable accepts any string value despite the description listing valid values (
Performance,Priority,Weighted,Geographic,Multivalue,Subnet). Without validation, invalid values bypass the conditional count logic in the endpoint resources, causing unexpected behavior. Add a validation block to enforce allowed values.variable "routing_method" { description = "Routing method for the Traffic Manager Profile. Valid values are 'Performance', 'Priority', 'Weighted', 'Geographic', 'Multivalue', 'Subnet'" type = string + + validation { + condition = contains(["Performance", "Priority", "Weighted", "Geographic", "Multivalue", "Subnet"], var.routing_method) + error_message = "Routing method must be one of: Performance, Priority, Weighted, Geographic, Multivalue, or Subnet." + } }
74-78: Remove empty string default for endpoint_location (unresolved from past review).The description states this is "Required for Performance routing method," but the empty string default masks this requirement. When Performance routing is selected without an explicit location, Azure deployment will fail. Remove the default to make it required, or add conditional validation that enforces presence when routing_method is "Performance".
variable "endpoint_location" { description = "Location of the endpoint. Required for Performance routing method. The location must be specified for endpoints of types: 'Performance'" type = string - default = "" }Alternatively, add conditional validation:
variable "endpoint_location" { description = "Location of the endpoint. Required for Performance routing method. The location must be specified for endpoints of types: 'Performance'" type = string default = "" + + validation { + condition = var.routing_method != "Performance" || var.endpoint_location != "" + error_message = "endpoint_location is required when routing_method is Performance." + } }
🧹 Nitpick comments (1)
modules/azurerm/Traffic-Manager-External-Endpoint/variables.tf (1)
62-72: Add validation for weight and priority ranges.The descriptions state valid ranges (1-1000) but no validation blocks enforce these constraints. This could allow users to pass invalid values that fail during deployment. Add validation blocks to match the stated constraints.
variable "weight" { description = "Weight of the endpoint. Required for Weighted routing method. Valid values are between 1 and 1000" type = number default = 1 + + validation { + condition = var.weight >= 1 && var.weight <= 1000 + error_message = "Weight must be between 1 and 1000." + } } variable "priority" { description = "Priority of the endpoint. Required for Priority routing method. Valid values are between 1 and 1000" type = number default = 1 + + validation { + condition = var.priority >= 1 && var.priority <= 1000 + error_message = "Priority must be between 1 and 1000." + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/azurerm/Traffic-Manager-External-Endpoint/traffic_manager_external_endpoint.tf(1 hunks)modules/azurerm/Traffic-Manager-External-Endpoint/variables.tf(1 hunks)
modules/azurerm/Traffic-Manager-External-Endpoint/traffic_manager_external_endpoint.tf
Outdated
Show resolved
Hide resolved
modules/azurerm/Traffic-Manager-External-Endpoint/traffic_manager_external_endpoint.tf
Outdated
Show resolved
Hide resolved
8efaa1c to
80d0428
Compare
80d0428 to
ea2cb70
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
modules/azurerm/Traffic-Manager-External-Endpoint/traffic_manager_external_endpoint.tf (1)
21-127: Consider refactoring to reduce code duplication.The six resources follow an identical pattern with only routing-method-specific attributes varying. You could improve maintainability by using a single resource with a
for_eachloop over a map of routing method configurations, or by extracting common attributes into a local variable. This would eliminate repetitive code while maintaining clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/azurerm/Traffic-Manager-External-Endpoint/traffic_manager_external_endpoint.tf(1 hunks)modules/azurerm/Traffic-Manager-External-Endpoint/variables.tf(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/azurerm/Traffic-Manager-External-Endpoint/variables.tf
🔇 Additional comments (2)
modules/azurerm/Traffic-Manager-External-Endpoint/traffic_manager_external_endpoint.tf (2)
21-21: Good—typos have been fixed.The resource names are now correctly spelled as
performance_based_external_endpoint(line 21) andmultivalue_based_external_endpoint(line 89), addressing the prior review comments.Also applies to: 89-89
21-127: Verify variable structure matches dynamic block expectations.The dynamic blocks assume specific variable structures (e.g.,
custom_header.value.header_nameandsubnet.value.first) that must be validated invariables.tf. Additionally, verify that all six routing methods legitimately support thecustom_headerblock—particularly the Subnet method, which appears as the only outlier in typical Azure Traffic Manager patterns.Please confirm:
var.custom_headersis defined as a list of objects withheader_nameandheader_valuefieldsvar.subnetsis defined as a list of objects withfirst,last, andscopefields- All routing methods (including Subnet) support the custom_header block in your provider version
You can verify this by examining
variables.tfand consulting the Azure provider documentation for the endpoint resource.
Change Description