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.

Sign up for GitHub

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

Jump to bottom

Enable RBAC without AAD #2347

Closed

raphaelquati wants to merge 2 commits into hashicorp:master from raphaelquati:rbacnoaad

Contributor

raphaelquati commented Nov 17, 2018

Implementation for #2345


          Enable RBAC without AAD

654afe1

ghost added the size/S label

acondrat mentioned this pull request

Support rbac and azure ad integration for Azure Kubernetes Service #1429

Closed

tombuildsstuff self-assigned this

Contributor

tombuildsstuff commented Nov 17, 2018

hi @raphaelquati

Thanks for this PR :)

During the investigation into RBAC recently it appeared that whilst you can enable/disable RBAC for AKS at this time it's tied to AzureAD. For this reason we've designed the block such that if/when additional methods are available for RBAC in the future that we can support them as nested items in the role_based_access_control block, by making the AzureAD block optional.

As such I believe we should be able to infer the value of the enable_rbac field and as such we should be able to not expose it - do you have a reference which shows that it's possible to Enable RBAC without Azure AD?

Thanks!

tombuildsstuff added question service/kubernetes-cluster labels

joakimhellum commented Nov 17, 2018 •

edited

Loading

To be clear: I completely agree on RBAC being related to Azure AD for user authentication, and we are currently re-deploying all our clusters to enable this. Also because it's in the security best practices .

However it is possible to only enable RBAC without Azure AD integration at the moment.

Using the Azure Portal we can in fact only enable RBAC and not the Azure AD integration:
Using Azure CLI then RBAC is enabled by default (and can be disabled with --disable-rbac). But to enable Azure AD integration you would need to set multiple optional parameters (--aad-client-app-id, --aad-server-app-id, --aad-server-app-secret and --aad-tenant-id).
Reference: https://docs.microsoft.com/en-us/cli/azure/aks?view=azure-cli-latest#az-aks-create

So I think multiple users might have an issue with this update unless they upgrade their clusters to enable Azure AD integration.

Contributor Author

raphaelquati commented Nov 18, 2018

@tombuildsstuff,

I try to follow the same "block idea" of others blocks (like oms_agent and http_application_routing), with the "enabled" flag, instead of inferring the presence of block.

alexvicegrab mentioned this pull request

Adding support for AzureAD auth to AKS via RBAC #1820

Merged

tombuildsstuff added enhancement and removed question labels

tombuildsstuff added this to the 1.20.0 milestone

timfpark reviewed

View reviewed changes

azurerm/resource_arm_kubernetes_cluster_test.go Outdated

@@ @@ -596,6 +597,7 @@ resource "azurerm_kubernetes_cluster" "test" { @@
                 }
                 role_based_access_control {
+              		enaled=true

timfpark Nov 30, 2018

it looks like you have a typo here: enaled -> enabled


          Fix typo

34a8fc7

neumanndaniel mentioned this pull request

Feature Request: Export AKS clusterAdmin credentials #2421

Closed

This comment has been minimized.

Sign in to view

acondrat mentioned this pull request

Update azurerm_kubernetes_cluster for GA #1389

Closed

tombuildsstuff suggested changes

View reviewed changes

Contributor

tombuildsstuff left a comment

hey @raphaelquati

Thanks for this PR - apologies for the delayed review here, I'd reviewed this but not hit submit.

On the whole this PR looks pretty good - I've left a few comments inline which need to be fixed for us to merge this. Since we're trying to include this in the 1.20 release I'm going to make these changes and include this in a branch which fixes both #2345 and #2421 - so whilst I'd like to thank you for this contribution I'm going to close this in favour of a combined PR for those (I hope you don't mind!).

Thanks!

azurerm/resource_arm_kubernetes_cluster.go

@@ @@ -315,12 +315,17 @@ func resourceArmKubernetesCluster() *schema.Resource { @@
               				Type:     schema.TypeList,
               				Optional: true,
               				ForceNew: true,
-              				MaxItems: 1,
+              				MaxItems: 2,

Contributor

tombuildsstuff Dec 12, 2018

there can only be a single block - so this can be flipped back to 1

azurerm/resource_arm_kubernetes_cluster.go

-              	azureADProfile := expandKubernetesClusterRoleBasedAccessControl(rbacRaw, tenantId)
-              	roleBasedAccessControlEnabled := azureADProfile != nil
+              	roleBasedAccessControlEnabled, azureADProfile := expandKubernetesClusterRoleBasedAccessControl(rbacRaw, tenantId)
+              	//roleBasedAccessControlEnabled := azureADProfile != nil

Contributor

tombuildsstuff Dec 12, 2018

we can remove this line, since it's not being used

azurerm/resource_arm_kubernetes_cluster.go

-              		return []interface{}{}
+              		return []interface{}{
+              			map[string]interface{}{
+              				"enabled": *enabledRBAC,

Contributor

tombuildsstuff Dec 12, 2018

there's a crash we should check here

azurerm/resource_arm_kubernetes_cluster.go

+              		return []interface{}{
+              			map[string]interface{}{
+              				"enabled": *enabledRBAC,
+              			},

Contributor

tombuildsstuff Dec 12, 2018

we still need to set the azure_active_directory block to an empty list, if it's not enabled (since then it'll show as a diff if necessary)

azurerm/resource_arm_kubernetes_cluster.go

               	return []interface{}{
               		map[string]interface{}{
+              			"enabled": *enabledRBAC,

Contributor

tombuildsstuff Dec 12, 2018

there's a potential crash here which we can nil-check around

azurerm/resource_arm_kubernetes_cluster_test.go

               				Check: resource.ComposeTestCheckFunc(
               					testCheckAzureRMKubernetesClusterExists(resourceName),
               					resource.TestCheckResourceAttr(resourceName, "role_based_access_control.#", "1"),
+              					resource.TestCheckResourceAttr(resourceName, "role_based_access_control.0.enabled", "true"),

Contributor

tombuildsstuff Dec 12, 2018

it'd be good to ensure this is set to false for the basic configuration

azurerm/resource_arm_kubernetes_cluster_test.go

@@ @@ -596,6 +597,7 @@ resource "azurerm_kubernetes_cluster" "test" { @@
                 }
                 role_based_access_control {
+              		enabled=true

Contributor

tombuildsstuff Dec 12, 2018

minor can we fix the formatting here?

tombuildsstuff mentioned this pull request

r/kubernetes_cluster: support for rbac without azuread / exporting the admin profile #2495

Merged

Contributor

tombuildsstuff commented Dec 12, 2018

As discussed above - closing this in favour of #2495

tombuildsstuff closed this

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 locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement service/kubernetes-cluster size/S