Add support for outputting kube config credentials from AKS #953

jjcollinge · 2018-03-08T13:28:19Z

This PR adds support for outputting the fields (from the kube config) necessary to input into the Kubernetes Terraform provider. This will enable a Terraform plan to stand up an AKS cluster and add Kubernetes objects directly - without having to use kubectl.

This will be used to deploy the agents needed for the container solution enabled by #952

cc: @sozercan

paultyng · 2018-03-08T14:33:04Z

I think this also addresses #934?

paultyng · 2018-03-08T14:35:05Z

azurerm/resource_arm_kubernetes_cluster.go

+	return nil
+}
+
+//TODO: Hide these


lol, agree with this comment

heh - I'd suggest these would also be good candidates to move out to ./azurerm/helpers/kubernetes?

paultyng · 2018-03-08T14:37:27Z

azurerm/resource_arm_kubernetes_cluster.go

@@ -454,3 +539,73 @@ func resourceAzureRMKubernetesClusterServicePrincipalProfileHash(v interface{})

 	return hashcode.String(buf.String())
 }
+
+func base64Decode(str string) (string, bool) {


not a huge thing, but would be nice to throw some unit testing on this and/or getKubeConfig

I don't think we need this function, fwiw - we could just check that err != nil below?

lawrencegripper · 2018-03-08T14:40:02Z

Yeah it should address #934. I'm using it like so to pass credentials to the kubernetes provider

resource "azurerm_kubernetes_cluster" "aks" {
    name        = "akc-${random_integer.random_int.result}"
    location    = "${azurerm_resource_group.test.location}"
    dns_prefix  = "akc-${random_integer.random_int.result}"

    resource_group_name = "${azurerm_resource_group.test.name}"
    kubernetes_version  = "1.8.7"


    linux_profile {
        admin_username = "${var.linux_admin_username}"
        ssh_key {
            key_data = "${var.linux_admin_ssh_publickey}"
        }
    }

    agent_pool_profile {
        name        = "agentpool"
        count       = "2"
        vm_size     = "Standard_DS2_v2"
        os_type     = "Linux"
    }

    service_principal {
        client_id     = "${var.client_id}"
        client_secret = "${var.client_secret}"
    }
}

provider "kubernetes" {
  host     = "${azurerm_kubernetes_cluster.aks.kube_config.0.host}"

  client_certificate     = "${base64decode(azurerm_kubernetes_cluster.aks.kube_config.0.client_certificate)}"
  client_key             = "${base64decode(azurerm_kubernetes_cluster.aks.kube_config.0.client_key)}"
  cluster_ca_certificate = "${base64decode(azurerm_kubernetes_cluster.aks.kube_config.0.cluster_ca_certificate)}"
}

resource "kubernetes_namespace" "monitoring" {
  metadata {
    name = "monitoring"
  }
}

paultyng · 2018-03-08T14:41:26Z

This also needs some acceptance test coverage, testing the outputs at least in the existing tests.

jjcollinge · 2018-03-08T14:51:49Z

@paultyng thanks for the review 👍 - I'll update ASAP.

tombuildsstuff

hey @jjcollinge

Thanks for this PR - this is looking really good! I've left some comments in-line but they basically follow what @paultyng has suggested, such that if we can add some tests for this (and potentially move the Kubernetes stuff into it's own package) this should be good to merge :)

Thanks!

tombuildsstuff · 2018-03-08T15:56:34Z

azurerm/resource_arm_kubernetes_cluster.go

+				}
+			}
+		}
+	}


minor/not a blocker: we can actually in-line these statements to condense it like so:

if profile != nil { if accessProfile := profile.AccessProfile; accessProfile != nil { if kubeConfig := accessProfile.KubeConfig; kubeConfig != nil { return flattenedKubeConfig(kubeConfig) } } }

tombuildsstuff · 2018-03-08T16:02:47Z

azurerm/resource_arm_kubernetes_cluster.go

+	return string(data), false
+}
+
+func getKubeConfig(config *string) *KubeConfig {


minor it might be worth pulling these out into a kubernetes specific package inside ./azurerm/helpers/kubernetes?

tombuildsstuff · 2018-03-08T16:04:13Z

azurerm/resource_arm_kubernetes_cluster.go

@@ -454,3 +539,73 @@ func resourceAzureRMKubernetesClusterServicePrincipalProfileHash(v interface{})

 	return hashcode.String(buf.String())
 }
+
+func base64Decode(str string) (string, bool) {


I don't think we need this function, fwiw - we could just check that err != nil below?

tombuildsstuff · 2018-03-08T16:04:35Z

azurerm/resource_arm_kubernetes_cluster.go

+
+	configStr, error := base64Decode(*config)
+	if error == false && configStr != "" {
+		log.Println(config)


given this is printing sensitive information, we should probably remove this?

Woops, didn't mean to leave that line in!

tombuildsstuff · 2018-03-08T16:05:06Z

azurerm/resource_arm_kubernetes_cluster.go

+	return nil
+}
+
+//TODO: Hide these


heh - I'd suggest these would also be good candidates to move out to ./azurerm/helpers/kubernetes?

tombuildsstuff · 2018-03-08T16:06:34Z

website/docs/r/kubernetes_cluster.html.markdown

+
+* `kube_config.0.username` - The username used to authenticate to the Kubernetes cluster
+
+* `kube_config.0.password` - The password or token used to authenticate to the Kubernetes cluster


could we update this to match the other documentation? In general we'd show these like so:

* `kube_config` - a `kube_config` block as defined below: --- * `client_key` - The base64-encoded Client Key used to authenticate to the Kubernetes Cluster # ...

it may be worth adding an output to the example above to demonstrate how to access them?

jjcollinge · 2018-03-08T18:04:41Z

Thanks for the review @tombuildsstuff - I'll get this fixed up tomorrow 😀

jjcollinge · 2018-03-09T17:08:23Z

@paultyng @tombuildsstuff - updated as per reviews

paultyng · 2018-03-09T19:00:56Z

FYI the testdata folder is the idiomatic place to store test data. https://golang.org/cmd/go/#hdr-Test_packages

tombuildsstuff

hey @jjcollinge

Thanks for pushing the latest changes, I've left one comment (which I'll push a commit for - I hope you don't mind!) - but this otherwise LGTM 👍

Thanks!

tombuildsstuff · 2018-03-13T13:57:40Z

website/docs/r/kubernetes_cluster.html.markdown

+
+output "host" {
+  value = "${azurerm_kubernetes_cluster.test.kube_config.0.host}"
+}


I'm going to push a commit to update the test to ensure these values are actually set

changes were pushed

jjcollinge · 2018-03-13T15:00:38Z

@tombuildsstuff thanks for the review - just a word of warning in case you're running the acceptance tests, AKS is currently having a few 'reliability' issues in certain regions. I've found I can only deploy into centralus at the moment.

tombuildsstuff · 2018-03-13T18:36:56Z

@jjcollinge thanks for the heads up - I can confirm that issue, I'll give it a few hours for this to recover and try running the tests for this later today; but this otherwise LGTM :)

tombuildsstuff · 2018-03-15T17:41:01Z

@jjcollinge heads up the API still appeared to be broken yesterday - so I've reached out to MS through some internal channels.. I've bumped this to the next release for the moment, sorry for the delay getting this merged!

jjcollinge · 2018-03-16T14:18:33Z

@tombuildsstuff stuff - no worries! I was thinking of adding another output field called 'source' which contains the original encoded config. This will be useful for people who want to inject it straight into kubectl rather than using the Kubernetes Terraform provider - thoughts?

tombuildsstuff · 2018-03-16T14:38:32Z

@jjcollinge sounds good to me - although I'd suggest the fields named kubeconfig or raw_kubeconfig or something; so it's clearer what it is :)

lawrencegripper · 2018-04-06T12:45:59Z

Hopefully this should be good to test now

tombuildsstuff · 2018-04-10T14:08:24Z

hey @jjcollinge

Thanks for pushing the latest updates - upon re-running the tests I noticed a compilation error caused by upgrading to v15 of the Azure SDK - which I've pushed a commit to fix (I hope you don't mind!) - which means that these tests should now run again.

Thanks!

…g(bytes)

tombuildsstuff · 2018-04-25T12:12:08Z

@jjcollinge I've spent some time looking into this this morning and there was a breaking change as part of the latest Azure SDK; as such I've pushed a commit to fix that which enables this to work as expected, I hope you don't mind.

tombuildsstuff · 2018-04-25T18:05:11Z

👋 hey @jjcollinge

Just to give an update here: so I've confirmed the tests for this look good - the failures here are AKS capacity issues:

I've kicked off one final test of TestAccAzureRMKubernetesCluster_basic - providing that passes (which it did for me earlier) - we'll merge this PR and release it as a part of v1.4.0 in the morning :)

Thanks!

tombuildsstuff · 2018-04-26T09:12:41Z

The _basic test passes:

tombuildsstuff

LGTM 👍

tombuildsstuff · 2018-04-26T10:04:39Z

hey @jjcollinge

Just to let you know that v1.4.0 of the Azure Provider has been released which includes support for this :)

Thanks!

jjcollinge · 2018-04-26T17:24:34Z

Great! Thanks for sticking with this @tombuildsstuff - appreciate it's not been a smooth process!

lawrencegripper · 2018-04-26T17:33:08Z

Example works on the new release :)

https://github.com/lawrencegripper/azure-aks-terraform

ghost · 2020-03-31T15:06:59Z

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!

paultyng added enhancement service/kubernetes-cluster labels Mar 8, 2018

paultyng previously requested changes Mar 8, 2018

View reviewed changes

jjcollinge force-pushed the lg-akscreds branch from 2e9ac5e to 6dce948 Compare March 8, 2018 14:45

tombuildsstuff reviewed Mar 8, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Mar 8, 2018

tombuildsstuff removed the waiting-response label Mar 13, 2018

tombuildsstuff approved these changes Mar 13, 2018

View reviewed changes

tombuildsstuff added this to the 1.3.1 milestone Mar 15, 2018

tombuildsstuff added the upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR label Mar 15, 2018

tombuildsstuff modified the milestones: 1.3.1, 1.3.2 Mar 28, 2018

tombuildsstuff modified the milestones: 1.3.2, 1.3.3 Apr 4, 2018

tombuildsstuff removed the upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR label Apr 10, 2018

tombuildsstuff added this to the Soon milestone Apr 25, 2018

lawrencegripper and others added 14 commits April 25, 2018 12:56

Add kubeconfig to aks resource output

3d353a1

Added kube config to output variables

1eddefb

Updated docs

d9cd3fb

Fixed naming in docs

8203b56

Rebased onto origin master

4845e41

Refactored out kubernetes helper

91ffd11

Made kubeconfig embedded structs private

5ad9666

Loaded test configs from testdata directory

54e7b79

Added additional unit tests

9f105d9

Checking the outputs are set

964cba2

Added raw kube config as output

a1ef0fd

Changes required for sdkv15

c4a7244

Parsing the Kube Config as a raw string, since it's decoded via strin…

e2867b0

…g(bytes)

Ensuring the Kubernetes Config is set & tested

9d5c761

tombuildsstuff force-pushed the lg-akscreds branch from 5157b9a to 9d5c761 Compare April 25, 2018 12:09

tombuildsstuff modified the milestones: Soon, 1.4.0 Apr 25, 2018

tombuildsstuff approved these changes Apr 26, 2018

View reviewed changes

tombuildsstuff merged commit 587c815 into hashicorp:master Apr 26, 2018

tombuildsstuff added a commit that referenced this pull request Apr 26, 2018

Updating to include #953

a8ac78d

jjcollinge deleted the lg-akscreds branch April 26, 2018 17:24

ghost locked and limited conversation to collaborators Mar 31, 2020


		* `kube_config.0.username` - The username used to authenticate to the Kubernetes cluster

		* `kube_config.0.password` - The password or token used to authenticate to the Kubernetes cluster

Add support for outputting kube config credentials from AKS #953

Add support for outputting kube config credentials from AKS #953

Conversation

jjcollinge commented Mar 8, 2018

paultyng commented Mar 8, 2018

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

lawrencegripper commented Mar 8, 2018

paultyng commented Mar 8, 2018

jjcollinge commented Mar 8, 2018

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

jjcollinge commented Mar 8, 2018

jjcollinge commented Mar 9, 2018

paultyng commented Mar 9, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjcollinge commented Mar 13, 2018

tombuildsstuff commented Mar 13, 2018

tombuildsstuff commented Mar 15, 2018

jjcollinge commented Mar 16, 2018 • edited Loading

tombuildsstuff commented Mar 16, 2018

lawrencegripper commented Apr 6, 2018

tombuildsstuff commented Apr 10, 2018

tombuildsstuff commented Apr 25, 2018

tombuildsstuff commented Apr 25, 2018

tombuildsstuff commented Apr 26, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff commented Apr 26, 2018

jjcollinge commented Apr 26, 2018

lawrencegripper commented Apr 26, 2018

ghost commented Mar 31, 2020

jjcollinge commented Mar 16, 2018 •

edited

Loading