-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
azurerm_kubernetes_cluster
- Support for KMS arguments
#19893
azurerm_kubernetes_cluster
- Support for KMS arguments
#19893
Conversation
internal/services/containers/kubernetes_cluster_resource_test.go
Outdated
Show resolved
Hide resolved
Enabled: utils.Bool(raw["enabled"].(bool)), | ||
KeyId: utils.String(raw["key_id"].(string)), | ||
KeyVaultNetworkAccess: &kvAccess, | ||
KeyVaultResourceId: utils.String(raw["key_vault_resource_id"].(string)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would raw["key_vault_resource_id"]
here be nil
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mkilchhofer , since the key_vault_resource_id
is an optional argument, would raw["key_vault_resource_id"]
return a nil
and cause panic here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't set it in my tests and did not get a panic here :-)
Will try to refactor this and remove it. (see comment of @stephybun #19893 (comment))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I don't know if its possible to attach a debugger to a provider I tested it with adding some logging:
$ git di
diff --git a/internal/services/containers/kubernetes_cluster_resource.go b/internal/services/containers/kubernetes_cluster_resource.go
index 0f4a4d70d9..1a3959cff5 100644
--- a/internal/services/containers/kubernetes_cluster_resource.go
+++ b/internal/services/containers/kubernetes_cluster_resource.go
@@ -3,6 +3,7 @@ package containers
import (
"context"
"encoding/base64"
+ "encoding/json"
"fmt"
"log"
"strconv"
@@ -3427,6 +3428,18 @@ func expandKubernetesClusterAzureKeyVaultKms(d *pluginsdk.ResourceData, input []
}
raw := input[0].(map[string]interface{})
+ rawJson, _ := json.Marshal(raw)
+
+ log.Printf("[DEBUG] AzureKeyVaultKms - raw %v", raw)
+ log.Printf("[DEBUG] AzureKeyVaultKms - raw %v", string(rawJson))
+
+ log.Printf("[DEBUG] AzureKeyVaultKms - raw[key_vault_key_id]: %v", raw["key_vault_key_id"])
+ log.Printf("[DEBUG] AzureKeyVaultKms - raw[key_vault_key_id](string): %v", raw["key_vault_key_id"].(string))
+
+ log.Printf("[DEBUG] AzureKeyVaultKms - raw[key_vault_resource_id]: %v", raw["key_vault_resource_id"])
+ log.Printf("[DEBUG] AzureKeyVaultKms - raw[key_vault_resource_id](string): %v", raw["key_vault_resource_id"].(string))
+
kvAccess := managedclusters.KeyVaultNetworkAccessTypes(raw["key_vault_network_access"].(string))
azureKeyVaultKms := &managedclusters.AzureKeyVaultKms{
Output of this is:
$ grep AzureKeyVaultKms runlog3.log
2023-01-11T15:59:32.812+0100 [DEBUG] provider.terraform-provider-azurerm: AzureKeyVaultKms - raw map[key_vault_key_id:https://kv-aks-ci-m98ln0012-gj9j.vault.azure.net/keys/aks-ci-m98ln0012-gj9jjw-etcd-encryption/c059c6be19c441df9520ced5f08efc72 key_vault_network_access:Public key_vault_resource_id:]: timestamp=2023-01-11T15:59:32.811+0100
2023-01-11T15:59:32.812+0100 [DEBUG] provider.terraform-provider-azurerm: AzureKeyVaultKms - raw {"key_vault_key_id":"https://kv-aks-ci-m98ln0012-gj9j.vault.azure.net/keys/aks-ci-m98ln0012-gj9jjw-etcd-encryption/c059c6be19c441df9520ced5f08efc72","key_vault_network_access":"Public","key_vault_resource_id":""}: timestamp=2023-01-11T15:59:32.811+0100
2023-01-11T15:59:32.812+0100 [DEBUG] provider.terraform-provider-azurerm: AzureKeyVaultKms - raw[key_vault_key_id]: https://kv-aks-ci-m98ln0012-gj9j.vault.azure.net/keys/aks-ci-m98ln0012-gj9jjw-etcd-encryption/c059c6be19c441df9520ced5f08efc72: timestamp=2023-01-11T15:59:32.812+0100
2023-01-11T15:59:32.812+0100 [DEBUG] provider.terraform-provider-azurerm: AzureKeyVaultKms - raw[key_vault_key_id](string): https://kv-aks-ci-m98ln0012-gj9j.vault.azure.net/keys/aks-ci-m98ln0012-gj9jjw-etcd-encryption/c059c6be19c441df9520ced5f08efc72: timestamp=2023-01-11T15:59:32.812+0100
2023-01-11T15:59:32.812+0100 [DEBUG] provider.terraform-provider-azurerm: AzureKeyVaultKms - raw[key_vault_resource_id]:: timestamp=2023-01-11T15:59:32.812+0100
2023-01-11T15:59:32.812+0100 [DEBUG] provider.terraform-provider-azurerm: AzureKeyVaultKms - raw[key_vault_resource_id](string):: timestamp=2023-01-11T15:59:32.812+0100
As you can see in the first 2 lines, raw["key_vault_resource_id"]
is empty string when unset in HCL.
27b86ba
to
44bcf48
Compare
} | ||
|
||
private := managedclusters.KeyVaultNetworkAccessTypesPrivate | ||
if kvAccess == private && len(*azureKeyVaultKms.KeyVaultResourceId) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If azureKeyVaultKms.KeyVaultResourceId
is nil
, then the dereference here would cause a panic.
If not, then what we're checking here is *azureKeyVaultKms.KeyVaultResourceId == ""
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR @mkilchhofer. This is off to a great start, some changes need to be made to the schema but once that's done we can take another look through this 👍
af2d767
to
988640c
Compare
988640c
to
f9a7977
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for making those changes @mkilchhofer!
I had one more comment regarding some nil checks in the flatten function which I unfortunately missed in the initial review, but once that's done this should be good to go!
No need to squash the commits, that will happen when this gets merged.
Great work and thanks again! 🙂
Apply suggestions from code review Co-authored-by: stephybun <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this @mkilchhofer LGTM 💯
Thank you for the approve and the merge. Your review was excellent 👍 |
This functionality has been released in v3.40.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Resolves #17957
Our security guys at @swisspost asked to implement Add Key Management Service (KMS) etcd encryption to an Azure Kubernetes Service (AKS) cluster. Unfortunately the provider did not implement it, so I implemented it for us and the community :-).
Example of implementation: