-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[TC-5] Add module for private dns zone with vnet links #15
base: devli
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be more consistent. Be considerate that people will us the example as documentation until such is provided and the examples must contain ready-to-go working configuration.
storage_account_blob = { | ||
resource_kind = "storage_blob" | ||
resource_group_ref = "rg_test" | ||
vnet_ref = ["vnet_test", "vnet_test2"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to vnet_refs
. Also the vnet object is not a child of the private dns zone. Rename this field to something that reflects the fact that we're linking with the vnets.
@@ -0,0 +1,44 @@ | |||
private_dns_zones = { | |||
storage_account_blob = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to pdns_storage_account_blob
cidr = ["10.1.0.0/16"] | ||
subnets = { | ||
snet_app_02 = { | ||
name = "snet-app" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example will not work if the subnets are named the same :)
var.global_settings.inherit_resource_group_tags ? local.resource_group.tags : {}, | ||
try(var.settings.tags, {}) | ||
) | ||
vnet_ids = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This object is called vnet_ids
but it holds names and ids. Reconsider.
locals { | ||
# local object used to map possible private dns zoone names | ||
zone_names = { | ||
"storage_blob" = "privatelink.blob.core.windows.net" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
storage_blob
is the only name in this map that is not plural. Consistency is key. Reconsider.
@@ -0,0 +1,5 @@ | |||
resource "azurerm_private_dns_zone" "main" { | |||
name = try(local.zone_names[var.settings.resource_kind], var.settings.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Experience shows that the name field is not used in practice. I recommend relying completely on the local object instead. We'll modify the module to be able to override the name, once such a case emerges.
resource "azurerm_private_dns_zone" "main" { | ||
name = try(local.zone_names[var.settings.resource_kind], var.settings.name) | ||
resource_group_name = local.resource_group_name | ||
tags = try(local.tags, null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will never fail. local.tags
is enough.
source = "./modules/_networking/private_dns_zone" | ||
for_each = var.private_dns_zones | ||
|
||
global_settings = var.global_settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this must be local.global_settings
for consistency with the rest of the modules
TC-5 Add module for private dns zone with vnet links