-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Support for sql auditing to Azure Monitor #10324
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.
Thanks, I really like this PR! I started basically the same yesterday, but yours is far more complete!!
I have a few small change suggestions for the examples, code looks good to me but I leave that to one of the HC folks to decide 😅
metric { | ||
category = "AllMetrics" | ||
|
||
retention_policy { | ||
enabled = false | ||
} | ||
} |
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.
I think this metric should be enabled/disabled, right? Maybe only for clarity reasons, because I don't know what the default behavior is 😅
metric { | |
category = "AllMetrics" | |
retention_policy { | |
enabled = false | |
} | |
} | |
metric { | |
category = "AllMetrics" | |
enabled = false | |
retention_policy { | |
enabled = false | |
} | |
} |
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.
Thanks for your comments. I copies the monitor_diagnotics_setting
example from its acctest . And it should be by default true
. But I'll wait to see hashicorp's review comments. Because here we should try to avoid ignoring changes for log
and metrics
.
metric { | ||
category = "AllMetrics" | ||
|
||
retention_policy { | ||
enabled = false | ||
} | ||
} |
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.
metric { | |
category = "AllMetrics" | |
retention_policy { | |
enabled = false | |
} | |
} | |
metric { | |
category = "AllMetrics" | |
enabled = false | |
retention_policy { | |
enabled = false | |
} | |
} |
|
||
metric { | ||
category = "AllMetrics" | ||
|
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.
I think this needs an enabled=false
enabled = false | |
|
||
metric { | ||
category = "AllMetrics" | ||
|
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.
Same
enabled = false | |
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.
thanks @yupwei68 - overall this looks goof but i wonder if we should change the property name to something that indicates its log analytics related? see my inline comment
azurerm/internal/services/mssql/helper/sql_extended_auditing.go
Outdated
Show resolved
Hide resolved
Hi @katbyte Thanks for your comments. Changes have been pushed. Please continue reviewing. |
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.
Thanks @yupwei68 - this LGTM 👍
This has been released in version 2.50.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example: provider "azurerm" {
version = "~> 2.50.0"
}
# ... other configuration ... |
This example shows how to enable auditing for database. But is it possible to enable it on server level? It is possible to do via Web GUI, "auditing" tab has options: Storage, Log Analytics and Event Hub https://docs.microsoft.com/en-us/azure/azure-sql/database/auditing-overview |
@varnav I believe the solution is to add a data resource for the data "azurerm_mssql_database" "master" {
name = "master"
server_id = azurerm_mssql_server.default.id
}
resource "azurerm_mssql_server_extended_auditing_policy" "example" {
server_id = azurerm_mssql_server.default.id
monitor_enabled = true
}
data "azurerm_monitor_diagnostic_categories" "default" {
resource_id = data.azurerm_mssql_database.master.id
}
// all diagnostics enabled
resource "azurerm_monitor_diagnostic_setting" "default" {
name = "diagnostics"
target_resource_id = data.azurerm_mssql_database.master.id
log_analytics_workspace_id = azurerm_log_analytics_workspace.default.id
dynamic "log" {
iterator = log_category
for_each = data.azurerm_monitor_diagnostic_categories.default.logs
content {
category = log_category.value
enabled = true
retention_policy {
enabled = true
days = 0
}
}
}
dynamic "metric" {
iterator = metric_category
for_each = data.azurerm_monitor_diagnostic_categories.default.metrics
content {
category = metric_category.value
enabled = true
retention_policy {
enabled = true
days = 0
}
}
}
} I'll try it and look if I can add a test and example for it |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks! |
Fix: #7695
#10100
#6849
=== RUN TestAccMsSqlDatabaseExtendedAuditingPolicy_basic
=== PAUSE TestAccMsSqlDatabaseExtendedAuditingPolicy_basic
=== CONT TestAccMsSqlDatabaseExtendedAuditingPolicy_basic
--- PASS: TestAccMsSqlDatabaseExtendedAuditingPolicy_basic (358.57s)
=== RUN TestAccMsSqlDatabaseExtendedAuditingPolicy_requiresImport
=== PAUSE TestAccMsSqlDatabaseExtendedAuditingPolicy_requiresImport
=== CONT TestAccMsSqlDatabaseExtendedAuditingPolicy_requiresImport
--- PASS: TestAccMsSqlDatabaseExtendedAuditingPolicy_requiresImport (391.47s)
=== RUN TestAccMsSqlDatabaseExtendedAuditingPolicy_complete
=== PAUSE TestAccMsSqlDatabaseExtendedAuditingPolicy_complete
=== CONT TestAccMsSqlDatabaseExtendedAuditingPolicy_complete
--- PASS: TestAccMsSqlDatabaseExtendedAuditingPolicy_complete (422.68s)
=== RUN TestAccMsSqlDatabaseExtendedAuditingPolicy_update
=== PAUSE TestAccMsSqlDatabaseExtendedAuditingPolicy_update
=== CONT TestAccMsSqlDatabaseExtendedAuditingPolicy_update
--- PASS: TestAccMsSqlDatabaseExtendedAuditingPolicy_update (547.36s)
=== RUN TestAccMsSqlDatabaseExtendedAuditingPolicy_storageAccBehindFireWall
=== PAUSE TestAccMsSqlDatabaseExtendedAuditingPolicy_storageAccBehindFireWall
=== CONT TestAccMsSqlDatabaseExtendedAuditingPolicy_storageAccBehindFireWall
--- PASS: TestAccMsSqlDatabaseExtendedAuditingPolicy_storageAccBehindFireWall (354.62s)
=== RUN TestAccMsSqlDatabase_withBlobAuditingPolices
=== PAUSE TestAccMsSqlDatabase_withBlobAuditingPolices
=== CONT TestAccMsSqlDatabase_withBlobAuditingPolices
--- PASS: TestAccMsSqlDatabase_withBlobAuditingPolices (564.62s)
=== RUN TestAccMsSqlDatabase_withBlobAuditingPolicesForMonitor
=== PAUSE TestAccMsSqlDatabase_withBlobAuditingPolicesForMonitor
=== CONT TestAccMsSqlDatabase_withBlobAuditingPolicesForMonitor
--- PASS: TestAccMsSqlDatabase_withBlobAuditingPolicesForMonitor (803.68s)
All regression tests have passed.