Adding support for AzureAD auth to AKS via RBAC

[timcurless]

I think this is a good starting point.

1. I would like to refactor to be much more DRY [UPDATE]: Refactoring complete
2. This could use some further testing
3. I have not yet updated docs [UPDATE]: Docs have been updated.

[metacpp]

@timcurless Thanks for the contribution. Can you resolve the conflicts with master branch?

[dpmerron-ltd]

Hey @timcurless are you going to be fixing this merge conflict? Happy to take a look if you aren't as we are waiting on a fix like this!

[timcurless]

Sorry about that - I resolved one set but I think another set crept up with some additional changes. In any case I think everything is now resolved.

@tombuildsstuff Please let me know what else you're waiting on me for and I'll do my best to respond quickly :)

[luisdavim]

when can this be merged?

[tombuildsstuff]

hey @timcurless

Thanks for this PR - apologies this has sat for a little while. I've spent some time today on AKS / RBAC - specifically prototyping/researching/investigating to try and ensure the design of the schema is correct as mentioned in my last comment.

Aside from the design of the schema this PR looks pretty good; I'm going to push some commits to update the schema so that this better matches other resources in Terraform (and merged in master / done some refactoring). The result of this is that the rbac_enabled field and aad_profile block have been replaced with a role_based_access_control block which is optional - this allows users to optionally configure RBAC by specifying this block / omit it by not specifying it (which better matches Terraform). I've also noticed that the Tenant ID field can become Optional - since we can source this from the tenant_id of the current subscription if it's not specified.

I'm going to push the comments I mentioned above and kick off the tests shortly - but I believe the schema design for this is now right (and that we should be able to proceed with this); since I've also included some refactoring in these changes I'm going to ask @katbyte to take another look through this.

Apologies again for the delayed review here - thanks for all the work in this PR, I hope you don't mind I've pushed a few commits to get this over the line :)

Thanks!

dismissing since changes have been pushed

[katbyte]

@tombuildsstuff, Looking good, left some comments inline

[katbyte]

I think we could invert the logic here:

result := []interface{}{}

if ap := profile.AccessProfile; ap != nil {
  if kubeConfigRaw := profile.AccessProfile.KubeConfig; kubeConfigRaw != nil {
    ...
  }
}

return nil, result

thoughts?

[tombuildsstuff]

let's leave this as-is for the moment - I have a feeling we'll end up pulling the Kubernetes parsing logic out into the new shared library

[katbyte]

Aside from two minor comments now LGTM 👍

[tombuildsstuff]

Resource Tests pass:

Data Source Tests pass:

[Johnch9]

I might have this wrong, but it now appears that we have to add an aadProfile to set the enableRBAC flag, whereas the command line 'az aks create .....' creates a cluster with RBAC enabled but without the aadProfile section. Is there a way through Terraform that we can achieve the same.

[alexvicegrab]

Agreed, I think there is a PR related to this #2347

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!