Skip to content
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

azurerm_monitor_diagnostic_setting - fix log_analytics_destination_type #20203

Conversation

teowa
Copy link
Contributor

@teowa teowa commented Jan 28, 2023

resolves #20140

The issue is caused by service API behavior change and related to #20048:

The above API behavior change happened after PR #20048 (set log_analytics_destination_type default to AzureDiagnostics) was merged. Days ago the API also returns AzureDiagnostics for resource like Key Vault, this can be proved by daily test record, it works well before Jan 26, and PR #20048 was merged before Jan 19.

From this doc:

Most Azure resources write data to the workspace in either Azure diagnostics or resource-specific mode without giving you a choice. For more information, see Common and service-specific schemas for Azure resource logs.

All Azure services eventually use the resource-specific mode. As part of this transition, some resources allow you to select a mode in the diagnostic setting. Specify resource-specific mode for any new diagnostic settings because this mode makes the data easier to manage. It also might help you avoid complex migrations later.

@teowa teowa changed the title azurerm_monitor_diagnostic_setting - set log_analytics_destination_type inferred value to AzureDiagnostics azurerm_monitor_diagnostic_setting - fix log_analytics_destination_type Jan 31, 2023
@Klaas-
Copy link
Contributor

Klaas- commented Jan 31, 2023

@teowa the doc you linked, https://github.com/MicrosoftDocs/azure-docs/blob/e294e228cf8249e82172695143e0a21084636b67/articles/azure-monitor/essentials/resource-logs.md#select-the-collection-mode , it says that we should use Dedicated because AzureDiagnostics will be removed in future, right? So we should maybe put a deprecaion on that and change default in 4.0 or something like that?

@teowa
Copy link
Contributor Author

teowa commented Feb 2, 2023

As I tested, for some target resources (e.g., KeyVault) this property is not settable, and for other resources (e.g., DataFactory) it still works well, see details in Azure/azure-rest-api-specs#22400. I am still contacting with service team about the details of the affected resource types. Temporarily close this and once a solution is determined I am going to reopen this PR.

@teowa teowa closed this Feb 2, 2023
@teowa teowa reopened this Feb 9, 2023
@github-actions github-actions bot added size/L and removed size/XS labels Feb 9, 2023
@teowa
Copy link
Contributor Author

teowa commented Feb 9, 2023

The two cases of log_analytics_destination_type property:

  1. for resources (Keyvault), the property is no longer applicable, either set AzureDiagnostics or Dedicated, returns null
  2. for others (DataFactory), defaults to AzureDiagnostics but can also be changed to Dedicated.

We are asking service team to change case1 to return an error. And I think it's better to change this property to Computed for now, from the doc, case1 is more common and set this as computed can remove the diff noise for users who dont set this property.

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

LGTM 📓

@katbyte katbyte merged commit 22aff45 into hashicorp:main Feb 24, 2023
@github-actions github-actions bot added this to the v3.45.0 milestone Feb 24, 2023
katbyte added a commit that referenced this pull request Feb 24, 2023
@github-actions
Copy link

This functionality has been released in v3.45.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants