feat: new updates #3
Conversation
Summary of ChangesHello @Manjeet-Singh2428, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request transforms a generic Terraform Azure module template into a specialized module for deploying Azure MySQL Flexible Servers. It introduces comprehensive configuration options, integrates private DNS, and enhances security features like Customer Managed Keys and Azure Entra authentication. The changes aim to provide a robust and configurable solution for MySQL deployments on Azure, complete with updated documentation and a detailed example. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
diffDiff--- a/examples/complete/plan-target.txt
+++ b/examples/complete/plan-pr.txt
@@ -1,5 +1,545 @@
-No changes. Your infrastructure matches the configuration.
+Terraform used the selected providers to generate the following execution
+plan. Resource actions are indicated with the following symbols:
+ + create
+ <= read (data resources)
-Terraform has compared your real infrastructure against your configuration
-and found no differences, so no changes are needed.
+Terraform will perform the following actions:
+
+ # module.flexible-mysql.data.azurerm_client_config.current will be read during apply
+ # (depends on a resource or a module with changes pending)
+ <= data "azurerm_client_config" "current" {
+ + client_id = (known after apply)
+ + id = (known after apply)
+ + object_id = (known after apply)
+ + subscription_id = (known after apply)
+ + tenant_id = (known after apply)
+ }
+
+ # module.flexible-mysql.azurerm_key_vault_key.geo_cmk_key[0] will be created
+ + resource "azurerm_key_vault_key" "geo_cmk_key" {
+ + curve = (known after apply)
+ + e = (known after apply)
+ + expiration_date = "2034-05-22T18:29:59Z"
+ + id = (known after apply)
+ + key_opts = [
+ + "encrypt",
+ + "decrypt",
+ + "sign",
+ + "verify",
+ + "wrapKey",
+ + "unwrapKey",
+ ]
+ + key_size = 2048
+ + key_type = "RSA"
+ + key_vault_id = (known after apply)
+ + n = (known after apply)
+ + name = "geo-cmk-key-core-dev-inci"
+ + public_key_openssh = (known after apply)
+ + public_key_pem = (known after apply)
+ + resource_id = (known after apply)
+ + resource_versionless_id = (known after apply)
+ + version = (known after apply)
+ + versionless_id = (known after apply)
+ + x = (known after apply)
+ + y = (known after apply)
+ }
+
+ # module.flexible-mysql.azurerm_key_vault_key.primary_cmk_key[0] will be created
+ + resource "azurerm_key_vault_key" "primary_cmk_key" {
+ + curve = (known after apply)
+ + e = (known after apply)
+ + expiration_date = "2034-05-22T18:29:59Z"
+ + id = (known after apply)
+ + key_opts = [
+ + "encrypt",
+ + "decrypt",
+ + "sign",
+ + "verify",
+ + "wrapKey",
+ + "unwrapKey",
+ ]
+ + key_size = 2048
+ + key_type = "RSA"
+ + key_vault_id = (known after apply)
+ + n = (known after apply)
+ + name = "s-cmk-key-core-dev-inci"
+ + public_key_openssh = (known after apply)
+ + public_key_pem = (known after apply)
+ + resource_id = (known after apply)
+ + resource_versionless_id = (known after apply)
+ + version = (known after apply)
+ + versionless_id = (known after apply)
+ + x = (known after apply)
+ + y = (known after apply)
+ }
+
+ # module.flexible-mysql.azurerm_monitor_diagnostic_setting.mysql[0] will be created
+ + resource "azurerm_monitor_diagnostic_setting" "mysql" {
+ + id = (known after apply)
+ + log_analytics_destination_type = "AzureDiagnostics"
+ + log_analytics_workspace_id = (known after apply)
+ + name = "mysql-diagnostic-log-core-dev-inci"
+ + target_resource_id = (known after apply)
+
+ + enabled_log {
+ + category = "MySqlAuditLogs"
+ # (1 unchanged attribute hidden)
+ }
+
+ + enabled_metric (known after apply)
+
+ + metric {
+ + category = "AllMetrics"
+ + enabled = true
+ }
+ }
+
+ # module.flexible-mysql.azurerm_mysql_flexible_database.main[0] will be created
+ + resource "azurerm_mysql_flexible_database" "main" {
+ + charset = "utf8mb3"
+ + collation = "utf8mb3_unicode_ci"
+ + id = (known after apply)
+ + name = "maindb"
+ + resource_group_name = "rg-core-dev-inci"
+ + server_name = "pgsql-fs-core-dev-inci"
+ }
+
+ # module.flexible-mysql.azurerm_mysql_flexible_server.main[0] will be created
+ + resource "azurerm_mysql_flexible_server" "main" {
+ + administrator_login = "mysqlusername"
+ + administrator_password = (sensitive value)
+ + administrator_password_wo = (write-only attribute)
+ + backup_retention_days = 7
+ + create_mode = "Default"
+ + delegated_subnet_id = (known after apply)
+ + fqdn = (known after apply)
+ + geo_redundant_backup_enabled = true
+ + id = (known after apply)
+ + location = "centralindia"
+ + name = "pgsql-fs-core-dev-inci"
+ + private_dns_zone_id = (known after apply)
+ + public_network_access = (known after apply)
+ + public_network_access_enabled = (known after apply)
+ + replica_capacity = (known after apply)
+ + replication_role = (known after apply)
+ + resource_group_name = "rg-core-dev-inci"
+ + sku_name = "GP_Standard_D8ds_v4"
+ + version = "8.0.21"
+ + zone = "1"
+
+ + customer_managed_key {
+ + geo_backup_key_vault_key_id = (known after apply)
+ + geo_backup_user_assigned_identity_id = (known after apply)
+ + key_vault_key_id = (known after apply)
+ + primary_user_assigned_identity_id = (known after apply)
+ }
+
+ + identity {
+ + identity_ids = (known after apply)
+ + type = "UserAssigned"
+ }
+
+ + storage {
+ + auto_grow_enabled = true
+ + io_scaling_enabled = false
+ + iops = 360
+ + log_on_disk_enabled = false
+ + size_gb = 20
+ }
+ }
+
+ # module.flexible-mysql.azurerm_mysql_flexible_server_configuration.main[0] will be created
+ + resource "azurerm_mysql_flexible_server_configuration" "main" {
+ + id = (known after apply)
+ + name = "interactive_timeout"
+ + resource_group_name = "rg-core-dev-inci"
+ + server_name = "pgsql-fs-core-dev-inci"
+ + value = "600"
+ }
+
+ # module.flexible-mysql.azurerm_mysql_flexible_server_configuration.main[1] will be created
+ + resource "azurerm_mysql_flexible_server_configuration" "main" {
+ + id = (known after apply)
+ + name = "audit_log_enabled"
+ + resource_group_name = "rg-core-dev-inci"
+ + server_name = "pgsql-fs-core-dev-inci"
+ + value = "ON"
+ }
+
+ # module.flexible-mysql.azurerm_mysql_flexible_server_configuration.main[2] will be created
+ + resource "azurerm_mysql_flexible_server_configuration" "main" {
+ + id = (known after apply)
+ + name = "audit_log_events"
+ + resource_group_name = "rg-core-dev-inci"
+ + server_name = "pgsql-fs-core-dev-inci"
+ + value = "CONNECTION,ADMIN,DDL,TABLE_ACCESS"
+ }
+
+ # module.flexible-mysql.azurerm_role_assignment.geo_cmk_role_assignment[0] will be created
+ + resource "azurerm_role_assignment" "geo_cmk_role_assignment" {
+ + condition_version = (known after apply)
+ + id = (known after apply)
+ + name = (known after apply)
+ + principal_id = (known after apply)
+ + principal_type = (known after apply)
+ + role_definition_id = (known after apply)
+ + role_definition_name = "Key Vault Crypto Service Encryption User"
+ + scope = (known after apply)
+ + skip_service_principal_aad_check = (known after apply)
+ }
+
+ # module.flexible-mysql.azurerm_role_assignment.primary_cmk_role_assignment[0] will be created
+ + resource "azurerm_role_assignment" "primary_cmk_role_assignment" {
+ + condition_version = (known after apply)
+ + id = (known after apply)
+ + name = (known after apply)
+ + principal_id = (known after apply)
+ + principal_type = (known after apply)
+ + role_definition_id = (known after apply)
+ + role_definition_name = "Key Vault Crypto Service Encryption User"
+ + scope = (known after apply)
+ + skip_service_principal_aad_check = (known after apply)
+ }
+
+ # module.flexible-mysql.azurerm_user_assigned_identity.geo_cmk_umi[0] will be created
+ + resource "azurerm_user_assigned_identity" "geo_cmk_umi" {
+ + client_id = (known after apply)
+ + id = (known after apply)
+ + location = "centralindia"
+ + name = "core-dev-inci-cmk-geo-identity"
+ + principal_id = (known after apply)
+ + resource_group_name = "rg-core-dev-inci"
+ + tenant_id = (known after apply)
+ }
+
+ # mSee the full diff from https://github.com/terraform-az-modules/terraform-azure-flexible-mysql/actions/runs/18132083853 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements to the Azure MySQL module, including support for Customer-Managed Keys (CMK), private DNS integration, and Entra authentication. It also overhauls the module's variables, outputs, and documentation. While the new features are valuable, the review identified several issues that need attention. These include a critical cyclic dependency in the example code, incorrect resource naming conventions (e.g., pgsql instead of mysql), incomplete logic for restore scenarios, and improper tag handling. Additionally, there are multiple inconsistencies in the documentation and example files, such as outdated links and hardcoded secrets. Addressing these points will greatly improve the module's correctness, security, and usability.
|
|
||
| # Name of this project | ||
| name : Terraform Azure Module Template | ||
| name : Terraform Azure Private DNS |
There was a problem hiding this comment.
The project name is set to Terraform Azure Private DNS, but this module is for Azure Flexible MySQL. Please update the name to reflect the module's purpose.
name : Terraform Azure Flexible MySQL| ##----------------------------------------------------------------------------- | ||
| resource "azurerm_mysql_flexible_server" "main" { | ||
| count = var.enabled ? 1 : 0 | ||
| name = var.resource_position_prefix ? format("pgsql-fs-%s", local.name) : format("%s-pgsql-fs", local.name) |
There was a problem hiding this comment.
The resource name format uses pgsql-fs, which appears to be a copy-paste from a PostgreSQL module. Since this is a MySQL module, it should be mysql-fs for consistency and clarity.
name = var.resource_position_prefix ? format("mysql-fs-%s", local.name) : format("%s-mysql-fs", local.name)
| geo_redundant_backup_enabled = var.geo_redundant_backup_enabled | ||
| point_in_time_restore_time_in_utc = var.create_mode == "PointInTimeRestore" ? var.point_in_time_restore_time_in_utc : null | ||
| replication_role = var.replication_role | ||
| source_server_id = var.create_mode == "PointInTimeRestore" ? var.source_server_id : null |
There was a problem hiding this comment.
The condition for setting source_server_id is incomplete. According to the Azure provider documentation, source_server_id is required when create_mode is Replica, GeoRestore, or PointInTimeRestore. The current logic only covers PointInTimeRestore, which will cause failures for the other restore modes.
source_server_id = contains(["Replica", "GeoRestore", "PointInTimeRestore"], var.create_mode) ? var.source_server_id : null
| <img title="Share on LinkedIn" src="https://user-images.githubusercontent.com/50652676/62817742-4e339e80-bb59-11e9-87b9-a1f68cae1049.png" /> | ||
| </a> | ||
| <a href='https://twitter.com/intent/tweet/?text=Terraform+Module+Template&url=https://github.com/clouddrove/terraform-module-template'> | ||
| <a href='https://twitter.com/intent/tweet/?text=Terraform+Module+Template&url=https://github.com/clouddrove/terraform-azure-flexible-mysql'> |
There was a problem hiding this comment.
The text in the Twitter share URL is still using the template's name. It should be updated to reflect this module's name for accurate sharing.
| <a href='https://twitter.com/intent/tweet/?text=Terraform+Module+Template&url=https://github.com/clouddrove/terraform-azure-flexible-mysql'> | |
| <a href='https://twitter.com/intent/tweet/?text=Terraform+Azure+Flexible+MySQL&url=https://github.com/clouddrove/terraform-azure-flexible-mysql'> |
| | <a name="input_private_dns"></a> [private\_dns](#input\_private\_dns) | Enable private DNS integration. | `bool` | `false` | no | | ||
| | <a name="input_registration_enabled"></a> [registration\_enabled](#input\_registration\_enabled) | Enable auto-registration of VM records in the Private DNS zone. | `bool` | `false` | no | | ||
| | <a name="input_replication_role"></a> [replication\_role](#input\_replication\_role) | Replication role for the MySQL Flexible Server (e.g., 'None'). | `string` | `null` | no | | ||
| | <a name="input_repository"></a> [repository](#input\_repository) | Module source repository URL. | `string` | `"https://github.com/terraform-az-modules/terraform-azure-vnet"` | no | |
There was a problem hiding this comment.
The default value for the repository input in the documentation points to a virtual network module. This should be updated to point to the correct repository for this MySQL module.
| | <a name="input_repository"></a> [repository](#input\_repository) | Module source repository URL. | `string` | `"https://github.com/terraform-az-modules/terraform-azure-vnet"` | no | | |
| | <a name="input_repository"></a> [repository](#input\_repository) | Module source repository URL. | `string` | `"https://github.com/terraform-az-modules/terraform-azure-flexible-mysql"` | no | |
| | <a name="input_resource_position_prefix"></a> [resource\_position\_prefix](#input\_resource\_position\_prefix) | Controls placement of the resource type keyword (e.g., "vnet", "ddospp") in resource names.<br><br>- If true, the keyword is prepended: "vnet-core-dev".<br>- If false, the keyword is appended: "core-dev-vnet".<br><br>Maintains naming consistency based on organizational preferences. | `bool` | `true` | no | | ||
| | <a name="input_role_definition_name"></a> [role\_definition\_name](#input\_role\_definition\_name) | Name of the Role Definition assigned for Key Vault crypto operations. | `string` | `"Key Vault Crypto Service Encryption User"` | no | | ||
| | <a name="input_server_configuration_names"></a> [server\_configuration\_names](#input\_server\_configuration\_names) | List of MySQL server configuration option names. | `list(string)` | `[]` | no | | ||
| | <a name="input_size_gb"></a> [size\_gb](#input\_size\_gb) | Maximum storage size in GB; valid range 20 to 16,384. | `string` | `"20"` | no | |
There was a problem hiding this comment.
The type for size_gb is documented as string, but the variable is defined as a number in variables.tf. Please update the documentation to reflect the correct type for consistency.
| | <a name="input_size_gb"></a> [size\_gb](#input\_size\_gb) | Maximum storage size in GB; valid range 20 to 16,384. | `string` | `"20"` | no | | |
| | <a name="input_size_gb"></a> [size\_gb](#input\_size\_gb) | Maximum storage size in GB; valid range 20 to 16,384. | `number` | `20` | no | |
|
|
||
| resource "azurerm_user_assigned_identity" "geo_cmk_umi" { | ||
| count = var.geo_redundant_backup_enabled && var.cmk_enabled ? 1 : 0 | ||
| name = format("%s-cmk-geo-identity", module.labels.id) |
There was a problem hiding this comment.
The naming convention for this user-assigned identity is inconsistent with the primary_cmk_umi resource. For consistency, please apply the same naming logic using local.name and var.resource_position_prefix.
name = var.resource_position_prefix ? format("cmk-geo-identity-%s", local.name) : format("%s-cmk-geo-identity", local.name)
|
|
||
| variable "expiration_date" { | ||
| type = string | ||
| default = "2034-05-22T18:29:59Z" |
There was a problem hiding this comment.
Using a hardcoded future date as a default for expiration_date is not ideal. It's better to set the default to null so that the key does not expire by default, allowing users to opt-in to an expiration date if needed.
default = null
|
💤 This PR has been automatically marked as stale because it has been inactive |
Description
Deployed resource:
Fixes #
Closes #
Type of Change
Checklist