-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Add support for azure-keyvault-secrets-provider AKS addon #14308
Conversation
@katbyte / @tombuildsstuff up for review! if we can have that in this' week release that would be awesome! |
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 @LP0101. This is off to a good start! I've left a few comments and suggestions, once they're addressed we can look over this again 🙂 .
internal/services/containers/kubernetes_cluster_addons_resource_test.go
Outdated
Show resolved
Hide resolved
internal/services/containers/kubernetes_cluster_addons_resource_test.go
Outdated
Show resolved
Hide resolved
internal/services/containers/kubernetes_cluster_data_source_test.go
Outdated
Show resolved
Hide resolved
Hi @LP0101, can you also make sure this supports exporting the managed identtiy information created? Example: "azureKeyvaultSecretsProvider": {
"enabled": true,
"config": {
"enableSecretRotation": "true"
},
"identity": {
"resourceId": "/subscriptions/<subscription>/resourcegroups/MC_<resource_group>_<cluster_name>_<location>/providers/Microsoft.ManagedIdentity/userAssignedIdentities/azurekeyvaultsecretsprovider-<cluster_name>",
"clientId": "6510b307-edd9-44c8-bc4a-2e9b443a0d73",
"objectId": "a5a771e1-4d59-4545-bb9c-352b7c6a01f6"
}
} |
Hey @nerddtvg Good call, I totally forgot about that. Added it in the latest commit. |
Wow that was fast! Thank you! |
Any update on this PR? We would love to have it merged in this week's release. |
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.
Looks like there is a test failure
------- Stdout: -------
=== RUN TestAccKubernetesCluster_addonProfileAzureKeyvaultSecretsProvider
=== PAUSE TestAccKubernetesCluster_addonProfileAzureKeyvaultSecretsProvider
=== CONT TestAccKubernetesCluster_addonProfileAzureKeyvaultSecretsProvider
testcase.go:110: Step 1/4 error: Error running pre-apply refresh: exit status 1
Error: Missing newline after argument
on terraform_plugin_test.tf line 35, in resource "azurerm_kubernetes_cluster" "test":
35: rotation_interval = 2m
An argument definition must end with a newline.
testing_new.go:63: Error retrieving state, there may be dangling resources: exit status 1
Error: Missing newline after argument
on terraform_plugin_test.tf line 35, in resource "azurerm_kubernetes_cluster" "test":
35: rotation_interval = 2m
An argument definition must end with a newline.
--- FAIL: TestAccKubernetesCluster_addonProfileAzureKeyvaultSecretsProvider (1.81s)
FAIL
internal/services/containers/kubernetes_cluster_addons_resource_test.go
Outdated
Show resolved
Hide resolved
@stephybun acceptance tests are fixed now. Looks like the golint failed due to a timeout :( Can someone with the ability to rerun checks restart it? |
Default: "2m", | ||
ValidateFunc: containerValidate.Duration, | ||
}, | ||
"azure_keyvault_secrets_provider_identity": { |
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.
Sorry I missed this one in the initial review, since this block is already called azure_keyvault_secrets_provider
could we rename this to just identity
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.
Sure, I was just following the convention with the other addons, where we have ingress_application_gateway_identity
and oms_agent_identity
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 think the convention might be there to prevent confusion?
Looking at the AKS module docs, it might get a bit confusing if there were multiple The identity block exports the following:
sections, which I guess is why each identity has its own name?
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 the schemas for each identity block were different I would agree, but they're all identical so I don't really see an issue if they all point to the same block in the docs. To be sure I will clarify internally whether there is any other reason for that naming convention. If not it should be renamed and the other two properties will also need to be renamed and deprecated (in a separate PR).
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.
Could also be that the AKS cluster itself already exports an identity
block, which does have a different schema from the addon identity blocks?
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 think it would add confusion with the AKS identity
and kubelet_identity
blocks like you said. Maybe something more generic for the addons (in a future PR) like addon_identity
? It's always going to be the same with a user_assigned_managed_identity
anyways.
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.
Yeah the background on this is each identity block currently has a distinct name because of how these are represented in the docs - as such I'd agree this probably should be secret_identity
here.
To call out the comment above too, we'll be making the add-on blocks consistent with the rest of the provider where possible in a future release, that is, having the presence of the block mean enabled = true
and the omission of the block meaning enabled = false
. This is required since there's a tri-state in the Azure API currently which needs resolving (e.g. the block being omitted in the API, or enabled=false mean the same thing - and then there's enabled=true - but we need to consolidate these into Terraform.
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.
hey @LP0101
Thanks for this PR - I've taken a look through and left some comments in addition to the ones that @stephybun has left, if we can fix those up then this should otherwise be good to go 👍
Thanks!
"enabled": { | ||
Type: pluginsdk.TypeBool, | ||
Required: true, | ||
}, |
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.
rather than exposing this field - we can take the presence of the parent block to mean enabled=true (and it's false if it's omitted)
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.
@tombuildsstuff, @stephybun asked a similar question earlier: #14308 (comment)
This wouldn't match with the other addon blocks which have their own enabled
status. Plus I worry if we have existing clusters that have it manually enabled but the Terraform isn't updated, it may try to disable them on subsequent applies.
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 just saw your below comment about consistency with other blocks. I'm still worried how this will play out as more addons are created and manually applied during previews. I realize that with a diff or preview, we should catch these possible changes, but if it is being automated it may not be.
Default: "2m", | ||
ValidateFunc: containerValidate.Duration, | ||
}, | ||
"azure_keyvault_secrets_provider_identity": { |
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.
Yeah the background on this is each identity block currently has a distinct name because of how these are represented in the docs - as such I'd agree this probably should be secret_identity
here.
To call out the comment above too, we'll be making the add-on blocks consistent with the rest of the provider where possible in a future release, that is, having the presence of the block mean enabled = true
and the omission of the block meaning enabled = false
. This is required since there's a tri-state in the Azure API currently which needs resolving (e.g. the block being omitted in the API, or enabled=false mean the same thing - and then there's enabled=true - but we need to consolidate these into Terraform.
enabled := value["enabled"].(bool) | ||
|
||
if secretRotation, ok := value["secret_rotation_enabled"]; ok && secretRotation.(bool) { | ||
config["enableSecretRotation"] = utils.String("true") |
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.
this'll also want a default value set of false
?
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 tried to match the provider's behaviour with what the CLI does. In that case, when disabling secret rotation (or just not enabling it), the field is completely omitted from the API response.
I can also set it to false
, if that's preferable.
|
||
* `secret_rotation_enabled` - (Optional) Is secret rotation enabled? | ||
|
||
* `secret_rotation_interval` - (Optional) The interval to poll for secret rotation. This attribute is only set when `secret_rotation` is true and defaults to `2m`. |
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.
this means this won't be unset in the API config (so will always have a value?)
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.
Hm, yeah. It does keep the secretRotationInterval value set in the API. However, that doesn't do anything without the enableSecretRotation
flag being set to true.
Fixed the docs and renamed the identity block. 👍 |
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 fixing up some of those suggestions @LP0101. There are a few finishing touches missing but once those are addressed this should be good to go!
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.
Resolved this last review comment and the tests are passing. Thanks for your patience @LP0101, LGTM! 🚀
This functionality has been released in v2.89.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 #12783