feat(aks): add microsoft defender support #232

eyenx · 2022-08-23T14:49:18Z

Changes proposed in the pull request:

Adds support for a microsoft_defender block introduced with hashicorp/terraform-provider-azurerm#16218

Signed-off-by: Toni Tauro [email protected]

Signed-off-by: Toni Tauro <[email protected]>

eyenx · 2022-08-27T07:49:34Z

@lonegunmanb can't run the test anymore.

Always getting same errors:

INFO: Styling Terraform configurations...
INFO: Done!
There are some problems with the configuration, described below.

The Terraform configuration must be valid before initialization so that
Terraform can determine which modules and providers need to be installed.

Error: Unsupported block type

  on main.tf line 5:
   5: moved {

Blocks of type "moved" are not expected here.


Error: Unsupported block type

  on main.tf line 178, in resource "azurerm_kubernetes_cluster" "main":
 178:     precondition {

Blocks of type "precondition" are not expected here.


Error: Unsupported block type

  on main.tf line 182, in resource "azurerm_kubernetes_cluster" "main":
 182:     precondition {

Blocks of type "precondition" are not expected here.


Error: Unsupported argument

  on variables.tf line 57, in variable "agents_pool_name":
  57:   nullable    = false

The terraform-test image we are using is quite outdated (versoin 0.14.4). Could it be because of that?

eyenx · 2022-08-27T08:03:54Z

I now created my own mcr.microsoft.com/terraform-test:1.2.5 by grabbing the https://github.com/azure/terraform-test repo and adjusting some vars.

I was able to run the rake build, but not the tests:

Terraform has been successfully initialized!

You may now begin working with Terraform. Try running "terraform plan" to see
any changes that are required for your infrastructure. All Terraform commands
should now work.

If you ever set or change modules or backend configuration for Terraform,
rerun this command to reinitialize your working directory. If you forget, other
commands will detect it and remind you to do so if necessary.
INFO: Linting Terraform configurations...
INFO: Done!
INFO: Styling Terraform README...
INFO: Done!
INFO: Styling Terraform fixture configurations...
INFO: Done!
go: cannot find main module, but found Gopkg.lock in /go/src/terraform-azurerm-aks
        to create a module there, run:
        go mod init
rake aborted!
ERROR: Go test failed!

/go/src/terraform-azurerm-aks/Rakefile:45:in `block (2 levels) in <top (required)>'
Tasks: TOP => full => e2e => integration:test
(See full trace by running task with --trace)

lonegunmanb · 2022-08-29T02:53:24Z

I now created my own mcr.microsoft.com/terraform-test:1.2.5 by grabbing the https://github.com/azure/terraform-test repo and adjusting some vars.

I was able to run the rake build, but not the tests:

Terraform has been successfully initialized!

You may now begin working with Terraform. Try running "terraform plan" to see
any changes that are required for your infrastructure. All Terraform commands
should now work.

If you ever set or change modules or backend configuration for Terraform,
rerun this command to reinitialize your working directory. If you forget, other
commands will detect it and remind you to do so if necessary.
INFO: Linting Terraform configurations...
INFO: Done!
INFO: Styling Terraform README...
INFO: Done!
INFO: Styling Terraform fixture configurations...
INFO: Done!
go: cannot find main module, but found Gopkg.lock in /go/src/terraform-azurerm-aks
        to create a module there, run:
        go mod init
rake aborted!
ERROR: Go test failed!

/go/src/terraform-azurerm-aks/Rakefile:45:in `block (2 levels) in <top (required)>'
Tasks: TOP => full => e2e => integration:test
(See full trace by running task with --trace)

Thanks @eyenx for opening this Pr! The dockerfile and docker imaged described in readme file is outdated and no longer been maintained, we're working on a new CI pipeline and we're close to a release (We'll release v6.0.0 with this new CI pipeline). I'm not going to update this mcr.microsoft.com/terraform-test image and rake tests since they'll be replaced very soon. If you're interested in this new CI, you can try it on my personal branch, we have instructions in readme file on how to run the pr checks and acc tests for our module. For this pr please give me some time to review and test with new CI on my machine. Once we've solved the issues that block our CI pipeline, we'll introduce this pipeline into some legacy azurerm Terraform modules. Thanks for your understanding.

lonegunmanb · 2022-08-30T03:39:37Z

main.tf

@@ -166,6 +166,14 @@ resource "azurerm_kubernetes_cluster" "main" {
    }
  }

+  dynamic "microsoft_defender" {


Could we move this block to line 144 please? We're sorting arguments(assignments with equal symbol) and blocks in resource block are sorted in the following order:

count and for_each

Required arguments in lexicographical order

Optional arguments in lexicographical order

Required blocks in lexicographical order

Optional blocks in lexicographical order

depends_on, then lifecycle

gonna do this ra

lonegunmanb · 2022-08-30T03:40:45Z

variables.tf

+}
+
+variable "microsoft_defender_enabled" {


Could we move this block to line 262 please? We are sorting variables in the following order:

Required variables in lexicographical order(variables without default value)

Optional variables in lexicographical order

gonna do this ra

Signed-off-by: Toni Tauro <[email protected]>

lonegunmanb · 2022-09-01T09:46:16Z

main.tf

+    for_each = var.microsoft_defender_enabled ? ["microsoft_defender"] : []
+
+    content {
+      log_analytics_workspace_id = var.log_analytics_workspace_id == null ? azurerm_log_analytics_workspace.main[0].id : var.log_analytics_workspace.id


Hi @eyenx , there might be a typo here since we have not var.log_analytics_workspace_id. Could we change this line to the following line?:

log_analytics_workspace_id = coalesce(try(var.log_analytics_workspace.id, null), azurerm_log_analytics_workspace.main[0].id)

Done. Sry for late reply

Signed-off-by: Toni Tauro <[email protected]>

lonegunmanb

Thanks @eyenx, LGTM

feat(aks): add microsoft defender support

6082e00

Signed-off-by: Toni Tauro <[email protected]>

lonegunmanb requested changes Aug 31, 2022

View reviewed changes

eyenx added 2 commits August 31, 2022 07:07

fix(main): move dynamic block to line 144

3f7d0fa

Signed-off-by: Toni Tauro <[email protected]>

fix(vars): move defender var to line 262

d254abe

Signed-off-by: Toni Tauro <[email protected]>

lonegunmanb requested changes Sep 1, 2022

View reviewed changes

fix(main.tf): var.log_analytics_workspace.id

242b671

Signed-off-by: Toni Tauro <[email protected]>

eyenx requested a review from lonegunmanb September 6, 2022 09:08

lonegunmanb approved these changes Sep 7, 2022

View reviewed changes

lonegunmanb merged commit be1cbdb into Azure:master Sep 7, 2022

eyenx deleted the feat/add-microsoft-defender-block branch September 7, 2022 21:23

gzur mentioned this pull request Sep 27, 2022

Unable to set log_analytics_workspace_enabled to false and supply my own log_analytics_workspace #257

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(aks): add microsoft defender support #232

feat(aks): add microsoft defender support #232

eyenx commented Aug 23, 2022

eyenx commented Aug 27, 2022

eyenx commented Aug 27, 2022 •

edited

Loading

lonegunmanb commented Aug 29, 2022

lonegunmanb Aug 30, 2022

eyenx Aug 31, 2022

eyenx Aug 31, 2022

lonegunmanb Aug 30, 2022

eyenx Aug 31, 2022

eyenx Aug 31, 2022

lonegunmanb Sep 1, 2022

eyenx Sep 6, 2022

lonegunmanb left a comment

feat(aks): add microsoft defender support #232

feat(aks): add microsoft defender support #232

Conversation

eyenx commented Aug 23, 2022

eyenx commented Aug 27, 2022

eyenx commented Aug 27, 2022 • edited Loading

lonegunmanb commented Aug 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lonegunmanb left a comment

Choose a reason for hiding this comment

eyenx commented Aug 27, 2022 •

edited

Loading