-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Add support for outputting kube config credentials from AKS #953
Add support for outputting kube config credentials from AKS #953
Conversation
I think this also addresses #934? |
return nil | ||
} | ||
|
||
//TODO: Hide these |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol, agree with this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heh - I'd suggest these would also be good candidates to move out to ./azurerm/helpers/kubernetes
?
@@ -454,3 +539,73 @@ func resourceAzureRMKubernetesClusterServicePrincipalProfileHash(v interface{}) | |||
|
|||
return hashcode.String(buf.String()) | |||
} | |||
|
|||
func base64Decode(str string) (string, bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a huge thing, but would be nice to throw some unit testing on this and/or getKubeConfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this function, fwiw - we could just check that err != nil
below?
Yeah it should address #934. I'm using it like so to pass credentials to the 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"
}
} |
This also needs some acceptance test coverage, testing the outputs at least in the existing tests. |
@paultyng thanks for the review 👍 - I'll update ASAP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
}
}
}
return string(data), false | ||
} | ||
|
||
func getKubeConfig(config *string) *KubeConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor it might be worth pulling these out into a kubernetes
specific package inside ./azurerm/helpers/kubernetes
?
@@ -454,3 +539,73 @@ func resourceAzureRMKubernetesClusterServicePrincipalProfileHash(v interface{}) | |||
|
|||
return hashcode.String(buf.String()) | |||
} | |||
|
|||
func base64Decode(str string) (string, bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this function, fwiw - we could just check that err != nil
below?
|
||
configStr, error := base64Decode(*config) | ||
if error == false && configStr != "" { | ||
log.Println(config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given this is printing sensitive information, we should probably remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woops, didn't mean to leave that line in!
return nil | ||
} | ||
|
||
//TODO: Hide these |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heh - I'd suggest these would also be good candidates to move out to ./azurerm/helpers/kubernetes
?
|
||
* `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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Thanks for the review @tombuildsstuff - I'll get this fixed up tomorrow 😀 |
@paultyng @tombuildsstuff - updated as per reviews |
FYI the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
|
||
output "host" { | ||
value = "${azurerm_kubernetes_cluster.test.kube_config.0.host}" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to push a commit to update the test to ensure these values are actually set
@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. |
@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 :) |
@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! |
@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 |
@jjcollinge sounds good to me - although I'd suggest the fields named |
Hopefully this should be good to test now |
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! |
5157b9a
to
9d5c761
Compare
@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. |
👋 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 Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
hey @jjcollinge Just to let you know that v1.4.0 of the Azure Provider has been released which includes support for this :) Thanks! |
Great! Thanks for sticking with this @tombuildsstuff - appreciate it's not been a smooth process! |
Example works on the new release :) https://github.com/lawrencegripper/azure-aks-terraform |
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 👉 [email protected]. Thanks! |
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