Skip to content
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

Adding support for AzureAD auth to AKS via RBAC #1820

Merged
merged 35 commits into from
Nov 15, 2018

Conversation

timcurless
Copy link
Contributor

@timcurless timcurless commented Aug 24, 2018

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
Copy link
Contributor

metacpp commented Aug 24, 2018

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

@ghost ghost added the size/L label Aug 24, 2018
@dpmerron-ltd
Copy link

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!

@ghost ghost added the size/L label Aug 31, 2018
@timcurless
Copy link
Contributor Author

timcurless commented Aug 31, 2018

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

@ghost ghost added the size/L label Aug 31, 2018
@luisdavim
Copy link

when can this be merged?

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

website/docs/index.html.markdown Outdated Show resolved Hide resolved
azurerm/resource_arm_kubernetes_cluster.go Outdated Show resolved Hide resolved
@ghost ghost added size/XXL and removed size/XL labels Nov 14, 2018
@tombuildsstuff tombuildsstuff dismissed stale reviews from metacpp and WodansSon November 14, 2018 20:59

dismissing since changes have been pushed

@hashicorp hashicorp unlocked this conversation Nov 14, 2018
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tombuildsstuff, Looking good, left some comments inline

if profile.AccessProfile == nil {
return nil, []interface{}{}
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

azurerm/data_source_kubernetes_cluster.go Show resolved Hide resolved
azurerm/data_source_kubernetes_cluster.go Show resolved Hide resolved
azurerm/helpers/kubernetes/kube_config.go Outdated Show resolved Hide resolved
azurerm/resource_arm_kubernetes_cluster.go Outdated Show resolved Hide resolved
azurerm/resource_arm_kubernetes_cluster.go Show resolved Hide resolved
azurerm/resource_arm_kubernetes_cluster.go Outdated Show resolved Hide resolved
website/docs/d/kubernetes_cluster.html.markdown Outdated Show resolved Hide resolved
website/docs/index.html.markdown Outdated Show resolved Hide resolved
@katbyte katbyte assigned tombuildsstuff and unassigned katbyte Nov 15, 2018
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from two minor comments now LGTM 👍

examples/kubernetes/advanced-networking/variables.tf Outdated Show resolved Hide resolved
examples/kubernetes/basic/variables.tf Outdated Show resolved Hide resolved
@tombuildsstuff
Copy link
Contributor

Resource Tests pass:

screenshot 2018-11-15 at 14 28 21

Data Source Tests pass:

screenshot 2018-11-15 at 14 28 12

@tombuildsstuff tombuildsstuff merged commit 89a15ed into hashicorp:master Nov 15, 2018
tombuildsstuff added a commit that referenced this pull request Nov 15, 2018
@Johnch9
Copy link

Johnch9 commented Nov 20, 2018

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
Copy link

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

@ghost
Copy link

ghost commented Mar 5, 2019

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!

@ghost ghost locked and limited conversation to collaborators Mar 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.