add msi_auth_for_monitoring_enabled by admincasper · Pull Request #446 · Azure/terraform-azurerm-aks

admincasper · 2023-09-21T09:29:14Z

No description provided.

admincasper · 2023-09-21T09:30:34Z

@microsoft-github-policy-service agree

zioproto · 2023-09-21T12:14:00Z

@admincasper thanks for your contribution. Could you please run the pre-commit and pr-check steps like described in our module docs ? https://github.com/Azure/terraform-azurerm-aks#pre-commit--pr-check--test

Thanks

admincasper · 2023-09-21T12:33:58Z

Just ran both pre-commit and pr-check.
They ran without error, pushed the formatted code.

zioproto · 2023-09-21T12:38:37Z


    content {
      log_analytics_workspace_id = local.log_analytics_workspace.id
+      msi_auth_for_monitoring_enabled = var.msi_auth_for_monitoring_enabled


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

No need to bump versions. All good.

zioproto · 2023-09-21T12:42:01Z

Just ran both pre-commit and pr-check. They ran without error, pushed the formatted code.

I dont see the new changes. main.tf had a formatting issue, and there must be a patch to the Readme.

zioproto · 2023-09-21T12:43:06Z


+variable "msi_auth_for_monitoring_enabled" {
+  type        = bool
+  default     = false


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

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.

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

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

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?

@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.

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

lonegunmanb

Hi @admincasper thanks for opening this pr! Almost LGTM but some review comments to be solved.

lonegunmanb · 2023-09-22T01:55:07Z


+variable "msi_auth_for_monitoring_enabled" {
+  type        = bool
+  default     = false


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?

lonegunmanb · 2023-09-22T01:56:00Z

+variable "msi_auth_for_monitoring_enabled" {
+  type        = bool
+  default     = false
+  description = "(Optional) Is managed identity authentication for monitoring enabled? Defaults to `false`"


We can remove "Defaults to false" here

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

Defaults to `false`

Thanks

@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?

@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

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

lonegunmanb · 2023-09-22T01:56:41Z

+  type        = bool
+  default     = false
+  description = "(Optional) Is managed identity authentication for monitoring enabled? Defaults to `false`"
+  nullable    = false


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

avekrivoy · 2023-10-03T16:03:02Z

@admincasper did you manage to resolve all the issues? Looking forward to the feature

admincasper · 2023-10-04T06:57:21Z

@admincasper did you manage to resolve all the issues? Looking forward to the feature

I thought I had already done so.. I couldn't figure out what @lonegunmanb meant..

I actually think this works okay already? I don't understand why msi_auth_for_monitoring_enabled != null should create oms_agent block.

If log_analytics_workspace_enabled = true, and msi_auth_for_monitoring_enabled = null:
oms_agent block is OK

If log_analytics_workspace_enabled = true, and msi_auth_for_monitoring_enabled = True || False:
oms_agent block is OK

If log_analytics_workspace_enabled = false:
oms_agent block is EMPTY (N/A).

avekrivoy · 2023-10-04T08:01:09Z

@lonegunmanb could you please take a look?

lonegunmanb · 2023-10-13T02:34:29Z

@admincasper @zioproto Apology for the confusion.

@lonegunmanb: I think adding msi_auth_for_monitoring_enabled just make this for_each complex, now if var.log_analytics_workspace_enabled is true OR var.msi_auth_for_monitoring_enabled is not null should create an oms_agent block. Would you please add a new locals expression in locals.tf file as a new toggle and use it here?

My representation was incorrect, as @admincasper pointed out.

@admincasper 3. If log_analytics_workspace_enabled = false: oms_agent block is EMPTY (N/A).

I've checked the current logic:

dynamic "oms_agent" {
    for_each = var.log_analytics_workspace_enabled ? ["oms_agent"] : []

    content {
      log_analytics_workspace_id = local.log_analytics_workspace.id
    }
  }

I think it is possible that var.log_analytics_workspace_enabled = true and local.log_analytics_workspace.id = null generate an empty block, now I understand that it's irrelevant to this pr.

I'd like to open a separate pr to add a precondition block to prevent such empty oms_agent block once this pr has been merged, thanks @zioproto for your suggestion!

lonegunmanb · 2023-10-13T02:42:32Z

@admincasper Only one issue left, modify your new variable's description then I think we're good to go.

lonegunmanb

A minor update for line 695 would unblock this pr from testing.

admincasper · 2023-10-13T07:24:51Z

A minor update for line 695 would unblock this pr from testing.

Done

zioproto · 2023-10-13T07:40:09Z

https://learn.microsoft.com/en-us/azure/azure-monitor/containers/container-insights-enable-aks?tabs=azure-cli#existing-clusters-with-system-or-user-assigned-identity

A minor update for line 695 would unblock this pr from testing.

Done

Have you run again the pre-commit and pr-check steps ? it seems the README is not up to date

admincasper · 2023-10-13T08:41:09Z

https://learn.microsoft.com/en-us/azure/azure-monitor/containers/container-insights-enable-aks?tabs=azure-cli#existing-clusters-with-system-or-user-assigned-identity

A minor update for line 695 would unblock this pr from testing.

Done

Have you run again the pre-commit and pr-check steps ? it seems the README is not up to date

Done

zioproto

LGTM ! @lonegunmanb please merge

avekrivoy · 2023-10-18T08:39:17Z

@lonegunmanb could you please merge it? without this feature we can't control metrics scrape period time

avekrivoy · 2023-10-23T07:19:20Z

@zioproto could you please merge those changes? @lonegunmanb is clearly not around and this tiny PR from a volunteer has already taken a month

lonegunmanb

Apology for the late reply @admincasper. LGTM.

* add msi_auth_for_monitoring_enabled

add msi_auth_for_monitoring_enabled

a4f6084

admincasper mentioned this pull request Sep 21, 2023

Support for setting msi_auth_for_monitoring_enabled in oms_agent #447

Closed

1 task

update

4b96aa2

zioproto reviewed Sep 21, 2023

View reviewed changes

lonegunmanb requested changes Sep 22, 2023

View reviewed changes

admincasper added 2 commits September 22, 2023 11:33

fix

218e831

remove nullable

498e79a

admincasper requested a review from lonegunmanb October 4, 2023 08:21

admincasper added 2 commits October 12, 2023 18:46

remove default = null

f71d278

set default = null again

46b5902

lonegunmanb requested changes Oct 13, 2023

View reviewed changes

admincasper temporarily deployed to acctests October 13, 2023 06:46 — with GitHub Actions Inactive

remove defaults to null in description

7807414

pr-check and pre-commit

7935982

zioproto approved these changes Oct 13, 2023

View reviewed changes

lonegunmanb approved these changes Oct 23, 2023

View reviewed changes

lonegunmanb merged commit a07880a into Azure:main Oct 23, 2023

admincasper deleted the add-msi_auth_for_monitoring_enabled branch October 23, 2023 09:59

skolobov pushed a commit to skolobov/terraform-azurerm-aks that referenced this pull request Oct 29, 2023

add msi_auth_for_monitoring_enabled (Azure#446)

8de23a7

* add msi_auth_for_monitoring_enabled

lonegunmanb added this to the 7.5.0 milestone Oct 31, 2023

admincasper had a problem deploying to acctests November 22, 2023 08:12 — with GitHub Actions Failure

This was referenced Dec 18, 2024

Container insights not working when using existing Log Analytics Workspace #607

Closed

Implement Data Collection Rule for Container Insights #623

Merged

Conversation

admincasper commented Sep 21, 2023

Uh oh!

admincasper commented Sep 21, 2023

Uh oh!

zioproto commented Sep 21, 2023

Uh oh!

admincasper commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zioproto commented Sep 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

admincasper Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

admincasper Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lonegunmanb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

avekrivoy commented Oct 3, 2023

Uh oh!

admincasper commented Oct 4, 2023

Uh oh!

avekrivoy commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lonegunmanb commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lonegunmanb commented Oct 13, 2023

Uh oh!

lonegunmanb left a comment

Choose a reason for hiding this comment

Uh oh!

admincasper commented Oct 13, 2023

Uh oh!

zioproto commented Oct 13, 2023

Uh oh!

admincasper commented Oct 13, 2023

Uh oh!

zioproto left a comment

Choose a reason for hiding this comment

Uh oh!

avekrivoy commented Oct 18, 2023

Uh oh!

avekrivoy commented Oct 23, 2023

Uh oh!

lonegunmanb left a comment

Choose a reason for hiding this comment

admincasper commented Sep 21, 2023 •

edited

Loading

admincasper Sep 21, 2023 •

edited

Loading

admincasper Sep 21, 2023 •

edited

Loading

avekrivoy commented Oct 4, 2023 •

edited

Loading

lonegunmanb commented Oct 13, 2023 •

edited

Loading