bugfix: Make the Azure Defender clause robust against a non-existent … by gzur · Pull Request #258 · Azure/terraform-azurerm-aks

gzur · 2022-09-27T13:10:14Z

Fixes #257

Changes proposed in the pull request:

Support setting var.microsoft_defender_enabled to false and supplying your own analytics workspace to Microsoft Defender.

Signed-off-by: Gissur Þórhallsson gissur@skyvafnir.is

…`azurerm_log_analytics_workspace` Signed-off-by: Gissur Þórhallsson <gissur@skyvafnir.is>

github-actions · 2022-09-28T04:55:46Z

MAIN BRANCH PUSH DETECTED DUE TO #259, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

lonegunmanb · 2022-09-28T05:04:07Z

Hi @gzur thanks a lot for opening this pr! Unfortunately we've got a bug in our CI pipeline code and I've just fixed it, would you please merge your branch with the latest master branch so I can re-run the failed job again? Thanks!

lonegunmanb · 2022-09-28T05:38:58Z

Btw I've reviewed your code, almost LGTM but I've got a new idea. How about adding a new local like this right behind the workspace resource:

locals {
  log_analytics_workspace = var.log_analytics_workspace_enabled ? (
    var.log_analytics_workspace == null ? {
      id   = azurerm_log_analytics_workspace.main[0].id
      name = azurerm_log_analytics_workspace.main[0].name
      } : {
      id   = var.log_analytics_workspace.id
      name = var.log_analytics_workspace.name
    }
    ) : {
    id   = null
    name = null
  }
}

Then we change all references to workspace id and name to this local, sounds good?

lonegunmanb

Please merge with the latest master branch, thanks for your understanding.

github-actions · 2022-09-28T08:26:22Z

MAIN BRANCH PUSH DETECTED DUE TO #262, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

…fix-disabled-log_analytics_workspace Signed-off-by: Gissur Þórhallsson <gissur@skyvafnir.is>

gzur · 2022-09-28T12:05:02Z

Btw I've reviewed your code, almost LGTM but I've got a new idea. How about adding a new local like this right behind the workspace resource:
locals {
  log_analytics_workspace = var.log_analytics_workspace_enabled ? (
    var.log_analytics_workspace == null ? {
      id   = azurerm_log_analytics_workspace.main[0].id
      name = azurerm_log_analytics_workspace.main[0].name
      } : {
      id   = var.log_analytics_workspace.id
      name = var.log_analytics_workspace.name
    }
    ) : {
    id   = null
    name = null
  }
}
Then we change all references to workspace id and name to this local, sounds good?

Sure thing.

But on a related note: we are introducing some fairly complex behaviour here. Do we have a framework for testing this?

…ogic and validation. In response to comment: Azure#258 (comment)

…ender.

ghost · 2022-09-28T13:52:57Z

All CLA requirements met.

…rkspace Signed-off-by: Gissur Þórhallsson <gissur@skyvafnir.is>

lonegunmanb · 2022-09-28T14:04:39Z

Btw I've reviewed your code, almost LGTM but I've got a new idea. How about adding a new local like this right behind the workspace resource:
locals {
  log_analytics_workspace = var.log_analytics_workspace_enabled ? (
    var.log_analytics_workspace == null ? {
      id   = azurerm_log_analytics_workspace.main[0].id
      name = azurerm_log_analytics_workspace.main[0].name
      } : {
      id   = var.log_analytics_workspace.id
      name = var.log_analytics_workspace.name
    }
    ) : {
    id   = null
    name = null
  }
}
Then we change all references to workspace id and name to this local, sounds good?
Sure thing.

But on a related note: we are introducing some fairly complex behaviour here. Do we have a framework for testing this?

Good question! What about we modify named_cluster folder in examples to create this log analysis workspace in root module and pass it via var.log_analytics_workspace so our e2e tests can cover both cases?

github-actions · 2022-09-28T14:07:08Z

MAIN BRANCH PUSH DETECTED DUE TO #260, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

gzur · 2022-09-28T14:09:17Z

Good question! What about we modify named_cluster folder in examples to create this log analysis workspace in root module and pass it via var.log_analytics_workspace so our e2e tests can cover both cases?

Sounds fun. I always wanted to try out how infrastructure tests work.

Should I modify the named_cluster test/example or duplicate it?

lonegunmanb · 2022-09-28T14:17:14Z

Good question! What about we modify named_cluster folder in examples to create this log analysis workspace in root module and pass it via var.log_analytics_workspace so our e2e tests can cover both cases?

Sounds fun. I always wanted to try out how infrastructure tests work.

Should I modify the named_cluster test/example or duplicate it?

You can modify examples/named_cluster/main.tf directly, our e2e test steps will apply every sub folders under examples to ensure that all our examples work.

lonegunmanb · 2022-09-28T14:20:58Z

Good question! What about we modify named_cluster folder in examples to create this log analysis workspace in root module and pass it via var.log_analytics_workspace so our e2e tests can cover both cases?

Sounds fun. I always wanted to try out how infrastructure tests work.

Should I modify the named_cluster test/example or duplicate it?

@gzur I had designed "unit tests" for our new CI pipeline but I haven't added it to this aks module yet. I'll add one to test this complex local after we've merged your pr, please stay tuned if you're interested in how to write quick and cheap tests for locals.

gzur · 2022-09-28T15:07:10Z

I'll add one to test this complex local after we've merged your pr, please stay tuned if you're interested in how to write quick and cheap tests for locals.

Ok. I'll hold off on diving into the tests until after we've merged this.

lonegunmanb

Thanks @gzur almost LGTM, only a few issues.

Btw we have a CI check issue, would you please run the following commands before you commit after you've finished your modification? Thanks in advance:

docker run --rm -v $(pwd):/src -w /src mcr.microsoft.com/azterraform:latest make pre-commit
docker run --rm -v $(pwd):/src -w /src mcr.microsoft.com/azterraform:latest make pr-check

If you're using Windows, please run the following commands:

docker run --rm -v ${pwd}:/src -w /src mcr.microsoft.com/azterraform:latest make pre-commit
docker run --rm -v ${pwd}:/src -w /src mcr.microsoft.com/azterraform:latest make pr-check

Once the pr-check command return with no error, you're good to commit.

lonegunmanb · 2022-09-29T05:28:47Z

+      id   = var.log_analytics_workspace.id
+      name = var.log_analytics_workspace.name
+    }
+  ) : null # Finally, the Log Analytics Workspace should be disabled.


Would an object with null field be better (the caller don't need to check whether this object is null)?

) : { id = null name = null }

But won't the caller just have to check whether local.log_analytics_workspace.id/name is null instead?

I feel that this breaks the Principle of Least Astonishment, since the module will have already decided that there is no log_analytics_workspace - based on the inputs.

In this scenario, I would not expect there to be an object of that type but with attributes as null.
I would expect the object itself to be null.

In Terraform assign null to an argument is equal to omit this argument, in some cases the caller doesn't need to check whether the log analytics workspace id is null or not, if the caller just assigns it to an optional argument, in that case we can save a null check. I personally prefer this "null safe" style, but I think your point also make sense. Please allow me to have more discuss with other people, thanks for your understanding.

did we reach a consensus on:

) : { id = null name = null }

vs.

) : null

?

github-actions · 2022-09-29T13:51:20Z

MAIN BRANCH PUSH DETECTED DUE TO #251, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

It was only used in decision making and made little sense.

lonegunmanb

Please allow me to have some discusses with others on the null object issue. Thanks for your understanding.

This reverts commit f10a149.

lonegunmanb

Thanks @gzur for your updating, I think we'll be ready to test this pr once we've rollbacked the count expression for azurerm_log_analytics_solution to the original version, and remove the tflint annotation. Thanks for your patience!

gzur · 2022-09-29T17:30:52Z

Thanks @gzur for your updating, I think we'll be ready to test this pr once we've rollbacked the count expression for azurerm_log_analytics_solution to the original version, and remove the tflint annotation. Thanks for your patience!

How about:

count = (local.create_analytics_workspace && var.log_analytics_solution_id == null) ? 1 : 0

so that we use our recently introduced control variable.

I feel that this preserves the original intent of var.log_analytics_solution_id while also making the module easier to reason about.

lonegunmanb · 2022-09-30T00:56:51Z

Thanks @gzur for your updating, I think we'll be ready to test this pr once we've rollbacked the count expression for azurerm_log_analytics_solution to the original version, and remove the tflint annotation. Thanks for your patience!

How about:
count = (local.create_analytics_workspace && var.log_analytics_solution_id == null) ? 1 : 0
so that we use our recently introduced control variable.

I feel that this preserves the original intent of var.log_analytics_solution_id while also making the module easier to reason about.

My apology, it was mid-night in my time zone because from Oct 1st to 7th it'll be national holiday in China and I'll be on vacation, so I'd like to close all remaining pull requests before that, apparently I've made a mistake. It should be:

count = var.log_analytics_workspace_enabled && var.log_analytics_solution_id == null ? 1 : 0

I think you've just pointed out a blind point of mine, that is we need a new local named local.create_analytics_solution just like local.create_analytics_workspace. Then the count expression will be:

count = local.create_analytics_solution ? 1 : 0

This local.create_analytics_solution will be:

create_analytics_workspace = var.log_analytics_workspace_enabled && var.log_analytics_solution_id == null

How does that sound?

lonegunmanb · 2022-09-30T01:10:39Z

An update, I've opened a new issue #263 to improve this var.log_analytics_solution_id and I'll submit a new pr to improve that after we merge this one.

github-actions · 2022-09-30T01:27:36Z

MAIN BRANCH PUSH DETECTED DUE TO #, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

github-actions · 2022-09-30T07:55:57Z

MAIN BRANCH PUSH DETECTED DUE TO #256, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

github-actions · 2022-09-30T07:58:21Z

MAIN BRANCH PUSH DETECTED DUE TO #, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

github-actions · 2022-09-30T10:50:09Z

MAIN BRANCH PUSH DETECTED DUE TO #248, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

github-actions · 2022-09-30T11:00:00Z

MAIN BRANCH PUSH DETECTED DUE TO #, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

…fix-disabled-log_analytics_workspace

gzur · 2022-09-30T12:27:33Z

My apology, it was mid-night in my time zone because from Oct 1st to 7th it'll be national holiday in China and I'll be on vacation, so I'd like to close all remaining pull requests before that, [...]

No worries. Let's see if we can't get it merged then :)

[...] It should be:

count = var.log_analytics_workspace_enabled && var.log_analytics_solution_id == null ? 1 : 0

I'm assuming you are referring to the azurerm_log_analytics_solution.main resource here - for which we are introducing the create_analytics_workspace control variable (per your suggestion above).

Right?

gzur · 2022-09-30T12:27:37Z

[...]
we need a new local named local.create_analytics_solution just like local.create_analytics_workspace.
[...]
How does that sound?

That sounds great. I'll put it in.

…n or not

gzur · 2022-09-30T12:38:12Z

@lonegunmanb - Done

lonegunmanb

Thanks @gzur , nice job and I cannot ask for more. It is the last pr we're going to merge for v6.1.0. LGTM!

bugfix: Make the Azure Defender clause robust against a non-existent …

32472bc

…`azurerm_log_analytics_workspace` Signed-off-by: Gissur Þórhallsson <gissur@skyvafnir.is>

lonegunmanb self-requested a review September 28, 2022 05:04

lonegunmanb requested changes Sep 28, 2022

View reviewed changes

Merge branch 'master' of github.com:Azure/terraform-azurerm-aks into …

8ac6c72

…fix-disabled-log_analytics_workspace Signed-off-by: Gissur Þórhallsson <gissur@skyvafnir.is>

gzur force-pushed the fix-disabled-log_analytics_workspace branch from 9f9a002 to 8ac6c72 Compare September 28, 2022 11:56

gzur added 3 commits September 28, 2022 13:50

Leverage the terraform locals clause to unify analytics workspace l…

58caf97

…ogic and validation. In response to comment: Azure#258 (comment)

Ensure the existence of an analytics workspace enabling Microsoft Def…

9d06fa5

…ender.

terraform fmt

5fe6e64

gzur added 3 commits September 28, 2022 13:59

Update docs to state that Microsoft Defender requires an Analytics Wo…

7fbe2ed

…rkspace Signed-off-by: Gissur Þórhallsson <gissur@skyvafnir.is>

Add link to analytics workspace var to defender var docs

eaab22d

Close parentheses

45e04d3

Merge branch 'Azure:master' into fix-disabled-log_analytics_workspace

cd498f6

lonegunmanb requested changes Sep 29, 2022

View reviewed changes

gzur added 2 commits September 29, 2022 14:09

Respond to comments

fb7f810

Apply changes made by automation suite

05ff54e

gzur commented Sep 29, 2022

View reviewed changes

Comment thread main.tf

Remove unused variable. I

f10a149

It was only used in decision making and made little sense.

lonegunmanb requested changes Sep 29, 2022

View reviewed changes

Comment thread main.tf

Comment thread variables.tf

Comment thread main.tf Outdated

Comment thread main.tf

Comment thread main.tf Outdated

gzur added 4 commits September 29, 2022 15:41

Revert "Remove unused variable."

38ac6d3

This reverts commit f10a149.

Instruct tflint to ignore unused variable

f393026

cleanup

63ffe5b

Refactor the Defender precondition expression

ae9e5d1

lonegunmanb requested changes Sep 29, 2022

View reviewed changes

Re-introduce log_analytics_solution_id

4fde09a

Merge branch 'master' of github.com:Azure/terraform-azurerm-aks into …

19e15ed

…fix-disabled-log_analytics_workspace

gzur added 2 commits September 30, 2022 12:36

Re-introduce autogenerated documentation after merge

e2c9204

Add control local variable for whether to create an analytics solutio…

e3016f2

…n or not

gzur temporarily deployed to acctests September 30, 2022 12:51 Inactive

lonegunmanb approved these changes Sep 30, 2022

View reviewed changes

lonegunmanb merged commit 1e65ee6 into Azure:master Sep 30, 2022

github-actions Bot mentioned this pull request Sep 30, 2022

[Breaking] Remove unused net_profile_docker_bridge_cidr #222

Merged

Conversation

gzur commented Sep 27, 2022

Changes proposed in the pull request:

Uh oh!

github-actions Bot commented Sep 28, 2022

Uh oh!

lonegunmanb commented Sep 28, 2022

Uh oh!

lonegunmanb commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lonegunmanb left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 28, 2022

Uh oh!

gzur commented Sep 28, 2022

Uh oh!

ghost commented Sep 28, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lonegunmanb commented Sep 28, 2022

Uh oh!

github-actions Bot commented Sep 28, 2022

Uh oh!

gzur commented Sep 28, 2022

Uh oh!

lonegunmanb commented Sep 28, 2022

Uh oh!

lonegunmanb commented Sep 28, 2022

Uh oh!

gzur commented Sep 28, 2022

Uh oh!

lonegunmanb left a comment • 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!

lonegunmanb Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

gzur Sep 29, 2022 • 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!

lonegunmanb Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

gzur Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 29, 2022

Uh oh!

Uh oh!

lonegunmanb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lonegunmanb left a comment

Choose a reason for hiding this comment

Uh oh!

gzur commented Sep 29, 2022

Uh oh!

lonegunmanb commented Sep 30, 2022

Uh oh!

lonegunmanb commented Sep 30, 2022

Uh oh!

github-actions Bot commented Sep 30, 2022

Uh oh!

github-actions Bot commented Sep 30, 2022

Uh oh!

github-actions Bot commented Sep 30, 2022

Uh oh!

github-actions Bot commented Sep 30, 2022

lonegunmanb commented Sep 28, 2022 •

edited

Loading

ghost commented Sep 28, 2022 •

edited by ghost

Loading

lonegunmanb left a comment •

edited

Loading

gzur Sep 29, 2022 •

edited

Loading