Add support for azure-keyvault-secrets-provider AKS addon #14308

LP0101 · 2021-11-23T18:26:45Z

Resolves #12783

djsly · 2021-11-23T18:39:12Z

@katbyte / @tombuildsstuff up for review! if we can have that in this' week release that would be awesome!

stephybun

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_addons.go

internal/services/containers/kubernetes_cluster_addons_resource_test.go

internal/services/containers/kubernetes_addons.go

internal/services/containers/kubernetes_cluster_data_source_test.go

website/docs/d/kubernetes_cluster.html.markdown

website/docs/r/kubernetes_cluster.html.markdown

nerddtvg · 2021-11-24T20:48:40Z

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"
        }
      }

LP0101 · 2021-11-24T21:23:09Z

Hey @nerddtvg

Good call, I totally forgot about that. Added it in the latest commit.

nerddtvg · 2021-11-24T21:24:04Z

Wow that was fast! Thank you!

LP0101 · 2021-11-30T14:08:44Z

Any update on this PR? We would love to have it merged in this week's release.

stephybun

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

LP0101 · 2021-11-30T17:18:54Z

@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?

stephybun · 2021-12-01T11:39:57Z

internal/services/containers/kubernetes_addons.go

+								Default:      "2m",
+								ValidateFunc: containerValidate.Duration,
+							},
+							"azure_keyvault_secrets_provider_identity": {


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

Sure, I was just following the convention with the other addons, where we have ingress_application_gateway_identity and oms_agent_identity

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?

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).

Could also be that the AKS cluster itself already exports an identity block, which does have a different schema from the addon identity blocks?

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.

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.

tombuildsstuff

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!

tombuildsstuff · 2021-11-24T12:36:34Z

internal/services/containers/kubernetes_addons.go

+							"enabled": {
+								Type:     pluginsdk.TypeBool,
+								Required: true,
+							},


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)

@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.

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.

tombuildsstuff · 2021-12-01T19:58:40Z

internal/services/containers/kubernetes_addons.go

+								Default:      "2m",
+								ValidateFunc: containerValidate.Duration,
+							},
+							"azure_keyvault_secrets_provider_identity": {


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.

tombuildsstuff · 2021-12-01T20:00:51Z

internal/services/containers/kubernetes_addons.go

+		enabled := value["enabled"].(bool)
+
+		if secretRotation, ok := value["secret_rotation_enabled"]; ok && secretRotation.(bool) {
+			config["enableSecretRotation"] = utils.String("true")


this'll also want a default value set of false?

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.

internal/services/containers/kubernetes_cluster_data_source.go

website/docs/d/kubernetes_cluster.html.markdown

website/docs/r/kubernetes_cluster.html.markdown

tombuildsstuff · 2021-12-01T20:05:44Z

website/docs/r/kubernetes_cluster.html.markdown

+
+* `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`.


this means this won't be unset in the API config (so will always have a value?)

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.

LP0101 · 2021-12-02T00:05:31Z

Fixed the docs and renamed the identity block. 👍

stephybun

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!

website/docs/r/kubernetes_cluster.html.markdown

internal/services/containers/kubernetes_cluster_data_source.go

stephybun

Resolved this last review comment and the tests are passing. Thanks for your patience @LP0101, LGTM! 🚀

github-actions · 2021-12-10T03:09:30Z

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!

github-actions · 2022-01-10T02:11:29Z

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

Luca Popesco added 7 commits November 22, 2021 17:45

Add support for CSI driver

bbc531d

Support for rotationPollInterval

9c6f010

Add test for AKV provider addon

d0f2058

Added validation for secret rotation interval

3f2d553

Fixed state file

5faf107

Added data source

f2fa9b7

Added docs for AKV provider

e8ed0d0

github-actions bot added documentation size/XL labels Nov 23, 2021

Format tests

3136409

stephybun requested changes Nov 24, 2021

View reviewed changes

Luca Popesco added 2 commits November 24, 2021 12:40

Address review comments

21aec33

Use better logic for defaults. Use existing validation

e5bb366

Support exporting identity for AKV provider

e0d1570

LP0101 requested a review from stephybun November 25, 2021 04:37

jackofallops added service/container-service enhancement labels Nov 26, 2021

stephybun added the service/kubernetes-cluster label Nov 30, 2021

stephybun requested a review from tombuildsstuff November 30, 2021 14:40

stephybun reviewed Nov 30, 2021

View reviewed changes

nerddtvg reviewed Nov 30, 2021

View reviewed changes

internal/services/containers/kubernetes_cluster_addons_resource_test.go Outdated Show resolved Hide resolved

Fix acceptance test

75f78e6

stephybun reviewed Dec 1, 2021

View reviewed changes

tombuildsstuff requested changes Dec 1, 2021

View reviewed changes

tombuildsstuff added waiting-response and removed service/container-service labels Dec 1, 2021

Fix docs, rename identity block

9a67bb1

LP0101 force-pushed the main branch from fc4515d to 9a67bb1 Compare December 1, 2021 23:50

github-actions bot removed the waiting-response label Dec 2, 2021

stephybun requested changes Dec 3, 2021

View reviewed changes

website/docs/r/kubernetes_cluster.html.markdown Outdated Show resolved Hide resolved

internal/services/containers/kubernetes_cluster_data_source.go Outdated Show resolved Hide resolved

Fix order of properties

09c7ab5

katbyte assigned stephybun Dec 3, 2021

Luca Popesco and others added 2 commits December 6, 2021 10:06

Rename identity block to secret_identity

ad15dde

default enableSecretRotation in expand func

aa19a84

stephybun approved these changes Dec 7, 2021

View reviewed changes

stephybun merged commit fae50d4 into hashicorp:main Dec 7, 2021

github-actions bot added this to the v2.89.0 milestone Dec 7, 2021

stephybun added a commit that referenced this pull request Dec 7, 2021

Update for #14308

53af1da

github-actions bot locked as resolved and limited conversation to collaborators Jan 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for azure-keyvault-secrets-provider AKS addon #14308

Add support for azure-keyvault-secrets-provider AKS addon #14308

LP0101 commented Nov 23, 2021

djsly commented Nov 23, 2021

stephybun left a comment

nerddtvg commented Nov 24, 2021

LP0101 commented Nov 24, 2021

nerddtvg commented Nov 24, 2021

LP0101 commented Nov 30, 2021

stephybun left a comment

LP0101 commented Nov 30, 2021 •

edited

stephybun Dec 1, 2021

LP0101 Dec 1, 2021

LP0101 Dec 1, 2021

stephybun Dec 1, 2021

LP0101 Dec 1, 2021

nerddtvg Dec 1, 2021

tombuildsstuff Dec 1, 2021

tombuildsstuff left a comment

tombuildsstuff Nov 24, 2021

nerddtvg Dec 1, 2021

nerddtvg Dec 1, 2021

tombuildsstuff Dec 1, 2021

tombuildsstuff Dec 1, 2021

LP0101 Dec 1, 2021

tombuildsstuff Dec 1, 2021

LP0101 Dec 1, 2021

LP0101 commented Dec 2, 2021

stephybun left a comment

stephybun left a comment

github-actions bot commented Dec 10, 2021

github-actions bot commented Jan 10, 2022


		* `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`.

Add support for azure-keyvault-secrets-provider AKS addon #14308

Add support for azure-keyvault-secrets-provider AKS addon #14308

Conversation

LP0101 commented Nov 23, 2021

djsly commented Nov 23, 2021

stephybun left a comment

Choose a reason for hiding this comment

nerddtvg commented Nov 24, 2021

LP0101 commented Nov 24, 2021

nerddtvg commented Nov 24, 2021

LP0101 commented Nov 30, 2021

stephybun left a comment

Choose a reason for hiding this comment

LP0101 commented Nov 30, 2021 • edited

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

tombuildsstuff left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LP0101 commented Dec 2, 2021

stephybun left a comment

Choose a reason for hiding this comment

stephybun left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 10, 2021

github-actions bot commented Jan 10, 2022

LP0101 commented Nov 30, 2021 •

edited