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.

Sign up for GitHub

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

add msi_auth_for_monitoring_enabled #446

Merged
merged 8 commits into from
Oct 23, 2023
Merged

add msi_auth_for_monitoring_enabled #446

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a4f6084
add msi_auth_for_monitoring_enabled
admincasper Sep 21, 2023
4b96aa2
update
admincasper Sep 21, 2023
218e831
fix
admincasper Sep 22, 2023
498e79a
remove nullable
admincasper Sep 22, 2023
f71d278
remove default = null
admincasper Oct 12, 2023
46b5902
set default = null again
admincasper Oct 12, 2023
7807414
remove defaults to null in description
admincasper Oct 13, 2023
7935982
pr-check and pre-commit
admincasper Oct 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,7 @@ resource "azurerm_kubernetes_cluster" "main" {

content {
log_analytics_workspace_id = local.log_analytics_workspace.id
msi_auth_for_monitoring_enabled = var.msi_auth_for_monitoring_enabled
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is already present in provider version 3.47.0:
hashicorp/terraform-provider-azurerm#20757

No need to bump versions. All good.

}
}
dynamic "service_principal" {
Expand Down
7 changes: 7 additions & 0 deletions variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -689,6 +689,13 @@ variable "maintenance_window_node_os" {
EOT
}

variable "msi_auth_for_monitoring_enabled" {
type = bool
default = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why you have the default at false and not null ?

https://github.com/hashicorp/terraform-provider-azurerm/blob/8078e1bcdb8503e6c3a581f91aa3b201c4545f5c/internal/services/containers/kubernetes_addons.go#L121

Copy link
Contributor Author

@admincasper admincasper Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

When running module aks with log_analytics_workspace_enabled = true without specifying msi_auth_for_monitoring_enabled the value is false by default. I checked by running az aks addon show --addon monitoring.

So I thought default value was false.

Copy link
Contributor Author

@admincasper admincasper Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It is weird, because enabling monitoring addon with az aks CLI the default value is True..
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also is nullable = false, I interpreted it as only possible values should be either False or True? Correct me if I'm wrong

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @zioproto, according to the provider schema, this msi_auth_for_monitoring_enabled argument's default value is null, setting default value to false could cause a configuration drift. Would you please set the default value to null?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@admincasper It is often the case that Terraform has true false and null. But when you read back from the azurerm API the values are just true or false. Now we are interested if this value is explicit in the Terraform state as false. I hope this clarifies why you dont get back null from your az aks show operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah thanks for clarifying! I've pushed up the changes.

description = "(Optional) Is managed identity authentication for monitoring enabled? Defaults to `false`"
Copy link
Member

Choose a reason for hiding this comment

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

We can remove "Defaults to false" here

Copy link
Collaborator

Choose a reason for hiding this comment

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

@admincasper on line 695 at the end of the description string, can you remove:

Defaults to `false`

Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@admincasper on line 695 at the end of the description string, can you remove:

Defaults to `false`

Thanks

@zioproto I think it needs to default to null. So I changed it to null.

I think everything should be ok now then?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@admincasper line 694 is ok, the correct default is null. don't touch it. In line 695 you have to remove the last statement in the description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zioproto Done. Removed "Defaults to 'null'" in description.

nullable = false
Copy link
Member

Choose a reason for hiding this comment

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

This variable could be set to null, we can just remove this setting.

}

variable "microsoft_defender_enabled" {
type = bool
default = false
Expand Down