Skip to content

Conversation

@lonegunmanb
Copy link
Member

Describe your changes

Issue number

#568

Checklist before requesting a review

  • The pr title can be used to describe what this pr did in CHANGELOG.md file
  • I have executed pre-commit on my machine
  • I have passed pr-check on my machine

Thanks for your cooperation!

@zioproto
Copy link
Contributor

Warning dns_prefix or dns_prefix_private_cluster are exclusive.

We do have a prefix variable that is not nullable:

variable "prefix" {
  type        = string
  default     = ""
  description = "(Optional) The prefix for the resources created in the specified Azure Resource Group. Omitting this variable requires both `var.cluster_log_analytics_workspace_name` and `var.cluster_name` have been set."
  nullable    = false
}

Screenshot 2024-08-20 at 11 10 24

Would it be okay to have dns_prefix as empty string "" and dns_prefix_private_cluster as a string ?

@lonegunmanb
Copy link
Member Author

Thanks for your review @zioproto , quite good catch! I've removed nullable=false from var.prefix now we can set it to null explicitly.

An empty string for var.prefix and non-empty string for var.dns_prefix_private_cluster would trigger a schema validation error so I think it's enough for those two variables. WDYT?

@zioproto
Copy link
Contributor

zioproto commented Sep 9, 2024

During the testing I figure out we need more code. I pushed 2 commits with more preconditions.

https://learn.microsoft.com/en-us/azure/aks/private-clusters?tabs=azure-portal#configure-a-private-dns-zone

There is another problem.

the private dns zone name should be in either of these formats:

  • private.eastus.azmk8s.io
  • privatelink.eastus.azmk8s.io
  • [a-zA-Z0-9-]{1,32}.private.eastus.azmk8s.io
  • [a-zA-Z0-9-]{1,32}.privatelink.eastus.azmk8s.io

Should we also create the necessary role assignments ?

The docs say:
"You also need a user-assigned identity or service principal with the Private DNS Zone Contributor and Network Contributor roles."
I enforced the identities with the precondition but I did not create the assignments yet

@zioproto
Copy link
Contributor

zioproto commented Sep 9, 2024

Another problem. The default value for var.prefix is "". In my tests I had to pass an explicit var.prefix = null. At one point we could introduce a braking change making the default value for var.prefix = null ?

locals.tf Outdated
use_brown_field_gw_for_ingress = var.brown_field_application_gateway_for_ingress != null
use_green_field_gw_for_ingress = var.green_field_application_gateway_for_ingress != null
valid_private_dns_zone_regexs = [
"private\\.[a-z]+\\.azmk8s\\.io",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the match [a-z] should be changed into [a-z0-9] to match region names with numbers like eastus2

Copy link
Contributor

@zioproto zioproto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the match [a-z] should be changed into [a-z0-9] to match region names with numbers like eastus2

locals.tf Outdated
use_brown_field_gw_for_ingress = var.brown_field_application_gateway_for_ingress != null
use_green_field_gw_for_ingress = var.green_field_application_gateway_for_ingress != null
valid_private_dns_zone_regexs = [
"private\\.[a-z]+\\.azmk8s\\.io",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"private\\.[a-z]+\\.azmk8s\\.io",
"private\\.[a-z0-9]+\\.azmk8s\\.io",

@lonegunmanb lonegunmanb merged commit b2465a8 into main Mar 24, 2025
5 checks passed
@github-project-automation github-project-automation bot moved this from Todo to Done in Azure Module Kanban Mar 24, 2025
@lonegunmanb lonegunmanb deleted the e-568 branch March 24, 2025 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

3 participants