New Resource - `azurerm_kubernetes_cluster_pod_identity` #11492

njuCZ · 2021-04-27T09:37:38Z

Support enabling ad-pod-identity, creating pod identities, and pod exceptions.

through doc https://docs.microsoft.com/en-us/azure/aks/use-azure-ad-pod-identity and discussion from thread #10206, and creating pod identity needs role assignment first, it's better to make a seperate resource

to test this resource, you must enable the feature Microsoft.ContainerService/EnablePodIdentityPreview in your subscription.
you could do this through azure cli

az feature register --namespace Microsoft.ContainerService --name EnablePodIdentityPreview

poll until the status is registered by

az feature list | grep -C 5 "EnablePodIdentityPreview"

then execute

az provider register -n Microsoft.ContainerService

magodo

@njuCZ Thanks for this PR!
I've taken a look through and left some comments inline, but this is mostly looking good to me 👍

website/docs/r/kubernetes_cluster_pod_identity.html.markdown

magodo · 2021-05-06T01:58:28Z

azurerm/internal/services/containers/kubernetes_cluster_pod_identity_resource.go

+		v := raw.(map[string]interface{})
+		name := v["name"].(string)
+		namespace := v["namespace"].(string)
+		if name == "" || namespace == "" {


Is this redundent given both properties are Required?

Can we add a comment above this line for this special handling, mentioning issue: hashicorp/terraform-plugin-sdk#588?

azurerm/internal/services/containers/kubernetes_cluster_pod_identity_resource_test.go

magodo · 2021-05-07T07:48:53Z

azurerm/internal/services/containers/kubernetes_cluster_pod_identity_resource.go

+
+		name := v["name"].(string)
+		namespace := v["namespace"].(string)
+		if name == "" || namespace == "" {


Persumably this only exists when your containing block has a Map (see: hashicorp/terraform-plugin-sdk#588). Therefore, it is safe to remove these lines here.

magodo · 2021-05-07T07:49:16Z

azurerm/internal/services/containers/kubernetes_cluster_pod_identity_resource.go

+		v := raw.(map[string]interface{})
+		name := v["name"].(string)
+		namespace := v["namespace"].(string)
+		if name == "" || namespace == "" {


Can we add a comment above this line for this special handling, mentioning issue: hashicorp/terraform-plugin-sdk#588?

magodo · 2021-05-07T08:59:58Z

Thank you @njuCZ, LGTM 👍

tiwood · 2021-06-08T09:23:39Z

@magodo Any chance this can be added in the next version?

piotr-muzyka · 2021-06-11T17:32:57Z

@magodo Any chance this can be added in the next version?

+1, also waiting for this to be implemented.

magodo · 2021-06-15T01:27:34Z

@njuCZ Could you please resolve the conflicts?

njuCZ · 2021-06-15T09:10:55Z

@magodo have rebased the code and fix all conflicts.

magodo · 2021-06-16T04:02:29Z

@njuCZ Thank you! The tests are passing.

tombuildsstuff · 2021-06-16T07:07:39Z

@magodo @njuCZ unfortunately we're going to need to revert this, since this should not be a separate resource - but needs to instead be inlined within the azurerm_kubernetes_cluster resource

)" This reverts commit db2d2a1.

njuCZ · 2021-06-16T07:17:09Z

@tombuildsstuff May I ask why it should not be a separate resource? and setting pod identity needs a role assignment first.

tombuildsstuff · 2021-06-16T10:55:52Z

@njuCZ firstly this feature (V1) is deprecated and will be removed by AKS in favour of V2 as detailed in the document above - so supporting this functionality for a couple of months to remove it is debatable.

Secondly - since this is a User Assigned Identity that's a Managed Identity - the Managed Identity can have be created and have it's Role Assignment created prior to the cluster creation (or change) - this means (for example) during deletion, the Role Assignment will be removed once the Cluster has been deleted, not beforehand which could cause Pods to fail during runtime.

Whilst arguably there's some minor usability benefits to splitting this out, the runtime trade-off and where only a single Pod Security Policy can be configured) - this makes more sense as a separate resource.

As such I think the larger question at this point is should we support this functionality, given it's in Preview and is scheduled to be replaced by a new version the near future (which presumably has a different schema):

The feature described in this document, pod-managed identities (preview), will be replaced with pod-managed identities V2 (preview)

jrxs-cg · 2021-06-16T15:58:19Z

@tombuildsstuff Your point about this feature being preview and scheduled for deprecation in favor of v2 makes sense and is a decision that you and the other maintainers will need to make. But I'm having trouble following your other arguments around why this should not be a separate resource.

this means (for example) during deletion, the Role Assignment will be removed once the Cluster has been deleted, not beforehand which could cause Pods to fail during runtime.

Isn't deletion of the identities after the cluster is deleted desirable? Why are we concerned about pods failing after the cluster has been deleted? There won't be any more pods at that point.

Whilst arguably there's some minor usability benefits to splitting this out, the runtime trade-off and where only a single Pod Security Policy can be configured) - this makes more sense as a separate resource.

I'm having trouble parsing this at all... it sounds like you're arguing in favor of it being a separate resource.

Regardless of whether this particular PR is accepted due to the v1 vs v2 issue, these design decisions will likely apply to the future v2 implementation as well.

In any case, since this was reverted can we reopen #9885 please?

github-actions · 2021-06-18T06:43:50Z

This functionality has been released in v2.64.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 · 2021-07-19T02:07:43Z

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.

ghost added size/XL documentation labels Apr 27, 2021

njuCZ changed the title ~~Kubernetes cluster pod identity~~ new resource "azurerm_kubernetes_cluster_pod_identity" Apr 27, 2021

njuCZ force-pushed the kubernetes_cluster_pod_identity branch from ae52a40 to acb9f89 Compare April 29, 2021 05:21

magodo requested changes May 6, 2021

View reviewed changes

njuCZ force-pushed the kubernetes_cluster_pod_identity branch from acb9f89 to 1c9ef38 Compare May 6, 2021 09:26

njuCZ requested a review from magodo May 6, 2021 09:27

magodo requested changes May 7, 2021

View reviewed changes

magodo changed the title ~~new resource "azurerm_kubernetes_cluster_pod_identity"~~ New Resource - azurerm_kubernetes_cluster_pod_identity May 7, 2021

magodo added service/kubernetes-cluster new-resource labels May 7, 2021

njuCZ force-pushed the kubernetes_cluster_pod_identity branch from c74d205 to 3651acc Compare May 19, 2021 02:42

piotr-muzyka mentioned this pull request Jun 11, 2021

Support for Pod Identity Add On for AKS #9885

Closed

njuCZ and others added 6 commits June 15, 2021 16:36

new resource "azurerm_kubernetes_cluster_pod_identity"

582aaba

update

6b232fe

rebase and refactor the codes

d931bcc

update

f16c415

add link to terraform plugin sdk issue

28d16a6

rebase

af4e35c

njuCZ force-pushed the kubernetes_cluster_pod_identity branch from 3651acc to af4e35c Compare June 15, 2021 09:04

magodo added this to the v2.64.0 milestone Jun 16, 2021

magodo merged commit db2d2a1 into hashicorp:master Jun 16, 2021

tombuildsstuff added a commit that referenced this pull request Jun 16, 2021

Revert "New Resource - azurerm_kubernetes_cluster_pod_identity (#11492

4421ba2

)" This reverts commit db2d2a1.

tombuildsstuff mentioned this pull request Jun 16, 2021

Revert "New Resource - azurerm_kubernetes_cluster_pod_identity" #12225

Merged

jackofallops mentioned this pull request Jul 8, 2021

azurerm_kubernetes_cluster: support aad-pod-identity #10206

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jul 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource - `azurerm_kubernetes_cluster_pod_identity` #11492

New Resource - `azurerm_kubernetes_cluster_pod_identity` #11492

njuCZ commented Apr 27, 2021 •

edited

Loading

magodo left a comment

magodo May 6, 2021

magodo May 7, 2021

magodo May 7, 2021

magodo May 7, 2021

magodo commented May 7, 2021

tiwood commented Jun 8, 2021

piotr-muzyka commented Jun 11, 2021

magodo commented Jun 15, 2021

njuCZ commented Jun 15, 2021

magodo commented Jun 16, 2021

tombuildsstuff commented Jun 16, 2021

njuCZ commented Jun 16, 2021

tombuildsstuff commented Jun 16, 2021

jrxs-cg commented Jun 16, 2021

github-actions bot commented Jun 18, 2021

github-actions bot commented Jul 19, 2021

New Resource - azurerm_kubernetes_cluster_pod_identity #11492

New Resource - azurerm_kubernetes_cluster_pod_identity #11492

Conversation

njuCZ commented Apr 27, 2021 • edited Loading

magodo left a comment

Choose a reason for hiding this comment

magodo May 6, 2021

Choose a reason for hiding this comment

magodo May 7, 2021

Choose a reason for hiding this comment

magodo May 7, 2021

Choose a reason for hiding this comment

magodo May 7, 2021

Choose a reason for hiding this comment

magodo commented May 7, 2021

tiwood commented Jun 8, 2021

piotr-muzyka commented Jun 11, 2021

magodo commented Jun 15, 2021

njuCZ commented Jun 15, 2021

magodo commented Jun 16, 2021

tombuildsstuff commented Jun 16, 2021

njuCZ commented Jun 16, 2021

tombuildsstuff commented Jun 16, 2021

jrxs-cg commented Jun 16, 2021

github-actions bot commented Jun 18, 2021

github-actions bot commented Jul 19, 2021

New Resource - `azurerm_kubernetes_cluster_pod_identity` #11492

New Resource - `azurerm_kubernetes_cluster_pod_identity` #11492

njuCZ commented Apr 27, 2021 •

edited

Loading