feat: Implement support for KMS arguments by mkilchhofer · Pull Request #288 · Azure/terraform-azurerm-aks

mkilchhofer · 2023-01-06T16:44:45Z

Describe your changes

This implements Add Key Management Service (KMS) etcd encryption to an Azure Kubernetes Service (AKS) cluster.

Feature is not yet available in azurerm provider, but also opened a PR over there:

azurerm_kubernetes_cluster - Support for KMS arguments hashicorp/terraform-provider-azurerm#19893

Issue number

-

Checklist before requesting a review

The pr title can be used to describe what this pr did in CHANGELOG.md file
I have executed pre-commit on my machine
I have passed pr-check on my machine

Thanks for your cooperation!

lonegunmanb

Thanks @mkilchhofer for opening this pr and submitting a new resource pr to terraform-azurerm-provider's repo, I've left some comments on provider's pr, once the pr has been merged into the provider, we can continue the code review for this pr.

Btw the review comments for your provider's pr are my personal suggestions, there's no guarantee that your pr would be accepted by HashiCorp if you accepted my suggestion.

lonegunmanb · 2023-01-08T14:26:56Z

    }
  }
+  dynamic "key_vault_kms" {
+    for_each = var.key_vault_kms_enabled ? ["key_vault_kms"] : []


Since we could turn enabled to false meanwhile keeping this key_vault_kms, I would recommend the following toggle expression:

for_each = var.key_vault_kms_enabled != null ? ["key_vault_kms"] : []

refactored this anyways (source was the upstream implementation)

mkilchhofer · 2023-01-20T09:54:06Z

PR is now updated after the upstream provider PR got merged AND released ;-)

@lonegunmanb can you please review again?

lonegunmanb

Thanks @mkilchhofer for updating this pr, some issues need to be solved.

It would be nice if we could have an new example to demonstrate this new feature. We have already created a KeyValut in startup example, but for disk encryption key management for now, would you please estimate whether if we can make that KeyVault work for this new feature? We might need to update azurerm provider's restriction in providers.tf file in startup folder. Or we can just crate a new example folder to do so, your call.

Again, thanks for your contribution!

mkilchhofer · 2023-01-23T13:18:07Z

I cannot test the examples as we @swisspost have no subscriptions which allows usage of public IPs.
Can someone test that for us?

zioproto · 2023-01-23T14:00:39Z

@mkilchhofer I can do that ! thanks

zioproto · 2023-01-23T14:18:11Z

@mkilchhofer could you please run the pre-commit step on your local machine ?:

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

You patched the example and you have duplicated definitions for identity_type and identity_ids. See the error:

Error: Attribute redefined
│
│   on examples/named_cluster/main.tf line 88, in module "aks_cluster_name":
│   88:   identity_type         = "UserAssigned"
│
│ The argument "identity_type" was already set at
│ examples/named_cluster/main.tf:66,3-16. Each argument may be set only once.
╵

╷
│ Error: Attribute redefined
│
│   on examples/named_cluster/main.tf line 89, in module "aks_cluster_name":
│   89:   identity_ids          = [azurerm_user_assigned_identity.test.id]
│
│ The argument "identity_ids" was already set at
│ examples/named_cluster/main.tf:65,3-15. Each argument may be set only once.

- nullable=false on `kms_enabled` - Validate kms_enabled and UserAssigned identity

zioproto

Could you patch only the named_cluster example ?

We dont need to the KMS in the others, it would also require more work because the needed UserAssigned identity_type is not set in the others. Thanks

zioproto · 2023-01-24T13:15:29Z

commit 7012178 is now passing both pre-commit and pr-check steps.

@lonegunmanb could you please review and run the e2e tests ? thanks

mkilchhofer · 2023-01-27T10:09:01Z

@lonegunmanb can you trigger E2E again? 😇

lonegunmanb

Thanks @mkilchhofer for this update, we've met some issues when we ran the test.

We've added network_acl on the KeyVault to restrict the source IP which can access this Key Vault, but the Aks's public IP is unpredictable so the original code would meet a 403 error.

We need config the aks to access the KeyVault via private network as this document described.

lonegunmanb · 2023-01-28T09:49:32Z

  rbac_aad                          = true
  rbac_aad_managed                  = true
  role_based_access_control_enabled = true
+


We need the following line below line 31:

service_endpoints = ["Microsoft.KeyVault"]

Service endpoint is not needed (at least in our scenario). I think its only needed if some part inside the VNET wants to access the keyvault.

lonegunmanb

Thanks @mkilchhofer, LGTM! 🚀

mkilchhofer · 2023-01-29T08:30:52Z

Thanks alot for your patience with me 👍

lonegunmanb requested changes Jan 8, 2023

View reviewed changes

mkilchhofer marked this pull request as ready for review January 20, 2023 09:53

mkilchhofer requested a review from lonegunmanb January 20, 2023 09:54

feat: Implement support for KMS arguments

25c45a7

lonegunmanb requested changes Jan 21, 2023

View reviewed changes

Comment thread variables.tf

Comment thread variables.tf Outdated

zioproto suggested changes Jan 23, 2023

View reviewed changes

Comment thread main.tf

chore: Apply changes from code review

0d2123c

- nullable=false on `kms_enabled` - Validate kms_enabled and UserAssigned identity

zioproto suggested changes Jan 23, 2023

View reviewed changes

Comment thread examples/named_cluster/main.tf Outdated

Comment thread examples/named_cluster/main.tf Outdated

chore: Update examples with KMS

eda1086

mkilchhofer requested review from lonegunmanb and zioproto and removed request for zioproto January 24, 2023 08:08

mkilchhofer added 3 commits January 24, 2023 09:48

chore: Set azurerm minimal version to 3.40

2bb6f10

chore: Fix TF linter errors on named_cluster example

f61f35c

chore: Fulfill checkov checks

7012178

zioproto reviewed Jan 24, 2023

View reviewed changes

Comment thread .checkov_config.yaml

fix: Fixup precondition check for UserAssigned identity type

07f427e

mkilchhofer had a problem deploying to acctests January 26, 2023 14:04 — with GitHub Actions Error

mkilchhofer had a problem deploying to acctests January 27, 2023 03:58 — with GitHub Actions Failure

lonegunmanb requested changes Jan 27, 2023

View reviewed changes

Comment thread variables.tf Outdated

Comment thread examples/named_cluster/kms.tf Outdated

mkilchhofer added 2 commits January 27, 2023 10:26

fix: Add depends_on for KV access policy in examples

c4a8bb9

chore: Use "Public" as default instead of null

05c1b8e

mkilchhofer had a problem deploying to acctests January 28, 2023 02:19 — with GitHub Actions Failure

lonegunmanb requested changes Jan 28, 2023

View reviewed changes

fix: Use private endpoint for vault connection

f13b78a

mkilchhofer temporarily deployed to acctests January 28, 2023 14:36 — with GitHub Actions Inactive

lonegunmanb approved these changes Jan 29, 2023

View reviewed changes

lonegunmanb merged commit 5443398 into Azure:master Jan 29, 2023

mkilchhofer deleted the feature/secrets_kms_encryption branch January 30, 2023 10:30

Conversation

mkilchhofer commented Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe your changes

Issue number

Checklist before requesting a review

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 Jan 8, 2023

Choose a reason for hiding this comment

Uh oh!

mkilchhofer Jan 20, 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!

mkilchhofer commented Jan 20, 2023

Uh oh!

lonegunmanb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mkilchhofer commented Jan 23, 2023

Uh oh!

zioproto commented Jan 23, 2023

Uh oh!

zioproto commented Jan 23, 2023

Uh oh!

zioproto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zioproto commented Jan 24, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mkilchhofer commented Jan 27, 2023

Uh oh!

lonegunmanb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lonegunmanb Jan 28, 2023

Choose a reason for hiding this comment

Uh oh!

mkilchhofer Jan 28, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lonegunmanb left a comment

Choose a reason for hiding this comment

Uh oh!

mkilchhofer commented Jan 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mkilchhofer commented Jan 6, 2023 •

edited

Loading

lonegunmanb left a comment •

edited

Loading

mkilchhofer Jan 20, 2023 •

edited

Loading